| Summary: | Add support for dumping GC heap snapshots, and a viewer | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
| Component: | New Bugs | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | commit-queue, dbates, ews-watchlist, joepeck, saam, sam, simon.fraser, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Simon Fraser (smfr)
2018-06-07 16:12:04 PDT
Created attachment 342231 [details]
Patch
Attachment 342231 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:16: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:17: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:18: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:19: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:20: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:21: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:22: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:23: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:24: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:25: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:26: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:27: Line contains tab character. [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5]
Total errors found: 14 in 174 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342231&action=review Will continue to look at this tomorrow, just one nit before I forget > Source/WebCore/bindings/js/GCController.cpp:163 > + sanitizeStackForVM(&vm); > + > + String jsonData; > + { > + DeferGCForAWhile deferGC(vm.heap); > + > + HeapSnapshotBuilder snapshotBuilder(vm.ensureHeapProfiler(), HeapSnapshotBuilder::SnapshotType::GCDebuggingSnapshot); > + snapshotBuilder.buildSnapshot(); > + > + jsonData = snapshotBuilder.json(); > + } nit: Seems like this could be a method on VM that returns to you a String Comment on attachment 342231 [details] 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. Comment on attachment 342231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342231&action=review > Source/WebCore/bindings/js/GCController.cpp:170 > + WTFLogAlways("Dumped GC heap to %s", tempFilePath.utf8().data()); Oh, we should probably clear the snapshot at this point. Nothing is going to go and access this, which is at least (sizeof(void*)*NumberOfNodesInHeap) bytes: vm.ensureHeapProfiler().clearSnapshots(); Created attachment 347981 [details]
Patch
Comment on attachment 347981 [details] Patch Attachment 347981 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/8965948 New failing tests: (JS) JSTestCallTracer.cpp (JS) JSTestCEReactions.cpp (JS) JSTestCEReactionsStringifier.cpp (JS) JSTestClassWithJSBuiltinConstructor.cpp (JS) JSTestCustomConstructorWithNoInterfaceObject.cpp (JS) JSTestActiveDOMObject.cpp (JS) JSTestDOMJIT.cpp (JS) JSTestEnabledBySetting.cpp (JS) JSTestEventConstructor.cpp (JS) JSTestEventTarget.cpp (JS) JSTestException.cpp (JS) JSTestGenerateIsReachable.cpp (JS) JSTestGlobalObject.cpp (JS) JSTestIndexedSetterNoIdentifier.cpp (JS) JSTestIndexedSetterThrowingException.cpp (JS) JSTestIndexedSetterWithIdentifier.cpp (JS) JSTestInterface.cpp (JS) JSTestInterfaceLeadingUnderscore.cpp (JS) JSTestIterable.cpp (JS) JSMapLike.cpp (JS) JSTestMediaQueryListListener.cpp (JS) JSTestNamedAndIndexedSetterNoIdentifier.cpp (JS) JSTestNamedAndIndexedSetterThrowingException.cpp (JS) JSTestNamedAndIndexedSetterWithIdentifier.cpp (JS) JSTestNamedConstructor.cpp (JS) JSTestNamedDeleterNoIdentifier.cpp (JS) JSTestNamedDeleterThrowingException.cpp (JS) JSTestNamedDeleterWithIdentifier.cpp (JS) JSTestNamedDeleterWithIndexedGetter.cpp (JS) JSTestNamedGetterCallWith.cpp (JS) JSTestNamedGetterNoIdentifier.cpp (JS) JSTestNamedGetterWithIdentifier.cpp (JS) JSTestNamedSetterNoIdentifier.cpp (JS) JSTestNamedSetterThrowingException.cpp (JS) JSTestNamedSetterWithIdentifier.cpp (JS) JSTestNamedSetterWithIndexedGetter.cpp (JS) JSTestNamedSetterWithIndexedGetterAndSetter.cpp (JS) JSTestNamedSetterWithOverrideBuiltins.cpp (JS) JSTestNamedSetterWithUnforgableProperties.cpp (JS) JSTestNamedSetterWithUnforgablePropertiesAndOverrideBuiltins.cpp (JS) JSTestNode.cpp (JS) JSTestObj.cpp (JS) JSTestOverloadedConstructors.cpp (JS) JSTestOverloadedConstructorsWithSequence.cpp (JS) JSTestOverrideBuiltins.cpp (JS) JSTestPluginInterface.cpp (JS) JSTestPromiseRejectionEvent.cpp (JS) JSReadOnlyMapLike.cpp (JS) JSInterfaceName.cpp (JS) JSTestSerialization.cpp (JS) JSTestSerializationIndirectInheritance.cpp (JS) JSTestSerializationInherit.cpp (JS) JSTestSerializationInheritFinal.cpp (JS) JSTestSerializedScriptValueInterface.cpp (JS) JSTestStringifier.cpp (JS) JSTestStringifierAnonymousOperation.cpp (JS) JSTestStringifierNamedOperation.cpp (JS) JSTestStringifierOperationImplementedAs.cpp (JS) JSTestStringifierOperationNamedToString.cpp (JS) JSTestStringifierReadOnlyAttribute.cpp (JS) JSTestStringifierReadWriteAttribute.cpp (JS) JSTestTypedefs.cpp Created attachment 347982 [details]
Patch
|