Bug 167285 – Add memory footprint reporting using diagnostic logging

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

[Andreas Kling]

We should have memory usage logging alongside the CPU usage logging for telemetry.

[Andreas Kling]

Created attachment 299446 [details]
Patch for EWS

[Andreas Kling]

Created attachment 299449 [details]
Patch for EWS

[Andreas Kling]

Created attachment 299498 [details]
Patch for EWS II

[Andreas Kling]

Created attachment 299501 [details]
Patch for EWS 4

[Andreas Kling]

Created attachment 299540 [details]
Patch

[Chris Dumez]

Does not build in Debug.

[Andreas Kling]

Created attachment 299545 [details]
Patch

[Chris Dumez]

Comment on attachment 299545 [details]
Patch

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

> Source/WebCore/page/PerformanceMonitor.cpp:177
> +        m_postBackgroundingCPUTime = std::nullopt;

muhahaha, you're resetting my CPU variables in your memory code.

> Source/WebCore/page/PerformanceMonitor.cpp:273
> +#if !RELEASE_LOG_DISABLED

This does not look needed?

> Source/WebCore/page/cocoa/PerformanceLoggingCocoa.mm:39
> +    if (err != KERN_SUCCESS)

err != success look weird. How about we call it result?

> Source/WebKit2/UIProcess/PerActivityStateMemoryUsageSampler.h:37
> +class PerActivityStateMemoryUsageSampler {

I would have merged this class with mine. PerActivityStatePerformanceSampler? Why didn't you do it?

[Chris Dumez]

Comment on attachment 299545 [details]
Patch

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

> Source/WebKit2/UIProcess/PerActivityStateMemoryUsageSampler.cpp:58
> +static inline String loggingKeyForActivityState(ActivityStateForPerformanceMonitoring state)

This is duplicated from my code.

> Source/WebKit2/UIProcess/PerActivityStateMemoryUsageSampler.cpp:88
> +WebPageProxy* PerActivityStateMemoryUsageSampler::pageForLogging() const

This is duplicated from my code.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5185
> +    fprintf(stderr, "DLog: %s ~ %s ~ %s\n", message.ascii().data(), description.ascii().data(), value.ascii().data());

I guess you did not mean to leave this in? :)

[Chris Dumez]

Comment on attachment 299545 [details]
Patch

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

>> Source/WebKit2/UIProcess/PerActivityStateMemoryUsageSampler.h:37
>> +class PerActivityStateMemoryUsageSampler {
> 
> I would have merged this class with mine. PerActivityStatePerformanceSampler? Why didn't you do it?

Or maybe WebPeformanceMetricsSampler

[Andreas Kling]

(In reply to comment #8)
> Comment on attachment 299545 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299545&action=review
> 
> > Source/WebCore/page/PerformanceMonitor.cpp:177
> > +        m_postBackgroundingCPUTime = std::nullopt;
> 
> muhahaha, you're resetting my CPU variables in your memory code.
> 
> > Source/WebCore/page/PerformanceMonitor.cpp:273
> > +#if !RELEASE_LOG_DISABLED
> 
> This does not look needed?

Ugh, copy/pasting code gone too far. :|

> > Source/WebCore/page/cocoa/PerformanceLoggingCocoa.mm:39
> > +    if (err != KERN_SUCCESS)
> 
> err != success look weird. How about we call it result?

Sure! That would read nicer.

> > Source/WebKit2/UIProcess/PerActivityStateMemoryUsageSampler.h:37
> > +class PerActivityStateMemoryUsageSampler {
> 
> I would have merged this class with mine.
> PerActivityStatePerformanceSampler? Why didn't you do it?

I'll merge them. And take out that printf :)

[Andreas Kling]

Created attachment 299567 [details]
Patch

[Andreas Kling]

Created attachment 299569 [details]
Patch

[Build Bot]

Comment on attachment 299569 [details]
Patch

Attachment 299569 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2938633

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-fullscreen.html

[Build Bot]

Created attachment 299571 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Andreas Kling]

Okay now that I've had some sleep & coffee let me actually fix up this patch properly :|

[Andreas Kling]

Created attachment 299572 [details]
Patch

Okay, now without the WK2 part since that logging didn't even make sense (combined footprints looked goofy and what am I gonna do with them anyway)
Also without the enum renames and stuff.

[Chris Dumez]

Comment on attachment 299572 [details]
Patch

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

r=me with comment

> Source/WebCore/page/PerformanceMonitor.cpp:90
> +        m_postPageLoadMemoryUsageTimer.startOneShot(memoryUsageMeasurementDelay);

You should probably stop this timer if a new Page load starts.

[Andreas Kling]

Committed r211120: <http://trac.webkit.org/changeset/211120>