Bug 166707 - Add a slice intrinsic to the DFG/FTL
Summary: Add a slice intrinsic to the DFG/FTL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 166859
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-04 17:17 PST by Saam Barati
Modified: 2017-01-12 20:04 PST (History)
13 users (show)

See Also:


Attachments
WIP (14.12 KB, patch)
2017-01-04 20:07 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (19.62 KB, patch)
2017-01-05 18:27 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (39.52 KB, patch)
2017-01-06 12:52 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (52.47 KB, patch)
2017-01-06 18:41 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (52.60 KB, patch)
2017-01-06 18:47 PST, Saam Barati
fpizlo: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (1.82 MB, application/zip)
2017-01-06 20:15 PST, Build Bot
no flags Details
patch (probably for landing) (51.12 KB, patch)
2017-01-06 21:00 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (51.12 KB, patch)
2017-01-06 21:18 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (28.11 KB, patch)
2017-01-09 20:28 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (75.88 KB, patch)
2017-01-10 18:37 PST, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff
patch for landing (75.96 KB, patch)
2017-01-12 19:20 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-01-04 17:17:57 PST
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.
Comment 1 Saam Barati 2017-01-04 20:07:16 PST
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.
Comment 2 Yusuke Suzuki 2017-01-04 20:09:29 PST
Cool!
Comment 3 Saam Barati 2017-01-04 20:12:21 PST
(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.
Comment 4 Saam Barati 2017-01-05 18:27:25 PST
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.
Comment 5 Saam Barati 2017-01-06 12:52:43 PST
Created attachment 298220 [details]
WIP

Close to being done.
Comment 6 Saam Barati 2017-01-06 18:41:34 PST
Created attachment 298246 [details]
patch
Comment 7 WebKit Commit Bot 2017-01-06 18:43:55 PST
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.
Comment 8 Saam Barati 2017-01-06 18:47:17 PST
Created attachment 298247 [details]
patch

rebased
Comment 9 WebKit Commit Bot 2017-01-06 18:48:54 PST
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.
Comment 10 Build Bot 2017-01-06 20:15:21 PST
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
Comment 11 Build Bot 2017-01-06 20:15:24 PST
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
Comment 12 Saam Barati 2017-01-06 20:29:37 PST
(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.
Comment 13 Saam Barati 2017-01-06 20:38:07 PST
(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.
Comment 14 Radar WebKit Bug Importer 2017-01-06 20:39:01 PST
<rdar://problem/29913445>
Comment 15 Saam Barati 2017-01-06 21:00:41 PST
Created attachment 298255 [details]
patch (probably for landing)
Comment 16 WebKit Commit Bot 2017-01-06 21:03:41 PST
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.
Comment 17 Saam Barati 2017-01-06 21:18:51 PST
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.
Comment 18 WebKit Commit Bot 2017-01-06 23:54:08 PST
Comment on attachment 298256 [details]
patch for landing

Clearing flags on attachment: 298256

Committed r210476: <http://trac.webkit.org/changeset/210476>
Comment 19 WebKit Commit Bot 2017-01-06 23:54:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Saam Barati 2017-01-07 09:54:01 PST
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.
Comment 21 WebKit Commit Bot 2017-01-09 13:07:59 PST
Re-opened since this is blocked by bug 166859
Comment 22 Saam Barati 2017-01-09 20:28:56 PST
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.
Comment 23 Saam Barati 2017-01-10 12:16:30 PST
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
Comment 24 Saam Barati 2017-01-10 18:37:32 PST
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.
Comment 25 WebKit Commit Bot 2017-01-10 18:39:35 PST
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.
Comment 26 Caio Lima 2017-01-11 00:20:11 PST
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.
Comment 27 Saam Barati 2017-01-11 14:38:15 PST
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.
Comment 28 Saam Barati 2017-01-12 19:20:09 PST
Created attachment 298748 [details]
patch for landing
Comment 29 WebKit Commit Bot 2017-01-12 20:04:52 PST
Comment on attachment 298748 [details]
patch for landing

Clearing flags on attachment: 298748

Committed r210695: <http://trac.webkit.org/changeset/210695>
Comment 30 WebKit Commit Bot 2017-01-12 20:04:57 PST
All reviewed patches have been landed.  Closing bug.