Bug 68329 – DFG should support continuous optimization

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

In the current tiered compilation scheme, the DFG may perform a speculation that turns out to be false later in the program's lifetime.  Currently this means that the program systematically fails speculation for that code block, resulting in poor performance.

The DFG should be able to reoptimize the relevant code block with updated speculations.

This involves:

1) Being able to jettison the old DFG code block, but so long as no stack frame is running on it.

2) Being able to update predictions over time.

3) Having heuristics that determine when it would be profitable to reoptimize.

[Filip Pizlo]

It looks like adding the ability to mark CodeBlocks in the conservative marker is mostly performance-neutral.



Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc
"JettisonCB" at /Volumes/Data/pizlo/octonary/OpenSource/WebKitBuild/Release/jsc

Collected 12 samples per benchmark/VM, with 4 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               JettisonCB                                   
SunSpider:
   3d-cube                                7.7796+-0.1853          7.7159+-0.1270       
   3d-morph                               7.5034+-0.1601          7.4898+-0.1230       
   3d-raytrace                            7.6260+-0.1326          7.5335+-0.1745         might be 1.0123x faster
   access-binary-trees                    2.3017+-0.0509          2.2925+-0.0934       
   access-fannkuch                       11.4303+-0.1660    ?    11.6360+-0.1596       ? might be 1.0180x slower
   access-nbody                           4.2085+-0.0754    ^     3.8239+-0.0846       ^ definitely 1.1006x faster
   access-nsieve                          2.6087+-0.0820          2.5766+-0.0415         might be 1.0125x faster
   bitops-3bit-bits-in-byte               1.6489+-0.0373    ?     1.6678+-0.0454       ? might be 1.0115x slower
   bitops-bits-in-byte                    2.7311+-0.0303    ?     2.7410+-0.0772       ?
   bitops-bitwise-and                     3.5697+-0.0873          3.5299+-0.0864         might be 1.0113x faster
   bitops-nsieve-bits                     5.2416+-0.0833    ?     5.2612+-0.0723       ?
   controlflow-recursive                  1.9781+-0.0433          1.9708+-0.0563       
   crypto-aes                             6.8613+-0.2177    ?     6.9913+-0.2410       ? might be 1.0189x slower
   crypto-md5                             2.7334+-0.0518    ?     2.7985+-0.0689       ? might be 1.0238x slower
   crypto-sha1                            2.2083+-0.0603          2.1808+-0.0475         might be 1.0126x faster
   date-format-tofte                      9.9839+-0.1474    ?    10.1863+-0.1228       ? might be 1.0203x slower
   date-format-xparb                      9.0108+-0.3826          8.7002+-0.1493         might be 1.0357x faster
   math-cordic                            6.1725+-0.1297    ?     6.2024+-0.1388       ?
   math-partial-sums                      7.3134+-0.1048          7.2668+-0.1245       
   math-spectral-norm                     2.5721+-0.0469    ?     2.6382+-0.0664       ? might be 1.0257x slower
   regexp-dna                            10.8081+-0.2950    ?    10.8432+-0.1563       ?
   string-base64                          5.6656+-0.1398    ?     5.7008+-0.1246       ?
   string-fasta                           6.9114+-0.1404    ?     6.9835+-0.1214       ? might be 1.0104x slower
   string-tagcloud                       11.8874+-0.1724         11.8395+-0.1860       
   string-unpack-code                    18.6655+-0.1657    ?    18.7603+-0.4900       ?
   string-validate-input                  6.5207+-0.1124          6.4334+-0.1503         might be 1.0136x faster

   <arithmetic>                           6.3824+-0.0342          6.3755+-0.0252       
   <geometric>                            5.2612+-0.0271          5.2475+-0.0176       
   <harmonic>                             4.2911+-0.0266          4.2802+-0.0274       

                                            TipOfTree               JettisonCB                                   
V8:
   crypto                                83.2440+-0.3707    ^    82.3064+-0.5042       ^ definitely 1.0114x faster
   deltablue                            241.8391+-2.0837    ?   243.4727+-1.5806       ?
   earley-boyer                          96.1417+-0.2934         95.7106+-0.5460       
   raytrace                              69.7033+-1.0869    ?    69.7215+-0.7712       ?
   regexp                               106.1064+-0.3785    ?   106.3670+-0.3294       ?
   richards                             216.8097+-0.5041        216.5710+-0.7715       
   splay                                 98.9097+-0.5848    ?    99.5432+-0.5767       ?

   <arithmetic>                         130.3934+-0.4094    ?   130.5275+-0.3473       ?
   <geometric>                          117.3718+-0.3798        117.3549+-0.3047       
   <harmonic>                           107.6083+-0.4179        107.4964+-0.3393       

                                            TipOfTree               JettisonCB                                   
