Bug 79700 - Old JIT's style of JSVALUE64 strict equality is subtly wrong
Summary: Old JIT's style of JSVALUE64 strict equality is subtly wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-27 14:11 PST by Filip Pizlo
Modified: 2012-02-27 16:38 PST (History)
1 user (show)

See Also:


Attachments
the patch (6.86 KB, patch)
2012-02-27 15:48 PST, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-02-27 14:11:06 PST
It tries to rule out either of the inputs being a double by or'ing the two values together and testing against TagTypeNumber.  But that is guaranteed to be false for integers as well.

It then does a 32-bit comparison, even though it might be comparing cells to non-cells.

This affects both the old JIT and the DFG's non-speculative strict equality comparison, but only for JSVALUE64.
Comment 1 Filip Pizlo 2012-02-27 14:11:32 PST
Effect of fixing this bug on the old JIT alone:


Benchmark report for SunSpider, V8, and Kraken on bigmac (MacPro5,1).

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/secondary/OpenSource/WebKitBuild/Release/jsc (r109012)
"FixStrictEq" at /Volumes/Data/pizlo/tertiary/OpenSource/WebKitBuild/Release/jsc (r109012)

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                FixStrictEq                                    
SunSpider:
   3d-cube                                 7.8731+-0.0866            7.7743+-0.0273          might be 1.0127x faster
   3d-morph                                9.0184+-0.0711     ?      9.0695+-0.0844        ?
   3d-raytrace                             8.8989+-0.0353     ?      8.9743+-0.0489        ?
   access-binary-trees                     2.1351+-0.0202     ?      2.1829+-0.0302        ? might be 1.0224x slower
   access-fannkuch                        12.8129+-0.0538     ?     12.8549+-0.0613        ?
   access-nbody                            7.5549+-0.0149     ?      7.5795+-0.0325        ?
   access-nsieve                           4.0846+-0.0496            4.0702+-0.0296        
   bitops-3bit-bits-in-byte                2.5402+-0.0143     ?      2.5597+-0.0254        ?
   bitops-bits-in-byte                     6.0092+-0.0462     ?      6.0612+-0.0879        ?
   bitops-bitwise-and                      4.1884+-0.0428            4.1626+-0.0284        
   bitops-nsieve-bits                      5.8416+-0.0245            5.8223+-0.0116        
   controlflow-recursive                   2.1768+-0.0101            2.1765+-0.0107        
   crypto-aes                              6.7658+-0.0526     ?      6.7666+-0.0462        ?
   crypto-md5                              3.0531+-0.0219     ?      3.0901+-0.0252        ? might be 1.0121x slower
   crypto-sha1                             2.5216+-0.0110     ?      2.5669+-0.0407        ? might be 1.0180x slower
   date-format-tofte                      11.5518+-0.0745     ?     11.6777+-0.0880        ? might be 1.0109x slower
   date-format-xparb                      10.0422+-0.0722           10.0357+-0.4060        
   math-cordic                             7.3075+-0.0304     ?      7.3452+-0.0550        ?
   math-partial-sums                      10.4582+-0.0430           10.4473+-0.0368        
   math-spectral-norm                      4.4460+-0.0106            4.4335+-0.0117        
   regexp-dna                              8.9352+-0.0591     !      9.1242+-0.1154        ! definitely 1.0212x slower
   string-base64                           4.4846+-0.0407            4.4737+-0.0360        
   string-fasta                            7.4731+-0.0683     ?      7.5027+-0.0554        ?
   string-tagcloud                        12.9983+-0.0806           12.9008+-0.0748        
   string-unpack-code                     20.5740+-0.1799     ?     20.7335+-0.2388        ?
   string-validate-input                   5.7883+-0.0450     ?      5.7972+-0.0675        ?

   <arithmetic> *                          7.2898+-0.0150     ?      7.3147+-0.0276        ? might be 1.0034x slower
   <geometric>                             6.2071+-0.0124     ?      6.2302+-0.0186        ? might be 1.0037x slower
   <harmonic>                              5.2143+-0.0125     ?      5.2415+-0.0167        ? might be 1.0052x slower

                                             TipOfTree                FixStrictEq                                    
V8:
   crypto                                200.5909+-0.8850          200.5259+-2.1154        
   deltablue                             273.9431+-1.0710     ?    275.8511+-2.2189        ?
   earley-boyer                          122.0667+-0.4705     !    125.0908+-0.9036        ! definitely 1.0248x slower
   raytrace                               72.4627+-1.3499           70.5994+-0.7281          might be 1.0264x faster
   regexp                                100.1878+-0.2697     ?    100.2717+-0.5699        ?
   richards                              273.5731+-1.6529          271.8100+-2.5250        
   splay                                  78.6417+-0.3525     ?     78.8001+-0.3159        ?

   <arithmetic>                          160.2094+-0.3694     ?    160.4213+-0.8186        ? might be 1.0013x slower
   <geometric> *                         139.8682+-0.4297     ?    139.8975+-0.5637        ? might be 1.0002x slower
   <harmonic>                            122.8290+-0.5907          122.5509+-0.4585          might be 1.0023x faster

                                             TipOfTree                FixStrictEq                                    
