Bug 63605 – DFG JIT does not perform get_by_id self list caching

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

The main JSC JIT performs polymorphic get_by_id caching by creating a sequence of structure tests and accesses.  The DFG JIT does not do this, but should.

[Filip Pizlo]

Created attachment 99041 [details]
the patch

This patch leads to a speed-up (versus unpatched DFG) on both sunspider and v8.  It does, however, increase the size of StructureStubInfo by two words, due to the need to save data through two state transitions (unset -> get_by_id -> get_by_id self list).  This could be optimized somewhat, but I thought I'd save it for a later patch after we take care of other forms of get/put_by_id optimizations and see what their StructureStubInfo requirements are.


TEST                   COMPARISON            FROM                 TO             DETAILS

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

** TOTAL **:           1.023x as fast    188.1ms +/- 0.4%   183.9ms +/- 0.4%     significant

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

  3d:                  1.027x as fast     34.2ms +/- 0.8%    33.3ms +/- 0.7%     significant
    cube:              -                   9.0ms +/- 0.6%     9.0ms +/- 0.4% 
    morph:             1.036x as fast      7.5ms +/- 1.9%     7.3ms +/- 1.8%     significant
    raytrace:          1.036x as fast     17.6ms +/- 1.0%    17.0ms +/- 1.2%     significant

  access:              ??                 21.2ms +/- 0.8%    21.3ms +/- 0.8%     not conclusive: might be *1.004x as slow*
    binary-trees:      ??                  2.2ms +/- 5.4%     2.3ms +/- 5.7%     not conclusive: might be *1.027x as slow*
    fannkuch:          ??                 11.0ms +/- 0.9%    11.0ms +/- 0.8%     not conclusive: might be *1.002x as slow*
    nbody:             -                   6.0ms +/- 0.0%     6.0ms +/- 0.0% 
    nsieve:            -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 

  bitops:              -                  12.0ms +/- 0.0%    12.0ms +/- 0.0% 
    3bit-bits-in-byte: -                   1.0ms +/- 0.0%     1.0ms +/- 0.0% 
    bits-in-byte:      -                   3.0ms +/- 0.0%     3.0ms +/- 0.0% 
    bitwise-and:       -                   3.0ms +/- 0.0%     3.0ms +/- 0.0% 
    nsieve-bits:       -                   5.0ms +/- 0.0%     5.0ms +/- 0.0% 

  controlflow:         -                   1.7ms +/- 7.7%     1.7ms +/- 7.7% 
    recursive:         -                   1.7ms +/- 7.7%     1.7ms +/- 7.7% 

  crypto:              ??                 10.1ms +/- 1.9%    10.2ms +/- 2.1%     not conclusive: might be *1.012x as slow*
    aes:               -                   6.7ms +/- 1.9%     6.6ms +/- 2.1% 
    md5:               -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 
    sha1:              *1.159x as slow*    1.4ms +/- 10.1%     1.6ms +/- 8.8%     significant

  date:                1.125x as fast     26.1ms +/- 0.6%    23.2ms +/- 0.8%     significant
    format-tofte:      1.199x as fast     17.0ms +/- 0.7%    14.2ms +/- 0.8%     significant
    format-xparb:      -                   9.1ms +/- 1.0%     9.0ms +/- 1.3% 

  math:                -                  16.9ms +/- 0.5%    16.8ms +/- 0.9% 
    cordic:            1.028x as fast      5.9ms +/- 1.3%     5.8ms +/- 2.1%     significant
    partial-sums:      -                   7.0ms +/- 0.0%     7.0ms +/- 0.8% 
    spectral-norm:     ??                  4.0ms +/- 0.0%     4.0ms +/- 1.0%     not conclusive: might be *1.005x as slow*

  regexp:              -                  10.1ms +/- 0.8%    10.0ms +/- 0.4% 
    dna:               -                  10.1ms +/- 0.8%    10.0ms +/- 0.4% 

  string:              -                  55.7ms +/- 0.9%    55.3ms +/- 0.6% 
    base64:            -                   6.0ms +/- 0.9%     6.0ms +/- 0.0% 
    fasta:             -                   7.8ms +/- 1.9%     7.8ms +/- 1.7% 
    tagcloud:          -                  13.6ms +/- 1.5%    13.4ms +/- 1.1% 
    unpack-code:       -                  21.3ms +/- 1.2%    21.2ms +/- 0.9% 
    validate-input:    -                   7.0ms +/- 1.4%     6.9ms +/- 1.2% 



TEST              COMPARISON            FROM                 TO             DETAILS

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

** TOTAL **:      1.045x as fast    1236.5ms +/- 1.0%   1183.1ms +/- 0.6%     significant

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

  v8:             1.045x as fast    1236.5ms +/- 1.0%   1183.1ms +/- 0.6%     significant
    crypto:       ??                  91.5ms +/- 1.6%     92.3ms +/- 1.0%     not conclusive: might be *1.009x as slow*
    deltablue:    1.20x as fast      325.0ms +/- 1.3%    270.3ms +/- 1.2%     significant
    earley-boyer: *1.020x as slow*   147.1ms +/- 1.5%    150.1ms +/- 1.5%     significant
    raytrace:     ??                  99.8ms +/- 0.8%    100.4ms +/- 1.1%     not conclusive: might be *1.006x as slow*
    regexp:       -                  117.7ms +/- 1.3%    117.4ms +/- 1.9% 
    richards:     -                  248.2ms +/- 1.0%    247.0ms +/- 0.9% 
    splay:        -                  207.2ms +/- 2.5%    205.6ms +/- 1.4%

[WebKit Review Bot]

Attachment 99041 [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/DFGRepatch.cpp:204:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 7 files


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

[Filip Pizlo]

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

[Gavin Barraclough]

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

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

Looks great! r- to consider the one comment about memory usage.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:132
> +        

I think we should probably look at trying to wrap these fields in an #if ENABLE(DFG_JIT).  These are only used if the DFG_JIT is enabled, and it seems unfortunate to increase the size of this object on platforms like ARM.  (We may want to later look at whether we can claw this space back, but if we can make these fields ENABLE(DFG_JIT) I don't think we need to be worried about this for now).

Also, along with accessType & seen (above) the size of this set of fields is 9 bytes, which to align the struct below will probably pad out to 16 bytes.  Probably worth changing accessType & seen to be uint8_t's too, that way all 7 fields should probably pack out to 8 bytes.

[Filip Pizlo]

Created attachment 99115 [details]
the patch (fix review)

[WebKit Review Bot]

Comment on attachment 99115 [details]
the patch (fix review)

Clearing flags on attachment: 99115

Committed r90035: <http://trac.webkit.org/changeset/90035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.