Kraken:
   ai-astar                             635.7456+-4.1857        630.3726+-4.3410       
   audio-beat-detection                 463.0082+-1.4343    !   472.5084+-2.1826       ! definitely 1.0205x slower
   audio-dft                            420.2750+-2.4124        420.0530+-2.2228       
   audio-fft                            364.8925+-3.5600    ?   369.1498+-0.9085       ? might be 1.0117x slower
   audio-oscillator                     315.5371+-0.4917    ^   312.9771+-0.5289       ^ definitely 1.0082x faster
   imaging-darkroom                     417.1771+-6.5275    ?   423.8325+-15.0247      ? might be 1.0160x slower
   imaging-desaturate                   217.6311+-1.1461    ?   218.2500+-0.2932       ?
   imaging-gaussian-blur                592.2451+-0.8057        590.7516+-2.4122       
   json-parse-financial                  49.6200+-0.3170    ?    50.0082+-0.3647       ?
   json-stringify-tinderbox              67.7365+-0.4645    ?    68.2077+-0.3253       ?
   stanford-crypto-aes                  144.1274+-0.9559    ?   144.7559+-0.8604       ?
   stanford-crypto-ccm                  110.2297+-0.3680    !   113.0208+-0.5793       ! definitely 1.0253x slower
   stanford-crypto-pbkdf2               394.1354+-3.2859    ?   396.0956+-3.0744       ?
   stanford-crypto-sha256-iterative     146.3190+-0.9362    !   148.6070+-0.7463       ! definitely 1.0156x slower

   <arithmetic>                         309.9057+-0.6356    ?   311.3279+-1.1983       ?
   <geometric>                          241.9557+-0.3347    !   243.5964+-0.7942       ! definitely 1.0068x slower
   <harmonic>                           172.8240+-0.4237    !   174.3479+-0.5745       ! definitely 1.0088x slower

                                            TipOfTree               JettisonCB                                   
All benchmarks:
   <arithmetic>                         115.2633+-0.2232    ?   115.7031+-0.4024       ?
   <geometric>                           26.1323+-0.0832    ?    26.1466+-0.0580       ?
   <harmonic>                             7.5744+-0.0460          7.5563+-0.0473

[Filip Pizlo]

Looks like adding a speculative success counter to the code that DFG emits is *almost* performance neutral.  It's a slight loss, but hopefully I can make up for it by the increased robustness.



Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc
"JettisonCB" at /Volumes/Data/pizlo/octonary/OpenSource/WebKitBuild/Release/jsc

Collected 12 samples per benchmark/VM, with 4 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               JettisonCB                                   
SunSpider:
   3d-cube                                7.7189+-0.1412          7.7112+-0.1051       
   3d-morph                               7.4322+-0.1846    ?     7.4622+-0.1422       ?
   3d-raytrace                            7.6861+-0.0888          7.5094+-0.0946         might be 1.0235x faster
   access-binary-trees                    2.3022+-0.0572    ?     2.3973+-0.0883       ? might be 1.0413x slower
   access-fannkuch                       11.5342+-0.1283    ?    11.5562+-0.1893       ?
   access-nbody                           4.1764+-0.1071    ^     3.8803+-0.0972       ^ definitely 1.0763x faster
   access-nsieve                          2.5951+-0.0377    ?     2.6183+-0.0797       ?
   bitops-3bit-bits-in-byte               1.6526+-0.0440    !     1.7839+-0.0379       ! definitely 1.0794x slower
   bitops-bits-in-byte                    2.7086+-0.0479          2.6894+-0.0520       
   bitops-bitwise-and                     3.6113+-0.0852    ?     3.6227+-0.0851       ?
   bitops-nsieve-bits                     5.3445+-0.0918    ?     5.3558+-0.1016       ?
   controlflow-recursive                  1.9275+-0.0509    !     2.1499+-0.0869       ! definitely 1.1154x slower
   crypto-aes                             6.9565+-0.2272          6.8406+-0.3221         might be 1.0169x faster
   crypto-md5                             2.7340+-0.0502    ?     2.7975+-0.0356       ? might be 1.0232x slower
   crypto-sha1                            2.2154+-0.0499    ?     2.3144+-0.0540       ? might be 1.0447x slower
   date-format-tofte                      9.7958+-0.1211    ?     9.9403+-0.1487       ? might be 1.0147x slower
   date-format-xparb                      8.7927+-0.1389    ?     8.8095+-0.1998       ?
   math-cordic                            6.2006+-0.1075          6.1583+-0.0908       
   math-partial-sums                      7.2989+-0.1076          7.2459+-0.0633       
   math-spectral-norm                     2.6274+-0.0671          2.5728+-0.0299         might be 1.0212x faster
   regexp-dna                            10.9608+-0.1526         10.7005+-0.1278         might be 1.0243x faster
   string-base64                          5.7498+-0.1633          5.7256+-0.1321       
   string-fasta                           6.9099+-0.1548    ?     7.0445+-0.1264       ? might be 1.0195x slower
   string-tagcloud                       11.5898+-0.1934    ?    11.8337+-0.1978       ? might be 1.0210x slower
   string-unpack-code                    18.4722+-0.1787    ?    18.8110+-0.5315       ? might be 1.0183x slower
   string-validate-input                  6.4533+-0.1169    ?     6.4611+-0.1352       ?

   <arithmetic>                           6.3633+-0.0157    ?     6.3843+-0.0220       ?
   <geometric>                            5.2546+-0.0131    !     5.2953+-0.0155       ! definitely 1.0077x slower
   <harmonic>                             4.2874+-0.0205    !     4.3699+-0.0179       ! definitely 1.0193x slower

                                            TipOfTree               JettisonCB                                   
