1352323 - Try using static string for short external string

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

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)

Comment 1

[Tooru Fujisawa [:arai]]

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.

Comment 2

[Tooru Fujisawa [:arai]]

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

Comment 3

[Tooru Fujisawa [:arai]]

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

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: 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)

Comment 6

[Jan de Mooij [:jandem]]

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

Comment 7

[Jan de Mooij [:jandem]]

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.

Comment 8

[Jan de Mooij [:jandem]]

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 :)

Comment 9

[Tooru Fujisawa [:arai]]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cda93365031