79700 – Old JIT's style of JSVALUE64 strict equality is subtly wrong

RESOLVED FIXED 79700

Old JIT's style of JSVALUE64 strict equality is subtly wrong

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

Summary Old JIT's style of JSVALUE64 strict equality is subtly wrong

Filip Pizlo

Reported 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.

Attachments
the patch (6.86 KB, patch) 2012-02-27 15:48 PST, Filip Pizlo	oliver: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 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

Filip Pizlo

Comment 2 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

Filip Pizlo

Comment 3 2012-02-27 15:48:29 PST

Created attachment 129117 [details] the patch

Oliver Hunt

Comment 4 2012-02-27 15:54:01 PST

Comment on attachment 129117 [details] the patch r=me, ASSERT all the things!!

Geoffrey Garen

Comment 5 2012-02-27 16:07:19 PST

> ASSERT all the things!! Gödel's incompleteness theorem disproves.

Geoffrey Garen

Comment 6 2012-02-27 16:08:20 PST

Can you write a regression test for this?

Filip Pizlo

Comment 7 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.

Filip Pizlo

Comment 8 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.

Filip Pizlo

Comment 9 2012-02-27 16:38:15 PST

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component JavaScriptCore

Assignee

Nobody

Reported

2012-02-27 14:11 PST

Modified

2012-02-27 16:38 PST History

CC List

1 user Show

URL

Keywords

Depends on

Blocks