V8:
   crypto                                82.8718+-0.5337         82.4999+-0.4342       
   deltablue                            241.8472+-2.3797    !   245.6910+-1.1746       ! definitely 1.0159x slower
   earley-boyer                          98.3761+-2.6772         96.7450+-0.2958         might be 1.0169x faster
   raytrace                              69.0379+-0.2578    !    70.1706+-0.4662       ! definitely 1.0164x slower
   regexp                               106.5903+-1.2884        105.7796+-0.9258       
   richards                             217.2081+-0.8096    !   225.6307+-1.1982       ! definitely 1.0388x slower
   splay                                 98.6183+-0.5517         98.4457+-0.6877       

   <arithmetic>                         130.6500+-0.3123    !   132.1375+-0.3785       ! definitely 1.0114x slower
   <geometric>                          117.5667+-0.3564    ?   118.2467+-0.3255       ?
   <harmonic>                           107.6964+-0.4021    ?   107.9803+-0.3229       ?

                                            TipOfTree               JettisonCB                                   
Kraken:
   ai-astar                             630.8458+-4.8120        628.8718+-4.5694       
   audio-beat-detection                 465.4545+-2.0257    !   473.6872+-0.9470       ! definitely 1.0177x slower
   audio-dft                            424.4798+-5.0051        418.3906+-2.0215         might be 1.0146x faster
   audio-fft                            362.5508+-1.6881    ?   367.1407+-2.9210       ? might be 1.0127x slower
   audio-oscillator                     315.8901+-0.7736    ^   312.8390+-1.2768       ^ definitely 1.0098x faster
   imaging-darkroom                     413.5906+-2.0065        412.9257+-1.1696       
   imaging-desaturate                   217.8714+-0.7518        212.2578+-5.0701         might be 1.0264x faster
   imaging-gaussian-blur                591.4198+-1.2417    !   598.7139+-2.5048       ! definitely 1.0123x slower
   json-parse-financial                  49.7857+-0.5397         49.6653+-0.6754       
   json-stringify-tinderbox              67.6122+-0.3064         67.4863+-0.2974       
   stanford-crypto-aes                  144.6965+-1.1703    ^   141.7946+-0.6062       ^ definitely 1.0205x faster
   stanford-crypto-ccm                  111.3790+-0.7516    ?   111.6174+-0.6448       ?
   stanford-crypto-pbkdf2               393.2011+-2.0680        391.7593+-2.1244       
   stanford-crypto-sha256-iterative     147.4451+-0.6313    ?   148.8170+-0.9358       ?

   <arithmetic>                         309.7302+-0.4355        309.7119+-0.6984       
   <geometric>                          242.2064+-0.3787        241.6892+-0.5748       
   <harmonic>                           173.2442+-0.5841        172.6992+-0.6739       

                                            TipOfTree               JettisonCB                                   
All benchmarks:
   <arithmetic>                         115.2387+-0.1474    ?   115.4664+-0.2303       ?
   <geometric>                           26.1288+-0.0404    !    26.2463+-0.0382       ! definitely 1.0045x slower
   <harmonic>                             7.5683+-0.0355    !     7.7104+-0.0309       ! definitely 1.0188x slower

[Filip Pizlo]

Created attachment 107808 [details]
work in progress

This is still a work in progress, and has known problems: https://bugs.webkit.org/show_bug.cgi?id=68335

[Filip Pizlo]

Created attachment 108051 [details]
work in progress - ruggedized it some more

This still needs some tuning.  I'm not yet sure if the control logic is right.

[Filip Pizlo]

OK, this is starting to pay dividends.



Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc
"JettisonCB" at /Volumes/Data/pizlo/octonary/OpenSource/WebKitBuild/Release/jsc

Collected 12 samples per benchmark/VM, with 4 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               JettisonCB                                   
SunSpider:
   3d-cube                                7.7479+-0.1731    ?     7.9242+-0.1771       ? might be 1.0227x slower
   3d-morph                               7.6610+-0.1180    ?     7.7199+-0.1663       ?
   3d-raytrace                            7.7748+-0.2494          7.7302+-0.2144       
   access-binary-trees                    2.3929+-0.1053    ?     2.4276+-0.0592       ? might be 1.0145x slower
   access-fannkuch                       11.6314+-0.2240         11.6311+-0.2365       
   access-nbody                           3.6431+-0.0896          3.5825+-0.0991         might be 1.0169x faster
   access-nsieve                          2.7376+-0.0882          2.6075+-0.0662         might be 1.0499x faster
   bitops-3bit-bits-in-byte               1.7140+-0.0182    !     1.8333+-0.0347       ! definitely 1.0696x slower
   bitops-bits-in-byte                    2.9296+-0.0530          2.8637+-0.0790         might be 1.0230x faster
   bitops-bitwise-and                     3.7218+-0.1415          3.6575+-0.1019         might be 1.0176x faster
   bitops-nsieve-bits                     5.2566+-0.0682    ?     5.3743+-0.1044       ? might be 1.0224x slower
   controlflow-recursive                  2.0677+-0.0778    ?     2.1981+-0.0528       ? might be 1.0631x slower
   crypto-aes                             7.0142+-0.3549    ?     7.4064+-0.3338       ? might be 1.0559x slower
   crypto-md5                             2.8135+-0.0794          2.7879+-0.0848       
   crypto-sha1                            2.3213+-0.0602    ?     2.3260+-0.0753       ?
   date-format-tofte                     10.3116+-0.2344    ?    10.4378+-0.2828       ? might be 1.0122x slower
   date-format-xparb                      8.4533+-0.2499    !     9.1867+-0.2216       ! definitely 1.0868x slower
   math-cordic                            6.4422+-0.1535    ^     6.1436+-0.1015       ^ definitely 1.0486x faster
   math-partial-sums                      7.5496+-0.1807          7.5091+-0.1570       
   math-spectral-norm                     2.6941+-0.0547          2.6068+-0.0639         might be 1.0335x faster
   regexp-dna                            10.9435+-0.1595         10.9386+-0.3098       
   string-base64                          5.9421+-0.2571          5.7747+-0.1178         might be 1.0290x faster
   string-fasta                           7.2544+-0.2391          7.2107+-0.2189       
   string-tagcloud                       12.1558+-0.3529    ?    12.4166+-0.3872       ? might be 1.0215x slower
   string-unpack-code                    18.8967+-0.5173    ?    19.1674+-0.5064       ? might be 1.0143x slower
   string-validate-input                  6.5833+-0.2060    ?     6.6301+-0.2131       ?

   <arithmetic>                           6.4867+-0.0486    ?     6.5420+-0.0324       ?
   <geometric>                            5.3690+-0.0395    ?     5.4003+-0.0259       ?
   <harmonic>                             4.4092+-0.0436    ?     4.4385+-0.0324       ?

                                            TipOfTree               JettisonCB                                   
