Bug 165375 - [Cocoa] Add some memory usage related information to sysdiagnose state dumps
Summary: [Cocoa] Add some memory usage related information to sysdiagnose state dumps
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
Keywords: InRadar, Performance
Depends on:
Reported: 2016-12-05 08:52 PST by Andreas Kling
Modified: 2016-12-06 14:39 PST (History)
3 users (show)

See Also:

Patch (9.05 KB, patch)
2016-12-05 09:04 PST, Andreas Kling
darin: review+
Details | Formatted Diff | Diff
Patch for landing (10.33 KB, patch)
2016-12-05 10:53 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Follow-up patch for landing (3.92 KB, patch)
2016-12-06 13:12 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2016-12-05 08:52:15 PST
Comment 1 Andreas Kling 2016-12-05 09:04:20 PST
Created attachment 296141 [details]
Comment 2 Darin Adler 2016-12-05 09:17:27 PST
Comment on attachment 296141 [details]

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]];

Comment 3 Andreas Kling 2016-12-05 10:53:22 PST
Created attachment 296159 [details]
Patch for landing

Thanks Darin! Here's a patch with fixes for the things you pointed out.
Comment 4 WebKit Commit Bot 2016-12-05 13:31:49 PST
Comment on attachment 296159 [details]
Patch for landing

Clearing flags on attachment: 296159

Committed r209346: <http://trac.webkit.org/changeset/209346>
Comment 5 WebKit Commit Bot 2016-12-05 13:31:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2016-12-05 15:57:19 PST
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]];
Comment 7 Andreas Kling 2016-12-06 13:12:56 PST
Created attachment 296312 [details]
Follow-up patch for landing

Eep! Fix Darin's two remaining comments as well.
Comment 8 Andreas Kling 2016-12-06 14:02:34 PST
Reopening to land follow-up
Comment 9 WebKit Commit Bot 2016-12-06 14:39:11 PST
Comment on attachment 296312 [details]
Follow-up patch for landing

Clearing flags on attachment: 296312

Committed r209423: <http://trac.webkit.org/changeset/209423>
Comment 10 WebKit Commit Bot 2016-12-06 14:39:15 PST
All reviewed patches have been landed.  Closing bug.