Bug 154835 - Turn String.prototype.replace into an intrinsic
Summary: Turn String.prototype.replace into an intrinsic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 154836 154840
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-29 13:48 PST by Filip Pizlo
Modified: 2016-03-01 13:40 PST (History)
7 users (show)

See Also:


Attachments
work in progress (14.34 KB, patch)
2016-02-29 16:16 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (23.98 KB, patch)
2016-02-29 19:26 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
slowly adding more optimizations (25.65 KB, patch)
2016-02-29 20:22 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (40.01 KB, patch)
2016-02-29 21:45 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (42.91 KB, patch)
2016-03-01 11:26 PST, Filip Pizlo
msaboff: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (971.32 KB, application/zip)
2016-03-01 12:37 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-02-29 13:48:33 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2016-02-29 13:50:31 PST
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
Comment 2 Filip Pizlo 2016-02-29 16:14:55 PST
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
Comment 3 Filip Pizlo 2016-02-29 16:16:13 PST
Created attachment 272528 [details]
work in progress
Comment 4 Filip Pizlo 2016-02-29 19:26:57 PST
Created attachment 272539 [details]
more
Comment 5 Filip Pizlo 2016-02-29 20:22:15 PST
Created attachment 272544 [details]
slowly adding more optimizations
Comment 6 Filip Pizlo 2016-02-29 21:45:14 PST
Created attachment 272549 [details]
the patch
Comment 7 WebKit Commit Bot 2016-02-29 21:47:25 PST
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.
Comment 8 Filip Pizlo 2016-02-29 22:47:36 PST
I know how to fix the build - JITOperations.h must become Private.
Comment 9 Joseph Pecoraro 2016-02-29 23:50:02 PST
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"
Comment 10 Filip Pizlo 2016-03-01 11:26:07 PST
Created attachment 272578 [details]
the patch
Comment 11 WebKit Commit Bot 2016-03-01 11:27:42 PST
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.
Comment 12 Michael Saboff 2016-03-01 12:14:51 PST
Comment on attachment 272578 [details]
the patch

r=me
Comment 13 Filip Pizlo 2016-03-01 12:17:09 PST
I'll wait to see what the string-replace test failure is.  It's strange that it crashes in debug.
Comment 14 Build Bot 2016-03-01 12:37:25 PST
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
Comment 15 Build Bot 2016-03-01 12:37:29 PST
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
Comment 16 Filip Pizlo 2016-03-01 13:14:12 PST
Totally found the issue causing debug crashes.  I forgot to add StringReplace to Node::hasHeapPrediction().
Comment 17 Filip Pizlo 2016-03-01 13:18:59 PST
Landed in http://trac.webkit.org/changeset/197408
Comment 18 Ryan Haddad 2016-03-01 13:36:16 PST
It looks like this change broke the CLoop build:
<https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3905>
Comment 19 Filip Pizlo 2016-03-01 13:37:13 PST
(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.
Comment 20 Filip Pizlo 2016-03-01 13:40:50 PST
(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