69716 – DFG should not always speculate that a ByVal access has an integer index

RESOLVED FIXED Bug 69716

DFG should not always speculate that a ByVal access has an integer index

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

Summary DFG should not always speculate that a ByVal access has an integer index

Filip Pizlo

Reported 2011-10-09 00:29:35 PDT

Patch forthcoming.

Attachments
the patch (20.38 KB, patch) 2011-10-09 01:33 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (20.37 KB, patch) 2011-10-09 01:37 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - another attempt to make stylebot happy (20.37 KB, patch) 2011-10-09 01:47 PDT, Filip Pizlo	oliver: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2011-10-09 00:58:12 PDT

This is great for SunSpider. Benchmark report for SunSpider, V8, and Kraken. VMs tested: "TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc "ByValNotInt" at /Volumes/Data/pizlo/tertiary/OpenSource/WebKitBuild/Release/jsc Collected 60 samples per benchmark/VM, with 20 VM invocations per benchmark. 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 ByValNotInt SunSpider: 3d-cube 7.6169+-0.0691 ? 7.6897+-0.0548 ? 3d-morph 8.0379+-0.0506 7.9423+-0.0516 might be 1.0120x faster 3d-raytrace 7.6978+-0.0749 ! 7.8649+-0.0652 ! definitely 1.0217x slower access-binary-trees 1.7896+-0.0205 ? 1.7972+-0.0153 ? access-fannkuch 6.6428+-0.0555 ? 6.6455+-0.0486 ? access-nbody 3.6240+-0.0311 ^ 3.5547+-0.0334 ^ definitely 1.0195x faster access-nsieve 2.6442+-0.0186 2.6143+-0.0273 might be 1.0114x faster bitops-3bit-bits-in-byte 1.7369+-0.0113 1.7297+-0.0148 bitops-bits-in-byte 2.8179+-0.0260 ? 2.8259+-0.0230 ? bitops-bitwise-and 3.4138+-0.0386 ? 3.4341+-0.0370 ? bitops-nsieve-bits 5.6968+-0.0492 5.6713+-0.0507 controlflow-recursive 2.1031+-0.0128 2.0855+-0.0168 crypto-aes 6.9742+-0.0649 ? 6.9839+-0.0776 ? crypto-md5 2.9023+-0.0270 2.8799+-0.0277 crypto-sha1 2.5166+-0.0190 ? 2.5291+-0.0188 ? date-format-tofte 10.3428+-0.0942 ? 10.3728+-0.0696 ? date-format-xparb 9.9708+-0.0791 ^ 9.1980+-0.1098 ^ definitely 1.0840x faster math-cordic 6.5107+-0.0610 ? 6.6185+-0.0654 ? might be 1.0165x slower math-partial-sums 7.8267+-0.0628 ? 7.9017+-0.0857 ? math-spectral-norm 2.9237+-0.0218 ^ 2.8757+-0.0215 ^ definitely 1.0167x faster regexp-dna 11.1428+-0.0708 11.0864+-0.0677 string-base64 5.4737+-0.0543 ? 5.5158+-0.0466 ? string-fasta 6.5874+-0.0637 ! 6.7215+-0.0646 ! definitely 1.0204x slower string-tagcloud 11.5905+-0.0998 ? 11.6331+-0.0924 ? string-unpack-code 22.0113+-0.1475 ^ 21.1380+-0.1202 ^ definitely 1.0413x faster string-validate-input 6.6112+-0.0600 6.5897+-0.0528 <arithmetic> * 6.4310+-0.0116 ^ 6.3807+-0.0101 ^ definitely 1.0079x faster <geometric> 5.2620+-0.0097 ^ 5.2412+-0.0083 ^ definitely 1.0040x faster <harmonic> 4.3026+-0.0098 4.2897+-0.0092 TipOfTree ByValNotInt V8: crypto 75.2488+-0.6194 ? 76.2060+-0.4581 ? might be 1.0127x slower deltablue 235.1796+-1.5580 232.8426+-1.6325 might be 1.0100x faster earley-boyer 94.9535+-0.5838 94.3785+-0.5640 raytrace 61.8106+-0.4933 ? 61.9132+-0.5004 ? regexp 106.5991+-0.6673 ! 108.4383+-0.6386 ! definitely 1.0173x slower richards 215.7850+-1.5067 ? 216.2889+-1.6113 ? splay 98.7555+-0.5208 98.2535+-0.5803 <arithmetic> 126.9046+-0.3816 126.9030+-0.4350 <geometric> * 113.0139+-0.3111 ? 113.2183+-0.2902 ? <harmonic> 102.2886+-0.3014 ? 102.5998+-0.2601 ? TipOfTree ByValNotInt Kraken: ai-astar 520.2923+-3.5557 ? 523.2330+-3.2066 ? audio-beat-detection 201.7911+-1.2701 ? 202.5094+-0.9876 ? audio-dft 294.6874+-2.5177 293.2471+-3.0596 audio-fft 132.0817+-0.8217 ? 132.1278+-0.7498 ? audio-oscillator 264.2873+-1.9596 ? 264.6216+-1.9070 ? imaging-darkroom 433.4907+-3.0211 ? 434.0920+-2.6112 ? imaging-desaturate 240.9382+-1.7110 ? 242.4303+-1.6966 ? imaging-gaussian-blur 607.5867+-4.2884 ? 609.5590+-3.8772 ? json-parse-financial 56.8202+-0.3853 ? 57.0804+-0.3341 ? json-stringify-tinderbox 71.6916+-0.4857 ? 71.7657+-0.5842 ? stanford-crypto-aes 137.2520+-1.1420 ? 137.5863+-1.1203 ? stanford-crypto-ccm 104.7301+-0.6575 ? 106.1123+-0.7404 ? might be 1.0132x slower stanford-crypto-pbkdf2 197.4362+-1.4359 ? 198.0337+-1.3287 ? stanford-crypto-sha256-iterative 73.9388+-0.4936 ! 74.7318+-0.2638 ! definitely 1.0107x slower <arithmetic> * 238.3589+-0.5663 ? 239.0808+-0.7029 ? <geometric> 185.7711+-0.3691 ? 186.4627+-0.4280 ? <harmonic> 144.6839+-0.3407 ! 145.3671+-0.3337 ! definitely 1.0047x slower TipOfTree ByValNotInt All benchmarks: <arithmetic> 93.4588+-0.1647 ? 93.6458+-0.2074 ? <geometric> 24.0202+-0.0275 24.0008+-0.0292 <harmonic> 7.5708+-0.0167 7.5495+-0.0159 TipOfTree ByValNotInt Geomean of preferred means: <scaled-result> 55.7443+-0.0630 55.6882+-0.0679 [pizlo@minime bencher]

