Bug 165375 – [Cocoa] Add some memory usage related information to sysdiagnose state dumps

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andreas Kling]

<rdar://problem/29057243>

[Andreas Kling]

Created attachment 296141 [details]
Patch

[Darin Adler]

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.

[Andreas Kling]

Created attachment 296159 [details]
Patch for landing

Thanks Darin! Here's a patch with fixes for the things you pointed out.

[WebKit Commit Bot]

Comment on attachment 296159 [details]
Patch for landing

Clearing flags on attachment: 296159

Committed r209346: <http://trac.webkit.org/changeset/209346>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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

[Andreas Kling]

Created attachment 296312 [details]
Follow-up patch for landing

Eep! Fix Darin's two remaining comments as well.

[Andreas Kling]

Reopening to land follow-up

[WebKit Commit Bot]

Comment on attachment 296312 [details]
Follow-up patch for landing

Clearing flags on attachment: 296312

Committed r209423: <http://trac.webkit.org/changeset/209423>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.