Bug 154664

Summary: [JSC] Temporal Dead Zone checks on "this" are eliminated when doing OSR Entry to FTL
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Benjamin Poulain 2016-02-24 19:02:00 PST
[JSC] Temporal Dead Zone checks on "this" are eliminated when doing OSR Entry to FTL
Comment 1 Benjamin Poulain 2016-02-24 19:06:26 PST
Created attachment 272168 [details]
Patch
Comment 2 Benjamin Poulain 2016-02-24 19:11:18 PST
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?
Comment 3 Filip Pizlo 2016-02-25 14:13:29 PST
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?
Comment 4 Filip Pizlo 2016-02-25 14:17:48 PST
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'!
Comment 5 Filip Pizlo 2016-02-25 14:19:02 PST
(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?
Comment 6 Benjamin Poulain 2016-02-25 15:22:40 PST
Created attachment 272250 [details]
Patch
Comment 7 Benjamin Poulain 2016-02-25 15:23:19 PST
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
Comment 8 WebKit Commit Bot 2016-02-25 18:04:37 PST
Comment on attachment 272250 [details]
Patch

Clearing flags on attachment: 272250

Committed r197155: <http://trac.webkit.org/changeset/197155>
Comment 9 WebKit Commit Bot 2016-02-25 18:04:41 PST
All reviewed patches have been landed.  Closing bug.