Bug 117386 – JSString should remember AtomicString

• Component: Bindings
• Status: REOPENED

[Ryosuke Niwa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/14102739>

[Ryosuke Niwa]

Created attachment 204123 [details]
Work in progress

[Benjamin Poulain]

I like the way this is going! :)

[Ryosuke Niwa]

Created attachment 204126 [details]
Fixe the bug

[Ryosuke Niwa]

Created attachment 204127 [details]
Updated for ToT

[Sam Weinig]

Is this a speedup on any benchmarks (normal or micro)?

[Ryosuke Niwa]

Created attachment 204129 [details]
Add Element.idl changes

[Geoffrey Garen]

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.

[Ryosuke Niwa]

(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.

[Gavin Barraclough]

(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).

[Ryosuke Niwa]

(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.

[Oliver Hunt]

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

[Ryosuke Niwa]

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.

[Geoffrey Garen]

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

[Ryosuke Niwa]

(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.

[Oliver Hunt]

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

[Ryosuke Niwa]

Created attachment 204178 [details]
Fixed the roping issue

[Geoffrey Garen]

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.

[Ryosuke Niwa]

Unfortunately, this patch doesn't seem to have any impact on html5-full-render.html load time.

[Ryosuke Niwa]

Created attachment 205363 [details]
Patch for landing

[Ryosuke Niwa]

Created attachment 205365 [details]
Addressed Geoff's comment

[Ryosuke Niwa]

Dromaeo results:
r151951 with patch: http://dromaeo.com/?id=197766
r151951 without patch: http://dromaeo.com/?id=197769

[WebKit Commit Bot]

Comment on attachment 205365 [details]
Addressed Geoff's comment

Clearing flags on attachment: 205365

Committed r151978: <http://trac.webkit.org/changeset/151978>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

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?

[Ryosuke Niwa]

(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?

[Gavin Barraclough]

(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>

[Ryosuke Niwa]

(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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 118651