Bug 63505 – DFG JIT lacks array.length caching

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

[Filip Pizlo]

The main JSC JIT has caching optimizations for get_by_id for array length.  The DFG JIT lacks these optimizations, and instead has to use a slow path C call for all array length accesses.

[Filip Pizlo]

Created attachment 98848 [details]
the patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGRegisterBank.h:83:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:104:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:114:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1577:  Missing spaces around <<  [whitespace/operators] [3]
Source/JavaScriptCore/assembler/X86Assembler.h:1584:  Missing spaces around >>  [whitespace/operators] [3]
Source/JavaScriptCore/assembler/X86Assembler.h:1588:  Missing spaces around <<  [whitespace/operators] [3]
Total errors found: 8 in 16 files


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

[Filip Pizlo]

Created attachment 98849 [details]
the patch (fix style)

[Early Warning System Bot]

Comment on attachment 98849 [details]
the patch (fix style)

Attachment 98849 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8952420

[Filip Pizlo]

Created attachment 98860 [details]
the patch (fix build)

[Collabora GTK+ EWS bot]

Comment on attachment 98860 [details]
the patch (fix build)

Attachment 98860 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8954277

[Filip Pizlo]

Created attachment 98973 [details]
the patch (fix build)

[Gavin Barraclough]

Comment on attachment 98973 [details]
the patch (fix build)

Hey Filip, 2 issues.

First, I'd like like to keep the JIT implementation separate from the C helper functions, so the code that performs JIT generation shouldn't be in DFGOperations - that's really what the file DFGRepatch is for.  (The stub generation could even be in a new DFGStubs file if we wished, but DFGRepatch seems right for patching the instruction stream).  Please move the new code in tryCacheGetByID out into a separate function.

Second, this patch introduces a bunch of new code to allow us to overwrite the instruction stream, which is a little messy.  I'm okay with us doing this provided that there is a benefit to doing so, but it seems like a bad idea to introduce this complexity if it's not a performance win, and I think you need to prove this.  I think you need to gather performance numbers with this patch, and I think you need to also roll a version of this optimization that just patches the offset of the branch instruction (as the old JIT does), and attach a performance comparison (you can get this by running sun spider in both forms, cat'ing the results to files, then using the sunspider-compare-results script passing the two results files as arguments).

Hope this all sounds reasonable.

[Filip Pizlo]

Array length caching is mainly for v8, not sunspider, and is a win (see attached).  The replaceWithJump portion is not a win on either v8 or sunspider, so I've removed it.


TEST              COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:      1.038x as fast    1248.1ms +/- 0.5%   1202.2ms +/- 0.3%     significant

=============================================================================

  v8:             1.038x as fast    1248.1ms +/- 0.5%   1202.2ms +/- 0.3%     significant
    crypto:       -                   92.8ms +/- 0.8%     92.3ms +/- 0.4% 
    deltablue:    1.148x as fast     329.7ms +/- 0.4%    287.1ms +/- 0.5%     significant
    earley-boyer: -                  149.7ms +/- 1.3%    147.6ms +/- 1.0% 
    raytrace:     -                  102.8ms +/- 3.6%    101.3ms +/- 1.0% 
    regexp:       -                  117.5ms +/- 0.6%    117.2ms +/- 0.4% 
    richards:     ??                 245.3ms +/- 0.9%    245.7ms +/- 0.7%     not conclusive: might be *1.002x as slow*
    splay:        ??                 210.3ms +/- 0.7%    211.0ms +/- 0.8%     not conclusive: might be *1.003x as slow*

[Filip Pizlo]

Created attachment 99025 [details]
the patch

Assembler modifications removed - they did not prove to produce any speed-up over the simpler approach.

[Gavin Barraclough]

Comment on attachment 99025 [details]
the patch

r+, I'll land.

[Gavin Barraclough]

Fixed in r89986