Bug 52167 – Remove unused isString() case in JSString::toPrimitiveString()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Xan Lopez]

JSString::toPrimitiveString() is called from two places in JSC, one of them very hot according to profiles. In both of them the code does something like:

v.isString() ? doSomething(v.asString()) : doSomething(v.toPrimitiveString());

so the method is never called with a string. However, this is the first thing that the code checks, so it seems we are wasting some cycles here. Removing it yields a significant perf win:


TEST                   COMPARISON            FROM                 TO             DETAILS

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

** TOTAL **:           1.007x as fast    264.5ms +/- 0.5%   262.7ms +/- 0.4%     significant

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

  3d:                  -                  37.7ms +/- 1.8%    37.7ms +/- 1.5% 
    cube:              ??                 14.0ms +/- 4.1%    14.3ms +/- 3.6%     not conclusive: might be *1.021x as slow*
    morph:             -                  10.8ms +/- 1.7%    10.8ms +/- 1.8% 
    raytrace:          1.022x as fast     12.9ms +/- 1.3%    12.6ms +/- 1.0%     significant

  access:              1.013x as fast     36.9ms +/- 0.7%    36.5ms +/- 0.5%     significant
    binary-trees:      -                   3.3ms +/- 2.7%     3.2ms +/- 2.4% 
    fannkuch:          1.010x as fast     18.3ms +/- 0.8%    18.1ms +/- 0.4%     significant
    nbody:             1.019x as fast     10.3ms +/- 0.9%    10.1ms +/- 0.6%     significant
    nsieve:            -                   5.0ms +/- 0.0%     5.0ms +/- 0.0% 

  bitops:              -                  23.0ms +/- 0.7%    22.8ms +/- 0.7% 
    3bit-bits-in-byte: ??                  3.0ms +/- 0.0%     3.0ms +/- 0.7%     not conclusive: might be *1.003x as slow*
    bits-in-byte:      -                   5.7ms +/- 1.7%     5.6ms +/- 1.8% 
    bitwise-and:       -                   5.3ms +/- 1.7%     5.2ms +/- 1.5% 
    nsieve-bits:       -                   9.0ms +/- 0.0%     9.0ms +/- 0.0% 

  controlflow:         -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 
    recursive:         -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 

  crypto:              1.012x as fast     15.3ms +/- 0.7%    15.1ms +/- 0.4%     significant
    aes:               1.019x as fast      9.2ms +/- 1.0%     9.1ms +/- 0.6%     significant
    md5:               -                   3.0ms +/- 0.9%     3.0ms +/- 0.7% 
    sha1:              -                   3.0ms +/- 0.0%     3.0ms +/- 0.0% 

  date:                -                  29.6ms +/- 0.6%    29.4ms +/- 0.5% 
    format-tofte:      -                  15.3ms +/- 0.6%    15.2ms +/- 0.5% 
    format-xparb:      -                  14.3ms +/- 0.8%    14.3ms +/- 0.7% 

  math:                -                  26.8ms +/- 0.9%    26.5ms +/- 0.8% 
    cordic:            1.015x as fast      8.2ms +/- 1.0%     8.1ms +/- 0.8%     significant
    partial-sums:      -                  14.3ms +/- 0.7%    14.2ms +/- 0.8% 
    spectral-norm:     -                   4.3ms +/- 2.0%     4.2ms +/- 1.8% 

  regexp:              ??                 11.9ms +/- 0.8%    12.0ms +/- 0.7%     not conclusive: might be *1.006x as slow*
    dna:               ??                 11.9ms +/- 0.8%    12.0ms +/- 0.7%     not conclusive: might be *1.006x as slow*

  string:              1.008x as fast     81.4ms +/- 0.4%    80.8ms +/- 0.4%     significant
    base64:            1.032x as fast      9.0ms +/- 0.4%     8.7ms +/- 1.0%     significant
    fasta:             -                  11.1ms +/- 0.5%    11.1ms +/- 0.5% 
    tagcloud:          -                  20.3ms +/- 0.5%    20.2ms +/- 0.4% 
    unpack-code:       -                  30.4ms +/- 0.5%    30.3ms +/- 0.5% 
    validate-input:    1.022x as fast     10.7ms +/- 0.8%    10.5ms +/- 0.9%     significant

If for some reason it's desired for the method to contemplate this case, I guess at minimum we should move the branch at the end. Patch coming.

[Xan Lopez]

Created attachment 78437 [details]
toprimitivestring.diff

[Gavin Barraclough]

Comment on attachment 78437 [details]
toprimitivestring.diff

Looks great! - ooi what's the perf win? - would be nice to mention in ChangeLog.

[Xan Lopez]

Comment on attachment 78437 [details]
toprimitivestring.diff

Landed in http://trac.webkit.org/changeset/75427

[Xan Lopez]

Closing bug.