| Summary: | Turn String.prototype.replace into an intrinsic | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||
| Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | commit-queue, ggaren, keith_miller, mark.lam, msaboff, ryanhaddad, saam | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Bug Depends on: | 154836, 154840 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Filip Pizlo
2016-02-29 13:48:33 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 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 Created attachment 272528 [details]
work in progress
Created attachment 272539 [details]
more
Created attachment 272544 [details]
slowly adding more optimizations
Created attachment 272549 [details]
the patch
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.
I know how to fix the build - JITOperations.h must become Private. 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" Created attachment 272578 [details]
the patch
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 on attachment 272578 [details]
the patch
r=me
I'll wait to see what the string-replace test failure is. It's strange that it crashes in debug. 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 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
Totally found the issue causing debug crashes. I forgot to add StringReplace to Node::hasHeapPrediction(). Landed in http://trac.webkit.org/changeset/197408 It looks like this change broke the CLoop build: <https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/3905> (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. (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 |