Bug 71436 – DFG inlining breaks function.arguments[something] if the argument being retrieved was subjected to DFG's unboxing optimizations

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

[Filip Pizlo]

InlineCallFrame should know not only where to find arguments but also how to transmogrify the value in the register file into an appopriate JSValue.

[Filip Pizlo]

The fix appears to be perf-neutral.


Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc
"FixArguments2" at /Volumes/Data/pizlo/tertiary/OpenSource/WebKitBuild/Release/jsc

Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime()
function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in
milliseconds.

                                            TipOfTree             FixArguments2                                  
SunSpider:
   3d-cube                                7.5212+-0.2259    ?     7.5264+-0.2016       ?
   3d-morph                               7.5806+-0.1293    ?     7.6602+-0.1197       ? might be 1.0105x slower
   3d-raytrace                            7.7395+-0.3591    ?     7.7609+-0.2671       ?
   access-binary-trees                    1.7049+-0.1229          1.5558+-0.0472         might be 1.0958x faster
   access-fannkuch                        6.4364+-0.0762    ?     6.6471+-0.1966       ? might be 1.0327x slower
   access-nbody                           3.6906+-0.0683    ?     3.7790+-0.1119       ? might be 1.0240x slower
   access-nsieve                          2.6218+-0.0958          2.5735+-0.0536         might be 1.0188x faster
   bitops-3bit-bits-in-byte               1.3111+-0.0258          1.3077+-0.0201       
   bitops-bits-in-byte                    2.4357+-0.0576    ?     2.4644+-0.0748       ? might be 1.0118x slower
   bitops-bitwise-and                     3.3885+-0.0629    ?     3.4189+-0.1450       ?
   bitops-nsieve-bits                     5.5731+-0.1750          5.4807+-0.1525         might be 1.0169x faster
   controlflow-recursive                  2.1854+-0.0507    ^     2.0846+-0.0403       ^ definitely 1.0483x faster
   crypto-aes                             7.6902+-0.2532          7.4891+-0.2266         might be 1.0269x faster
   crypto-md5                             2.6997+-0.0774    ?     2.7296+-0.0899       ? might be 1.0111x slower
   crypto-sha1                            2.5243+-0.0595          2.4615+-0.0598         might be 1.0255x faster
   date-format-tofte                     10.4991+-0.2963         10.4769+-0.4194       
   date-format-xparb                      9.5655+-0.3331          9.1388+-0.1880         might be 1.0467x faster
   math-cordic                            6.4924+-0.1236    ?     6.5218+-0.1695       ?
   math-partial-sums                      7.6284+-0.1768          7.5994+-0.1429       
   math-spectral-norm                     2.5934+-0.0582    ?     2.6100+-0.0851       ?
   regexp-dna                            11.9187+-0.2538         11.5446+-0.2464         might be 1.0324x faster
   string-base64                          4.0977+-0.0684    ?     4.1413+-0.1647       ? might be 1.0106x slower
   string-fasta                           6.4471+-0.2620          6.4470+-0.2433       
   string-tagcloud                       11.9003+-0.3490         11.7710+-0.3049         might be 1.0110x faster
   string-unpack-code                    20.6661+-0.5599    ?    20.8308+-0.4553       ?
   string-validate-input                  5.5422+-0.2844          5.3479+-0.2877         might be 1.0363x faster

   <arithmetic> *                         6.2482+-0.0445          6.2065+-0.0422       
   <geometric>                            5.0304+-0.0402          4.9870+-0.0373       
   <harmonic>                             4.0181+-0.0461          3.9653+-0.0365         might be 1.0133x faster

                                            TipOfTree             FixArguments2                                  
V8:
   crypto                                73.0528+-0.3744    ?    73.9699+-0.7800       ? might be 1.0126x slower
   deltablue                            166.6788+-1.5937    ?   167.7910+-1.4707       ?
   earley-boyer                          90.6311+-0.5744         90.2339+-0.6199       
   raytrace                              62.4455+-0.5666    ?    63.4027+-0.5737       ? might be 1.0153x slower
   regexp                               106.2410+-0.7429        105.9297+-0.4786       
   richards                             126.4750+-1.2671        126.3324+-0.7869       
   splay                                 73.0375+-0.8011         72.7076+-1.3812       

   <arithmetic>                          99.7945+-0.3571    ?   100.0525+-0.3435       ?
   <geometric> *                         94.5590+-0.2962    ?    94.8452+-0.3748       ?
   <harmonic>                            90.0230+-0.2745    ?    90.3694+-0.4148       ?

                                            TipOfTree             FixArguments2                                  
Kraken:
   ai-astar                             490.2909+-3.3742    !   499.2071+-3.3754       ! definitely 1.0182x slower
   audio-beat-detection                 192.5149+-1.5999    ^   188.1374+-1.3947       ^ definitely 1.0233x faster
   audio-dft                            266.3441+-4.9703    ?   272.0965+-5.4826       ? might be 1.0216x slower
   audio-fft                            124.5641+-1.5011        124.4237+-0.7452       
   audio-oscillator                     253.3752+-2.1550        251.6580+-2.1351       
   imaging-darkroom                     302.2765+-4.9493        300.9969+-3.9633       
   imaging-desaturate                   225.7327+-1.1631        224.6029+-0.6902       
   imaging-gaussian-blur                553.2719+-3.7647        552.5799+-2.7106       
   json-parse-financial                  58.4904+-1.2073         57.5959+-0.3045         might be 1.0155x faster
   json-stringify-tinderbox              67.2634+-0.5878    ?    67.5680+-0.4717       ?
   stanford-crypto-aes                   98.0389+-0.6873         96.4402+-0.9744         might be 1.0166x faster
   stanford-crypto-ccm                  100.6559+-0.6126         99.8655+-0.6742       
   stanford-crypto-pbkdf2               196.0212+-1.5899    ^   192.4810+-0.9740       ^ definitely 1.0184x faster
   stanford-crypto-sha256-iterative      81.2195+-0.5315         80.4779+-0.8924       

   <arithmetic> *                       215.0042+-0.3776        214.8665+-0.5934       
   <geometric>                          171.1345+-0.4278        170.3530+-0.4277       
   <harmonic>                           137.1886+-0.5532    ^   136.2271+-0.3852       ^ definitely 1.0071x faster

                                            TipOfTree             FixArguments2                                  
