Bug 156562

<rdar://problem/25715263>

Created attachment 277339 [details] Patch

Attachment 277339 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1928: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.

Here are performance results. Octane regexp is ~2.5% slower, likely due to the added 4 checks that are needed to see if the RegExp has altered .exec, .global, .unicode and Symbol.match. VMs tested: "Baseline" at /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc "BuiltinReplace" at /Volumes/Data/src/webkit/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc Collected 4 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. Baseline BuiltinReplace SunSpider: 3d-cube 6.6799+-0.5086 6.4760+-0.4113 might be 1.0315x faster 3d-morph 6.1168+-0.2656 ? 6.2485+-0.4328 ? might be 1.0215x slower 3d-raytrace 7.2584+-0.4425 ? 7.3538+-0.4746 ? might be 1.0131x slower access-binary-trees 2.6850+-0.1518 2.6233+-0.2883 might be 1.0235x faster access-fannkuch 7.3678+-0.1167 7.2873+-0.1690 might be 1.0110x faster access-nbody 3.4064+-0.1915 3.3703+-0.1640 might be 1.0107x faster access-nsieve 3.9490+-0.1819 ? 4.0035+-0.1252 ? might be 1.0138x slower bitops-3bit-bits-in-byte 1.4946+-0.1149 ? 1.5120+-0.0968 ? might be 1.0117x slower bitops-bits-in-byte 3.5220+-0.1336 3.5142+-0.1674 bitops-bitwise-and 2.4728+-0.1371 ? 2.5139+-0.1441 ? might be 1.0166x slower bitops-nsieve-bits 3.6646+-0.1457 ? 3.7678+-0.2089 ? might be 1.0281x slower controlflow-recursive 2.9023+-0.1930 ? 2.9473+-0.1874 ? might be 1.0155x slower crypto-aes 5.2960+-0.2403 ? 5.6232+-0.3948 ? might be 1.0618x slower crypto-md5 3.1622+-0.3297 3.1320+-0.1978 crypto-sha1 3.0569+-0.0926 ? 3.0913+-0.1236 ? might be 1.0113x slower date-format-tofte 11.1715+-0.4856 ? 11.3375+-1.0306 ? might be 1.0149x slower date-format-xparb 5.8257+-0.5265 ? 5.9382+-0.3029 ? might be 1.0193x slower math-cordic 3.5265+-0.0706 3.5201+-0.1989 math-partial-sums 6.0107+-0.2506 6.0091+-0.2195 math-spectral-norm 2.5695+-0.3351 2.5613+-0.1737 regexp-dna 8.7949+-0.3757 8.0684+-0.4824 might be 1.0900x faster string-base64 5.5405+-0.5956 ? 5.5753+-0.3794 ? string-fasta 6.8537+-0.2872 ? 7.0009+-0.5947 ? might be 1.0215x slower string-tagcloud 10.0908+-0.2862 9.9282+-0.1297 might be 1.0164x faster string-unpack-code 22.0372+-0.6760 21.8492+-0.4505 string-validate-input 5.1163+-0.0908 ? 5.1512+-0.2156 ? <arithmetic> 5.7912+-0.0388 5.7848+-0.0654 might be 1.0011x faster Baseline BuiltinReplace Octane: encrypt 0.20545+-0.00545 0.20362+-0.00139 decrypt 3.59259+-0.07077 3.59119+-0.02934 deltablue x2 0.16854+-0.00617 0.16453+-0.00685 might be 1.0244x faster earley 0.37740+-0.00102 ? 0.37844+-0.00060 ? boyer 6.08825+-0.03419 6.05354+-0.03263 navier-stokes x2 5.62795+-0.06956 5.62695+-0.03374 raytrace x2 1.02202+-0.02085 ? 1.02272+-0.01411 ? richards x2 0.11292+-0.00426 ? 0.11341+-0.00380 ? splay x2 0.40449+-0.01121 0.40021+-0.00567 might be 1.0107x faster regexp x2 20.48452+-0.12318 ! 20.92319+-0.25260 ! definitely 1.0214x slower pdfjs x2 49.14264+-0.21648 ? 49.30188+-0.78867 ? mandreel x2 53.56751+-0.23703 ? 53.72601+-0.60483 ? gbemu x2 34.47862+-0.95425 ? 34.83207+-1.58528 ? might be 1.0103x slower closure 0.73730+-0.00467 0.73308+-0.00606 jquery 9.02009+-0.05381 ? 9.03561+-0.05419 ? box2d x2 13.06846+-0.16935 ? 13.11477+-0.19314 ? zlib x2 431.05800+-3.90056 431.05452+-3.29097 typescript x2 922.48114+-5.06618 912.24835+-9.99150 might be 1.0112x faster <geometric> 6.46351+-0.03027 6.46001+-0.04597 might be 1.0005x faster Baseline BuiltinReplace Kraken: ai-astar 115.285+-1.636 114.089+-2.300 might be 1.0105x faster audio-beat-detection 51.808+-1.351 51.306+-1.506 audio-dft 116.992+-2.480 116.909+-1.080 audio-fft 42.004+-0.537 41.574+-0.606 might be 1.0103x faster audio-oscillator 59.943+-0.318 ? 60.421+-1.319 ? imaging-darkroom 76.140+-1.867 ? 77.552+-1.918 ? might be 1.0185x slower imaging-desaturate 67.226+-0.194 66.801+-1.836 imaging-gaussian-blur 88.890+-13.914 85.267+-12.587 might be 1.0425x faster json-parse-financial 45.500+-2.232 ? 45.849+-0.706 ? json-stringify-tinderbox 28.567+-0.236 ? 28.627+-0.419 ? stanford-crypto-aes 46.830+-1.001 ? 47.108+-1.503 ? stanford-crypto-ccm 43.006+-2.771 ? 44.031+-0.860 ? might be 1.0238x slower stanford-crypto-pbkdf2 113.967+-3.212 113.822+-2.337 stanford-crypto-sha256-iterative 45.613+-0.483 45.295+-2.291 <arithmetic> 67.269+-0.919 67.047+-0.955 might be 1.0033x faster Baseline BuiltinReplace AsmBench: bigfib.cpp 551.7959+-5.2559 ? 556.1614+-5.4205 ? cray.c 437.1600+-13.1495 ? 438.0700+-2.6269 ? dry.c 517.4236+-23.8858 ? 530.8558+-34.8669 ? might be 1.0260x slower FloatMM.c 827.4593+-20.4047 821.1450+-7.1253 gcc-loops.cpp 4798.1882+-44.5754 4777.9066+-25.0404 n-body.c 1033.4825+-6.4338 1032.2607+-4.6811 Quicksort.c 486.4155+-7.0416 ? 486.9384+-5.9598 ? stepanov_container.cpp 3768.7362+-20.2172 3767.8273+-28.2371 Towers.c 311.5974+-1.8319 310.7805+-2.1443 <geometric> 872.2947+-7.5742 ? 874.2957+-7.4539 ? might be 1.0023x slower Baseline BuiltinReplace Geomean of preferred means: <scaled-result> 38.4968+-0.1113 38.4710+-0.2224 might be 1.0007x faster

