Summary: | [Cocoa] Add some memory usage related information to sysdiagnose state dumps | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||
Component: | WebCore Misc. | Assignee: | Andreas Kling <kling> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, kling | ||||||||
Priority: | P2 | Keywords: | InRadar, Performance | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Andreas Kling
2016-12-05 08:52:15 PST
Created attachment 296141 [details]
Patch
Comment on attachment 296141 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296141&action=review > Source/WebCore/page/PerformanceLogging.cpp:54 > stats.set("javascript_gc_heap_capacity", vm.heap.capacity()); We should use "add" instead of "set" once we change this so that it always creates a new HashMap. > Source/WebCore/page/PerformanceLogging.cpp:74 > + auto& vm = JSDOMWindow::commonVM(); > + return vm.heap.objectTypeCounts(); Why the local variable > Source/WebCore/page/PerformanceLogging.h:50 > + WEBCORE_EXPORT static std::unique_ptr<HashCountedSet<const char*>> javaScriptObjectCounts(); I think we should just return HashCountedSet<const char*>, not a unique_ptr. We can just move the thing out of the unique_ptr that is used internally, and eventually get rid of the unique_ptr entirely. > Source/WebCore/page/PerformanceLogging.h:51 > + WEBCORE_EXPORT static void getMemoryUsageStatistics(HashMap<const char*, size_t>&, ShouldIncludeExpensiveComputations); I suggest we use a return value here instead of an out argument. We used to avoid that but I think there is no longer any reason to do so. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:203 > + [memoryUsageStats setValue:[[NSNumber alloc] initWithUnsignedLongLong:it.value] forKey:[[NSString alloc] initWithUTF8String: it.key]]; Looks like the NSNumber and NSString both leak here; could fix that with autorelease or adoptNS. I don’t think we want setValue:forKey: because the setValue version is part of KVO or something like that. We want setObject:forKey: instead. If our minimum version of Objective-C is high enough I think we could use the @(it.value) syntax to create the NSNumber instead of alloc/init, and the x[key] = value syntax for the setObject:forKey: part. No space needed after the colon from initWithUTF8String: in our coding style. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:210 > + [jsObjectCounts setValue:[[NSNumber alloc] initWithUnsignedInt:it.value] forKey:[[NSString alloc] initWithUTF8String: it.key]]; Ditto. Created attachment 296159 [details]
Patch for landing
Thanks Darin! Here's a patch with fixes for the things you pointed out.
Comment on attachment 296159 [details] Patch for landing Clearing flags on attachment: 296159 Committed r209346: <http://trac.webkit.org/changeset/209346> All reviewed patches have been landed. Closing bug. Comment on attachment 296159 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=296159&action=review Sorry, was away from keyboard when you posted this earlier. Two small additional things. > Source/WebCore/page/PerformanceLogging.cpp:77 > + return *JSDOMWindow::commonVM().heap.objectTypeCounts(); I think we can avoid one copy of the set by writing: return WTFMove(*JSDOMWindow::commonVM().heap.objectTypeCounts()); I don’t think the compiler/language knows that the unique_ptr going away and that means we can do a move, but it can do it with our help. > Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:201 > + [memoryUsageStats setObject:@(it.value) forKey:[[[NSString alloc] initWithUTF8String:it.key] autorelease]]; A little inconsistent to use adoptNS two lines above, but use autorelease here. I would have used adoptNS here. But. if you do want to use autorelease, there’s a more compact way to write it: ... forKey:[NSString stringWithUTF8String:it.key]]; Created attachment 296312 [details]
Follow-up patch for landing
Eep! Fix Darin's two remaining comments as well.
Reopening to land follow-up Comment on attachment 296312 [details] Follow-up patch for landing Clearing flags on attachment: 296312 Committed r209423: <http://trac.webkit.org/changeset/209423> All reviewed patches have been landed. Closing bug. |