Bug 109329 – Add WKContext API to retrieve basic network process statistics

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Add WKContext API to retrieve basic network process statistics

This will allow a UI client to monitor the workings of the NetworkProcess for debug/diagnostic purposes.

The type of data gathered can be expanded in the future.

[Brady Eidson]

Created attachment 187374 [details]
Patch v1

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/HostRecord.cpp', u'Source/WebKit2/NetworkProcess/HostRecord.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkProcess.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.messages.in', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h', u'Source/WebKit2/Shared/Authentication/AuthenticationManager.h', u'Source/WebKit2/Shared/Downloads/DownloadManager.h', u'Source/WebKit2/UIProcess/API/C/WKContext.cpp', u'Source/WebKit2/UIProcess/API/C/WKContext.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/UIProcess/WebContext.h', u'Source/WebKit2/UIProcess/WebContext.messages.in']" exit_code: 1
Source/WebKit2/NetworkProcess/NetworkProcess.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


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

[Brady Eidson]

(In reply to comment #2)
> Attachment 187374 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', 
> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 1 in 15 files
> 

Weird...  totally seems like 4 spaces locally.  I deleted them and re-inserted them to make sure.

[Early Warning System Bot]

Comment on attachment 187374 [details]
Patch v1

Attachment 187374 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16465021

[Brady Eidson]

Created attachment 187385 [details]
Patch v2 - Use LoaderStrategy

Maybe fixed the bizarre style error, definitely fixed the qt compile error.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/HostRecord.cpp', u'Source/WebKit2/NetworkProcess/HostRecord.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.cpp', u'Source/WebKit2/NetworkProcess/NetworkProcess.h', u'Source/WebKit2/NetworkProcess/NetworkProcess.messages.in', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h', u'Source/WebKit2/Shared/Authentication/AuthenticationManager.h', u'Source/WebKit2/Shared/Downloads/DownloadManager.h', u'Source/WebKit2/UIProcess/API/C/WKContext.cpp', u'Source/WebKit2/UIProcess/API/C/WKContext.h', u'Source/WebKit2/UIProcess/WebContext.cpp', u'Source/WebKit2/UIProcess/WebContext.h', u'Source/WebKit2/UIProcess/WebContext.messages.in']" exit_code: 1
Source/WebKit2/NetworkProcess/NetworkProcess.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


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

[Brady Eidson]

(In reply to comment #6)
> Attachment 187385 [details] did not pass style-queue:
> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 1 in 15 files

Aha, got it.  Fixed locally.

[Sam Weinig]

Comment on attachment 187385 [details]
Patch v2 - Use LoaderStrategy

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:180
> +    uint64_t count =  m_nonHTTPProtocolHost->pendingRequestCount() ? 1 : 0;

Extra space after the =.  This code is also not very self explanatory.  Why are you doing the ? 1 : 0 part?

> Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:193
> +    uint64_t count =  m_nonHTTPProtocolHost->pendingRequestCount();

Extra space after the =.

> Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:204
> +    uint64_t count =  m_nonHTTPProtocolHost->activeLoadCount() ? 1 : 0;

Extra space after the =.

> Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:217
> +    uint64_t count =  m_nonHTTPProtocolHost->activeLoadCount();

Extra space after the =.

> Source/WebKit2/Shared/Authentication/AuthenticationManager.h:66
> +    uint64_t outstandingAuthenticationChallengeCount() const { return m_challenges.size(); }

What makes them so outstanding? ;)

> Source/WebKit2/UIProcess/API/C/WKContext.h:191
> +WK_EXPORT void WKContextGetNetworkingStatistics(WKContextRef context, void* functionContext, WKContextGetStatisticsFunction function);

Why did you make the decision to use a new function, rather than just add the new stats to WKContextGetStatistics's dictionary?

[Brady Eidson]

(In reply to comment #8)
> (From update of attachment 187385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187385&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.cpp:180
> > +    uint64_t count =  m_nonHTTPProtocolHost->pendingRequestCount() ? 1 : 0;
> 
> Extra space after the =.  This code is also not very self explanatory.  Why are you doing the ? 1 : 0 part?
>

If the host has any pending requests, then the total count of hosts with pending requests gets incremented.

I'll make it more clear.

I'll also fix all the bizarre extra spaces.

 
> > Source/WebKit2/UIProcess/API/C/WKContext.h:191
> > +WK_EXPORT void WKContextGetNetworkingStatistics(WKContextRef context, void* functionContext, WKContextGetStatisticsFunction function);
> 
> Why did you make the decision to use a new function, rather than just add the new stats to WKContextGetStatistics's dictionary?

We can chat on IRC or you can look at the other patch related for this to see why it made sense to have networking stats be in a separate call.

An alternative solution would be to change WKContextGetStatistics to take a mask of which statistics are wanted, but it's already shipped API...

[Sam Weinig]

(In reply to comment #9)
> (In reply to comment #8)

> > > Source/WebKit2/UIProcess/API/C/WKContext.h:191
> > > +WK_EXPORT void WKContextGetNetworkingStatistics(WKContextRef context, void* functionContext, WKContextGetStatisticsFunction function);
> > 
> > Why did you make the decision to use a new function, rather than just add the new stats to WKContextGetStatistics's dictionary?
> 
> We can chat on IRC or you can look at the other patch related for this to see why it made sense to have networking stats be in a separate call.
> 
> An alternative solution would be to change WKContextGetStatistics to take a mask of which statistics are wanted, but it's already shipped API...

Well, you could always just add WKContextGetStatisticsWithOptions(), but this seems fine.

[Brady Eidson]

Created attachment 187909 [details]
Patch v3 - Based on Sam's feedback

A bit more complicated, this adds WKContextGetStatisticsWithOptions that allows for gathering stats from multiple processes at once.

It is both a better API going forward, and will help separately resolve the "get statistics from multiple WebProcesses" bug.

[Sam Weinig]

Comment on attachment 187909 [details]
Patch v3 - Based on Sam's feedback

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

> Source/WebKit2/UIProcess/API/C/WKContext.h:150
> +enum {
> +    kWKStatisticsTypeWebContent = 0x00000001,
> +    kWKStatisticsTypeNetworking = 0x00000002
> +};
> +typedef uint32_t WKStatisticsType;

Please use 0 << 1 and such.

> Source/WebKit2/UIProcess/API/C/WKContext.h:197
> +WK_EXPORT void WKContextGetStatisticsWithOptions(WKContextRef context, WKStatisticsType statisticsMask, void* functionContext, WKContextGetStatisticsFunction function);

This doesn't take options.  Please rename.

[Eric Seidel (no email)]

Comment on attachment 187385 [details]
Patch v2 - Use LoaderStrategy

Cleared Sam Weinig's review+ from obsolete attachment 187385 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Csaba Osztrogonác]

It was already landed in http://trac.webkit.org/changeset/142651