WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
67354
ValueProfile does not make it safe to introspect cell values after garbage collection
https://bugs.webkit.org/show_bug.cgi?id=67354
Summary
ValueProfile does not make it safe to introspect cell values after garbage co...
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.
Top of Page
Format For Printing
XML
Clone This Bug