Filip Pizlo

Comment 2 2011-10-09 01:33:18 PDT

Created attachment 110293 [details] the patch

WebKit Review Bot

Comment 3 2011-10-09 01:36:24 PDT

Attachment 110293 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:270: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/JavaScriptCore/dfg/DFGOperations.h:57: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/JavaScriptCore/dfg/DFGOperations.h:69: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 4 2011-10-09 01:37:52 PDT

Created attachment 110294 [details] the patch

WebKit Review Bot

Comment 5 2011-10-09 01:42:07 PDT

Attachment 110294 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.cpp:270: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/JavaScriptCore/dfg/DFGOperations.h:57: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/JavaScriptCore/dfg/DFGOperations.h:69: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 6 2011-10-09 01:47:55 PDT

Created attachment 110295 [details] the patch - another attempt to make stylebot happy

WebKit Review Bot

Comment 7 2011-10-09 01:51:20 PDT

Attachment 110295 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/dfg/DFGOperations.h:57: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/JavaScriptCore/dfg/DFGOperations.h:69: DFG_OPERATION is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Oliver Hunt

Comment 8 2011-10-09 10:13:57 PDT

Comment on attachment 110295 [details] the patch - another attempt to make stylebot happy View in context: https://bugs.webkit.org/attachment.cgi?id=110295&action=review r=me, but update those comments > Source/JavaScriptCore/dfg/DFGPropagator.cpp:1025 > + // PutByVal currently always speculates that it's accessing an array with an > + // integer index, which means that it's impossible for it to cause a structure > + // change. This comment should be updated to indicate this only applies when integer properties are being accessed > Source/JavaScriptCore/dfg/DFGPropagator.cpp:1093 > + // PutByVal currently always speculates that it's accessing an array with an > + // integer index, which means that it's impossible for it to cause a structure > + // change. as above > Source/JavaScriptCore/dfg/DFGPropagator.cpp:1136 > + // PutByVal currently always speculates that it's accessing an array with an > + // integer index, which means that it's impossible for it to cause a structure > + // change. ditto > Source/JavaScriptCore/dfg/DFGPropagator.cpp:1172 > + // PutByVal currently always speculates that it's accessing an array with an > + // integer index, which means that it's impossible for it to cause a structure > + // change. ditto

Filip Pizlo

Comment 9 2011-10-09 12:12:42 PDT

(In reply to comment #8) > (From update of attachment 110295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110295&action=review > > r=me, but update those comments Oh, yeah, oops. Will do.

Filip Pizlo

Comment 10 2011-10-09 13:07:04 PDT

Landed in r97030.

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

2011-10-09 00:29 PDT

Modified

2011-10-09 13:07 PDT History

CC List

1 user Show

URL

Keywords

Depends on

Blocks