All benchmarks:
   <arithmetic>                          82.3633+-0.1384         82.3376+-0.1803       
   <geometric>                           22.2647+-0.0986         22.1379+-0.0918       
   <harmonic>                             7.0671+-0.0787          6.9762+-0.0625         might be 1.0130x faster

                                            TipOfTree             FixArguments2                                  
Geomean of preferred means:
   <scaled-result>                       50.2685+-0.1402         50.1959+-0.1225

[Filip Pizlo]

Created attachment 113425 [details]
the patch

It's ready for review modulo that I'm going to wait with committing until the EWS is also happy, since this introduces some new headers.  Most of the build systems don't care so I might get lucky.

[Early Warning System Bot]

Comment on attachment 113425 [details]
the patch

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

[Gyuyoung Kim]

Comment on attachment 113425 [details]
the patch

Attachment 113425 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10257464

[Filip Pizlo]

Created attachment 113431 [details]
the patch

Trying to fix !ENABLE(DFG_JIT) build errors.

[Oliver Hunt]

Comment on attachment 113431 [details]
the patch

is it possible to break this into two patches -- one to separate the headers one to do the functional change?

[Filip Pizlo]

(In reply to comment #6)
> (From update of attachment 113431 [details])
> is it possible to break this into two patches -- one to separate the headers one to do the functional change?

See: https://bugs.webkit.org/show_bug.cgi?id=71439

[Filip Pizlo]

Created attachment 113438 [details]
the patch

[Oliver Hunt]

Comment on attachment 113438 [details]
the patch

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

r+, but i don't see any reason not to merge stackOffset and isCall

> Source/JavaScriptCore/bytecode/CodeOrigin.h:84
>      unsigned stackOffset;
> -    unsigned numArgumentsIncludingThis : 31;
> -    bool isCall : 1;
> +    bool isCall;

Can we merge stackOffset and isCall here?  I can't see us needing a 1gig stack offset...

[Filip Pizlo]

(In reply to comment #9)
> (From update of attachment 113438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113438&action=review
> 
> r+, but i don't see any reason not to merge stackOffset and isCall
> 
> > Source/JavaScriptCore/bytecode/CodeOrigin.h:84
> >      unsigned stackOffset;
> > -    unsigned numArgumentsIncludingThis : 31;
> > -    bool isCall : 1;
> > +    bool isCall;
> 
> Can we merge stackOffset and isCall here?  I can't see us needing a 1gig stack offset...

Fair point.  I missed that somehow.

[Filip Pizlo]

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

[Patrick R. Gansterer]

(In reply to comment #11)
> Landed in http://trac.webkit.org/changeset/99148

It broke the interpreter build, since MacroAssembler isn't defined in ValueRecovery.h. Can somebody have a look at it? I'm not sure about the best solution to keep the #ifdefs at a minimum. :-/

[Filip Pizlo]

(In reply to comment #12)
> (In reply to comment #11)
> > Landed in http://trac.webkit.org/changeset/99148
> 
> It broke the interpreter build, since MacroAssembler isn't defined in ValueRecovery.h. Can somebody have a look at it? I'm not sure about the best solution to keep the #ifdefs at a minimum. :-/

Ooops, sorry about that!  I should have seen that one coming.  The best fix is probably to make sure that MacroAssembler::RegisterID, or some variant thereof, is defined regardless of whether or not we actually have a valid MacroAssembler.

[Patrick R. Gansterer]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Landed in http://trac.webkit.org/changeset/99148
> > 
> > It broke the interpreter build, since MacroAssembler isn't defined in ValueRecovery.h. Can somebody have a look at it? I'm not sure about the best solution to keep the #ifdefs at a minimum. :-/
> 
> Ooops, sorry about that!
NP!

> I should have seen that one coming.
FYI: WinCE buildbot builds with interpreter.

> The best fix is probably to make sure that MacroAssembler::RegisterID, or some variant thereof, is defined regardless of whether or not we actually have a valid MacroAssembler.

Can you take care of this? Or tell me the best place (and name) for defining the (FP)RegisterID type?

[Filip Pizlo]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > Landed in http://trac.webkit.org/changeset/99148
> > > 
> > > It broke the interpreter build, since MacroAssembler isn't defined in ValueRecovery.h. Can somebody have a look at it? I'm not sure about the best solution to keep the #ifdefs at a minimum. :-/
> > 
> > Ooops, sorry about that!
> NP!
> 
> > I should have seen that one coming.
> FYI: WinCE buildbot builds with interpreter.
> 
> > The best fix is probably to make sure that MacroAssembler::RegisterID, or some variant thereof, is defined regardless of whether or not we actually have a valid MacroAssembler.
> 
> Can you take care of this? Or tell me the best place (and name) for defining the (FP)RegisterID type?

I'm doing it! :-)

See: https://bugs.webkit.org/show_bug.cgi?id=71498