Bug 170525 - Web Inspector: Only Capture Extra Network Load Metrics when there is a Web Inspector Frontend
Summary: Web Inspector: Only Capture Extra Network Load Metrics when there is a Web In...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on: 16531
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-05 16:00 PDT by Joseph Pecoraro
Modified: 2018-07-12 12:47 PDT (History)
17 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (21.73 KB, patch)
2017-04-05 16:05 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots (All Parts) (44.25 KB, patch)
2017-04-05 16:06 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots (All Parts) (44.21 KB, patch)
2017-04-05 16:25 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots (All Parts) (44.21 KB, patch)
2017-04-05 16:49 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots (All Parts) (44.24 KB, patch)
2017-04-05 17:36 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (37.35 MB, application/zip)
2017-04-05 19:28 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (21.81 KB, patch)
2017-04-06 15:03 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-04-05 16:00:08 PDT
Summary:
Only Capture Extra Network Load Metrics when there is a Web Inspector Frontend

To limit any potential performance impact from making another copy of all HTTP Headers, only do this work in the NetworkProcess if the WebProcess has a Web Inspector frontend open.

We can extend this to a few other Metrics properties as well.
Comment 1 Joseph Pecoraro 2017-04-05 16:05:54 PDT
Created attachment 306328 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2017-04-05 16:06:39 PDT
Created attachment 306329 [details]
[PATCH] For Bots (All Parts)
Comment 3 Joseph Pecoraro 2017-04-05 16:15:33 PDT
This prevents copying of headers in NetworkProcess, Serializing across to the WebProcess, and potentially copying of the HTTPHeaderMap in the WebProcess. It also avoids setting a few other properties only needed for Web Inspector (and a few others are coming).
Comment 4 Joseph Pecoraro 2017-04-05 16:25:25 PDT
Comment on attachment 306328 [details]
[PATCH] Proposed Fix

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

> Source/WebKit2/NetworkProcess/NetworkLoad.h:73
> +    bool shouldCaptureExtraNetworkLoadMetrics() const final;

This needs to be inside of a USE(NETWORK_SESSION) block. Fixed locally.
Comment 5 Joseph Pecoraro 2017-04-05 16:25:42 PDT
Created attachment 306333 [details]
[PATCH] For Bots (All Parts)
Comment 6 Joseph Pecoraro 2017-04-05 16:49:47 PDT
Created attachment 306337 [details]
[PATCH] For Bots (All Parts)

A few more USE(NETWORK_SESSION)
Comment 7 Joseph Pecoraro 2017-04-05 17:36:32 PDT
Created attachment 306342 [details]
[PATCH] For Bots (All Parts)
Comment 8 Build Bot 2017-04-05 19:28:31 PDT
Comment on attachment 306342 [details]
[PATCH] For Bots (All Parts)

Attachment 306342 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3482094

New failing tests:
http/tests/cache/disk-cache/speculative-validation/http-auth.html
Comment 9 Build Bot 2017-04-05 19:28:34 PDT
Created attachment 306356 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 10 Joseph Pecoraro 2017-04-06 13:35:34 PDT
Ping reviewers.
Comment 11 youenn fablet 2017-04-06 14:06:32 PDT
Comment on attachment 306328 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/InspectorInstrumentation.h:1357
> +    if (s_frontendCounter == 1)

one liner?

> Source/WebCore/inspector/InspectorInstrumentation.h:1365
> +    if (!s_frontendCounter)

one liner?

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:315
> +                requestHeaders.set(String(name), String(value));

Might want to consider sending only the modified headers if this can be easily done.
Comment 12 youenn fablet 2017-04-06 14:06:32 PDT
Comment on attachment 306328 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/InspectorInstrumentation.h:1357
> +    if (s_frontendCounter == 1)

one liner?

> Source/WebCore/inspector/InspectorInstrumentation.h:1365
> +    if (!s_frontendCounter)

one liner?

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:315
> +                requestHeaders.set(String(name), String(value));

Might want to consider sending only the modified headers if this can be easily done.
Comment 13 youenn fablet 2017-04-06 14:06:51 PDT
(In reply to Joseph Pecoraro from comment #10)
> Ping reviewers.

Can you rebase it?
Comment 14 Joseph Pecoraro 2017-04-06 14:18:46 PDT
(In reply to youenn fablet from comment #13)
> (In reply to Joseph Pecoraro from comment #10)
> > Ping reviewers.
> 
> Can you rebase it?

It depends on a patch that hasn't landed yet (hence the "All Parts" patch). But I'll go ahead and land the earlier part and put up a new patch.
Comment 15 Joseph Pecoraro 2017-04-06 14:20:38 PDT
> > Source/WebCore/inspector/InspectorInstrumentation.h:1357
> > +    if (s_frontendCounter == 1)
> 
> one liner?
> 
> > Source/WebCore/inspector/InspectorInstrumentation.h:1365
> > +    if (!s_frontendCounter)
> 
> one liner?

The compiler should optimize this, I aimed for the code to have maximum readability / clarity.
Comment 16 Joseph Pecoraro 2017-04-06 15:03:38 PDT
Created attachment 306431 [details]
[PATCH] Proposed Fix
Comment 17 WebKit Commit Bot 2017-04-06 15:58:11 PDT
Comment on attachment 306431 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 306431

Committed r215065: <http://trac.webkit.org/changeset/215065>
Comment 18 WebKit Commit Bot 2017-04-06 15:58:13 PDT
All reviewed patches have been landed.  Closing bug.