Bug 158054

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 InspectorAssignee: 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 Flags
WIP for Brian to try out
none
WIP for Brian # 2
none
WIP for Brian # 3
none
patch
joepeck: review+
patch for landing
none
patch for landing
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite none

Description BJ Burg 2016-05-24 20:55:24 PDT
STEPS TO REPRODUCE:

1. Make a recording without allocations timeline, note the smooth framerate.
2. Make a new recording with allocations timeline enabled.

EXPECTED:

No change in framerate, maybe a small increase in GC

ACTUAL:

GC time goes from < 1ms to > 60ms, causing visible stutters


NOTES:

The page continues to stutter even after recording is stopped, and after the allocations timeline is turned off. The latter seems to indicate a separate bug, where we don't turn off the extra tracking work during GC.
Comment 1 BJ Burg 2016-05-24 21:34:52 PDT
<rdar://problem/25280762>
Comment 2 Saam Barati 2016-05-29 17:04:02 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.
Comment 3 Saam Barati 2016-05-30 10:07:49 PDT
Created attachment 280101 [details]
WIP for Brian to try out
Comment 4 Saam Barati 2016-05-30 12:30:49 PDT
Created attachment 280108 [details]
WIP for Brian # 2
Comment 5 Saam Barati 2016-05-30 12:59:34 PDT
Created attachment 280109 [details]
WIP for Brian # 3
Comment 6 Joseph Pecoraro 2016-05-31 10:50:05 PDT
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 7 Joseph Pecoraro 2016-05-31 10:54:05 PDT
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.
Comment 8 Joseph Pecoraro 2016-05-31 11:02:52 PDT
(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.
Comment 9 Saam Barati 2016-05-31 11:16:40 PDT
Created attachment 280157 [details]
patch
Comment 10 Joseph Pecoraro 2016-05-31 11:22:52 PDT
Comment on attachment 280157 [details]
patch

r=me!
Comment 11 Saam Barati 2016-05-31 11:40:42 PDT
Created attachment 280161 [details]
patch for landing
Comment 12 Saam Barati 2016-05-31 11:53:50 PDT
Created attachment 280163 [details]
patch for landing

Fixed capitalization on my name
Comment 13 Build Bot 2016-05-31 12:29:58 PDT
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.
Comment 14 Build Bot 2016-05-31 12:30:04 PDT
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
Comment 15 Saam Barati 2016-05-31 12:44:21 PDT
landed in:
http://trac.webkit.org/changeset/201520
Comment 16 Saam Barati 2016-05-31 12:45:16 PDT
(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.