Bug 109329

Summary: Add WKContext API to retrieve basic network process statistics
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: ossy, sam, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1
webkit-ews: commit-queue-
Patch v2 - Use LoaderStrategy
none
Patch v3 - Based on Sam's feedback sam: review+

Description Brady Eidson 2013-02-08 15:47:07 PST
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.
Comment 1 Brady Eidson 2013-02-08 15:48:44 PST
Created attachment 187374 [details]
Patch v1
Comment 2 WebKit Review Bot 2013-02-08 16:01:17 PST
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.
Comment 3 Brady Eidson 2013-02-08 16:04:08 PST
(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.
Comment 4 Early Warning System Bot 2013-02-08 16:26:05 PST
Comment on attachment 187374 [details]
Patch v1

Attachment 187374 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16465021
Comment 5 Brady Eidson 2013-02-08 16:30:08 PST
Created attachment 187385 [details]
Patch v2 - Use LoaderStrategy

Maybe fixed the bizarre style error, definitely fixed the qt compile error.
Comment 6 WebKit Review Bot 2013-02-08 16:31:38 PST
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.
Comment 7 Brady Eidson 2013-02-08 16:48:51 PST
(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.
Comment 8 Sam Weinig 2013-02-08 23:35:34 PST
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?
Comment 9 Brady Eidson 2013-02-09 10:41:17 PST
(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...
Comment 10 Sam Weinig 2013-02-10 13:12:08 PST
(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.
Comment 11 Brady Eidson 2013-02-12 11:58:24 PST
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.
Comment 12 Sam Weinig 2013-02-12 12:02:23 PST
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.
Comment 13 Eric Seidel (no email) 2013-03-01 10:38:37 PST
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.
Comment 14 Csaba Osztrogon√°c 2013-07-26 08:07:36 PDT
It was already landed in http://trac.webkit.org/changeset/142651