Bug 158054 - Web Inspector: capturing with Allocations timeline causes GC to take 100x longer and cause frame drops
Summary: Web Inspector: capturing with Allocations timeline causes GC to take 100x lon...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Saam Barati
URL: http://bl.ocks.org/syntagmatic/6c149c...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-24 20:55 PDT by BJ Burg
Modified: 2016-05-31 12:45 PDT (History)
14 users (show)

See Also:


Attachments
WIP for Brian to try out (2.84 KB, patch)
2016-05-30 10:07 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP for Brian # 2 (6.93 KB, patch)
2016-05-30 12:30 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP for Brian # 3 (1.59 KB, patch)
2016-05-30 12:59 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (3.95 KB, patch)
2016-05-31 11:16 PDT, Saam Barati
joepeck: review+
Details | Formatted Diff | Diff
patch for landing (4.12 KB, patch)
2016-05-31 11:40 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (3.99 KB, patch)
2016-05-31 11:53 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (996.52 KB, application/zip)
2016-05-31 12:30 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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.