Comment on attachment 277339 [details] Patch Attachment 277339 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1221736 New failing tests: js/dom/string-prototype-properties.html sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.11_String.prototype.replace/S15.5.4.11_A1_T3.html fast/profiler/nested-start-and-stop-profiler.html js/dom/delete-syntax.html

Created attachment 277341 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 277339 [details] Patch Attachment 277339 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1221738 New failing tests: js/dom/string-prototype-properties.html fast/profiler/nested-start-and-stop-profiler.html js/dom/delete-syntax.html sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.11_String.prototype.replace/S15.5.4.11_A1_T3.html

Created attachment 277342 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 277339 [details] Patch Attachment 277339 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1221735 New failing tests: js/dom/string-prototype-properties.html sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.11_String.prototype.replace/S15.5.4.11_A1_T3.html js/dom/delete-syntax.html fast/profiler/nested-start-and-stop-profiler.html

Created attachment 277344 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4

Comment on attachment 277339 [details] Patch Attachment 277339 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1221748 New failing tests: js/dom/string-prototype-properties.html sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.11_String.prototype.replace/S15.5.4.11_A1_T3.html fast/profiler/nested-start-and-stop-profiler.html js/dom/delete-syntax.html

Created attachment 277346 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 277339 [details] Patch Looks like Layout tests need a little work.

