Bug 69716

Summary:

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

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
the patch	none
the patch	none
the patch - another attempt to make stylebot happy	oliver: review+

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.