Bug 69716 - DFG should not always speculate that a ByVal access has an integer index
Summary: DFG should not always speculate that a ByVal access has an integer index
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: 2011-10-09 00:29 PDT by Filip Pizlo
Modified: 2011-10-09 13:07 PDT (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-10-09 00:29:35 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 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]
Comment 2 Filip Pizlo 2011-10-09 01:33:18 PDT
Created attachment 110293 [details]
the patch
Comment 3 WebKit Review Bot 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.
Comment 4 Filip Pizlo 2011-10-09 01:37:52 PDT
Created attachment 110294 [details]
the patch
Comment 5 WebKit Review Bot 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.
Comment 6 Filip Pizlo 2011-10-09 01:47:55 PDT
Created attachment 110295 [details]
the patch - another attempt to make stylebot happy
Comment 7 WebKit Review Bot 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.
Comment 8 Oliver Hunt 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
Comment 9 Filip Pizlo 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.
Comment 10 Filip Pizlo 2011-10-09 13:07:04 PDT
Landed in r97030.