| Summary: | Fix endless OSR exits when creating a rope that contains an object that ToPrimitive's to a number. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||
| Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | benjamin, ggaren | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Keith Miller
2015-10-26 17:10:50 PDT
Created attachment 264103 [details]
Patch
Created attachment 264105 [details]
Benchmark Results
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
Created attachment 264106 [details]
Patch
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 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. > 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.
(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 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.) > Fix endless OSR exits when concatenating strings.
Did you mean when concatenating strings with objects that stringify as number?
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. (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? Created attachment 264609 [details]
Extra Test
Putting the extra test here that Ben requested for posterity.
Committed r192034: <http://trac.webkit.org/changeset/192034> |