Bug 63505

Summary: DFG JIT lacks array.length caching
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, gustavo.noronha, gustavo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
none
the patch (fix style)
webkit-ews: commit-queue-
the patch (fix build)
gustavo.noronha: commit-queue-
the patch (fix build)
barraclough: review-
the patch barraclough: review+

Description Filip Pizlo 2011-06-27 20:02:13 PDT
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.
Comment 1 Filip Pizlo 2011-06-27 20:48:56 PDT
Created attachment 98848 [details]
the patch
Comment 2 WebKit Review Bot 2011-06-27 20:51:00 PDT
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.
Comment 3 Filip Pizlo 2011-06-27 20:59:10 PDT
Created attachment 98849 [details]
the patch (fix style)
Comment 4 Early Warning System Bot 2011-06-27 21:43:16 PDT
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
Comment 5 Filip Pizlo 2011-06-27 22:38:17 PDT
Created attachment 98860 [details]
the patch (fix build)
Comment 6 Collabora GTK+ EWS bot 2011-06-28 01:11:46 PDT
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
Comment 7 Filip Pizlo 2011-06-28 14:33:41 PDT
Created attachment 98973 [details]
the patch (fix build)
Comment 8 Gavin Barraclough 2011-06-28 15:45:48 PDT
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.
Comment 9 Filip Pizlo 2011-06-28 17:49:05 PDT
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*
Comment 10 Filip Pizlo 2011-06-28 18:55:47 PDT
Created attachment 99025 [details]
the patch

Assembler modifications removed - they did not prove to produce any speed-up over the simpler approach.
Comment 11 Gavin Barraclough 2011-06-28 20:14:16 PDT
Comment on attachment 99025 [details]
the patch

r+, I'll land.
Comment 12 Gavin Barraclough 2011-06-28 20:19:12 PDT
Fixed in r89986