Created attachment 277394 [details] Patch with Layout test fixes

Attachment 277394 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1928: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 277394 [details] Patch with Layout test fixes Phil and I discussed changes that need to be made to this patch. 1. Put the primordial properties on the global object when we create RegExp.prototype. 2. Have DFGByteCodeParser check to see if the RegExp.prototype properties we add checks at Fixup phase have change. If so, don't inline the intrinsic. 3. In Fixup, handle the case where we should speculate String by adding a Check(@node, StringUse). Also add a ForceOSRExit when we don't know the speculation type for child2.

Created attachment 277409 [details] Updated Patch with agreed upon changes

Comment on attachment 277409 [details] Updated Patch with agreed upon changes View in context: https://bugs.webkit.org/attachment.cgi?id=277409&action=review > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:931 > + > + if (op == StringReplace) { > + if (!tryAddStringReplacePrimordialChecks(node->child2().node())) > + m_insertionSet.insertNode(m_indexInBlock, SpecNone, ForceOSRExit, node->origin); > + } > + This logic should not be conditioned upon the prediction of the second child being RegExpObject. What if child2 is not predicted either RegExpObject or String? We still want to do something in this case. probably we want to either speculate RegExpObject anyway, or we want to ForceOSRExit.

(In reply to comment #18) > Comment on attachment 277409 [details] > Updated Patch with agreed upon changes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277409&action=review > > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:931 > > + > > + if (op == StringReplace) { > > + if (!tryAddStringReplacePrimordialChecks(node->child2().node())) > > + m_insertionSet.insertNode(m_indexInBlock, SpecNone, ForceOSRExit, node->origin); > > + } > > + > > This logic should not be conditioned upon the prediction of the second child > being RegExpObject. > > What if child2 is not predicted either RegExpObject or String? We still > want to do something in this case. probably we want to either speculate > RegExpObject anyway, or we want to ForceOSRExit. Isn't the ForceOSRExit below on lines 938-939 sufficient?

Comment on attachment 277409 [details] Updated Patch with agreed upon changes View in context: https://bugs.webkit.org/attachment.cgi?id=277409&action=review > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:921 > + if (node->child2()->shouldSpeculateString()) { > + m_insertionSet.insertNode( > + m_indexInBlock, SpecNone, Check, node->origin, > + Edge(node->child2().node(), StringUse)); > + fixEdge<StringUse>(node->child2()); > + break; > + } I think that right after this, you should have: else if (op == StringReplace) { if (node->child2()->shouldSpeculateRegExpObject()) { // try to insert primordial checks or ForceOSRExit } else insertNode(ForceOSRExit); } Then you'd be able to remove both of your changes below (check insertion and ForceOSRExit). This would allow this code to handle things like StringReplace(StringObject, RegExpObject, Number) and StringReplace(String, RegExpObject, null), etc... >>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:931 >>> + >> >> This logic should not be conditioned upon the prediction of the second child being RegExpObject. >> >> What if child2 is not predicted either RegExpObject or String? We still want to do something in this case. probably we want to either speculate RegExpObject anyway, or we want to ForceOSRExit. > > Isn't the ForceOSRExit below on lines 938-939 sufficient? Oh. But then this is a different kind of bug: we aren't allowing StringReplace(Untyped:, Untyped:, Untyped:). You could allow for StringReplace(Untyped:, Untyped:, Untyped:) if you just moved this above.

Created attachment 277415 [details] Made suggested DFGFixupPhase.cpp changes

Attachment 277415 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1928: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2278: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 277415 [details] Made suggested DFGFixupPhase.cpp changes View in context: https://bugs.webkit.org/attachment.cgi?id=277415&action=review > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1928 > + auto emitPrimordialCheckFor = [&] (JSValue primordialProperty, UniquedStringImpl* propertyUID) { This can't return false.

Comment on attachment 277415 [details] Made suggested DFGFixupPhase.cpp changes View in context: https://bugs.webkit.org/attachment.cgi?id=277415&action=review > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1919 > + bool tryAddStringReplacePrimordialChecks(Node* searchRegExp) Maybe we should just call this "addStringReplacePrimordialChecks" since it won't return false?

(In reply to comment #24) > Comment on attachment 277415 [details] > Made suggested DFGFixupPhase.cpp changes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277415&action=review > > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1919 > > + bool tryAddStringReplacePrimordialChecks(Node* searchRegExp) > > Maybe we should just call this "addStringReplacePrimordialChecks" since it > won't return false? I made this change locally as well as eliminating the bool return and checks from the lambda emitPrimordialCheckFor().

Comment on attachment 277415 [details] Made suggested DFGFixupPhase.cpp changes Attachment 277415 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1225679 New failing tests: imported/w3c/web-platform-tests/dom/ranges/Range-surroundContents.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/srcset/parse-a-srcset-attribute.html css3/shapes/shape-outside/values/shape-outside-inset-001.html imported/w3c/web-platform-tests/dom/nodes/DOMImplementation-createDocument.html css3/shapes/shape-outside/values/shape-outside-circle-004.html fast/dom/DOMImplementation/createDocument-namespace-err.html imported/w3c/web-platform-tests/dom/nodes/Document-createElementNS.html css3/shapes/shape-outside/values/shape-outside-ellipse-004.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html jquery/attributes.html fast/dom/Document/createElementNS-namespace-err.html imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html

Created attachment 277425 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

(In reply to comment #26) > Comment on attachment 277415 [details] > Made suggested DFGFixupPhase.cpp changes > > Attachment 277415 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/1225679 > > New failing tests: > imported/w3c/web-platform-tests/dom/ranges/Range-surroundContents.html > imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img- > element/srcset/parse-a-srcset-attribute.html > css3/shapes/shape-outside/values/shape-outside-inset-001.html > imported/w3c/web-platform-tests/dom/nodes/DOMImplementation-createDocument. > html > css3/shapes/shape-outside/values/shape-outside-circle-004.html > fast/dom/DOMImplementation/createDocument-namespace-err.html > imported/w3c/web-platform-tests/dom/nodes/Document-createElementNS.html > css3/shapes/shape-outside/values/shape-outside-ellipse-004.html > imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type- > change-state.html > jquery/attributes.html > fast/dom/Document/createElementNS-namespace-err.html > imported/w3c/web-platform-tests/dom/nodes/Document-characterSet- > normalization.html Fixed this ASSERT crash locally. It was due to this recent change: case StringReplace: case StringReplaceRegExp: { if (node->child2()->shouldSpeculateString()) { m_insertionSet.insertNode( m_indexInBlock, SpecNone, Check, node->origin, Edge(node->child2().node(), StringUse)); => fixEdge<StringUse>(node->child2()); The fixEdge<StringUse> will cause the ASSERT when we generate the code with the construct JSValueOperand regExp(this, node->child2()); as it assumes that there hasn't been any speculation on that edge. I changed this to JSValueOperand regExp(this, node->child2(), ManualOperandSpeculation); when child2().useKind() == StringUse.

Committed r200117: <http://trac.webkit.org/changeset/200117>

(In reply to comment #29) > Committed r200117: <http://trac.webkit.org/changeset/200117> There are 14 JSC test failures on the 32 bit bots since this change.

(In reply to comment #30) > (In reply to comment #29) > > Committed r200117: <http://trac.webkit.org/changeset/200117> > > There are 14 JSC test failures on the 32 bit bots since this change. r200113 was the cuplrit, not this one.

*** Bug 150812 has been marked as a duplicate of this bug. ***