Bug 109329 - Add WKContext API to retrieve basic network process statistics
Summary: Add WKContext API to retrieve basic network process statistics
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-08 15:47 PST by Brady Eidson
Modified: 2013-07-26 08:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (15.99 KB, patch)
2013-02-08 15:48 PST, Brady Eidson
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v2 - Use LoaderStrategy (16.03 KB, patch)
2013-02-08 16:30 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v3 - Based on Sam's feedback (35.04 KB, patch)
2013-02-12 11:58 PST, Brady Eidson
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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