Bug 158054 – Web Inspector: capturing with Allocations timeline causes GC to take 100x longer and cause frame drops

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

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.

[Blaze Burg]

<rdar://problem/25280762>

[Saam Barati]

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.

[Saam Barati]

Created attachment 280101 [details]
WIP for Brian to try out

[Saam Barati]

Created attachment 280108 [details]
WIP for Brian # 2

[Saam Barati]

Created attachment 280109 [details]
WIP for Brian # 3

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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

[Saam Barati]

Created attachment 280157 [details]
patch

[Joseph Pecoraro]

Comment on attachment 280157 [details]
patch

r=me!

[Saam Barati]

Created attachment 280161 [details]
patch for landing

[Saam Barati]

Created attachment 280163 [details]
patch for landing

Fixed capitalization on my name

[Build Bot]

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.

[Build Bot]

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

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/201520

[Saam Barati]

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