V8:
   crypto                                74.6046+-0.4070    ^    72.2672+-0.7195       ^ definitely 1.0323x faster
   deltablue                            225.3433+-1.7379    !   236.4885+-1.9668       ! definitely 1.0495x slower
   earley-boyer                          92.6612+-0.3991    !    93.8201+-0.3120       ! definitely 1.0125x slower
   raytrace                              64.1926+-0.8124    ?    65.7157+-0.7643       ? might be 1.0237x slower
   regexp                               108.8970+-1.1601        107.2254+-0.9922         might be 1.0156x faster
   richards                             201.0203+-1.6227        199.9424+-1.4012       
   splay                                 99.7791+-0.9625    ?    99.9235+-1.1515       ?

   <arithmetic>                         123.7854+-0.3773    !   125.0547+-0.4416       ! definitely 1.0103x slower
   <geometric>                          111.7982+-0.2680    ?   112.3245+-0.2848       ?
   <harmonic>                           102.3350+-0.3228    ?   102.5040+-0.2686       ?

                                            TipOfTree               JettisonCB                                   
Kraken:
   ai-astar                             616.7712+-6.1676    ?   645.2968+-46.3494      ? might be 1.0462x slower
   audio-beat-detection                 473.3009+-3.6924    ?   475.1594+-1.1196       ?
   audio-dft                            451.4413+-8.3904    ^   431.5748+-10.9037      ^ definitely 1.0460x faster
   audio-fft                            365.7535+-3.3272    !   370.9031+-1.0182       ! definitely 1.0141x slower
   audio-oscillator                     397.7187+-1.8295    ^   257.9338+-1.5908       ^ definitely 1.5419x faster
   imaging-darkroom                     418.8900+-1.2035    !   427.1599+-5.2615       ! definitely 1.0197x slower
   imaging-desaturate                   220.7046+-1.7507    ^   209.8197+-1.0989       ^ definitely 1.0519x faster
   imaging-gaussian-blur                579.1448+-2.5212    !   601.6267+-4.1585       ! definitely 1.0388x slower
   json-parse-financial                  49.3908+-0.4090    ^    48.4404+-0.3049       ^ definitely 1.0196x faster
   json-stringify-tinderbox              67.7263+-0.3530    !    69.1170+-0.8160       ! definitely 1.0205x slower
   stanford-crypto-aes                  141.1874+-1.5834        138.7216+-1.0138         might be 1.0178x faster
   stanford-crypto-ccm                  112.6815+-1.0384    ^   109.8747+-0.7353       ^ definitely 1.0255x faster
   stanford-crypto-pbkdf2               376.7632+-2.9107    ^   210.0397+-2.0894       ^ definitely 1.7938x faster
   stanford-crypto-sha256-iterative      85.6741+-0.6838    !    98.8062+-5.4666       ! definitely 1.1533x slower

   <arithmetic>                         311.2249+-0.8779    ^   292.4624+-4.0149       ^ definitely 1.0642x faster
   <geometric>                          236.9719+-0.3964    ^   222.2244+-1.5810       ^ definitely 1.0664x faster
   <harmonic>                           164.4145+-0.4336    ^   159.6218+-0.9891       ^ definitely 1.0300x faster

                                            TipOfTree               JettisonCB                                   
All benchmarks:
   <arithmetic>                         114.7298+-0.2931    ^   109.3606+-1.2316       ^ definitely 1.0491x faster
   <geometric>                           26.0741+-0.1104    ^    25.6799+-0.1002       ^ definitely 1.0153x faster
   <harmonic>                             7.7682+-0.0751    ?     7.8152+-0.0554       ?

[Filip Pizlo]

Another set of numbers, after I retuned heuristics.



Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc
"JettisonCB" at /Volumes/Data/pizlo/octonary/OpenSource/WebKitBuild/Release/jsc