Kraken:
   ai-astar                              2066.426+-23.896     ?    2082.081+-32.564        ?
   audio-beat-detection                   565.301+-8.300            559.745+-2.729         
   audio-dft                              454.652+-1.785      !     457.614+-0.894         ! definitely 1.0065x slower
   audio-fft                              427.331+-1.656      !     438.617+-5.425         ! definitely 1.0264x slower
   audio-oscillator                       408.272+-8.005            403.664+-4.012           might be 1.0114x faster
   imaging-darkroom                       589.170+-13.623           581.885+-9.494           might be 1.0125x faster
   imaging-desaturate                     620.326+-2.433      ?     620.356+-2.651         ?
   imaging-gaussian-blur                 2161.544+-6.454           2160.878+-3.860         
   json-parse-financial                    63.443+-0.335             63.031+-0.349         
   json-stringify-tinderbox                76.483+-0.301      ?      76.857+-0.358         ?
   stanford-crypto-aes                    150.218+-0.553      ^     136.480+-0.459         ^ definitely 1.1007x faster
   stanford-crypto-ccm                    111.246+-0.449      ^     107.975+-0.204         ^ definitely 1.0303x faster
   stanford-crypto-pbkdf2                 389.989+-0.585      ?     392.650+-3.546         ?
   stanford-crypto-sha256-iterative       153.509+-0.718      ^     148.666+-0.397         ^ definitely 1.0326x faster

   <arithmetic> *                         588.422+-1.960            587.893+-2.361           might be 1.0009x faster
   <geometric>                            346.167+-0.806      ^     342.577+-0.834         ^ definitely 1.0105x faster
   <harmonic>                             210.267+-0.541      ^     206.656+-0.469         ^ definitely 1.0175x faster

                                             TipOfTree                FixStrictEq                                    
All benchmarks:
   <arithmetic>                          203.1683+-0.5834          203.0560+-0.7756          might be 1.0006x faster
   <geometric>                            32.7020+-0.0484           32.6687+-0.0813          might be 1.0010x faster
   <harmonic>                              9.1979+-0.0216     ?      9.2423+-0.0290        ? might be 1.0048x slower

                                             TipOfTree                FixStrictEq                                    
Geomean of preferred means:
   <scaled-result>                        39.1475+-0.0646     ?     39.1831+-0.1085        ? might be 1.0009x slower
Comment 2 Filip Pizlo 2012-02-27 14:27:55 PST
Performance with DFG.



Benchmark report for SunSpider, V8, Kraken, and JSRegress on bigmac (MacPro5,1).

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/secondary/OpenSource/WebKitBuild/Release/jsc (r109012)
"FixStrictEq" at /Volumes/Data/pizlo/tertiary/OpenSource/WebKitBuild/Release/jsc (r109012)

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                FixStrictEq                                    
SunSpider:
   3d-cube                                 5.7878+-0.0292            5.7798+-0.0334        
   3d-morph                                9.6428+-0.2639     ?      9.7994+-0.1624        ? might be 1.0162x slower
   3d-raytrace                             7.7233+-0.0619            7.7181+-0.0454        
   access-binary-trees                     1.6780+-0.0058     ?      1.6786+-0.0080        ?
   access-fannkuch                         7.4875+-0.0459            7.4527+-0.0250        
   access-nbody                            3.8721+-0.0135            3.8532+-0.0080        
   access-nsieve                           3.5310+-0.0456     ?      3.5326+-0.0457        ?
   bitops-3bit-bits-in-byte                1.2949+-0.0083            1.2891+-0.0058        
   bitops-bits-in-byte                     5.2439+-0.0086     ?      5.2599+-0.0159        ?
   bitops-bitwise-and                      3.3031+-0.0067     ?      3.3098+-0.0138        ?
   bitops-nsieve-bits                      3.3457+-0.0280            3.3425+-0.0147        
   controlflow-recursive                   2.3435+-0.0114            2.3390+-0.0133        
   crypto-aes                              7.5679+-0.0737     ?      7.6165+-0.1179        ?
   crypto-md5                              2.8711+-0.0208            2.8600+-0.0276        
   crypto-sha1                             2.4318+-0.0306     ?      2.4327+-0.0248        ?
   date-format-tofte                      10.9612+-0.1059           10.8825+-0.1165        
   date-format-xparb                      10.3301+-0.1790     ^      9.9954+-0.1007        ^ definitely 1.0335x faster
   math-cordic                             7.5361+-0.0745            7.4621+-0.0375        
   math-partial-sums                      10.5408+-0.0211     ?     10.5829+-0.0433        ?
   math-spectral-norm                      2.6736+-0.0070     ?      2.6779+-0.0069        ?
   regexp-dna                              9.0073+-0.0342            8.9869+-0.1078        
   string-base64                           4.3977+-0.0246     ?      4.4235+-0.0642        ?
   string-fasta                            7.3184+-0.0615            7.2865+-0.0309        
   string-tagcloud                        13.0326+-0.1270           12.9417+-0.0838        
   string-unpack-code                     22.0234+-0.1559     ?     22.1035+-0.1600        ?
   string-validate-input                   6.4552+-0.0558            6.4391+-0.0843        

   <arithmetic> *                          6.6308+-0.0215            6.6172+-0.0235          might be 1.0021x faster
   <geometric>                             5.3550+-0.0157            5.3458+-0.0169          might be 1.0017x faster
   <harmonic>                              4.2606+-0.0134            4.2547+-0.0120          might be 1.0014x faster

                                             TipOfTree                FixStrictEq                                    
