Bug 63505 - DFG JIT lacks array.length caching
: DFG JIT lacks array.length caching
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-06-27 20:02 PST by
Modified: 2011-06-28 20:19 PST (History)


Attachments
the patch (26.38 KB, patch)
2011-06-27 20:48 PST, Filip Pizlo
no flags Review Patch | Details | Formatted Diff | Diff
the patch (fix style) (26.37 KB, patch)
2011-06-27 20:59 PST, Filip Pizlo
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
the patch (fix build) (26.38 KB, patch)
2011-06-27 22:38 PST, Filip Pizlo
gustavo.noronha: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
the patch (fix build) (26.81 KB, patch)
2011-06-28 14:33 PST, Filip Pizlo
barraclough: review-
Review Patch | Details | Formatted Diff | Diff
the patch (18.83 KB, patch)
2011-06-28 18:55 PST, Filip Pizlo
barraclough: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-27 20:02:13 PST
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 From 2011-06-27 20:48:56 PST -------
Created an attachment (id=98848) [details]
the patch
------- Comment #2 From 2011-06-27 20:51:00 PST -------
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 From 2011-06-27 20:59:10 PST -------
Created an attachment (id=98849) [details]
the patch (fix style)
------- Comment #4 From 2011-06-27 21:43:16 PST -------
(From update of attachment 98849 [details])
Attachment 98849 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8952420
------- Comment #5 From 2011-06-27 22:38:17 PST -------
Created an attachment (id=98860) [details]
the patch (fix build)
------- Comment #6 From 2011-06-28 01:11:46 PST -------
(From update of attachment 98860 [details])
Attachment 98860 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8954277
------- Comment #7 From 2011-06-28 14:33:41 PST -------
Created an attachment (id=98973) [details]
the patch (fix build)
------- Comment #8 From 2011-06-28 15:45:48 PST -------
(From update of attachment 98973 [details])
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 From 2011-06-28 17:49:05 PST -------
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 From 2011-06-28 18:55:47 PST -------
Created an attachment (id=99025) [details]
the patch

Assembler modifications removed - they did not prove to produce any speed-up over the simpler approach.
------- Comment #11 From 2011-06-28 20:14:16 PST -------
(From update of attachment 99025 [details])
r+, I'll land.
------- Comment #12 From 2011-06-28 20:19:12 PST -------
Fixed in r89986