Collected 30 samples per benchmark/VM, with 10 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               JettisonCB                                   
SunSpider:
   3d-cube                                7.8287+-0.1801          7.7857+-0.1290       
   3d-morph                               7.4884+-0.0773    ?     7.5740+-0.0923       ? might be 1.0114x slower
   3d-raytrace                            7.7084+-0.1338    ?     7.7715+-0.1481       ?
   access-binary-trees                    2.3010+-0.0647    ?     2.3496+-0.0664       ? might be 1.0211x slower
   access-fannkuch                       11.7645+-0.1926         11.7477+-0.1384       
   access-nbody                           3.5246+-0.0608    ?     3.5652+-0.0426       ? might be 1.0115x slower
   access-nsieve                          2.6844+-0.0391          2.6315+-0.0415         might be 1.0201x faster
   bitops-3bit-bits-in-byte               1.7098+-0.0180    !     1.8508+-0.0221       ! definitely 1.0825x slower
   bitops-bits-in-byte                    2.8851+-0.0596          2.8684+-0.0465       
   bitops-bitwise-and                     3.6722+-0.0491          3.6144+-0.0580         might be 1.0160x faster
   bitops-nsieve-bits                     5.3017+-0.0575          5.2930+-0.0508       
   controlflow-recursive                  2.0053+-0.0217    !     2.2261+-0.0320       ! definitely 1.1101x slower
   crypto-aes                             6.8837+-0.1819    ?     7.1632+-0.1964       ? might be 1.0406x slower
   crypto-md5                             2.7919+-0.0505    ?     2.8382+-0.0552       ? might be 1.0166x slower
   crypto-sha1                            2.2869+-0.0389    ?     2.3167+-0.0327       ? might be 1.0131x slower
   date-format-tofte                     10.2693+-0.1475    ?    10.4425+-0.1998       ? might be 1.0169x slower
   date-format-xparb                      8.5748+-0.1433    !     8.9042+-0.1078       ! definitely 1.0384x slower
   math-cordic                            6.2616+-0.0786    ?     6.3709+-0.2127       ? might be 1.0175x slower
   math-partial-sums                      7.3857+-0.0780    ?     7.4007+-0.0649       ?
   math-spectral-norm                     2.6995+-0.0357          2.6817+-0.0456       
   regexp-dna                            11.0040+-0.1446         10.8350+-0.0903         might be 1.0156x faster
   string-base64                          5.8660+-0.1334    ?     5.8988+-0.1038       ?
   string-fasta                           7.1117+-0.1103          7.1099+-0.0881       
   string-tagcloud                       12.2060+-0.2481         11.9507+-0.1893         might be 1.0214x faster
   string-unpack-code                    18.6964+-0.2333    ?    18.8706+-0.2691       ?
   string-validate-input                  6.5685+-0.1292          6.4310+-0.0923         might be 1.0214x faster

   <arithmetic>                           6.4415+-0.0217    ?     6.4805+-0.0219       ?
   <geometric>                            5.3131+-0.0157    !     5.3704+-0.0167       ! definitely 1.0108x slower
   <harmonic>                             4.3488+-0.0208    !     4.4321+-0.0200       ! definitely 1.0192x slower

                                            TipOfTree               JettisonCB                                   
V8:
   crypto                                73.6284+-0.4434    ^    71.4785+-0.3800       ^ definitely 1.0301x faster
   deltablue                            226.9347+-1.8829    !   234.8437+-1.0423       ! definitely 1.0349x slower
   earley-boyer                          92.7009+-0.1703    !    93.5526+-0.2832       ! definitely 1.0092x slower
   raytrace                              63.5931+-0.2230    !    65.9612+-0.4625       ! definitely 1.0372x slower
   regexp                               107.0856+-0.3063    ^   105.8491+-0.2453       ^ definitely 1.0117x faster
   richards                             199.1884+-0.7157        198.4284+-0.3962       
   splay                                 98.6054+-0.4033    ?    98.6821+-0.3222       ?

   <arithmetic>                         123.1052+-0.3103    !   124.1137+-0.2069       ! definitely 1.0082x slower
   <geometric>                          110.9591+-0.2091    !   111.5271+-0.1670       ! definitely 1.0051x slower
   <harmonic>                           101.4372+-0.1854    !   101.8516+-0.1787       ! definitely 1.0041x slower

                                            TipOfTree               JettisonCB                                   
