Bug 156562 – [ES] Implement RegExp.prototype.@@replace and use it for String.prototype.replace

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

Section 21.2.5.8 of the ES6 specification describes RegExp.prototype [ @@replace ] behavior (https://tc39.github.io/ecma262/2016/#sec-regexp.prototype-@@replace).  This needs to be implemented.

[Radar WebKit Bug Importer]

<rdar://problem/25715263>

[Michael Saboff]

Created attachment 277339 [details]
Patch

[WebKit Commit Bot]

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.

[Michael Saboff]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Michael Saboff]

Comment on attachment 277339 [details]
Patch

Looks like Layout tests need a little work.

[Michael Saboff]

Created attachment 277394 [details]
Patch with Layout test fixes

[WebKit Commit Bot]

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.

[Michael Saboff]

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.

[Michael Saboff]

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

[Filip Pizlo]

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.

[Michael Saboff]

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

[Filip Pizlo]

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.

[Michael Saboff]

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

[WebKit Commit Bot]

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.

[Saam Barati]

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.

[Saam Barati]

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?

[Michael Saboff]

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

[Build Bot]

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

[Build Bot]

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

[Michael Saboff]

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

[Michael Saboff]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Joseph Pecoraro]

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