Summary: | Web Inspector: capturing with Allocations timeline causes GC to take 100x longer and cause frame drops | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||||||||||||
Component: | Web Inspector | Assignee: | Saam Barati <saam> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bburg, buildbot, commit-queue, graouts, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, rniwa, saam, timothy, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
URL: | http://bl.ocks.org/syntagmatic/6c149c08fc9cde682635 | ||||||||||||||||||
Attachments: |
|
Description
BJ Burg
2016-05-24 20:55:24 PDT
Is there a reason HeapSnapshot doesn't have a HashSet of JSCells' so it doesn't have to binary search every time? I ran some numbers and we only find something through the binary search 10% of the time on this website. So it's quite valuable to not binary search. Created attachment 280101 [details]
WIP for Brian to try out
Created attachment 280108 [details]
WIP for Brian # 2
Created attachment 280109 [details]
WIP for Brian # 3
Comment on attachment 280109 [details] WIP for Brian # 3 View in context: https://bugs.webkit.org/attachment.cgi?id=280109&action=review > Source/JavaScriptCore/heap/HeapSnapshot.h:56 > + uintptr_t m_filter { 0 }; Is this the same as TinyBloomFilter? Comment on attachment 280108 [details] WIP for Brian # 2 View in context: https://bugs.webkit.org/attachment.cgi?id=280108&action=review > Source/JavaScriptCore/heap/HeapSnapshot.h:56 > + HashMap<JSCell*, HeapSnapshotNode> m_nodes; I think this adds 30% extra memory use for a HeapSnapshot on the backend. HeapSnapshotNode is a (JSCell*, unsigned) and now this stores an extra pointer per-live-node. Keeping the memory use on the backend low is a concern to avoid jetsams when taking heap snapshots on iOS. (In reply to comment #2) > Is there a reason HeapSnapshot doesn't have a HashSet of JSCells' so it > doesn't have to binary search every time? The reason was to keep snapshots as small as possible. Since it keeps memory per-live-node, it grows with the number of objects in the heap. If a heap is large, and a snapshot requires a lot of memory, we won't succeed. > I ran some numbers and we only find something through the binary search > 10% of the time on this website. So it's quite valuable to not binary search. Good numbers to have! A filter (like your patch #3) was exactly what I was thinking we should do. Also, if needed, this work can be done in parallel. These snapshots, once finalized, can be safe to act on concurrently. For sweeping, each dead cell is unique, and sweeping marks the unique cell as dead and sets a shared flag in one direction (m_hasCellsToSweep = true). The shrinking can then done after the parallel work. Created attachment 280157 [details]
patch
Comment on attachment 280157 [details]
patch
r=me!
Created attachment 280161 [details]
patch for landing
Created attachment 280163 [details]
patch for landing
Fixed capitalization on my name
Comment on attachment 280163 [details] patch for landing Attachment 280163 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1413716 Number of test failures exceeded the failure limit. Created attachment 280167 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
landed in: http://trac.webkit.org/changeset/201520 (In reply to comment #14) > Created attachment 280167 [details] > Archive of layout-test-results from ews102 for mac-yosemite > > The attached test failures were seen while running run-webkit-tests on the > mac-ews. > Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5 These don't seem related at all to this patch. |