Kraken:
   ai-astar                             617.5456+-3.9583    !   651.3233+-28.0772      ! definitely 1.0547x slower
   audio-beat-detection                 472.5390+-1.3604        472.4413+-0.8757       
   audio-dft                            421.9768+-1.8677        420.5146+-1.2780       
   audio-fft                            362.8723+-0.7421    !   369.4401+-0.6687       ! definitely 1.0181x slower
   audio-oscillator                     395.1065+-0.5801    ^   256.0258+-0.6453       ^ definitely 1.5432x faster
   imaging-darkroom                     416.9501+-0.7603    !   422.6921+-2.3323       ! definitely 1.0138x slower
   imaging-desaturate                   218.0053+-0.5017    ^   209.1474+-0.3462       ^ definitely 1.0424x faster
   imaging-gaussian-blur                577.9357+-0.9834    !   594.7917+-1.6422       ! definitely 1.0292x slower
   json-parse-financial                  48.9424+-0.2693         48.7625+-0.2108       
   json-stringify-tinderbox              67.5042+-0.1889    !    68.2908+-0.3009       ! definitely 1.0117x slower
   stanford-crypto-aes                  140.8445+-0.3586    ^   137.5372+-0.4014       ^ definitely 1.0240x faster
   stanford-crypto-ccm                  110.5309+-0.6147        109.6368+-0.5200       
   stanford-crypto-pbkdf2               371.6750+-1.0003    ^   207.7955+-0.7730       ^ definitely 1.7887x faster
   stanford-crypto-sha256-iterative      85.3478+-0.2156         85.3476+-0.2550       

   <arithmetic>                         307.6983+-0.3252    ^   289.5533+-2.1522       ^ definitely 1.0627x faster
   <geometric>                          234.4093+-0.2309    ^   218.6223+-0.7991       ^ definitely 1.0722x faster
   <harmonic>                           162.9500+-0.2738    ^   156.3084+-0.3215       ^ definitely 1.0425x faster

                                            TipOfTree               JettisonCB                                   
All benchmarks:
   <arithmetic>                         113.5530+-0.0900    ^   108.3199+-0.6472       ^ definitely 1.0483x faster
   <geometric>                           25.8106+-0.0427    ^    25.4498+-0.0588       ^ definitely 1.0142x faster
   <harmonic>                             7.6628+-0.0358    !     7.8014+-0.0344       ! definitely 1.0181x slower

[Filip Pizlo]

Created attachment 108101 [details]
the patch

I'll let this simmer here for a bit.  I'm expecting bot failures.

[WebKit Review Bot]

