Bug 67354

Summary:

ValueProfile does not make it safe to introspect cell values after garbage collection

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, fpizlo, ggaren, oliver, rniwa, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
the patch	none
the patch - fix style, build, conflicts	none
the patch - simplifications	none
the patch - fix a silly bug	barraclough: review+, barraclough: commit-queue-
the patch - fix review	none
the patch - fix conflict	webkit.review.bot: commit-queue-
the patch - fix more conflicts	none

Filip Pizlo

Reported 2011-08-31 20:28:05 PDT

The ValueProfile class holds 8 recent values seen at a particular JavaScript bytecode site. But if a value is a JSCell pointer, then after a garbage collection the pointer may no longer be valid. This prevents clients of the profiler from doing introspection of values beyond checking if they are cells. For example, it's not currently safe to check if a value is an array. The ValueProfile should weakly reference JSCells, but should do it intelligently, so that if the JSCell is deleted by the collector, we still have some information about it: like, what was its structure, or if the structure is also deleted, then what is its class.

Attachments
the patch (24.04 KB, patch) 2011-08-31 20:32 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - fix style, build, conflicts (22.71 KB, patch) 2011-08-31 20:37 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - simplifications (21.68 KB, patch) 2011-08-31 22:30 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - fix a silly bug (21.92 KB, patch) 2011-08-31 23:41 PDT, Filip Pizlo	barraclough: review+ barraclough: commit-queue-	Details Formatted Diff Diff
the patch - fix review (22.00 KB, patch) 2011-09-02 15:59 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - fix conflict (22.30 KB, patch) 2011-09-02 19:42 PDT, Filip Pizlo	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
the patch - fix more conflicts (22.05 KB, patch) 2011-09-02 21:06 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2011-08-31 20:32:20 PDT

Created attachment 105892 [details] the patch Still running tests, so this is somewhat of a work in progress.

WebKit Review Bot

Comment 2 2011-08-31 20:34:02 PDT

