Bug 112376 – DFG string conversions and allocations should be inlined

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

This will be fun.

[Filip Pizlo]

Created attachment 193170 [details]
not the patch

[Filip Pizlo]

Created attachment 193209 [details]
work in progress

Eventually, this will be epic.  But it is not epic, yet, by virtue of the fact that it doesn't compile.

[Oliver Hunt]

(In reply to comment #2)
> Created an attachment (id=193209) [details]
> work in progress
> 
> Eventually, this will be epic.  But it is not epic, yet, by virtue of the fact that it doesn't compile.

r=me :D

[Filip Pizlo]

Created attachment 193231 [details]
it runs things

Still not epic though, because it doesn't necessarily run things correctly.

[Filip Pizlo]

This isn't bad.


Benchmark report for SunSpider, V8Spider, Octane, Kraken, and JSRegress on oldmac (MacPro4,1).

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/OpenSource/WebKitBuild/Release/jsc (r145868)
"StringCons" at /Volumes/Data/fromMiniMe/secondary/OpenSource/WebKitBuild/Release/jsc (r145868)

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                 StringCons                                    
SunSpider:
   3d-cube                                         9.1065+-0.1904            9.0214+-0.1378        
   3d-morph                                        8.6679+-0.1157     ?      8.8900+-0.1580        ? might be 1.0256x slower
   3d-raytrace                                    10.3976+-0.1586           10.3640+-0.1598        
   access-binary-trees                             1.8982+-0.0077     !      1.9202+-0.0096        ! definitely 1.0116x slower
   access-fannkuch                                 7.6377+-0.0899     ?      7.6493+-0.0694        ?
   access-nbody                                    4.6266+-0.0668     ?      4.6988+-0.0851        ? might be 1.0156x slower
   access-nsieve                                   4.9189+-0.0643            4.8844+-0.1177        
   bitops-3bit-bits-in-byte                        1.8165+-0.0091     ?      1.8264+-0.0102        ?
   bitops-bits-in-byte                             6.9436+-0.1004     ?      7.0440+-0.0880        ? might be 1.0144x slower
   bitops-bitwise-and                              2.6003+-0.1062     ?      2.6395+-0.0421        ? might be 1.0151x slower
   bitops-nsieve-bits                              4.1705+-0.0188            4.1689+-0.0168        
   controlflow-recursive                           3.1690+-0.1151            3.1201+-0.0524          might be 1.0157x faster
   crypto-aes                                      7.7100+-0.1221     ?      7.7916+-0.1084        ? might be 1.0106x slower
   crypto-md5                                      4.0115+-0.0327     ?      4.0411+-0.0302        ?
   crypto-sha1                                     3.2019+-0.0146            3.2001+-0.0239        
   date-format-tofte                              15.0487+-0.1248           14.9595+-0.1878        
   date-format-xparb                              15.7899+-0.1829     ^     12.0203+-0.1640        ^ definitely 1.3136x faster
   math-cordic                                     4.0146+-0.0068     ?      4.0244+-0.0044        ?
   math-partial-sums                              12.3785+-0.1202     ?     12.4648+-0.1138        ?
   math-spectral-norm                              3.1187+-0.0080     !      3.1394+-0.0123        ! definitely 1.0066x slower
   regexp-dna                                     11.3224+-0.1924     ?     11.3687+-0.1805        ?
   string-base64                                   5.4188+-0.1186            5.3015+-0.1021          might be 1.0221x faster
   string-fasta                                   10.7115+-0.1250           10.6896+-0.0915        
   string-tagcloud                                14.3849+-0.2384     ?     14.4168+-0.1788        ?
   string-unpack-code                             27.2553+-0.0983     ^     26.8085+-0.1283        ^ definitely 1.0167x faster
   string-validate-input                           7.9298+-0.1753            7.7844+-0.1329          might be 1.0187x faster

   <arithmetic> *                                  8.0096+-0.0652     ^      7.8553+-0.0551        ^ definitely 1.0196x faster
   <geometric>                                     6.3877+-0.0421            6.3284+-0.0342          might be 1.0094x faster
   <harmonic>                                      5.0917+-0.0282            5.0880+-0.0175          might be 1.0007x faster

                                                     TipOfTree                 StringCons                                    
V8Spider:
   crypto                                         87.2690+-0.1952           87.1204+-0.3437        
   deltablue                                     125.9214+-0.6256          124.8763+-0.4566        
   earley-boyer                                   82.7938+-0.3734           82.6773+-0.3051        
   raytrace                                       61.7622+-0.2447           61.5508+-0.1323        
   regexp                                        101.5721+-0.2768          101.2731+-0.1933        
   richards                                      117.7740+-0.2505     !    119.1393+-0.4541        ! definitely 1.0116x slower
   splay                                          57.9815+-0.3758           57.9708+-0.2580        

   <arithmetic>                                   90.7249+-0.1636           90.6583+-0.0926          might be 1.0007x faster
   <geometric> *                                  87.4048+-0.1709           87.3241+-0.0927          might be 1.0009x faster
   <harmonic>                                     84.0788+-0.1854           83.9850+-0.0971          might be 1.0011x faster

                                                     TipOfTree                 StringCons                                    
Octane and V8v7:
   encrypt                                        0.47286+-0.00053    ^     0.47018+-0.00066       ^ definitely 1.0057x faster
   decrypt                                        8.63283+-0.01939    ?     8.66501+-0.02531       ?
   deltablue                             x2       0.57186+-0.00136    ^     0.56675+-0.00117       ^ definitely 1.0090x faster
   earley                                         0.90377+-0.00461    ^     0.87603+-0.00309       ^ definitely 1.0317x faster
   boyer                                         12.94747+-0.20442         12.85190+-0.11111       
   raytrace                              x2       4.40008+-0.00596    ?     4.42128+-0.03630       ?
   regexp                                x2      32.64276+-0.78269         32.19324+-0.32552         might be 1.0140x faster
   richards                              x2       0.31170+-0.00034    ^     0.30628+-0.00098       ^ definitely 1.0177x faster
   splay                                 x2       0.70366+-0.00854    ?     0.70766+-0.02208       ?
   navier-stokes                         x2      10.79701+-0.01820    ?    10.80205+-0.01545       ?
   closure                                        0.30876+-0.03542          0.30874+-0.03548       
   jquery                                         4.38986+-0.54877    ?     4.39469+-0.54260       ?
   gbemu                                 x2     257.31633+-18.01715       250.76893+-17.63310        might be 1.0261x faster
   box2d                                 x2      31.72204+-0.17159    ?    31.88904+-0.13782       ?

V8v7:
   <arithmetic>                                   7.61319+-0.09382          7.55360+-0.04848         might be 1.0079x faster
   <geometric> *                                  2.46046+-0.00725          2.44513+-0.01192         might be 1.0063x faster
   <harmonic>                                     0.94830+-0.00199    ^     0.93833+-0.00487       ^ definitely 1.0106x faster

Octane including V8v7:
   <arithmetic>                                  32.02666+-1.63127         31.40350+-1.63022         might be 1.0198x faster
   <geometric> *                                  4.41832+-0.06654          4.39050+-0.07249         might be 1.0063x faster
   <harmonic>                                     1.07357+-0.01961          1.06431+-0.02037         might be 1.0087x faster

                                                     TipOfTree                 StringCons                                    
Kraken:
   ai-astar                                       494.822+-0.859            493.620+-0.795         
   audio-beat-detection                           247.474+-2.519            246.576+-1.190         
   audio-dft                                      311.886+-2.970      ?     312.426+-1.418         ?
   audio-fft                                      143.113+-0.236      !     143.924+-0.178         ! definitely 1.0057x slower
   audio-oscillator                               233.585+-0.964      ?     233.602+-0.985         ?
   imaging-darkroom                               291.887+-1.134      ?     292.846+-1.291         ?
   imaging-desaturate                             160.118+-0.152            159.925+-0.168         
   imaging-gaussian-blur                          399.017+-0.408      ^     397.429+-0.379         ^ definitely 1.0040x faster
   json-parse-financial                            80.492+-0.144      ^      79.691+-0.171         ^ definitely 1.0101x faster
   json-stringify-tinderbox                       101.054+-0.913      ?     103.888+-2.111         ? might be 1.0280x slower
   stanford-crypto-aes                             98.973+-1.252             98.254+-0.360         
   stanford-crypto-ccm                            104.524+-3.974      ?     105.176+-4.047         ?
   stanford-crypto-pbkdf2                         272.301+-2.125            270.835+-3.775         
   stanford-crypto-sha256-iterative               115.514+-0.542      ?     115.860+-0.640         ?

   <arithmetic> *                                 218.197+-0.466            218.147+-0.597           might be 1.0002x faster
   <geometric>                                    186.778+-0.687      ?     186.958+-0.761         ? might be 1.0010x slower
   <harmonic>                                     160.792+-0.852      ?     161.073+-0.908         ? might be 1.0017x slower

                                                     TipOfTree                 StringCons                                    
JSRegress:
   adapt-to-double-divide                         22.4244+-0.0631     ?     22.5208+-0.1059        ?
   aliased-arguments-getbyval                      0.8872+-0.0117     ?      0.9002+-0.0103        ? might be 1.0146x slower
   allocate-big-object                             2.5059+-0.0344     ?      2.5066+-0.0363        ?
   arity-mismatch-inlining                         0.7517+-0.0135     ?      0.7667+-0.0129        ? might be 1.0199x slower
   array-access-polymorphic-structure              7.0831+-0.1310            7.0329+-0.1144        
   array-with-double-add                           5.8835+-0.0302     ?      5.8985+-0.0597        ?
   array-with-double-increment                     4.0834+-0.0245     ?      4.1553+-0.0784        ? might be 1.0176x slower
   array-with-double-mul-add                       7.0354+-0.1301            7.0184+-0.0959        
   array-with-double-sum                           7.9006+-0.1329            7.8381+-0.1048        
   array-with-int32-add-sub                       10.4395+-0.1268     ?     10.4594+-0.1119        ?
   array-with-int32-or-double-sum                  7.8317+-0.0786     ?      7.9540+-0.0925        ? might be 1.0156x slower
   big-int-mul                                     4.8928+-0.1037     ?      4.9554+-0.0383        ? might be 1.0128x slower
   boolean-test                                    4.3590+-0.0303     ?      4.4200+-0.0814        ? might be 1.0140x slower
   cast-int-to-double                             13.8447+-0.1080     ?     13.8725+-0.1279        ?
   cell-argument                                  14.5461+-0.2171           14.3977+-0.0881          might be 1.0103x faster
   cfg-simplify                                    4.0125+-0.0728            3.9918+-0.0736        
   cmpeq-obj-to-obj-other                         11.6558+-0.0962           11.6298+-0.1440        
   constant-test                                   8.4987+-0.1345            8.3871+-0.1215          might be 1.0133x faster
   direct-arguments-getbyval                       0.8148+-0.0136     ?      0.8296+-0.0096        ? might be 1.0181x slower
   double-pollution-getbyval                      10.7338+-0.1301           10.6820+-0.0883        
   double-pollution-putbyoffset                    5.0417+-0.0933     !      5.4125+-0.1005        ! definitely 1.0736x slower
   external-arguments-getbyval                     2.1958+-0.0426            2.1872+-0.0307        
   external-arguments-putbyval                     3.2943+-0.0087     !      3.3975+-0.0897        ! definitely 1.0313x slower
   Float32Array-matrix-mult                       14.2220+-0.2653     ^     13.8359+-0.1038        ^ definitely 1.0279x faster
   fold-double-to-int                             22.4168+-0.3668           22.0511+-0.2084          might be 1.0166x faster
   function-dot-apply                              3.1566+-0.0094     ?      3.1788+-0.0128        ?
   function-test                                   5.0885+-0.0631            5.0431+-0.0283        
   get-by-id-chain-from-try-block                  7.3446+-0.0901     ?      7.4131+-0.1047        ?
   HashMap-put-get-iterate-keys                   98.1128+-0.9461           97.4678+-0.7056        
   HashMap-put-get-iterate                        97.5032+-0.4350     !     98.4550+-0.5059        ! definitely 1.0098x slower
   HashMap-string-put-get-iterate                 74.0913+-1.0576     ?     74.5539+-0.6617        ?
   indexed-properties-in-objects                   4.5008+-0.0436            4.4822+-0.0739        
   inline-arguments-access                         1.2306+-0.0100     ?      1.2419+-0.0086        ?
   inline-arguments-local-escape                  23.6635+-0.3459     ?     23.7345+-0.3823        ?
   inline-get-scoped-var                           6.6521+-0.1254            6.5186+-0.1200          might be 1.0205x faster
   inlined-put-by-id-transition                   16.6621+-0.2380     ?     16.7302+-0.3698        ?
   int-or-other-abs-then-get-by-val                8.7536+-0.0938     ?      8.8819+-0.1282        ? might be 1.0147x slower
   int-or-other-abs-zero-then-get-by-val          37.2089+-0.1570     !     39.9900+-0.2051        ! definitely 1.0747x slower
   int-or-other-add-then-get-by-val               10.2408+-0.1126           10.1577+-0.0815        
   int-or-other-add                               10.4716+-0.0916     ?     10.5127+-0.1240        ?
   int-or-other-div-then-get-by-val                7.9669+-0.1269            7.9338+-0.1403        
   int-or-other-max-then-get-by-val                9.9456+-0.1854     ?     10.0399+-0.1799        ?
   int-or-other-min-then-get-by-val                8.1348+-0.1159     ?      8.1453+-0.1116        ?
   int-or-other-mod-then-get-by-val                7.9537+-0.0925     ?      8.0236+-0.1129        ?
   int-or-other-mul-then-get-by-val                7.2038+-0.1139            7.1291+-0.0871          might be 1.0105x faster
   int-or-other-neg-then-get-by-val                8.1102+-0.1137            8.0698+-0.1027        
   int-or-other-neg-zero-then-get-by-val          36.9660+-0.1479     !     39.6939+-0.1186        ! definitely 1.0738x slower
   int-or-other-sub-then-get-by-val               10.1778+-0.0847     ?     10.1801+-0.1093        ?
   int-or-other-sub                                8.1731+-0.1087     ?      8.2764+-0.1115        ? might be 1.0126x slower
   int-overflow-local                             12.8298+-0.0817     ?     12.8538+-0.0784        ?
   Int16Array-bubble-sort                         49.3375+-0.1548     ?     49.3705+-0.1946        ?
   Int16Array-load-int-mul                         1.8610+-0.0071     ?      1.9149+-0.0482        ? might be 1.0290x slower
   Int8Array-load                                  4.8531+-0.0070            4.8109+-0.0629        
   integer-divide                                 14.9981+-0.0731     ?     15.0803+-0.1069        ?
   integer-modulo                                  2.0442+-0.0079     ?      2.0655+-0.0139        ? might be 1.0104x slower
   make-indexed-storage                            3.8639+-0.0348     ?      3.8956+-0.0346        ?
   method-on-number                               23.5174+-0.2475           23.3400+-0.3035        
   nested-function-parsing-random                385.4077+-13.0905         377.3643+-13.0249         might be 1.0213x faster
   nested-function-parsing                        51.5561+-1.1521           51.4643+-1.0686        
   new-array-buffer-dead                           3.6031+-0.0111     ?      3.6202+-0.0140        ?
   new-array-buffer-push                          10.3018+-0.1776     ?     10.4013+-0.1241        ?
   new-array-dead                                 28.3050+-0.1199     ?     28.3143+-0.1531        ?
   new-array-push                                  6.9620+-0.0777     ?      7.0523+-0.2052        ? might be 1.0130x slower
   number-test                                     4.2178+-0.0508     !      4.3443+-0.0662        ! definitely 1.0300x slower
   object-closure-call                             8.3543+-0.1189            8.2467+-0.0794          might be 1.0130x faster
   object-test                                     4.9989+-0.0524     ^      4.8298+-0.0956        ^ definitely 1.0350x faster
   poly-stricteq                                  90.6754+-0.1286     ?     91.6998+-1.1554        ? might be 1.0113x slower
   polymorphic-structure                          19.9482+-0.0983     ?     20.0747+-0.1444        ?
   polyvariant-monomorphic-get-by-id              12.4655+-0.1139     ?     12.5089+-0.1167        ?
   rare-osr-exit-on-local                         20.5464+-0.0916     ?     20.5762+-0.1013        ?
   register-pressure-from-osr                     31.5746+-0.1839           31.4939+-0.0918        
   simple-activation-demo                         34.5505+-0.1651           34.4557+-0.1058        
   slow-array-profile-convergence                  4.3095+-0.0189     ?      4.3141+-0.0262        ?
   slow-convergence                                3.7680+-0.0205     ?      3.7843+-0.0102        ?
   sparse-conditional                              1.2978+-0.0097     ?      1.3104+-0.0132        ?
   splice-to-remove                               50.2194+-0.1624     ?     50.3449+-0.1784        ?
   string-concat-object                           23.3937+-0.1902     ^      5.5409+-0.0756        ^ definitely 4.2220x faster
   string-concat-pair-object                      22.2419+-0.1133     ^     18.1664+-0.1619        ^ definitely 1.2243x faster
   string-concat-pair-simple                      28.9390+-0.2634     !     29.5539+-0.1628        ! definitely 1.0212x slower
   string-concat-simple                           46.3325+-0.2553           46.2287+-0.5911        
   string-cons-repeat                             46.3212+-0.3559     ^     10.1230+-0.0354        ^ definitely 4.5758x faster
   string-cons-tower                             185.0862+-1.6205     ^     10.6123+-0.0227        ^ definitely 17.4407x faster
   string-hash                                     2.6356+-0.0149     ?      2.6459+-0.0086        ?
   string-lookup-hit-identifier                    9.9722+-0.1881            9.9500+-0.2337        
   string-lookup-hit                              23.2336+-0.0950     ?     23.3365+-0.1463        ?
   string-lookup-miss                             32.1602+-0.6210     ^     31.1954+-0.1973        ^ definitely 1.0309x faster
   string-repeat-arith                            44.8397+-0.3114           44.4494+-0.3076        
   string-sub                                     85.0808+-0.2219     ?     86.4919+-1.5699        ? might be 1.0166x slower
   string-test                                     4.2732+-0.0425     ?      4.2858+-0.0363        ?
   structure-hoist-over-transitions                3.2465+-0.0211     ?      3.2546+-0.0220        ?
   tear-off-arguments-simple                       1.7529+-0.0089     ?      1.7733+-0.0137        ? might be 1.0116x slower
   tear-off-arguments                              3.3664+-0.0102     ?      3.3825+-0.0134        ?
   temporal-structure                             20.8643+-0.1345           20.8319+-0.0863        
   to-int32-boolean                               27.0589+-0.1530     ?     27.1295+-0.1359        ?
   undefined-test                                  4.4822+-0.0491     ?      4.5694+-0.0533        ? might be 1.0194x slower

   <arithmetic>                                   23.2454+-0.1476     ^     20.8001+-0.1568        ^ definitely 1.1176x faster
   <geometric> *                                  10.3938+-0.0286     ^      9.7966+-0.0314        ^ definitely 1.0610x faster
   <harmonic>                                      5.4253+-0.0176     ^      5.3740+-0.0153        ^ definitely 1.0095x faster

                                                     TipOfTree                 StringCons                                    
All benchmarks:
   <arithmetic>                                   41.5304+-0.3348     ^     39.9987+-0.3427        ^ definitely 1.0383x faster
   <geometric>                                    12.0220+-0.0573     ^     11.5903+-0.0582        ^ definitely 1.0372x faster
   <harmonic>                                      3.7886+-0.0337            3.7583+-0.0357          might be 1.0081x faster

                                                     TipOfTree                 StringCons                                    
Geomean of preferred means:
   <scaled-result>                                23.3987+-0.1341     ^     22.9990+-0.1306        ^ definitely 1.0174x faster

[Filip Pizlo]

Created attachment 193245 [details]
the patch

Still running layout tests so I don't have expectation files for the new tests yet.

[WebKit Review Bot]

Attachment 193245 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/js/dfg-to-string-bad-toString.html', u'LayoutTests/fast/js/dfg-to-string-bad-valueOf.html', u'LayoutTests/fast/js/dfg-to-string-int-or-string.html', u'LayoutTests/fast/js/dfg-to-string-int.html', u'LayoutTests/fast/js/dfg-to-string-side-effect.html', u'LayoutTests/fast/js/dfg-to-string-toString-becomes-bad-with-dictionary-string-prototype.html', u'LayoutTests/fast/js/dfg-to-string-toString-becomes-bad.html', u'LayoutTests/fast/js/dfg-to-string-toString-in-string.html', u'LayoutTests/fast/js/dfg-to-string-valueOf-becomes-bad.html', u'LayoutTests/fast/js/dfg-to-string-valueOf-in-string.html', u'LayoutTests/fast/js/jsc-test-list', u'LayoutTests/fast/js/regress/script-tests/string-concat-object.js', u'LayoutTests/fast/js/regress/script-tests/string-concat-pair-object.js', u'LayoutTests/fast/js/regress/script-tests/string-concat-pair-simple.js', u'LayoutTests/fast/js/regress/script-tests/string-concat-simple.js', u'LayoutTests/fast/js/regress/script-tests/string-cons-repeat.js', u'LayoutTests/fast/js/regress/script-tests/string-cons-tower.js', u'LayoutTests/fast/js/regress/string-concat-object-expected.txt', u'LayoutTests/fast/js/regress/string-concat-object.html', u'LayoutTests/fast/js/regress/string-concat-pair-object-expected.txt', u'LayoutTests/fast/js/regress/string-concat-pair-object.html', u'LayoutTests/fast/js/regress/string-concat-pair-simple-expected.txt', u'LayoutTests/fast/js/regress/string-concat-pair-simple.html', u'LayoutTests/fast/js/regress/string-concat-simple-expected.txt', u'LayoutTests/fast/js/regress/string-concat-simple.html', u'LayoutTests/fast/js/regress/string-cons-repeat-expected.txt', u'LayoutTests/fast/js/regress/string-cons-repeat.html', u'LayoutTests/fast/js/regress/string-cons-tower-expected.txt', u'LayoutTests/fast/js/regress/string-cons-tower.html', u'LayoutTests/fast/js/script-tests/dfg-to-string-bad-toString.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-bad-valueOf.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-int-or-string.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-int.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-side-effect.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-toString-becomes-bad-with-dictionary-string-prototype.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-toString-becomes-bad.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-toString-in-string.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-valueOf-becomes-bad.js', u'LayoutTests/fast/js/script-tests/dfg-to-string-valueOf-in-string.js', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/DerivedSources.make', u'Source/JavaScriptCore/DerivedSources.pri', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/DFGExitProfile.cpp', u'Source/JavaScriptCore/bytecode/DFGExitProfile.h', u'Source/JavaScriptCore/bytecode/ExitKind.cpp', u'Source/JavaScriptCore/bytecode/ExitKind.h', u'Source/JavaScriptCore/bytecode/SpeculatedType.cpp', u'Source/JavaScriptCore/bytecode/SpeculatedType.h', u'Source/JavaScriptCore/create_hash_table', u'Source/JavaScriptCore/dfg/DFGAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractState.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.cpp', u'Source/JavaScriptCore/dfg/DFGUseKind.h', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/Intrinsic.h', u'Source/JavaScriptCore/runtime/JSDestructibleObject.h', u'Source/JavaScriptCore/runtime/JSGlobalData.cpp', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/JSObject.cpp', u'Source/JavaScriptCore/runtime/JSObject.h', u'Source/JavaScriptCore/runtime/JSWrapperObject.h', u'Source/JavaScriptCore/runtime/StringPrototype.cpp', u'Source/JavaScriptCore/runtime/StringPrototype.h', u'Tools/ChangeLog', u'Tools/Scripts/integrate-fast-js-tests']" exit_code: 1
Source/JavaScriptCore/runtime/StringPrototype.h:37:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/StringPrototype.h:37:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/StringPrototype.h:37:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/DFGExitProfile.h:140:  The parameter name "site" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2225:  The parameter name "structureLocation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGNode.h:1134:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGNode.h:1139:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 76 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 193252 [details]
the patch

[Build Bot]

Comment on attachment 193252 [details]
the patch

Attachment 193252 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17114510

[Geoffrey Garen]

Comment on attachment 193252 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193252&action=review

r=me, plz to fix Windows before landing.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:899
> +        if (stringPrototypeStructure->transitionWatchpointSetHasBeenInvalidated())
> +            return false;

It's pretty good that we set up all the string built-ins in advance, and don't set this watchpoint until the first DFG-optimized string access. I still think it would be even better if we had a specific watchpoint for overriding toString() and valueOf(). Mark's data convinced me that libraries will add to the string prototype, but won't override toString() and valueOf().

> Tools/ChangeLog:11
> +        * Scripts/integrate-fast-js-tests: Added.

Does this duplicate make-script-test-wrappers or make-new-script-test?

[Filip Pizlo]

(In reply to comment #10)
> (From update of attachment 193252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193252&action=review
> 
> r=me, plz to fix Windows before landing.

Yup, will do.

> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:899
> > +        if (stringPrototypeStructure->transitionWatchpointSetHasBeenInvalidated())
> > +            return false;
> 
> It's pretty good that we set up all the string built-ins in advance, and don't set this watchpoint until the first DFG-optimized string access. I still think it would be even better if we had a specific watchpoint for overriding toString() and valueOf(). Mark's data convinced me that libraries will add to the string prototype, but won't override toString() and valueOf().

This patch allows you to add things to String.prototype and it will continue to optimize those intrinsics even if you did.

Note that stringPrototypeStructure in this code is not the original structure of the original prototype, but the structure of the current string prototype at the time of DFG optimization.  We then verify if you've overridden toString or valueOf using isStringPrototypeMethodSane().  And then we watchpoint the whole structure - i.e. we're watching that you don't muck with the string prototype after optimization.

So the following code will get optimized:

String.prototype.myThingy = function() { stuff; }
String.prototype.somethingElse = function() { more stuff; }

for (var i = 0; 1000; ++i) {
    var x = new String(i);
    var y = String(x);
}

It's only if you do this that you'll bail out:

for (var i = 0; 1000; ++i) {
    if (i == 900) {
        String.prototype.myThingy = function() { stuff; }
        String.prototype.somethingElse = function() { more stuff; }
    }
    var x = new String(i);
    var y = String(x);
}

Basically, I'm doing exactly what you say, except that I'm using existing structure watch pointing functionality rather than adding some new watch pointing for fields.

> 
> > Tools/ChangeLog:11
> > +        * Scripts/integrate-fast-js-tests: Added.
> 
> Does this duplicate make-script-test-wrappers or make-new-script-test?

I guess it does.  I'll remove it for now and figure out a way of making make-script-test-wrappers do what I want for JSRegress tests.

[Geoffrey Garen]

> Basically, I'm doing exactly what you say, except that I'm using existing structure watch pointing functionality rather than adding some new watch pointing for fields.

Here's the case I'm thinking about, which isn't covered by the current approach:

function helper(key, value)
{
    // Do some string operations that are optimizable with DFG intrinsics.
}

function loadSettings()
{
    ....
    for (var i = 0; i < entries.length; ++i)
        helper(entries[i].key, entries[i].value);
    ....
}

function initLibrary()
{
    ....
    if (settings.XXX)
        String.prototype.myStringFunction = function() { ... };
    else
        String.prototype.myStringFunction = function() { ... };
    ....
}

loadSettings();
initLibrary();

In this case, 'loadSettings' will tier up 'helper' into the DFG, setting the structure transition watchpoint on String.prototype, and then 'initLibrary' will fire the watchpoint.

Like I said, I think the current approach is fine, but I think solving this case would be even better.

[Filip Pizlo]

(In reply to comment #12)
> > Basically, I'm doing exactly what you say, except that I'm using existing structure watch pointing functionality rather than adding some new watch pointing for fields.
> 
> Here's the case I'm thinking about, which isn't covered by the current approach:
> 
> function helper(key, value)
> {
>     // Do some string operations that are optimizable with DFG intrinsics.
> }
> 
> function loadSettings()
> {
>     ....
>     for (var i = 0; i < entries.length; ++i)
>         helper(entries[i].key, entries[i].value);
>     ....
> }
> 
> function initLibrary()
> {
>     ....
>     if (settings.XXX)
>         String.prototype.myStringFunction = function() { ... };
>     else
>         String.prototype.myStringFunction = function() { ... };
>     ....
> }
> 
> loadSettings();
> initLibrary();
> 
> In this case, 'loadSettings' will tier up 'helper' into the DFG, setting the structure transition watchpoint on String.prototype, and then 'initLibrary' will fire the watchpoint.
> 
> Like I said, I think the current approach is fine, but I think solving this case would be even better.

Depends on whether or not you wanted to call loadSettings again in the future.

The simple fix is to just not have the speculation watchpoint not set the NotStringObject kind.  Then when we bail and recompile we'll just set a watchpoint on the new structure.

How do you envision setting a watchpoint on a *property* that won't fire when a structure transition happens unless that transition clobbers the property?

[Geoffrey Garen]

> > In this case, 'loadSettings' will tier up 'helper' into the DFG, setting the structure transition watchpoint on String.prototype, and then 'initLibrary' will fire the watchpoint.
> > 
> > Like I said, I think the current approach is fine, but I think solving this case would be even better.
> 
> Depends on whether or not you wanted to call loadSettings again in the future.

No. After the watchpoint fires, all functions that operate on strings become ineligible for the string intrinsic optimizations -- not just loadSettings.

[Filip Pizlo]

(In reply to comment #14)
> > > In this case, 'loadSettings' will tier up 'helper' into the DFG, setting the structure transition watchpoint on String.prototype, and then 'initLibrary' will fire the watchpoint.
> > > 
> > > Like I said, I think the current approach is fine, but I think solving this case would be even better.
> > 
> > Depends on whether or not you wanted to call loadSettings again in the future.
> 
> No. After the watchpoint fires, all functions that operate on strings become ineligible for the string intrinsic optimizations -- not just loadSettings.

No they don't. How would they?  The string prototype now has a new structure and that structure has a distinct watch point, which is still valid. 

We already make use of this quite extensively. It happens in ordinary get_by_if proto chain accesses in V8v7: first we cache on the prototype having one structure, which we accomplish using a watch point. Then the prototype's structure changes. That invalidates the cache. But the prototype now has a new structure with a pristine watch point. So we can, if we want to, cache again. And certainly any new code we compile can cache. The old structure's invalidated watch point doesn't matter for code that is compiled once the prototype no longer has that structure.

[Filip Pizlo]

Created attachment 193585 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 193585 [details]
patch for landing

Attachment 193585 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17128518

New failing tests:
fast/js/dfg-to-string-side-effect-clobbers-toString.html

[Filip Pizlo]

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

[Geoffrey Garen]

> No they don't. How would they?  The string prototype now has a new structure and that structure has a distinct watch point, which is still valid. 

Ack! I was missing this key detail.

[Roger Fong]

On Windows:
** Danger, Will Robinson! Danger! The following failures have been introduced:
	js1_5/Regress/regress-76054.js

[Filip Pizlo]

(In reply to comment #20)
> On Windows:
> ** Danger, Will Robinson! Danger! The following failures have been introduced:
>     js1_5/Regress/regress-76054.js

Looking at it now.

[Ryosuke Niwa]

Is it possible that this caused a 20% memory regression on Mac?

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22Mac%20MountainLion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%2C%5B%22Mac%20Lion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%5D&zoom=%5B1363572327935.7915%2C1363623676769.9104%5D

[Filip Pizlo]

(In reply to comment #22)
> Is it possible that this caused a 20% memory regression on Mac?
> 
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22Mac%20MountainLion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%2C%5B%22Mac%20Lion%22%2C%22Parser%2Fhtml5-full-render%3AJSHeap%22%5D%5D&zoom=%5B1363572327935.7915%2C1363623676769.9104%5D

It is possible, just highly unlikely!

I'm looking at other regressions from this right now...  It's a tricky changeset. :-/

[Ryosuke Niwa]

It seems like this patch also caused https://bugs.webkit.org/show_bug.cgi?id=112656

[Csaba Osztrogonác]

(In reply to comment #18)
> Landed in http://trac.webkit.org/changeset/146089

It caused one more regression on ARM (traditional and Thumb2 too): https://bugs.webkit.org/show_bug.cgi?id=112676

[Filip Pizlo]

*** Bug 110175 has been marked as a duplicate of this bug. ***