Bug 67160 – Add a way to get JavaScript, memory, WebCore cache, and font statistics in WebProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Ada Chan]

This is analogous to WebCoreStatistics in WebKit1.

[Ada Chan]

Created attachment 105570 [details]
First patch which just lays the groundwork to fetch performance statistics from WebProcess.

I'll add code to actually fetch the WebCore statistics in future patches.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebKit2/UIProcess/WebContext.h:207:  The parameter name "statisticsData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 24 files


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

[Ada Chan]

Created attachment 105580 [details]
First patch which just lays the groundwork to fetch performance statistics from WebProcess.

Fixed an issue found by style-queue (removed the parameter name "statisticsData" from the declaration of WebContext::didGetWebCoreStatistics()).

[Sam Weinig]

Comment on attachment 105580 [details]
First patch which just lays the groundwork to fetch performance statistics from WebProcess.

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

> Source/WebKit2/UIProcess/API/C/WKContext.cpp:235
> +void WKContextGetStatistics(WKContextRef contextRef, WKContextGetStatisticsFunction callback, void* context)

In the other places where we have callback functions, we tend to have the context first, then the callback.  See WKPageGetContentsAsString

[Darin Adler]

Comment on attachment 105580 [details]
First patch which just lays the groundwork to fetch performance statistics from WebProcess.

I’m not sure the statistics result should come back as an object of its own custom class. Another way to do it is to return a dictionary with keys and values for the various statistics or use a struct. A reference counted object to hold the data doesn’t seem significantly better than those other two options.

[Ada Chan]

(In reply to comment #5)
> (From update of attachment 105580 [details])
> I’m not sure the statistics result should come back as an object of its own custom class. Another way to do it is to return a dictionary with keys and values for the various statistics or use a struct. A reference counted object to hold the data doesn’t seem significantly better than those other two options.

That's a good idea.  I'm going to rework the patch to eliminate the custom APIObject class for packaging the statistics.

[Ada Chan]

Created attachment 105675 [details]
First patch which just lays the groundwork to fetch performance statistics from WebProcess.

Patch was modified to return the statistics in a dictionary.

[Ada Chan]

Created attachment 105812 [details]
Gather JavaScript, FastMalloc, icon, font, and glyph page statistics in WebProcess::getWebCoreStatistics()

[Sam Weinig]

Comment on attachment 105812 [details]
Gather JavaScript, FastMalloc, icon, font, and glyph page statistics in WebProcess::getWebCoreStatistics()

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

> Source/WebKit2/UIProcess/WebContext.cpp:807
> +    RefPtr<MutableDictionary> result = MutableDictionary::create();
> +    HashMap<String, uint32_t>::const_iterator end = map.end();
> +    for (HashMap<String, uint32_t>::const_iterator it = map.begin(); it != end; ++it)
> +        result->set(it->first, RefPtr<WebUInt64>(WebUInt64::create(it->second)).get());
> +    

It seems odd that we are using uint32_t and then making the API uint64_t.  I think the internal representation (in the HashMap) should be uint64_t.

> Source/WebKit2/UIProcess/WebContext.cpp:820
> +    statistics->set("JavaScriptProtectedObjectTypeCounts", RefPtr<MutableDictionary>(createDictionaryFromHashMap(statisticsData.javaScriptProtectedObjectTypeCounts)).get());

The RefPtr car should no be necessary.

[Ada Chan]

(In reply to comment #9)
> (From update of attachment 105812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105812&action=review
> 
> > Source/WebKit2/UIProcess/WebContext.cpp:807
> > +    RefPtr<MutableDictionary> result = MutableDictionary::create();
> > +    HashMap<String, uint32_t>::const_iterator end = map.end();
> > +    for (HashMap<String, uint32_t>::const_iterator it = map.begin(); it != end; ++it)
> > +        result->set(it->first, RefPtr<WebUInt64>(WebUInt64::create(it->second)).get());
> > +    
> 
> It seems odd that we are using uint32_t and then making the API uint64_t.  I think the internal representation (in the HashMap) should be uint64_t.

Changed to uint64_t.

> 
> > Source/WebKit2/UIProcess/WebContext.cpp:820
> > +    statistics->set("JavaScriptProtectedObjectTypeCounts", RefPtr<MutableDictionary>(createDictionaryFromHashMap(statisticsData.javaScriptProtectedObjectTypeCounts)).get());
> 
> The RefPtr car should no be necessary.

Removed RefPtr wrap.

Thanks for reviewing!

[Ada Chan]

Created attachment 105860 [details]
Gather JavaScript, FastMalloc, icon, font, and glyph page statistics in WebProcess::getWebCoreStatistics()

This time with Windows build fix!

[Ada Chan]

Created attachment 105986 [details]
Gather memory cache statistics in WebProcess::getWebCoreStatistics()

[Darin Adler]

Comment on attachment 105986 [details]
Gather memory cache statistics in WebProcess::getWebCoreStatistics()

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

> Source/WebKit2/WebProcess/WebProcess.cpp:852
> +#if ENABLE(XSLT)
> +    counts.set(xslString, memoryCacheStatistics.xslStyleSheets.count);
> +#else
> +    counts.set(xslString, 0);
> +#endif

It would cut down on ifdefs if the structure always had this field but all the numbers were zeroes.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/ChangeLog:3:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 5 files


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

[Ada Chan]

(In reply to comment #13)
> (From update of attachment 105986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105986&action=review
> 
> > Source/WebKit2/WebProcess/WebProcess.cpp:852
> > +#if ENABLE(XSLT)
> > +    counts.set(xslString, memoryCacheStatistics.xslStyleSheets.count);
> > +#else
> > +    counts.set(xslString, 0);
> > +#endif
> 
> It would cut down on ifdefs if the structure always had this field but all the numbers were zeroes.

Very true!  I'll look into doing that in a followup patch.

[Ada Chan]

Created attachment 106033 [details]
Cut down ifdefs in WebKit::getWebCoreMemoryCacheStatistics()

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/ChangeLog:3:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


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

[Darin Adler]

Comment on attachment 106033 [details]
Cut down ifdefs in WebKit::getWebCoreMemoryCacheStatistics()

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

> Source/WebCore/loader/cache/MemoryCache.h:102
>          TypeStatistic xslStyleSheets;

Is there any code anywhere that needs to be changed to initialize this to zero?

[Ada Chan]

Comment on attachment 106033 [details]
Cut down ifdefs in WebKit::getWebCoreMemoryCacheStatistics()

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

>> Source/WebCore/loader/cache/MemoryCache.h:102
>>          TypeStatistic xslStyleSheets;
> 
> Is there any code anywhere that needs to be changed to initialize this to zero?

TypeStatistic has a default constructor that initializes everything to zero.

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/94115
http://trac.webkit.org/changeset/94298
http://trac.webkit.org/changeset/94340
http://trac.webkit.org/changeset/94371