[webkit-reviews] review granted: [Bug 186416] Add support for dumping GC heap snapshots, and a viewer : [Attachment 342231] Patch

Tue Jul 31 16:31:05 PDT 2018

Joseph Pecoraro <joepeck at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 186416: Add support for dumping GC heap snapshots, and a viewer
https://bugs.webkit.org/show_bug.cgi?id=186416

Attachment 342231: Patch

https://bugs.webkit.org/attachment.cgi?id=342231&action=review

--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 342231
  --> https://bugs.webkit.org/attachment.cgi?id=342231
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342231&action=review

This looks good to me. The HeapSnapshot JSON modifications are well contained,
but I'd like to see the extra work only performed during GCDebugging snapshots.
I'd suggest someone who understands the WebCore opaque roots process more give
this a look over.

If (1) you've found this useful (2) and think you'll use it again in the future
then I think its worth landing. r=me

> Source/JavaScriptCore/ChangeLog:16
> +		SlotVisitor maintains a RootMarkReason (via
SetRootMarkReasonScope) that allows

Weird indentation

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:57
> +    if (m_snapshotType == SnapshotType::GCDebuggingSnapshot)

This deserves the comment that was in the ChangeLog. GCDebugging snapshots are
always full and aren't diffed, so we can clear previous snapshots and take 1
full snapshot.

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:98
> +    if (!from) {

Seems like you can only collect rootData for GCDebuggingSnapshot and I'd
probably flip these so that in the normal path this is 1 check:

    if (m_snapshotType == SnapshotType::GCDebuggingSnapshot) {
	...
    }

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:140
> +void HeapSnapshotBuilder::setOpaqueRootReachabilityReasonForCell(JSCell*
cell, const char* reason)

Likewise you could early return if not GCDebuggingSnapshot.

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:155
> +bool HeapSnapshotBuilder::previousSnapshotHasNodeForCell(JSCell* cell,
NodeIdentifier& identifier)

Better name!

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:202
> +//	   ],
>  //	}

A bunch of these notes don't match up. I see I had already forgotten
nodeClassnameIndex myself...

How about just including a 2nd section for the debugging snapshot? The
intermixed is getting a little messy:

    {
	"version": 1.0,
	"type": "Inspector",
	...
    }

    {
	"version": 1.0,
	"type": "GCDebugging",
	...
    }

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:305
> +    if (cell->isString())
> +	   return emptyString(); // FIXME: get part of string.

Interesting idea. I suppose you could substring the first part if you wanted.
Depends on how often the string part reveals something that the rest of the
object tree doesn't reveal.

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:333
> +    labelIndexes.set("", 0);

Nit: Maybe emptyString() instead of ""?

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:361
> +	       String nodeLabel;

I'd want to see much of this work being done inside of an
`SnapshotType::GCDebuggingSnapshot` block.

Building and sending this JSON already takes a bunch of memory. I'd hate for
this debugging only data to cause problems during an Inspector snapshot where
it ends up not being used. Worst case would be a Jetsam when building the JSON
which we've seen in some cases since the entire JSON gets built in one string.
I've had ideas to split it out, and essentially stream it, to reduce peak
memory consumption. But also this introduces work on each node, so a graph with
a lot of nodes gets hit with more overhead.

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:390
> +	   // <nodeId>, <sizeInBytes>, <nodeClassNameIndex>, <internal>,
[<labelIndex>, <cellEddress>, <wrappedEddress>]

Typo: "Eddress" => "Address"

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:586
> +	       // Maybe we should just always encode the root names.

I don't like Maybe comments. It seems you've already done deduplication so I
think it's good to just remove the comment. Deduplication saves a lot of space
in JSON, and you've already done it!

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:609
> +    }
> +
> +    if (m_snapshotType == SnapshotType::GCDebuggingSnapshot) {

No need to close and reopen the same if block.

> Source/JavaScriptCore/heap/WeakHandleOwner.h:37
> +    // reason will only be non-null when generating a debug GC heap
snapshot.

Comment sentences start with a capital so this looks weird. Maybe "The `reason`
parameter will ..."

> Source/WebCore/bindings/js/GCController.cpp:64
> +	   PAL::registerNotifyCallback("com.apple.WebKit.dumpGCHeap", [] {
> +	       GCController::singleton().dumpHeap();
> +	   });

I can't help but think we might want to protect against someone just spamming
this notification at Safari's Web Content Processes.

I can just imagine right now a scenario where someone takes an Inspector Heap
Snapshot, and then something triggers this notification, which clears the
previous backend snapshots. This would break the web inspector (we shouldn't
crash, we should just error with each command). Thats a pretty unlikely
scenario, but just another reason to consider isolating this surface.

> Source/WebCore/bindings/js/GCController.cpp:145
> +    FileSystem::PlatformFileHandle fileHandle;
> +    String tempFilePath =
FileSystem::openTemporaryFile(ASCIILiteral("GCHeap"), fileHandle);
> +    if (!FileSystem::isHandleValid(fileHandle)) {

This is unique per-WebContentProcess / WebKitLegacy process. It might be nice
if it had the PID in the name but I suppose the WTFLogAlways will include that?

> Source/WebCore/bindings/js/GCController.cpp:157
> +	   DeferGCForAWhile deferGC(vm.heap);

So HeapSnapshotBuilder does its own GC, and json() does its own defer. Does
this defer do anything vital in addition to those? If so I'd like to see a
comment, and maybe we should be doing the same at the other builder sites?
Otherwise this DeferGC seems very sketchy (especially given performing snapshot
does a GC!)

> Source/WebCore/bindings/js/JSDOMTokenListCustom.cpp:36
> +void JSDOMTokenList::heapSnapshot(JSCell* cell, JSC::HeapSnapshotBuilder&
builder)

We already have JSDocument below (which doesn't even include a "doc" prefix).
What is JSDOMTokenList and why does it get such special treatment?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4555
> +	       push(@implContent, "	}\n");

Nit: Incorrect leading whitespace in the generated content string.

> Source/WebCore/html/DOMTokenList.idl:29
> +    CustomHeapSnapshot

Add that trailing comma! Makes the next diff so nice and clean. Also it looks
like some effort was made to make some of these alphabetical order. Up to you I
guess if you think thats a worthwhile goal.

> Tools/GCHeapInspector/gc-heap-inspector.html:26
> +	       top: 20px;	     right: 20px;

Style: 1 property per line

> Tools/GCHeapInspector/gc-heap-inspector.html:156
> +    <script>
> +
> +    </script>

Heh

> Tools/GCHeapInspector/heap-analysis/HeapSnapshot.js:97
> +	   console.assert(type === "GCDebugging", "Expect a GCDebugging-type
snapshot");

Can we do this in:
Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js

There we would want to be backwards compatible and do:

    console.assert(!type || type === "Inspector, "Inspect an Inspector Heap
Snapshot");

> Tools/GCHeapInspector/heap-analysis/HeapSnapshot.js:167
> +	   let {liveSize, categories} =
HeapSnapshot.updateCategoriesAndMetadata(this);
> +	   this._liveSize = liveSize;
> +	   this._categories = categories;

You could also remove this stuff. Its just processing everything to get some
displayable metadata you probably don't use.

> Tools/GCHeapInspector/heap-analysis/HeapSnapshot.js:851
> +HeapSnapshotDiff = class HeapSnapshotDiff

You could remove this as well, you don't use it.