Attachment 108101 [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/heap/Heap.h:103:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:2001:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:2002:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/heap/ConservativeRoots.h:41:  The parameter name "codeBlocks" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 19 files


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

[Filip Pizlo]

Created attachment 108102 [details]
the patch

Fixed conflict that caused a file to get dropped from the patch.  Fixed style.

[WebKit Review Bot]

Attachment 108102 [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/heap/Heap.h:103:  The parameter name "codeBlock" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


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

[Filip Pizlo]

Created attachment 108103 [details]
the patch - fix more style

[Early Warning System Bot]

Comment on attachment 108103 [details]
the patch - fix more style

Attachment 108103 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9767540

[Filip Pizlo]

Created attachment 108104 [details]
the patch

Apparently the last patch did not have enough kilobytes in it.  Added necessary build stuff to the various project files.

[Sam Weinig]

Comment on attachment 108104 [details]
the patch

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

> Source/JavaScriptCore/heap/JettisonedCodeBlocks.cpp:18
> +/*
> + *  Copyright (C) 2011 Apple Inc. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */

We usually use a BSD license for new code.

> Source/JavaScriptCore/heap/JettisonedCodeBlocks.h:18
> +/*
> + *  Copyright (C) 2011 Apple Inc. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */

We usually use a BSD license for new code.

> Source/JavaScriptCore/wtf/BitVector.h:27
> +/*
> + * Copyright (C) 2010, 2011 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

We usually use the 2 clause BSD license (http://www.webkit.org/coding/bsd-license.html) for new code.

[Filip Pizlo]

Created attachment 108106 [details]
the patch - fix license

Changed the licenses on the three new files to 2-clause BSD.

[Oliver Hunt]

Comment on attachment 108106 [details]
the patch - fix license

I'm getting sick of these #if ENABLE(JIT_VERBOSE_OSR) blocks, can we please add a #define JIT_OSR_LOG(...

[Oliver Hunt]

Comment on attachment 108106 [details]
the patch - fix license

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

Alas i won't finish reviewing this tonight -- i'm a little tired to follow the logic for keeping the jettisoned code blocks live for the right amount of time :-(

It may be worth making code block refcounted though if you think that would simplify things

> Source/JavaScriptCore/heap/JettisonedCodeBlocks.h:73
> +    // It would be great to use an OwnPtr<CodeBlock> here but that would
> +    // almost certainly not work.
> +    HashMap<CodeBlock*, bool> m_map;

Why can't we make CodeBlock RefCounted?  Then just use RefPtr<CodeBlock> -- a single word for the ref count vs. the hulking mass of code block doesn't seem too expensive

[Filip Pizlo]

(In reply to comment #17)
> (From update of attachment 108106 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108106&action=review
> 
> Alas i won't finish reviewing this tonight -- i'm a little tired to follow the logic for keeping the jettisoned code blocks live for the right amount of time :-(
> 
> It may be worth making code block refcounted though if you think that would simplify things
> 
> > Source/JavaScriptCore/heap/JettisonedCodeBlocks.h:73
> > +    // It would be great to use an OwnPtr<CodeBlock> here but that would
> > +    // almost certainly not work.
> > +    HashMap<CodeBlock*, bool> m_map;
> 
> Why can't we make CodeBlock RefCounted?  Then just use RefPtr<CodeBlock> -- a single word for the ref count vs. the hulking mass of code block doesn't seem too expensive

There is a benefit to the way it works now: I know when a CodeBlock gets destroyed by looking at the code.

[Geoffrey Garen]

Comment on attachment 108106 [details]
the patch - fix license

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

Some good stuff here, but the ConservativeRoots thing needs reworking, and some refactoring would improve clarity.

> Source/JavaScriptCore/heap/ConservativeRoots.cpp:72
> +    m_codeBlocks->mark(p);

Doing a full pointer-hashed lookup on every pointer defeats a lot of prior optimization work on conservative root gathering, which was a win at the time. It also kind of confuses the role of the ConservativeRoots class, since the ConservativeRoots class is used to gather roots form anywhere -- currently, the C stack or the JS stack -- but you're only interested in CodeBlock entries in the JS stack (and then, only in call frames).

You should rework this.

One option is just to wait and blow away all jettisoned CodeBlocks unconditionally when exiting the VM. (In some cases, this is better, since there's no guarantee that a given VM invocation that fails lots of speculations will ever run the GC.)

Another option is to do a specific scan for CodeBlocks in the JS stack if and only if the set of jettisoned CodeBlocks reaches a threshold. (This scan can become much more efficient in the future, once JC's top-of-stack work is done, and we have exact backtrace information.)

Or you can combine the two options.

> Source/JavaScriptCore/heap/Heap.cpp:397
> +void Heap::jettisonCodeBlock(PassOwnPtr<CodeBlock> codeBlock)

Since this doesn't actually do the jettisoning, I'd call it "addJettisonedCodeBlock" instead.

> Source/JavaScriptCore/heap/Heap.h:197
> +        JettisonedCodeBlocks m_codeBlocks;

"m_jettisonedCodeBlocks", please. Otherwise, m_codeBlocks reads like it's the set of all CodeBlocks.

> Source/JavaScriptCore/heap/JettisonedCodeBlocks.cpp:42
> +    HashMap<CodeBlock*, bool>::iterator begin = m_map.begin();
> +    HashMap<CodeBlock*, bool>::iterator end = m_map.end();
> +    for (HashMap<CodeBlock*, bool>::iterator iter = begin; iter != end; ++iter)
> +        delete iter->first;

Would deleteAllKeys work here?

> Source/JavaScriptCore/heap/JettisonedCodeBlocks.h:72
> +    // It would be great to use an OwnPtr<CodeBlock> here but that would
> +    // almost certainly not work.

Yeah, HashMap doesn't support OwnPtr.

> Source/JavaScriptCore/wtf/BitVector.h:57
> +        : m_bitsOrPointer(static_cast<uintptr_t>(1) << maxInlineBits())

Since tagging bits with the inline tag comes up a lot, I'd suggest a helper function: uintptr_t makeInlineBits(uintptr_t bits). So, this would turn into a call to makeInlineBits(0). The helper function can also ASSERT that bits does not already have the tag bit set.

> Source/JavaScriptCore/wtf/BitVector.h:90
> +    size_t bounds() const

Since this is modified by resize() and ensureSize(), "size()" is probably a better name for it.

> Source/JavaScriptCore/wtf/BitVector.h:112
> +            m_bitsOrPointer = (static_cast<uintptr_t>(1) << maxInlineBits()) | *myOutOfLineBits->bits();

This would become "makeInlineBits(*myOutOfLineBits->bits())", which seems cleaner.

> Source/JavaScriptCore/wtf/BitVector.h:120
> +    void clear()

I think this would read a little better as "clearAll", matching Bitmap::clearAll. Our other collection types treat clear as setSize(0), so seeing clear() right after setSize(n) in client code is pretty weird.

> Source/JavaScriptCore/wtf/BitVector.h:125
> +            bzero(outOfLineBits()->bits(), bounds() >> 3);

>> 3 would be a little less magical as a helper function: size_t byteCount(size_t bitCount); Seems to come up a lot.

[Filip Pizlo]

So this discussion uncovered a bug: JettisonedCodeBlocks traces all CodeBlocks it has, not just the ones that were marked.  I'll fix that.

But that observation also uncovers why your two proposals (blowing away JettisonedCodeBlocks on VM exit and/or having a separate mini-GC for JettisonedCodeBlocks) will cause a memory leak.

When a GC happens, we want all dead state to go away, within the limits of conservative scanning.  CodeBlocks can refer to a lot of stuff.  So we only want to scan them if they're live.  That means marking those jettisoned CodeBlocks that are found to be referenced from the RegisterFile.  Your two proposals, as I understand them, won't do this, since they both imply scanning the RegisterFile for CodeBlocks in a separate phase that is not part of the GC.  This means that the GC risks marking too much stuff, and hence, leaking memory.  A memory leak is likely to be less performant than calling JettisonedCodeBlocks::mark() in ConservativeRoots.

I see two options that are robust, and that address some of your concerns:

1) Add a markCodeBlocks(JettisonedCodeBlocks&) method to RegisterFile.  This can have a fast check to see if JettisonedCodeBlocks is empty, in which case it does no work.  This is probably the easiest conceptually, and will probably perform just fine.

2) Add an enum parameter to ConservativeRoots::add() that says if you're looking at a RegisterFile.  This is the best option.  ConservativeRoots already does a linear scan over the RegisterFile.  It's most efficient to have that scan include CodeBlock detection, so we're only touching each location in the RegisterFile once during a collection.  If that enum tells ConservativeRoots::add() that it's looking at the RegisterFile, it can go and look at JettisonedCodeBlocks.  Otherwise it can ignore them.  This parameter could even be a template parameter, or something like that, to ensure that we don't do extra checks when scanning the C stack.

There's a separate question of whether or not to add the bloom filter (or similar) optimizations to JettisonedCodeBlocks.  I don't think that will be necessary.  JettisonedCodeBlocks should be small or empty.  If it's empty then in (1) we can just bail out immediately.  It's more likely to be small, but not empty, though, since the continuous optimization system does seem to want to recompile stuff in roughly half of our benchmarks.  A small set of JettisonedCodeBlocks means a small hashtable, which is likely to be no slower than a bloom filter.

If JettisonedCodeBlocks gets big, then it means that we've already lost, since it could only happen if a lot of recompilation occurred.  My goal is to ensure that this doesn't happen.  This should already be the case, because of the exponential back-off in recompilation that is already part of this patch.

[Filip Pizlo]

Created attachment 108112 [details]
the patch - fix review

I addressed Geoff's review, mostly.  I believe that the GC must mark jettisoned CodeBlocks.  I changed this so that ConservativeMarker no longer marks CodeBlocks unconditionally; it will only do so if add() is passed a JettisonedCodeBlocks.  Internally this is templatized, so we could support any number of different "MarkHooks" in the ConservativeMarker.

All of the other issues are addressed.

I haven't addressed Oliver's two concerns (RefCounted on CodeBlock and logging).  I'll look at the logging tomorrow.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1

Source/JavaScriptCore/heap/ConservativeRoots.h:55:  The parameter name "markHook" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/heap/ConservativeRoots.h:58:  The parameter name "markHook" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 27 files


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

[Gustavo Noronha (kov)]

Comment on attachment 108112 [details]
the patch - fix review

Attachment 108112 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9774617

[Filip Pizlo]

Created attachment 108113 [details]
the patch - fix style, attempt to fix GTK

[Gustavo Noronha (kov)]

Comment on attachment 108113 [details]
the patch - fix style, attempt to fix GTK

Attachment 108113 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9773576

[Filip Pizlo]

Created attachment 108116 [details]
the patch - another GTK attempt

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1

Source/JavaScriptCore/wtf/BitVector.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 27 files


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

[Filip Pizlo]

Created attachment 108117 [details]
the patch - fix style

[Geoffrey Garen]

Comment on attachment 108117 [details]
the patch - fix style

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

Looks like the Windows build is still angry:

2>c:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\wtf/BitVector.h(31) : fatal error C1083: Cannot open include file: 'strings.h': No such file or directory

> CodeBlocks can refer to a lot of stuff.  So we only want to scan them if they're live.

Fair point.

> It's most efficient to have that scan include CodeBlock detection, so we're only touching each location in the RegisterFile once during a collection.

Another fair point.

I'm happy with the solution you came up with. (Thinking about this, it occurred to me that, long-term, if we GC-allocate CodeBlocks, the normal conservative scan will "just work".)

r- for the Windows build.

> Source/JavaScriptCore/heap/Heap.cpp:504
> +    m_jettisonedCodeBlocks.traceCodeBlocks(visitor);

This should be followed by visitor.drain().

> Source/JavaScriptCore/heap/Heap.h:103
> +        void addJettisonCodeBlock(PassOwnPtr<CodeBlock>);

Typo: Should be "addJettisonedCodeBlock", not "addJettisonCodeBlock".

> Source/JavaScriptCore/wtf/BitVector.h:96
> +        if (m_bitsOrPointer >> maxInlineBits())

Should be "isInline()".

[Filip Pizlo]

Created attachment 108215 [details]
the patch - fix review, windows

Fixed the vaerious typos that Geoff found.  Fixed Windows, hopefully.

[Geoffrey Garen]

Comment on attachment 108215 [details]
the patch - fix review, windows

Windows build failure:
2>c:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\wtf/BitVector.h(132) : error C3861: 'bzero': identifier not found

[Filip Pizlo]

Created attachment 108230 [details]
the patch - another attempt to fix Windows

Today I learned that bzero is deprecated.

[Geoffrey Garen]

Comment on attachment 108230 [details]
the patch - another attempt to fix Windows

I'm guessing this will build. Hope I don't jinx it.

[Filip Pizlo]

(In reply to comment #33)
> (From update of attachment 108230 [details])
> I'm guessing this will build. Hope I don't jinx it.

Thanks, I'll wait until it's happy.

[Filip Pizlo]

Landed in r95681.

[Eric Seidel (no email)]

Comment on attachment 108230 [details]
the patch - another attempt to fix Windows

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

> Source/JavaScriptCore/heap/JettisonedCodeBlocks.h:58
> +        // This checks for both of those nasty cases in one go.
> +        // 0 + 1 = 1
> +        // -1 + 1 = 0
> +        if (value + 1 <= 1)
> +            return;

Why not just value < 1 ?  I guess ptr_t is unsigned?

[Filip Pizlo]

(In reply to comment #36)
> (From update of attachment 108230 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108230&action=review
> 
> > Source/JavaScriptCore/heap/JettisonedCodeBlocks.h:58
> > +        // This checks for both of those nasty cases in one go.
> > +        // 0 + 1 = 1
> > +        // -1 + 1 = 0
> > +        if (value + 1 <= 1)
> > +            return;
> 
> Why not just value < 1 ?  I guess ptr_t is unsigned?

value is a uintptr_t, so yes.  value < 1 would only check for the zero case.