Bug 150583 – Fix endless OSR exits when creating a rope that contains an object that ToPrimitive's to a number.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Fix endless OSR exits when concatenating strings.

[Keith Miller]

Created attachment 264103 [details]
Patch

[Keith Miller]

Created attachment 264105 [details]
Benchmark Results

[Keith Miller]

Regressions in Benchmark results did not reproduce:

                                                      TOT                        MC
SunSpider:
   controlflow-recursive                         2.4504+-0.3939            2.3247+-0.1083          might be 1.0541x faster

   <arithmetic>                                  2.4504+-0.3939            2.3247+-0.1083          might be 1.0541x faster

                                                      TOT                        MC
LongSpider:
   controlflow-recursive                       428.8641+-11.6692    ?    430.4963+-16.7602       ?

   <geometric>                                 428.8641+-11.6692    ?    430.4963+-16.7602       ? might be 1.0038x slower

                                                      TOT                        MC
JSRegress:
   global-var-const-infer-fire-from-opt          0.8022+-0.1973     ?      0.8531+-0.0255        ? might be 1.0634x slower
   string-get-by-val-out-of-bounds-insane        3.2679+-0.6258            3.1248+-0.2316          might be 1.0458x faster
   string-get-by-val-out-of-bounds               4.1632+-0.7309     ?      4.2820+-0.1860        ? might be 1.0285x slower

   <geometric>                                   2.2087+-0.2466     ?      2.2508+-0.0455        ? might be 1.0191x slower

                                                      TOT                        MC
Geomean of preferred means:
   <scaled-result>                              13.2316+-1.2167           13.1067+-0.2477          might be 1.0095x faster

[Keith Miller]

Created attachment 264106 [details]
Patch

[Keith Miller]

Results for new regression test (none of the existing tests exemplified the bad behavior):

                                    TOT                        MC

string-rope-with-object      445.0135+-3.9811     ^    399.1523+-6.7657        ^ definitely 1.1149x faster

<geometric>                  445.0135+-3.9811     ^    399.1523+-6.7657        ^ definitely 1.1149x faster

[Benjamin Poulain]

Comment on attachment 264106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264106&action=review

> Source/JavaScriptCore/ChangeLog:32
> +

You should be able to make a js-regress that is very slow without your patch because of the OSR exits.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1459
> +bool Graph::isStringPrototypeMethodSane(
> +    JSObject* stringPrototype, Structure* stringPrototypeStructure, UniquedStringImpl* uid)

Come on, this could be on a single line :)

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1463
> +    PropertyOffset offset =
> +        stringPrototypeStructure->getConcurrently(uid, attributesUnused);

Single line.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1468
> +    JSValue value = tryGetConstantProperty(
> +        stringPrototype, stringPrototypeStructure, offset);

Single line.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1494
> +    StringPrototype* stringPrototypeObject =
> +        stringPrototypeObjectValue->dynamicCast<StringPrototype*>();

Single line.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:904
> +
> +

Two blank lines here.

> LayoutTests/js/regress/script-tests/string-rope-with-object.js:1
> +function body() {

For the sake of coverage. Can you please add tests for Object with valueOf returning a non-string primitive.

And a mix of types, like String + Int32, String + Double, or something like that.

[Benjamin Poulain]

> You should be able to make a js-regress that is very slow without your patch
> because of the OSR exits.

Ignore this. I had not read the test yet at that point.

Note that you are missing the changelog for the regress test.

[Benjamin Poulain]

(In reply to comment #5)
> Results for new regression test (none of the existing tests exemplified the
> bad behavior):
> 
>                                     TOT                        MC
> 
> string-rope-with-object      445.0135+-3.9811     ^    399.1523+-6.7657     
> ^ definitely 1.1149x faster
> 
> <geometric>                  445.0135+-3.9811     ^    399.1523+-6.7657     
> ^ definitely 1.1149x faster

That's a bit long for a regress test. We try to make them 5-10 ms.

[Geoffrey Garen]

Comment on attachment 264106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264106&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        Before we assumed that the result of ToPrimitive on any object was a string.
> +        This had a couple of negative effects. First, for any non-StringObject Object
> +        the result of ToPrimitive will, by default, be a number. Second, even after

This isn't true. The default ToPrimitive on object converts to string, not number. (That's why your test case needs to override valueOf.)

[Geoffrey Garen]

> Fix endless OSR exits when concatenating strings.

Did you mean when concatenating strings with objects that stringify as number?

[Keith Miller]

Comment on attachment 264106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264106&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +        the result of ToPrimitive will, by default, be a number. Second, even after
> 
> This isn't true. The default ToPrimitive on object converts to string, not number. (That's why your test case needs to override valueOf.)

You are correct, will fix. I thought the valueOf function on Objects returned a numerical value since { } + 0 === 0 but that appears that appears to be different. It now says: "First, the result ToPrimitive on an object can be overridden to be any primitive type. In fact, as of ES6, ToPrimitive as part of a addition expression will type hint a number value."

>> Source/JavaScriptCore/ChangeLog:32
>> +
> 
> You should be able to make a js-regress that is very slow without your patch because of the OSR exits.

I changed the test to have a loop in the function doing the concatenation and it's now a 2.2x speedup.

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1459
>> +    JSObject* stringPrototype, Structure* stringPrototypeStructure, UniquedStringImpl* uid)
> 
> Come on, this could be on a single line :)

Fixed.

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1463
>> +        stringPrototypeStructure->getConcurrently(uid, attributesUnused);
> 
> Single line.

Fixed.

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1468
>> +        stringPrototype, stringPrototypeStructure, offset);
> 
> Single line.

Fixed.

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1494
>> +        stringPrototypeObjectValue->dynamicCast<StringPrototype*>();
> 
> Single line.

Fixed.

>> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:904
>> +
> 
> Two blank lines here.

Fixed.

>> LayoutTests/js/regress/script-tests/string-rope-with-object.js:1
>> +function body() {
> 
> For the sake of coverage. Can you please add tests for Object with valueOf returning a non-string primitive.
> 
> And a mix of types, like String + Int32, String + Double, or something like that.

done.

[Keith Miller]

(In reply to comment #10)
> > Fix endless OSR exits when concatenating strings.
> 
> Did you mean when concatenating strings with objects that stringify as
> number?

I suppose a better wording would be "Fix endless OSR exits when creating a rope that contains an object that ToPrimitive's to a number." Does that seem better?

[Keith Miller]

Created attachment 264609 [details]
Extra Test

Putting the extra test here that Ben requested for posterity.

[Keith Miller]

Committed r192034: <http://trac.webkit.org/changeset/192034>