V8:
   crypto                                 75.3908+-0.1855     ?     75.4741+-0.3787        ?
   deltablue                             159.7924+-1.0516          158.4659+-2.2612        
   earley-boyer                          100.4762+-0.4308          100.4021+-0.5263        
   raytrace                               51.4556+-0.3124           51.3541+-0.2857        
   regexp                                102.3794+-0.5036     ?    102.6521+-0.4021        ?
   richards                              144.6157+-0.8815     ?    144.9124+-0.9666        ?
   splay                                  60.4189+-0.3177           59.7649+-0.3960          might be 1.0109x faster

   <arithmetic>                           99.2184+-0.2541           99.0037+-0.3796          might be 1.0022x faster
   <geometric> *                          91.9812+-0.2079           91.7671+-0.2637          might be 1.0023x faster
   <harmonic>                             85.2133+-0.1944           84.9778+-0.2067          might be 1.0028x faster

                                             TipOfTree                FixStrictEq                                    
Kraken:
   ai-astar                               819.774+-11.268     ?     831.364+-2.321         ? might be 1.0141x slower
   audio-beat-detection                   190.824+-0.362      ?     191.439+-0.899         ?
   audio-dft                              288.218+-0.989            285.179+-2.098           might be 1.0107x faster
   audio-fft                              116.894+-0.199            116.818+-0.154         
   audio-oscillator                       315.429+-7.329            310.281+-1.766           might be 1.0166x faster
   imaging-darkroom                       293.341+-7.574      ?     294.922+-7.831         ?
   imaging-desaturate                     237.735+-0.130      !     238.341+-0.433         ! definitely 1.0025x slower
   imaging-gaussian-blur                  456.249+-0.534      ?     456.279+-0.429         ?
   json-parse-financial                    63.999+-0.244             63.777+-0.273         
   json-stringify-tinderbox                78.067+-0.504             77.594+-0.302         
   stanford-crypto-aes                    102.854+-0.563            102.481+-0.507         
   stanford-crypto-ccm                    100.917+-0.496      ^      99.741+-0.531         ^ definitely 1.0118x faster
   stanford-crypto-pbkdf2                 199.995+-0.505      ?     201.674+-1.434         ?
   stanford-crypto-sha256-iterative        91.106+-0.346             91.031+-0.209         

   <arithmetic> *                         239.672+-1.264      ?     240.066+-0.607         ? might be 1.0016x slower
   <geometric>                            183.718+-0.601            183.465+-0.425           might be 1.0014x faster
   <harmonic>                             146.571+-0.329            146.141+-0.270           might be 1.0029x faster
Comment 3 Filip Pizlo 2012-02-27 15:48:29 PST
Created attachment 129117 [details]
the patch
Comment 4 Oliver Hunt 2012-02-27 15:54:01 PST
Comment on attachment 129117 [details]
the patch

r=me, ASSERT all the things!!
Comment 5 Geoffrey Garen 2012-02-27 16:07:19 PST
> ASSERT all the things!!

Gödel's incompleteness theorem disproves.
Comment 6 Geoffrey Garen 2012-02-27 16:08:20 PST
Can you write a regression test for this?
Comment 7 Filip Pizlo 2012-02-27 16:18:07 PST
(In reply to comment #6)
> Can you write a regression test for this?

There was not behavioral regression as it turns out.
Comment 8 Filip Pizlo 2012-02-27 16:18:38 PST
(In reply to comment #7)
> (In reply to comment #6)
> > Can you write a regression test for this?
> 
> There was not behavioral regression as it turns out.

I fail at grammar.  There was no behavioral regression.  There's nothing we can test other than performance.

And Oliver's assertions.
Comment 9 Filip Pizlo 2012-02-27 16:38:15 PST
Landed in http://trac.webkit.org/changeset/109040