Bug 166707 – Add a slice intrinsic to the DFG/FTL

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

[Saam Barati]

Slice is hot inside some kraken crypto tests. I'm going to investigate if there is any profit in making these intrinsics with fast allocation/memcpy paths.

[Saam Barati]

Created attachment 298062 [details]
WIP

It begins.

Preliminary tests are it's 5-6% faster on pbkdf2.

There is still a lot to do to make the code correct, and to make it spec compliant.

[Yusuke Suzuki]

Cool!

[Saam Barati]

(In reply to comment #2)
> Cool!

Yeah. If we're lucky we'll get 0.5-1% on Kraken overall.

My initial 5-6% number will probably decrease slightly since I'll need to add a couple branches to verify we're doing the right thing.

[Saam Barati]

Created attachment 298163 [details]
WIP

I think it's correct now w.r.t weird inputs to slice.
I still need to figure out how to quickly detect if we're constructing a subclass of Array and bail out if so.

[Saam Barati]

Created attachment 298220 [details]
WIP

Close to being done.

[Saam Barati]

Created attachment 298246 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3890:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1658:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 2 in 29 files


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

[Saam Barati]

Created attachment 298247 [details]
patch

rebased

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3890:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1658:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 2 in 29 files


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

[Build Bot]

Comment on attachment 298247 [details]
patch

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

New failing tests:
fast/workers/postMessage-missing-parameter.html
webgl/1.0.2/conformance/context/context-release-with-workers.html
fast/workers/worker-terminate-forever.html

[Build Bot]

Created attachment 298254 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

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

[Saam Barati]

(In reply to comment #11)
> Created attachment 298254 [details]
> Archive of layout-test-results from ews117 for mac-elcapitan
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

I'm building a debug webkit now to figure out why we're crashing during global object creation while setting up the proper watchpoints.

[Saam Barati]

(In reply to comment #12)
> (In reply to comment #11)
> > Created attachment 298254 [details]
> > Archive of layout-test-results from ews117 for mac-elcapitan
> > 
> > The attached test failures were seen while running run-webkit-tests on the
> > mac-debug-ews.
> > Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
> 
> I'm building a debug webkit now to figure out why we're crashing during
> global object creation while setting up the proper watchpoints.

It's very curious that the assertion being hit is that an exception is being thrown.

[Radar WebKit Bug Importer]

<rdar://problem/29913445>

[Saam Barati]

Created attachment 298255 [details]
patch (probably for landing)

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3890:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1656:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 2 in 27 files


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

[Saam Barati]

Created attachment 298256 [details]
patch for landing

(Fixed style issues).
The previously crashing tests are now passing for me locally. Will wait until EWS finishes before landing.

[WebKit Commit Bot]

Comment on attachment 298256 [details]
patch for landing

Clearing flags on attachment: 298256

Committed r210476: <http://trac.webkit.org/changeset/210476>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

So this is showing up as a 5% improvement on phidf2 on the bots.

It's also showing a 5% slowdown on AES on the bots. This is really weird because I can't reproduce this slowdown on aes at all locally. I'll try to reproduce it on another device.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 166859

[Saam Barati]

Created attachment 298441 [details]
patch

Just about done. I'm not showing a speedup on aes and pbkdf2 locally. I've made Slice be polymorphic over the input array's indexing type when it is Contiguous/Int32/Double.

I just need to rebase and revise the changelog as needed.

[Saam Barati]

Kraken results I'm seeing locally:

VMs tested:
"og" at /Volumes/Data/WK/c/OpenSource/WebKitBuild/Release/jsc (r210553)
"change" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r210553)

Collected 40 samples per benchmark/VM, with 40 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.

                                              og                      change                                      

ai-astar                                91.657+-0.948             91.154+-0.939         
audio-beat-detection                    35.801+-0.227             35.673+-0.231         
audio-dft                               94.211+-1.592             92.901+-0.595           might be 1.0141x faster
audio-fft                               27.581+-0.133      ?      27.679+-0.221         ?
audio-oscillator                        43.191+-0.340      ?      43.325+-0.378         ?
imaging-darkroom                        56.904+-0.403      ?      57.297+-1.189         ?
imaging-desaturate                      44.541+-1.555      ?      44.976+-1.562         ?
imaging-gaussian-blur                   59.351+-0.925      ?      59.730+-1.127         ?
json-parse-financial                    31.741+-0.276      ?      32.184+-0.396         ? might be 1.0139x slower
json-stringify-tinderbox                22.105+-0.452             21.763+-0.345           might be 1.0157x faster
stanford-crypto-aes                     35.369+-0.285      ^      34.852+-0.170         ^ definitely 1.0148x faster
stanford-crypto-ccm                     33.375+-0.747             32.865+-1.719           might be 1.0155x faster
stanford-crypto-pbkdf2                  91.430+-0.695      ^      86.424+-2.628         ^ definitely 1.0579x faster
stanford-crypto-sha256-iterative        30.389+-0.255             29.630+-0.845           might be 1.0256x faster

<arithmetic>                            49.832+-0.215      ^      49.318+-0.250         ^ definitely 1.0104x faster

[Saam Barati]

Created attachment 298543 [details]
patch

Putting this up for review.
My tests are revealing a bug that's already in ToT. (Probably concurrent GC related).
I'll hold off on landing until that's resolved since my tests trigger crashes with high frequency.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:133:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:134:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:135:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:137:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:138:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 7 in 29 files


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

[Caio Lima]

Comment on attachment 298543 [details]
patch

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

> JSTests/stress/array-slice-intrinsic.js:14
> +let tests = [

It should be helpful if ```shallowEq```  helper function logs what you were expecting and what is the current result, because test cases are going to be iterated in further steps and makes debug this test a little painful.

> JSTests/stress/array-slice-jettison-on-constructor-change.js:19
> +let tests = [

Ditto.

> JSTests/stress/array-slice-osr-exit-2.js:19
> +let tests = [

Ditto.

> JSTests/stress/array-slice-osr-exit.js:19
> +let tests = [

Ditto.

[Saam Barati]

Comment on attachment 298543 [details]
patch

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

>> JSTests/stress/array-slice-intrinsic.js:14
>> +let tests = [
> 
> It should be helpful if ```shallowEq```  helper function logs what you were expecting and what is the current result, because test cases are going to be iterated in further steps and makes debug this test a little painful.

SGTM, I'll have the assert function take an optional message.

[Saam Barati]

Created attachment 298748 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 298748 [details]
patch for landing

Clearing flags on attachment: 298748

Committed r210695: <http://trac.webkit.org/changeset/210695>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.