Bug 50799

Summary: Add Memory Sampler to WebKit
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, darin, slewis, s.mathur, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.6   
Attachments:
Description Flags
patch
ggaren: review-
reworked patch
slewis: review-
fix qt
ggaren: review-
remove more cf types
none
fix typo
slewis: review-
patch
none
patch ggaren: review+

Description Stephanie Lewis 2010-12-09 19:59:12 PST
Add a tool for sampling memory use for the webkit process.
Comment 1 Stephanie Lewis 2010-12-09 20:27:23 PST
Created attachment 76157 [details]
patch
Comment 2 WebKit Review Bot 2010-12-10 08:07:46 PST
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.
Comment 3 Geoffrey Garen 2010-12-14 15:57:32 PST
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.
Comment 4 Stephanie Lewis 2011-01-04 17:34:30 PST
Created attachment 77952 [details]
reworked patch
Comment 5 Early Warning System Bot 2011-01-04 17:45:39 PST
Attachment 77952 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7321429
Comment 6 WebKit Review Bot 2011-01-04 17:56:53 PST
Attachment 77952 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7301409
Comment 7 Stephanie Lewis 2011-01-04 18:00:31 PST
Created attachment 77957 [details]
fix qt
Comment 8 Early Warning System Bot 2011-01-04 18:19:15 PST
Attachment 77957 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7213388
Comment 9 Build Bot 2011-01-04 18:29:00 PST
Attachment 77957 [details] did not build on win:
Build output: http://queues.webkit.org/results/7268405
Comment 10 Build Bot 2011-01-04 18:31:43 PST
Attachment 77952 [details] did not build on win:
Build output: http://queues.webkit.org/results/7300389
Comment 11 Geoffrey Garen 2011-01-04 18:37:31 PST
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.
Comment 12 WebKit Review Bot 2011-01-04 19:13:41 PST
Attachment 77957 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7266413
Comment 13 Stephanie Lewis 2011-01-04 21:06:22 PST
Created attachment 77965 [details]
remove more cf types
Comment 14 Early Warning System Bot 2011-01-04 21:17:38 PST
Attachment 77965 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7334333
Comment 15 Stephanie Lewis 2011-01-04 21:22:32 PST
Created attachment 77969 [details]
fix typo
Comment 16 Build Bot 2011-01-04 21:27:13 PST
Attachment 77965 [details] did not build on win:
Build output: http://queues.webkit.org/results/7307430
Comment 17 Build Bot 2011-01-04 21:43:27 PST
Attachment 77969 [details] did not build on win:
Build output: http://queues.webkit.org/results/7335301
Comment 18 Early Warning System Bot 2011-01-04 21:47:26 PST
Attachment 77969 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7261428
Comment 19 Stephanie Lewis 2011-01-04 23:08:06 PST
Created attachment 77971 [details]
patch
Comment 20 Early Warning System Bot 2011-01-04 23:20:07 PST
Attachment 77971 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7199409
Comment 21 Stephanie Lewis 2011-01-04 23:48:49 PST
Created attachment 77976 [details]
patch
Comment 22 Geoffrey Garen 2011-01-05 22:43:00 PST
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.
Comment 23 Stephanie Lewis 2011-01-06 01:26:22 PST
> 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.
Comment 24 Siddharth Mathur 2011-04-19 13:55:07 PDT
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.