We've found out that we're constantly converting the same String to AtomicString in the binding code. Make JSString smarter and remember the conversion so that the subsequent calls to the same DOM API will not result in another String-to-AtomicString conversion.
<rdar://problem/14102739>
Created attachment 204123 [details] Work in progress
I like the way this is going! :)
Created attachment 204126 [details] Fixe the bug
Created attachment 204127 [details] Updated for ToT
Is this a speedup on any benchmarks (normal or micro)?
Created attachment 204129 [details] Add Element.idl changes
Side note: This is good evidence that we should merge the Identifier and AtomicString tables. Most of these strings are probably Identifiers to start with.
(In reply to comment #8) > Side note: This is good evidence that we should merge the Identifier and AtomicString tables. Most of these strings are probably Identifiers to start with. Right. Oliver suggested that when we were discussing it on Friday but I think this is a good starting at least for now.
(In reply to comment #9) > (In reply to comment #8) > > Side note: This is good evidence that we should merge the Identifier and AtomicString tables. Most of these strings are probably Identifiers to start with. > > Right. Oliver suggested that when we were discussing it on Friday but I think this is a good starting at least for now. Last time I tried this it was a regression, but that was a few years back & won't necessarily still be the case (especially if there are opportunities, as here, to take advantage of it).
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Side note: This is good evidence that we should merge the Identifier and AtomicString tables. Most of these strings are probably Identifiers to start with. > > > > Right. Oliver suggested that when we were discussing it on Friday but I think this is a good starting at least for now. > > Last time I tried this it was a regression, but that was a few years back & won't necessarily still be the case (especially if there are opportunities, as here, to take advantage of it). That's good to know. FWIW, I didn't tried that so it'll be good to get data again. Somewhat related to this, it might make sense to add a special member function like toAtomicString() on String to generalize this change so that other parts of WebKit can benefit from the same optimization. A much more aggressive approach will be to online-swap String::m_impl inside AtomicString(const String&) but that may be tricky since doing so might create a stable pointer to StringImpl* elsewhere.
Comment on attachment 204129 [details] Add Element.idl changes View in context: https://bugs.webkit.org/attachment.cgi?id=204129&action=review I kind of like this approach as an intermediate step. I think the real problem with Atomic vs. Identifier (aside from thread semantics, etc) is the intended lifetime - a constant string in source code is always going to last as long as the page is live, but selectors, or ids, etc may not. That said, i find manual annotation of DOMStrings to be somewhat saddening but i'm not sure how to avoid that in the interim > Source/JavaScriptCore/runtime/JSString.h:378 > + if (isRope()) > + static_cast<const JSRopeString*>(this)->resolveRope(exec); This can fail, so you'll need to handle that with an exception check
Comment on attachment 204129 [details] Add Element.idl changes View in context: https://bugs.webkit.org/attachment.cgi?id=204129&action=review >> Source/JavaScriptCore/runtime/JSString.h:378 >> + static_cast<const JSRopeString*>(this)->resolveRope(exec); > > This can fail, so you'll need to handle that with an exception check Can you think of a concrete case where this would fail? Ideally, I’d like to add a test case for it.
> > This can fail, so you'll need to handle that with an exception check > > Can you think of a concrete case where this would fail? Ideally, I’d like to add a test case for it. The two failures cases are (1) Out of memory: A rope that fails to malloc when resolved. (2) Too long: A rope whose length is > 2^32-1 when resolved. On a 64bit system, it's not practical to trigger (1), but you may be able to trigger (2).
(In reply to comment #14) > The two failures cases are > > (1) Out of memory: A rope that fails to malloc when resolved. > > (2) Too long: A rope whose length is > 2^32-1 when resolved. > > On a 64bit system, it's not practical to trigger (1), but you may be able to trigger (2). Sounds like I shouldn't necessary make a test case either way... :( Will just fix the code.
(In reply to comment #15) > (In reply to comment #14) > > The two failures cases are > > > > (1) Out of memory: A rope that fails to malloc when resolved. > > > > (2) Too long: A rope whose length is > 2^32-1 when resolved. > > > > On a 64bit system, it's not practical to trigger (1), but you may be able to trigger (2). > > Sounds like I shouldn't necessary make a test case either way... :( Will just fix the code. It may be possible to create a large enough rope to fail without actually relying on memory usage: var str = "a" for (var i = 0; i < 31; i++) str = str + str str = str + "a" atomicify(str)
Created attachment 204178 [details] Fixed the roping issue
Comment on attachment 204178 [details] Fixed the roping issue View in context: https://bugs.webkit.org/attachment.cgi?id=204178&action=review r=me > Source/JavaScriptCore/runtime/JSString.h:380 > + if (exec->hadException()) > + return nullAtom; You can move this exception check inside the isRope() block, to tell the compiler that an exception can only happen if we have to resolve a rope.
Unfortunately, this patch doesn't seem to have any impact on html5-full-render.html load time.
Created attachment 205363 [details] Patch for landing
Created attachment 205365 [details] Addressed Geoff's comment
Dromaeo results: r151951 with patch: http://dromaeo.com/?id=197766 r151951 without patch: http://dromaeo.com/?id=197769
Comment on attachment 205365 [details] Addressed Geoff's comment Clearing flags on attachment: 205365 Committed r151978: <http://trac.webkit.org/changeset/151978>
All reviewed patches have been landed. Closing bug.
This change broke 3 sites that we know of already: http://sigalert.com http://www.ipernity.com/doc/ndlinegeek http://io9.com/this-movie-would-have-been-perfect-if-not-for-that-awf-736457142 With how quickly those 3 have been discovered, it's somewhat frightening. Should we role this out while the regressions are revisited?
(In reply to comment #25) > This change broke 3 sites that we know of already: > http://sigalert.com > http://www.ipernity.com/doc/ndlinegeek > http://io9.com/this-movie-would-have-been-perfect-if-not-for-that-awf-736457142 > > With how quickly those 3 have been discovered, it's somewhat frightening. > > Should we role this out while the regressions are revisited? Oops, are there bugs filed about these failures? Is the kind of failures known?
(In reply to comment #26) > Oops, are there bugs filed about these failures? Is the kind of failures known? This is in radar: <rdar://problem/14408012>
(In reply to comment #27) > (In reply to comment #26) > > Oops, are there bugs filed about these failures? Is the kind of failures known? > > This is in radar: <rdar://problem/14408012> Okay, sorry about that. We should roll this patch out because 1. It wasn't a huge win on PLT3 and our Dromaeo score doesn't improve that much either. 2. We should merge Identifier and AtomicString instead.
Re-opened since this is blocked by bug 118651