Try using static string for short external string

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
I see so much duplication of short string like 0 or 1, allocated as external strings.
it would be nice to use static string instead.
(if there's no unexpected side effect)
(Assignee)

Comment 1

5 months ago
here's the steps to reproduce:
  1. run Nightly with clean profile
  2. open https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp
  3. open about:memory in new tab
  4. measure memory report with verbose
  5. save GC & CC logs
  6. count the number of "0" string there

bug 1352317 patch (not yet attached) would help figuring out what kind of string it is.
(Assignee)

Comment 2

5 months ago
my current concerns are:
  1. if it's okay to call finalizer instantly in JS_NewExternalString
  2. if it's okay to return latin1 string from JS_NewExternalString
(Assignee)

Comment 3

5 months ago
so, that case needs to be handled properly in
https://dxr.mozilla.org/mozilla-central/rev/3364cc17988c013c36f2a8123315db2855393011/js/xpconnect/src/xpcpublic.h#255-286
Good find.

We should definitely try to use static strings. Maybe we can add an outparam to JS_NewExternalString so it can let the caller know it really allocated an external string?

I also filed bug 1038099 to create (Latin1) inline strings for short external strings.
(Assignee)

Comment 5

5 months ago
Created attachment 8853599 [details] [diff] [review]
Add JS_NewMaybeExternalString function that creates either an external string or returns a static string.

Added JS_NewMaybeExternalString instead of modifying existing JS_NewExternalString.
to clarify what would happen with the API.

currently JS_NewMaybeExternalString returns either external string or static string.
we could modify it later to return even more different kind of strings.

if JS_NewMaybeExternalString returns non-external string, finalizer won't be called.
since the current caller (StringBufferToJSVal) expects the finalizer to be called after the function returns.
and now we can tell the caller that the returned string is not external string (and finalizer won't be called)
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8853599 - Flags: review?(jdemooij)
Comment on attachment 8853599 [details] [diff] [review]
Add JS_NewMaybeExternalString function that creates either an external string or returns a static string.

Review of attachment 8853599 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

::: js/xpconnect/src/xpcpublic.h
@@ +275,5 @@
>              return false;
>          }
>          rval.setString(str);
> +
> +        // If JS_NewMaybeExternalString returns non-exgternal string, finalizer

Nit: typo, non-external
Attachment #8853599 - Flags: review?(jdemooij) → review+
As a followup, it would be interesting to see how many external strings are backed by string literals (the sLiteralFinalizer case). In that case atomizing might make sense (at least if the string is not huge) as there's a finite number of these strings and they're likely used by many different zones.

Updated

5 months ago
Blocks: 1352486
This should help some Ember benchmarks (bug 1352486) a lot, they allocate many "A", "TR", "TD" external strings.

We also allocate "http://www.w3.org/1999/xhtml" many times, I'm hoping that after this lands, XPConnect's zone cache will hit for that one in most cases. We should consider moving that external string cache into the JS engine and changing it from 1 slot to 4 slots or so. I'll look into that after this lands :)
(Assignee)

Comment 9

5 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cda9336503172c5bad871155c1abf1c44f45fa9
Bug 1352323 - Add JS_NewMaybeExternalString function that creates either an external string or returns a static string. r=jandem
(Assignee)

Updated

5 months ago
See Also: → bug 1353279

Comment 10

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4cda93365031
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.