Bug 154664 – [JSC] Temporal Dead Zone checks on "this" are eliminated when doing OSR Entry to FTL

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] Temporal Dead Zone checks on "this" are eliminated when doing OSR Entry to FTL

[Benjamin Poulain]

Created attachment 272168 [details]
Patch

[Benjamin Poulain]

I went this way because that was easy :)

I am not familiar with the responsibilities of VariableAccessData. Would it make more sense to have a flush format FlushedJSValueOrEmpty and a flag on VariableAccessData when empty is an option?

[Filip Pizlo]

Comment on attachment 272168 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        that this may have been set to null by a previously executed block.

"null" or the empty value?  Two different things and I think you mean empty value.

> Source/JavaScriptCore/ChangeLog:14
> +        In this patch, I added a simple workaround: A new node CanBeEmptyHint

This is really ugly.  Can we just make ExtractOSREntryLocal have a flag or something that indicates that it might return the empty value?

[Filip Pizlo]

Comment on attachment 272168 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:166
> +            changed |= variableAccessData->predict(SpecBytecodeTop);

This is extremely bad.  You're turning off all type inference for 'this'!

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 272168 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272168&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        that this may have been set to null by a previously executed block.
> 
> "null" or the empty value?  Two different things and I think you mean empty
> value.
> 
> > Source/JavaScriptCore/ChangeLog:14
> > +        In this patch, I added a simple workaround: A new node CanBeEmptyHint
> 
> This is really ugly.  Can we just make ExtractOSREntryLocal have a flag or
> something that indicates that it might return the empty value?

Right - you can't do this because there is no ExtractOSREntryLocal.

I think that this problem has nothing to do with prediction propagation.  Why isn't this an AbstractInterpreter change that makes sure that AI knows that 'this' could be the empty value?

[Benjamin Poulain]

Created attachment 272250 [details]
Patch

[Benjamin Poulain]

I was convinced this would be a perf regression but it is completely neutral:


                                                  Conf#1                    Conf#2                                      
Octane:
   encrypt                                   0.26922+-0.00229          0.26788+-0.00274       
   decrypt                                   4.98148+-0.02076    ?     4.98607+-0.01449       ?
   deltablue                        x2       0.22267+-0.00673          0.22164+-0.00638       
   earley                                    0.51225+-0.00240          0.50874+-0.00208       
   boyer                                     8.34783+-0.03074          8.34247+-0.03670       
   navier-stokes                    x2       6.47051+-0.00903    ?     6.47304+-0.00743       ?
   raytrace                         x2       1.51409+-0.01446          1.51369+-0.01413       
   richards                         x2       0.14226+-0.00122          0.14169+-0.00130       
   splay                            x2       0.53940+-0.00313          0.53757+-0.00336       
   regexp                           x2      39.26591+-0.32509    ?    39.48002+-0.62183       ?
   pdfjs                            x2      61.29357+-0.26341         61.11385+-0.19545       
   mandreel                         x2      70.67145+-0.65415         70.09903+-0.42861       
   gbemu                            x2      46.61627+-1.11143         46.12028+-0.78242         might be 1.0108x faster
   closure                                   0.93911+-0.00197          0.93727+-0.00265       
   jquery                                   11.91794+-0.07194    ?    12.01694+-0.16999       ?
   box2d                            x2      16.65716+-0.12012         16.64404+-0.08303       
   zlib                             x2     582.30538+-2.23464    ?   582.94125+-1.60820       ?
   typescript                       x2    1115.89500+-3.30588    ?  1119.10688+-6.57086       ?

   <geometric>                               8.65685+-0.03625          8.64086+-0.02104         might be 1.0019x faster

                                                  Conf#1                    Conf#2                                      
Kraken:
   ai-astar                                  151.121+-3.629      ?     151.314+-1.159         ?
   audio-beat-detection                       71.331+-0.263      ?      71.519+-0.323         ?
   audio-dft                                 127.270+-1.026      ?     127.357+-0.840         ?
   audio-fft                                  57.120+-0.125             57.041+-0.117         
   audio-oscillator                           85.345+-0.151      ?      85.349+-0.158         ?
   imaging-darkroom                           95.645+-0.178             95.468+-0.133         
   imaging-desaturate                         92.703+-0.271             92.581+-0.333         
   imaging-gaussian-blur                     132.539+-2.278            130.049+-3.360           might be 1.0191x faster
   json-parse-financial                       66.548+-0.428      ?      66.781+-0.349         ?
   json-stringify-tinderbox                   40.954+-0.088      ?      41.128+-0.320         ?
   stanford-crypto-aes                        63.024+-0.348             62.991+-0.614         
   stanford-crypto-ccm                        60.982+-1.559      ?      61.174+-1.411         ?
   stanford-crypto-pbkdf2                    153.449+-1.405            152.747+-1.040         
   stanford-crypto-sha256-iterative           59.976+-0.413      ?      60.595+-0.440         ? might be 1.0103x slower

   <arithmetic>                               89.858+-0.345             89.721+-0.259           might be 1.0015x faster

                                                  Conf#1                    Conf#2                                      
AsmBench:
   bigfib.cpp                               661.8311+-9.5454          657.4292+-6.3489        
   cray.c                                   577.4569+-2.2899     ?    580.9863+-2.2050        ?
   dry.c                                    675.2116+-17.1696         674.9946+-7.3268        
   FloatMM.c                                907.1338+-8.6865          897.4068+-8.5989          might be 1.0108x faster
   gcc-loops.cpp                           6307.8684+-6.3469     ?   6316.4279+-9.5400        ?
   n-body.c                                1583.8569+-3.1433         1582.5694+-2.4083        
   Quicksort.c                              589.0678+-1.4176     ?    589.9603+-1.7609        ?
   stepanov_container.cpp                  4538.2725+-32.5647    ?   4548.8271+-27.7810       ?
   Towers.c                                 386.0708+-0.3375     ?    386.7750+-0.8667        ?

   <geometric>                             1102.7601+-2.8050         1102.1545+-1.5262          might be 1.0005x faster

                                                  Conf#1                    Conf#2                                      
Geomean of preferred means:
   <scaled-result>                           95.0157+-0.2075           94.8919+-0.0949          might be 1.0013x faster

[WebKit Commit Bot]

Comment on attachment 272250 [details]
Patch

Clearing flags on attachment: 272250

Committed r197155: <http://trac.webkit.org/changeset/197155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.