Bug 68329

Summary:

DFG should support continuous optimization

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, fpizlo, ggaren, gustavo, oliver, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

68335

Bug Blocks:

Attachments:

Description	Flags
work in progress	none
work in progress - ruggedized it some more	none
the patch	none
the patch	none
the patch - fix more style	webkit-ews: commit-queue-
the patch	none
the patch - fix license	ggaren: review-
the patch - fix review	gustavo: commit-queue-
the patch - fix style, attempt to fix GTK	gustavo: commit-queue-
the patch - another GTK attempt	none
the patch - fix style	ggaren: review-
the patch - fix review, windows	ggaren: review-
the patch - another attempt to fix Windows	ggaren: review+

Filip Pizlo

Reported 2011-09-18 17:54:21 PDT

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.

Attachments
work in progress (38.48 KB, patch) 2011-09-18 23:20 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress - ruggedized it some more (57.68 KB, patch) 2011-09-20 14:26 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (55.81 KB, patch) 2011-09-20 19:41 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (60.82 KB, patch) 2011-09-20 19:45 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - fix more style (60.98 KB, patch) 2011-09-20 19:55 PDT, Filip Pizlo	webkit-ews: commit-queue-	Details Formatted Diff Diff
the patch (65.91 KB, patch) 2011-09-20 20:17 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - fix license (66.69 KB, patch) 2011-09-20 21:20 PDT, Filip Pizlo	ggaren: review-	Details Formatted Diff Diff
the patch - fix review (69.78 KB, patch) 2011-09-21 00:16 PDT, Filip Pizlo	gustavo: commit-queue-	Details Formatted Diff Diff
the patch - fix style, attempt to fix GTK (69.79 KB, patch) 2011-09-21 00:33 PDT, Filip Pizlo	gustavo: commit-queue-	Details Formatted Diff Diff
the patch - another GTK attempt (69.86 KB, patch) 2011-09-21 00:49 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - fix style (69.86 KB, patch) 2011-09-21 00:54 PDT, Filip Pizlo	ggaren: review-	Details Formatted Diff Diff
the patch - fix review, windows (69.89 KB, patch) 2011-09-21 13:05 PDT, Filip Pizlo	ggaren: review-	Details Formatted Diff Diff
the patch - another attempt to fix Windows (69.84 KB, patch) 2011-09-21 14:04 PDT, Filip Pizlo	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2011-09-18 19:35:40 PDT

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

Comment 2 2011-09-18 22:23:52 PDT

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

Comment 3 2011-09-18 23:20:03 PDT

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

Comment 4 2011-09-20 14:26:30 PDT

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

Comment 5 2011-09-20 16:19:08 PDT

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

Comment 6 2011-09-20 19:18:14 PDT

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

Comment 7 2011-09-20 19:41:18 PDT

Created attachment 108101 [details] the patch I'll let this simmer here for a bit. I'm expecting bot failures.

WebKit Review Bot

Comment 8 2011-09-20 19:43:18 PDT

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

Comment 9 2011-09-20 19:45:14 PDT

Created attachment 108102 [details] the patch Fixed conflict that caused a file to get dropped from the patch. Fixed style.

WebKit Review Bot

Comment 10 2011-09-20 19:50:49 PDT

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

Comment 11 2011-09-20 19:55:34 PDT

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

Early Warning System Bot

Comment 12 2011-09-20 20:09:28 PDT

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

Comment 13 2011-09-20 20:17:26 PDT

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 14 2011-09-20 20:45:00 PDT

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

Comment 15 2011-09-20 21:20:38 PDT

Created attachment 108106 [details] the patch - fix license Changed the licenses on the three new files to 2-clause BSD.

Oliver Hunt

Comment 16 2011-09-20 21:36:20 PDT

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 17 2011-09-20 21:52:45 PDT

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

Comment 18 2011-09-20 22:25:52 PDT

(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 19 2011-09-20 22:47:25 PDT

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

Comment 20 2011-09-20 23:32:12 PDT

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

Comment 21 2011-09-21 00:16:17 PDT

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

Comment 22 2011-09-21 00:18:37 PDT

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 23 2011-09-21 00:28:34 PDT

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

Comment 24 2011-09-21 00:33:29 PDT

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

Gustavo Noronha (kov)

Comment 25 2011-09-21 00:45:16 PDT

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

Comment 26 2011-09-21 00:49:07 PDT

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

WebKit Review Bot

Comment 27 2011-09-21 00:52:46 PDT

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

Comment 28 2011-09-21 00:54:51 PDT

Created attachment 108117 [details] the patch - fix style

Geoffrey Garen

Comment 29 2011-09-21 10:49:01 PDT

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

Comment 30 2011-09-21 13:05:19 PDT

Created attachment 108215 [details] the patch - fix review, windows Fixed the vaerious typos that Geoff found. Fixed Windows, hopefully.

Geoffrey Garen

Comment 31 2011-09-21 14:03:55 PDT

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

Comment 32 2011-09-21 14:04:21 PDT

Created attachment 108230 [details] the patch - another attempt to fix Windows Today I learned that bzero is deprecated.

Geoffrey Garen

Comment 33 2011-09-21 14:12:22 PDT

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

Comment 34 2011-09-21 14:13:51 PDT

(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

Comment 35 2011-09-21 16:36:51 PDT

Landed in r95681.

Eric Seidel (no email)

Comment 36 2011-09-27 12:44:41 PDT

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

Comment 37 2011-09-27 12:48:44 PDT

(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.

Note You need to log in before you can comment on or make changes to this bug.