Bug 167285 - Add memory footprint reporting using diagnostic logging
Summary: Add memory footprint reporting using diagnostic logging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-21 14:50 PST by Andreas Kling
Modified: 2017-01-24 16:16 PST (History)
2 users (show)

See Also:


Attachments
Patch for EWS (32.45 KB, patch)
2017-01-21 14:51 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch for EWS (32.46 KB, patch)
2017-01-21 15:17 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch for EWS II (39.68 KB, patch)
2017-01-22 21:51 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch for EWS 4 (39.70 KB, patch)
2017-01-22 22:21 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (44.43 KB, patch)
2017-01-23 14:39 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (43.92 KB, patch)
2017-01-23 15:14 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (46.20 KB, patch)
2017-01-23 19:36 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (45.49 KB, patch)
2017-01-23 20:13 PST, Andreas Kling
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-elcapitan (1.62 MB, application/zip)
2017-01-23 21:18 PST, Build Bot
no flags Details
Patch (12.49 KB, patch)
2017-01-23 21:24 PST, Andreas Kling
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2017-01-21 14:50:14 PST
We should have memory usage logging alongside the CPU usage logging for telemetry.
Comment 1 Andreas Kling 2017-01-21 14:51:31 PST
Created attachment 299446 [details]
Patch for EWS
Comment 2 Andreas Kling 2017-01-21 15:17:52 PST
Created attachment 299449 [details]
Patch for EWS
Comment 3 Andreas Kling 2017-01-22 21:51:55 PST
Created attachment 299498 [details]
Patch for EWS II
Comment 4 Andreas Kling 2017-01-22 22:21:19 PST
Created attachment 299501 [details]
Patch for EWS 4
Comment 5 Andreas Kling 2017-01-23 14:39:59 PST
Created attachment 299540 [details]
Patch
Comment 6 Chris Dumez 2017-01-23 14:55:19 PST
Does not build in Debug.
Comment 7 Andreas Kling 2017-01-23 15:14:35 PST
Created attachment 299545 [details]
Patch
Comment 8 Chris Dumez 2017-01-23 15:40:09 PST
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?
Comment 9 Chris Dumez 2017-01-23 15:44:08 PST
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? :)
Comment 10 Chris Dumez 2017-01-23 15:45:57 PST
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
Comment 11 Andreas Kling 2017-01-23 15:46:07 PST
(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 :)
Comment 12 Andreas Kling 2017-01-23 19:36:54 PST
Created attachment 299567 [details]
Patch
Comment 13 Andreas Kling 2017-01-23 20:13:46 PST
Created attachment 299569 [details]
Patch
Comment 14 Build Bot 2017-01-23 21:18:01 PST
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
Comment 15 Build Bot 2017-01-23 21:18:04 PST
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
Comment 16 Andreas Kling 2017-01-23 21:19:08 PST
Okay now that I've had some sleep & coffee let me actually fix up this patch properly :|
Comment 17 Andreas Kling 2017-01-23 21:24:27 PST
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.
Comment 18 Chris Dumez 2017-01-24 10:14:47 PST
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.
Comment 19 Andreas Kling 2017-01-24 16:16:24 PST
Committed r211120: <http://trac.webkit.org/changeset/211120>