Attachment 105892 [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.cpp:591: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/heap/WeakReferenceHarvester.h:75: "stdint.h" already included at Source/JavaScriptCore/heap/WeakReferenceHarvester.h:23 [build/include] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 3 2011-08-31 20:37:22 PDT

Created attachment 105893 [details] the patch - fix style, build, conflicts

Filip Pizlo

Comment 4 2011-08-31 21:02:47 PDT

Comment on attachment 105893 [details] the patch - fix style, build, conflicts Passes tests, and does not break the web. Still plan to do some benchmarking and some more tests of various permutations of profiling enabled. But it's ready for review.

Filip Pizlo

Comment 5 2011-08-31 21:42:10 PDT

Comment on attachment 105893 [details] the patch - fix style, build, conflicts Tests pass and the web is usable no matter what permutation of value profiling options I use. One task left: check performance.

Filip Pizlo

Comment 6 2011-08-31 22:30:49 PDT

Created attachment 105895 [details] the patch - simplifications Simplified the WeakReferenceHarvester API

Filip Pizlo

Comment 7 2011-08-31 22:31:32 PDT

This patch is performance-neutral. Only differences are ~0.1% and not statistically significant. Benchmark report for SunSpider, V8, and Kraken. VMs tested: "TipOfTree" at /Volumes/Data/fromMiniMe/quinary/OpenSource/WebKitBuild/Release/jsc "ValueProfileOff" at /Volumes/Data/fromMiniMe/septenary/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 ValueProfileOff SunSpider: 3d-cube 8.2470+-0.0678 ? 8.2512+-0.0164 ? 3d-morph 8.1477+-0.0334 8.1383+-0.0191 3d-raytrace 8.2481+-0.0654 ? 8.3196+-0.0619 ? access-binary-trees 2.5407+-0.0381 ? 2.5602+-0.0468 ? access-fannkuch 12.9529+-0.0845 12.9174+-0.0933 access-nbody 4.9614+-0.0487 4.9296+-0.0202 access-nsieve 3.0567+-0.0331 ? 3.0604+-0.0294 ? bitops-3bit-bits-in-byte 1.8573+-0.0223 1.8571+-0.0231 bitops-bits-in-byte 5.6315+-0.1239 5.6254+-0.1335 bitops-bitwise-and 4.0975+-0.0087 4.0974+-0.0040 bitops-nsieve-bits 5.6903+-0.0320 ? 5.7280+-0.0336 ? controlflow-recursive 2.2887+-0.0390 ? 2.2975+-0.0341 ? crypto-aes 6.8440+-0.0369 ? 6.8944+-0.0393 ? crypto-md5 3.0490+-0.0421 3.0027+-0.0277 might be 1.0154x faster crypto-sha1 2.4497+-0.0374 ? 2.4579+-0.0359 ? date-format-tofte 10.8528+-0.0671 10.8410+-0.0551 date-format-xparb 9.5181+-0.1963 ? 9.5994+-0.1459 ? math-cordic 7.1568+-0.0921 ? 7.1719+-0.1217 ? math-partial-sums 10.6718+-0.0497 ? 10.6829+-0.0345 ? math-spectral-norm 2.7385+-0.0336 2.7294+-0.0277 regexp-dna 12.0845+-0.1535 12.0535+-0.1141 string-base64 6.5590+-0.1419 6.4790+-0.0848 might be 1.0123x faster string-fasta 8.2849+-0.0352 ? 8.3050+-0.0290 ? string-tagcloud 15.0656+-0.0444 14.9926+-0.0728 string-unpack-code 20.8166+-0.1082 20.7653+-0.0622 string-validate-input 7.3637+-0.3009 ? 7.6644+-0.2635 ? might be 1.0408x slower <arithmetic> 7.3529+-0.0327 ? 7.3624+-0.0254 ? <geometric> 6.0956+-0.0316 ? 6.1052+-0.0280 ? <harmonic> 4.9775+-0.0342 ? 4.9834+-0.0324 ? TipOfTree ValueProfileOff V8: crypto 102.7447+-0.2485 ? 103.1249+-0.2542 ? deltablue 298.6366+-3.7481 ? 299.4765+-2.1650 ? earley-boyer 123.2329+-0.4334 122.7717+-0.3203 raytrace 87.9043+-0.9485 87.6650+-0.1515 regexp 130.8935+-0.4964 ! 131.9634+-0.1790 ! definitely 1.0082x slower richards 302.3704+-1.4350 ? 302.8294+-1.5887 ? splay 155.8481+-1.6318 ? 156.0731+-0.9100 ? <arithmetic> 171.6615+-0.4166 ? 171.9863+-0.3329 ? <geometric> 153.9464+-0.1610 ? 154.1990+-0.1916 ? <harmonic> 140.1632+-0.2503 ? 140.3450+-0.1274 ? TipOfTree ValueProfileOff Kraken: ai-astar 1668.8957+-15.1645 1658.5114+-18.9078 audio-beat-detection 542.8552+-3.1601 540.6212+-3.8099 audio-dft 454.6839+-2.3303 ? 454.8397+-2.0129 ? audio-fft 420.1154+-0.7348 ! 423.3887+-1.6151 ! definitely 1.0078x slower audio-oscillator 403.3020+-0.8085 ? 404.7480+-0.9289 ? imaging-darkroom 600.3491+-16.2923 ? 613.3811+-10.6584 ? might be 1.0217x slower imaging-desaturate 636.5449+-17.4696 636.1784+-17.4582 imaging-gaussian-blur 1859.3143+-3.9366 1855.5966+-4.3500 json-parse-financial 61.4857+-0.2132 ! 63.2649+-0.1963 ! definitely 1.0289x slower json-stringify-tinderbox 76.2311+-0.4255 75.8158+-0.2337 stanford-crypto-aes 165.7540+-0.4441 ? 166.3629+-0.5324 ? stanford-crypto-ccm 131.5271+-0.6480 ? 132.6696+-0.9418 ? stanford-crypto-pbkdf2 374.2624+-1.6078 ? 374.7365+-1.2595 ? stanford-crypto-sha256-iterative 144.7434+-0.6826 144.1186+-0.2453 <arithmetic> 538.5760+-1.2589 ? 538.8738+-2.0228 ? <geometric> 339.8348+-0.8563 ? 341.1243+-1.0454 ? <harmonic> 212.5037+-0.4490 ! 214.1243+-0.4311 ! definitely 1.0076x slower TipOfTree ValueProfileOff All benchmarks: <arithmetic> 190.0611+-0.3985 ? 190.2034+-0.5963 ? <geometric> 32.6614+-0.1152 ? 32.7347+-0.1047 ? <harmonic> 8.8026+-0.0593 ? 8.8137+-0.0562 ?

Filip Pizlo

Comment 8 2011-08-31 22:41:18 PDT

This is the overhead of value profiling with this patch, with DFG turned off. As in, it's the overhead that baseline (before tier-up) code will experience. Benchmark report for SunSpider, V8, and Kraken. VMs tested: "TipOfTree" at /Volumes/Data/fromMiniMe/quinary/OpenSource/WebKitBuild/Release/jsc "ValueProfileOff" at /Volumes/Data/fromMiniMe/septenary/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 ValueProfileOff SunSpider: 3d-cube 8.8595+-0.0179 ! 9.2453+-0.0312 ! definitely 1.0435x slower 3d-morph 8.1416+-0.0307 ? 8.2134+-0.0447 ? 3d-raytrace 9.4134+-0.0248 ! 10.4939+-0.0476 ! definitely 1.1148x slower access-binary-trees 2.2512+-0.0197 ! 2.5353+-0.0231 ! definitely 1.1262x slower access-fannkuch 12.9424+-0.0835 ! 13.6188+-0.0511 ! definitely 1.0523x slower access-nbody 7.4584+-0.0102 ! 8.0224+-0.0572 ! definitely 1.0756x slower access-nsieve 3.8109+-0.0260 ? 3.8529+-0.0177 ? might be 1.0110x slower bitops-3bit-bits-in-byte 2.5576+-0.0398 ! 2.6751+-0.0281 ! definitely 1.0459x slower bitops-bits-in-byte 6.6623+-0.1132 ^ 6.0954+-0.1115 ^ definitely 1.0930x faster bitops-bitwise-and 4.1196+-0.0338 ? 4.1530+-0.0457 ? bitops-nsieve-bits 5.6830+-0.0388 ! 5.9572+-0.0279 ! definitely 1.0482x slower controlflow-recursive 2.2253+-0.0410 ! 2.5391+-0.0337 ! definitely 1.1410x slower crypto-aes 7.1616+-0.0281 ! 7.6889+-0.0542 ! definitely 1.0736x slower crypto-md5 3.1589+-0.0465 ! 3.4858+-0.0329 ! definitely 1.1035x slower crypto-sha1 2.6565+-0.0305 ! 2.9111+-0.0303 ! definitely 1.0958x slower date-format-tofte 11.7682+-0.0902 ! 12.1948+-0.0730 ! definitely 1.0362x slower date-format-xparb 9.6990+-0.1297 ? 10.0291+-0.2465 ? might be 1.0340x slower math-cordic 7.3453+-0.3298 ? 7.3599+-0.0501 ? math-partial-sums 10.4958+-0.0438 10.4889+-0.0372 math-spectral-norm 4.4546+-0.0169 ! 4.7794+-0.0634 ! definitely 1.0729x slower regexp-dna 12.0460+-0.1184 ? 12.0732+-0.1311 ? string-base64 5.7189+-0.0666 ! 6.0201+-0.0766 ! definitely 1.0527x slower string-fasta 7.9478+-0.0248 ! 8.0574+-0.0268 ! definitely 1.0138x slower string-tagcloud 15.2231+-0.0732 ? 15.3147+-0.0470 ? string-unpack-code 20.5997+-0.0690 ! 20.9489+-0.1046 ! definitely 1.0170x slower string-validate-input 7.0806+-0.0941 ? 7.2559+-0.1048 ? might be 1.0247x slower <arithmetic> 7.6724+-0.0358 ! 7.9235+-0.0279 ! definitely 1.0327x slower <geometric> 6.5102+-0.0354 ! 6.7849+-0.0279 ! definitely 1.0422x slower <harmonic> 5.4322+-0.0355 ! 5.7459+-0.0297 ! definitely 1.0577x slower TipOfTree ValueProfileOff V8: crypto 206.8537+-0.4849 ! 221.1158+-0.9468 ! definitely 1.0689x slower deltablue 267.2850+-0.8388 ! 318.9918+-1.8254 ! definitely 1.1935x slower earley-boyer 126.5721+-0.3546 ! 139.5270+-0.5612 ! definitely 1.1024x slower raytrace 86.8362+-1.7937 ! 93.1722+-0.8733 ! definitely 1.0730x slower regexp 131.3743+-0.5879 ? 132.2735+-0.5262 ? richards 279.6937+-0.7931 ! 312.9718+-1.1768 ! definitely 1.1190x slower splay 155.0793+-1.1745 ? 157.0248+-1.3658 ? might be 1.0125x slower <arithmetic> 179.0992+-0.4639 ! 196.4396+-0.5115 ! definitely 1.0968x slower <geometric> 165.9162+-0.6798 ! 179.3075+-0.4987 ! definitely 1.0807x slower <harmonic> 153.3871+-0.9578 ! 163.9957+-0.5743 ! definitely 1.0692x slower TipOfTree ValueProfileOff Kraken: ai-astar 2072.9013+-30.9877 ! 2196.3937+-15.3871 ! definitely 1.0596x slower audio-beat-detection 539.6634+-2.2724 ! 568.2251+-1.1907 ! definitely 1.0529x slower audio-dft 454.8677+-4.2642 ! 488.1209+-3.1658 ! definitely 1.0731x slower audio-fft 421.0421+-0.9320 ! 442.3705+-0.5048 ! definitely 1.0507x slower audio-oscillator 379.6714+-5.3440 ! 411.9139+-1.8822 ! definitely 1.0849x slower imaging-darkroom 590.8199+-7.1784 ? 597.6895+-3.8731 ? might be 1.0116x slower imaging-desaturate 617.4223+-2.4087 ! 649.5490+-4.5528 ! definitely 1.0520x slower imaging-gaussian-blur 2176.1665+-7.1226 ! 2388.3892+-8.3513 ! definitely 1.0975x slower json-parse-financial 61.9086+-0.2256 ! 62.5446+-0.2643 ! definitely 1.0103x slower json-stringify-tinderbox 76.1372+-0.2654 75.5563+-0.3188 stanford-crypto-aes 166.0477+-0.6312 ! 181.3763+-0.5867 ! definitely 1.0923x slower stanford-crypto-ccm 132.4801+-1.1672 ! 143.9892+-0.2739 ! definitely 1.0869x slower stanford-crypto-pbkdf2 434.5020+-2.4759 ! 441.8006+-2.5230 ! definitely 1.0168x slower stanford-crypto-sha256-iterative 160.7851+-0.5561 ! 165.1758+-0.5175 ! definitely 1.0273x slower <arithmetic> 591.7440+-2.0751 ! 629.5067+-1.1978 ! definitely 1.0638x slower <geometric> 353.0441+-0.4950 ! 370.7383+-0.4695 ! definitely 1.0501x slower <harmonic> 216.3901+-0.4013 ! 223.6948+-0.3026 ! definitely 1.0338x slower TipOfTree ValueProfileOff All benchmarks: <arithmetic> 207.1828+-0.6364 ! 221.1528+-0.3620 ! definitely 1.0674x slower <geometric> 34.6434+-0.1193 ! 36.3830+-0.0929 ! definitely 1.0502x slower <harmonic> 9.5985+-0.0615 ! 10.1506+-0.0515 ! definitely 1.0575x slower

Filip Pizlo

Comment 9 2011-08-31 22:42:10 PDT

(In reply to comment #8) > TipOfTree ValueProfileOff That's a typo - it should be ValueProfileOn.

Filip Pizlo

Comment 10 2011-08-31 22:59:16 PDT

Comment on attachment 105895 [details] the patch - simplifications Everything looks good.

Filip Pizlo

Comment 11 2011-08-31 23:41:09 PDT

Created attachment 105903 [details] the patch - fix a silly bug Fixed a problem where numberOfSamples() neglected to count buckets that got weakened.

Gavin Barraclough

Comment 12 2011-09-02 14:30:54 PDT

Comment on attachment 105903 [details] the patch - fix a silly bug Per discussion please fix the two ptr&flags types to work the same way, either +1/-1 or |1/&~1.

Filip Pizlo

Comment 13 2011-09-02 15:59:38 PDT

Created attachment 106213 [details] the patch - fix review

Filip Pizlo

Comment 14 2011-09-02 19:42:48 PDT

Created attachment 106239 [details] the patch - fix conflict

Filip Pizlo

Comment 15 2011-09-02 19:46:37 PDT

Comment on attachment 106239 [details] the patch - fix conflict Looks like tests are still passing and conflicts are resolved ... ready to land.

WebKit Review Bot

Comment 16 2011-09-02 21:02:29 PDT

Comment on attachment 106239 [details] the patch - fix conflict Rejecting attachment 106239 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run step(tool, options).run(state) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run if self._has_valid_reviewer(changelog_entry): File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer if changelog_entry.reviewer(): AttributeError: 'NoneType' object has no attribute 'reviewer' Full output: http://queues.webkit.org/results/9583839

Filip Pizlo

Comment 17 2011-09-02 21:06:46 PDT

Created attachment 106242 [details] the patch - fix more conflicts

WebKit Review Bot

Comment 18 2011-09-02 22:13:12 PDT

Comment on attachment 106242 [details] the patch - fix more conflicts Clearing flags on attachment: 106242 Committed r94477: <http://trac.webkit.org/changeset/94477>

WebKit Review Bot

Comment 19 2011-09-02 22:13:17 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 20 2011-09-02 22:57:57 PDT

It seems like this broke a whole bunch of tests. http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/1984 http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/25709 http://build.webkit.org/builders/Qt%20Linux%20Release/builds/37117

Filip Pizlo

Comment 21 2011-09-02 23:15:48 PDT

(In reply to comment #20) > It seems like this broke a whole bunch of tests. > http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/1984 > http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/25709 > http://build.webkit.org/builders/Qt%20Linux%20Release/builds/37117 It looks to me like the change started earlier, possibly r94457. http://build.webkit.org/builders/Qt%20Linux%20Release?numbuilds=50

Ryosuke Niwa

Comment 22 2011-09-02 23:59:52 PDT

(In reply to comment #21) > (In reply to comment #20) > > It seems like this broke a whole bunch of tests. > > http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/1984 > > http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/25709 > > http://build.webkit.org/builders/Qt%20Linux%20Release/builds/37117 > > It looks to me like the change started earlier, possibly r94457. > http://build.webkit.org/builders/Qt%20Linux%20Release?numbuilds=50 You're right. I was fooled about build failures. It appears that the failure is caused by http://trac.webkit.org/changeset/94453.

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