Bug 50799 – Add Memory Sampler to WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Stephanie Lewis]

Add a tool for sampling memory use for the webkit process.

[Stephanie Lewis]

Created attachment 76157 [details]
patch

[WebKit Review Bot]

Attachment 76157 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/ChangeLog', u'WebKit/WebKit.xcodeproj/project.pbxproj', u'WebKit/mac/ChangeLog', u'WebKit/mac/Misc/WebKitMemorySampler.cpp', u'WebKit/mac/Misc/WebKitMemorySampler.h', u'WebKit/mac/Misc/WebKitMemorySampler.mac.mm', u'WebKit/mac/WebView/WebPreferenceKeysPrivate.h', u'WebKit/mac/WebView/WebPreferences.mm', u'WebKit/mac/WebView/WebPreferencesPrivate.h', u'WebKit/mac/WebView/WebView.mm']" exit_code: 1
WebKit/mac/Misc/WebKitMemorySampler.h:54:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/mac/Misc/WebKitMemorySampler.h:55:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/mac/Misc/WebKitMemorySampler.h:56:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/mac/Misc/WebKitMemorySampler.h:61:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/mac/Misc/WebKitMemorySampler.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/mac/Misc/WebKitMemorySampler.cpp:27:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/mac/Misc/WebKitMemorySampler.cpp:47:  Missing space before ( in if(  [whitespace/parens] [5]
WebKit/mac/Misc/WebKitMemorySampler.cpp:49:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/mac/Misc/WebKitMemorySampler.cpp:135:  Missing space before ( in if(  [whitespace/parens] [5]
WebKit/mac/Misc/WebKitMemorySampler.cpp:137:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/mac/Misc/WebKitMemorySampler.cpp:138:  Missing space before ( in if(  [whitespace/parens] [5]
WebKit/mac/Misc/WebKitMemorySampler.cpp:140:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/mac/Misc/WebKitMemorySampler.mac.mm:91:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 13 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 76157 [details]
patch

Since you don't want this setting to persist across launches of the application, I don't think a preference is the way to go. Instead, the current start/stop state should be stored as a variable, and toggling the setting should send a message to WebKit to start or stop. 

WebKitMemorySampler::shared() should not allocate anything in the case where nothing has been started -- that way, end users aren't impacted by this developer-only tool.

[Stephanie Lewis]

Created attachment 77952 [details]
reworked patch

[Early Warning System Bot]

Attachment 77952 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7321429

[WebKit Review Bot]

Attachment 77952 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7301409

[Stephanie Lewis]

Created attachment 77957 [details]
fix qt

[Early Warning System Bot]

Attachment 77957 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7213388

[Build Bot]

Attachment 77957 [details] did not build on win:
Build output: http://queues.webkit.org/results/7268405

[Build Bot]

Attachment 77952 [details] did not build on win:
Build output: http://queues.webkit.org/results/7300389

[Geoffrey Garen]

Comment on attachment 77957 [details]
fix qt

Still failing to build on non-CF platforms. You should either change to using cross-platform time functions, or #ifdef the code for CF platforms.

[WebKit Review Bot]

Attachment 77957 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7266413

[Stephanie Lewis]

Created attachment 77965 [details]
remove more cf types

[Early Warning System Bot]

Attachment 77965 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7334333

[Stephanie Lewis]

Created attachment 77969 [details]
fix typo

[Build Bot]

Attachment 77965 [details] did not build on win:
Build output: http://queues.webkit.org/results/7307430

[Build Bot]

Attachment 77969 [details] did not build on win:
Build output: http://queues.webkit.org/results/7335301

[Early Warning System Bot]

Attachment 77969 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7261428

[Stephanie Lewis]

Created attachment 77971 [details]
patch

[Early Warning System Bot]

Attachment 77971 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7199409

[Stephanie Lewis]

Created attachment 77976 [details]
patch

[Geoffrey Garen]

Comment on attachment 77976 [details]
patch

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

Please make sure to address these comments before landing.

> WebKit2/WebKit2Prefix.h:71
> +#if PLATFORM(MAC)
> +#define ENABLE_MEMORY_SAMPLER 1
> +#endif

I don't see this #define used anywhere. What's it for?

> WebKit2/Shared/WebMemorySampler.cpp:34
> +static WebMemorySampler *sharedMemorySampler = 0;

Should be "WebMemorySampler* ".

You can put this static inside WebMemorySampler::shared().

Since statics are implicitly initialized to 0, there's no need to explicitly assign 0 here.

> WebKit2/Shared/WebMemorySampler.cpp:137
> +    m_separator = String("\t");    

You should initialize m_separator in the WebMemorySampler constructor. It's difficult to reason about data members that are initialized at unpredictable times.

> WebKit2/Shared/WebMemorySampler.cpp:138
> +    String header = String(); 

A simpler way to say this is "String header;"

> WebKit2/Shared/WebMemorySampler.cpp:166
> +    String statString = String(); 

"String statString;"

> WebKit2/UIProcess/WebContext.cpp:208
> +        // way to get name of process from UI side?

What does this comment mean?

> WebKit2/UIProcess/WebContext.cpp:557
> +    

Our style is to exclude braces from one-line if statements.

> WebKit2/UIProcess/WebContext.cpp:580
> +    

Our style is to exclude braces from one-line if statements.

[Stephanie Lewis]

> WebKit2/WebKit2Prefix.h:71
> +#if PLATFORM(MAC)
> +#define ENABLE_MEMORY_SAMPLER 1
> +#endif

I don't see this #define used anywhere. What's it for?


The memory part of the MemorySampler currently only works on Mac, so I am only adding the files to the mac build.  There are a couple of call sites in WebContext.cpp and WebProcess.cpp that call into those classes and I wrapped those in the #enable.  I should probably wrap the MemorySampler files in the #define as well even though they aren't even compiled on other platforms.

[Siddharth Mathur]

Thank you for implementing this super useful feature. I may be interested in adding support for the Qt WebKit port on Linux and Symbian in the future. While Linux uses fastMalloc (IIRC), Symbian uses its own RAllocator derived allocator whose code is not in WebKit.org (lives in Qt Core actually). We also don't use printf on Symbian, but Qt's qDebug() as the latter writes to the trace channel.