Bug 154835 – Turn String.prototype.replace into an intrinsic

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

My plan for this involves having StringPrototype.cpp directly export the operations functions that the DFG/FTL will call.

To do that, it needs access to the declarations in JITOperations.h.  In fact it needs to include that in StringPrototype.h in order to properly declare the operations functions.

So, this depends on this: https://bugs.webkit.org/show_bug.cgi?id=154836

[Filip Pizlo]

This isn't showing up as a speed-up when I just implement it as a raw intrinsic.  But it ought to be a speed-up once the compiler can reason about what's going on.  I believe that for this to work I'll need to add a RegExpUse UseKind and a SpecRegExpObject speculated type.  Hence I need this: https://bugs.webkit.org/show_bug.cgi?id=154840

[Filip Pizlo]

Created attachment 272528 [details]
work in progress

[Filip Pizlo]

Created attachment 272539 [details]
more

[Filip Pizlo]

Created attachment 272544 [details]
slowly adding more optimizations

[Filip Pizlo]

Created attachment 272549 [details]
the patch

[WebKit Commit Bot]

Attachment 272549 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:57:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:61:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

I know how to fix the build - JITOperations.h must become Private.

[Joseph Pecoraro]

Comment on attachment 272549 [details]
the patch

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

> Source/JavaScriptCore/bytecode/SpeculatedType.h:76
> +static const SpeculatedType SpecDoublePureNaN      = 1u << 26; // It's definitely a NaN that is sae to tag (i.e. pure).

Could drive by fix this typo: "sae" => "safe"

[Filip Pizlo]

Created attachment 272578 [details]
the patch

[WebKit Commit Bot]

Attachment 272578 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:57:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/StringPrototype.h:61:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Comment on attachment 272578 [details]
the patch

r=me

[Filip Pizlo]

I'll wait to see what the string-replace test failure is.  It's strange that it crashes in debug.

[Build Bot]

Comment on attachment 272578 [details]
the patch

Attachment 272578 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/907370

New failing tests:
webgl/1.0.2/conformance/glsl/misc/shader-uniform-packing-restrictions.html
js/regress/string-replace.html

[Build Bot]

Created attachment 272587 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Filip Pizlo]

Totally found the issue causing debug crashes.  I forgot to add StringReplace to Node::hasHeapPrediction().

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/197408

[Ryan Haddad]

It looks like this change broke the CLoop build:
<https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3905>

[Filip Pizlo]

(In reply to comment #18)
> It looks like this change broke the CLoop build:
> <https://build.webkit.org/builders/
> Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3905>

Fixing.

[Filip Pizlo]

(In reply to comment #19)
> (In reply to comment #18)
> > It looks like this change broke the CLoop build:
> > <https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3905>
> 
> Fixing.

Should be fixed in http://trac.webkit.org/changeset/197411