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)
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.
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
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.
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.
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 :)
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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.