Bug 150583 - Fix endless OSR exits when creating a rope that contains an object that ToPrimitive's to a number.
Summary: Fix endless OSR exits when creating a rope that contains an object that ToPri...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-26 17:10 PDT by Keith Miller
Modified: 2015-11-04 13:47 PST (History)
2 users (show)

See Also:


Attachments
Patch (14.53 KB, patch)
2015-10-26 17:27 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Benchmark Results (62.54 KB, text/plain)
2015-10-26 17:28 PDT, Keith Miller
no flags Details
Patch (15.25 KB, patch)
2015-10-26 17:33 PDT, Keith Miller
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff
Extra Test (1.54 KB, application/x-javascript)
2015-11-02 11:57 PST, Keith Miller
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2015-10-26 17:10:50 PDT
Fix endless OSR exits when concatenating strings.
Comment 1 Keith Miller 2015-10-26 17:27:19 PDT
Created attachment 264103 [details]
Patch
Comment 2 Keith Miller 2015-10-26 17:28:50 PDT
Created attachment 264105 [details]
Benchmark Results
Comment 3 Keith Miller 2015-10-26 17:30:37 PDT
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
Comment 4 Keith Miller 2015-10-26 17:33:27 PDT
Created attachment 264106 [details]
Patch
Comment 5 Keith Miller 2015-10-26 17:35:34 PDT
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
Comment 6 Benjamin Poulain 2015-10-26 18:09:12 PDT
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.
Comment 7 Benjamin Poulain 2015-10-26 18:10:43 PDT
> 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.
Comment 8 Benjamin Poulain 2015-10-26 18:15:52 PDT
(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.
Comment 9 Geoffrey Garen 2015-10-26 20:29:38 PDT
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.)
Comment 10 Geoffrey Garen 2015-10-26 20:31:05 PDT
> Fix endless OSR exits when concatenating strings.

Did you mean when concatenating strings with objects that stringify as number?
Comment 11 Keith Miller 2015-10-30 16:19:29 PDT
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.
Comment 12 Keith Miller 2015-10-30 16:21:34 PDT
(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?
Comment 13 Keith Miller 2015-11-02 11:57:17 PST
Created attachment 264609 [details]
Extra Test

Putting the extra test here that Ben requested for posterity.
Comment 14 Keith Miller 2015-11-04 13:47:05 PST
Committed r192034: <http://trac.webkit.org/changeset/192034>