Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(466)

Issue 2552703003: Make reprioritization IPC batch rather than per-request.

Created:
4 years ago by Randy Smith (Not in Mondays)
Modified:
4 years ago
Reviewers:
*kinuko, *Charlie Harrison
CC:
chromium-reviews, loading-reviews_chromium.org, jam, darin-cc_chromium.org, mmenke
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make reprioritization IPC batch rather than per-request. This is to reduce IPC overhead and quadratic searching through requests on batch reprioritizations (primarily image reprioritizations, but soon also reprioritization due to parsing stage). BUG=600839

Patch Set 1 #

Patch Set 2 : Fix some typos. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+193 lines, -22 lines) Patch
M content/browser/loader/DEPS View 2 chunks +2 lines, -0 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.h View 2 chunks +4 lines, -4 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 chunk +15 lines, -10 lines 0 comments Download
M content/browser/loader/resource_scheduler.h View 2 chunks +11 lines, -0 lines 0 comments Download
M content/browser/loader/resource_scheduler.cc View 1 2 chunks +78 lines, -0 lines 0 comments Download
M content/child/resource_dispatcher.h View 2 chunks +7 lines, -0 lines 0 comments Download
M content/child/resource_dispatcher.cc View 1 1 chunk +17 lines, -2 lines 2 comments Download
M content/common/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
M content/common/resource_messages.h View 4 chunks +11 lines, -6 lines 0 comments Download
A content/common/resource_priority_change_info.h View 1 chunk +27 lines, -0 lines 0 comments Download
A content/common/resource_priority_change_info.cc View 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (3 generated)
Randy Smith (Not in Mondays)
Kinuko: Could you take a look at this CL and give me high level comments? ...
4 years ago (2016-12-05 16:15:12 UTC) #4
Charlie Harrison
Thanks. Let me wait for high level thoughts from kinuko@ before reviewing this.
4 years ago (2016-12-05 17:39:05 UTC) #5
kinuko
Thanks for asking for high-level comment before you go too far. Depending on situations I ...
4 years ago (2016-12-06 00:34:33 UTC) #6
Charlie Harrison
https://codereview.chromium.org/2552703003/diff/20001/content/child/resource_dispatcher.cc File content/child/resource_dispatcher.cc (right): https://codereview.chromium.org/2552703003/diff/20001/content/child/resource_dispatcher.cc#newcode576 content/child/resource_dispatcher.cc:576: request_id, new_priority, intra_priority_value)); On 2016/12/06 00:34:33, kinuko wrote: > ...
4 years ago (2016-12-06 01:21:27 UTC) #7
Randy Smith (Not in Mondays)
So I'd lay out the issues as follows: * Current performance gain from this change: ...
4 years ago (2016-12-07 17:49:21 UTC) #8
kinuko
Thanks for giving more contexts, Randy and Charles! On 2016/12/07 17:49:21, Randy Smith - Not ...
4 years ago (2016-12-08 06:49:11 UTC) #9
Randy Smith (Not in Mondays)
On 2016/12/08 06:49:11, kinuko wrote: > Thanks for giving more contexts, Randy and Charles! > ...
4 years ago (2016-12-08 15:19:23 UTC) #10
kinuko
On 2016/12/08 15:19:23, Randy Smith - Not in Mondays wrote: > On 2016/12/08 06:49:11, kinuko ...
4 years ago (2016-12-09 03:01:29 UTC) #11
kinuko
On 2016/12/09 03:01:29, kinuko wrote: > On 2016/12/08 15:19:23, Randy Smith - Not in Mondays ...
4 years ago (2016-12-09 10:52:44 UTC) #12
Randy Smith (Not in Mondays)
On 2016/12/09 10:52:44, kinuko wrote: > On 2016/12/09 03:01:29, kinuko wrote: > > On 2016/12/08 ...
4 years ago (2016-12-09 13:44:08 UTC) #13
kinuko
4 years ago (2016-12-09 17:04:43 UTC) #14
On 2016/12/09 13:44:08, Randy Smith - Not in Mondays wrote:
> On 2016/12/09 10:52:44, kinuko wrote:
> > On 2016/12/09 03:01:29, kinuko wrote:
> > > On 2016/12/08 15:19:23, Randy Smith - Not in Mondays wrote:
> > > > On 2016/12/08 06:49:11, kinuko wrote:
> > > > > Thanks for giving more contexts, Randy and Charles! 
> > > > > 
> > > > > On 2016/12/07 17:49:21, Randy Smith - Not in Mondays wrote:
> > > > > > So I'd lay out the issues as follows:
> > > > > > 
> > > > > > * Current performance gain from this change:  I used Charles'
example
> of
> > > > > > http://cr.kungfoo.net/loading/1000spacers/ and the time
> reprioritization
> > > > > > (presumably images) takes on the IO thread drops from ~60ms to ~3ms
> > (Total
> > > > IO
> > > > > > Thread CPU time is ~430ms).  This is a highly contrived benchmark,
and
> I
> > > > > > wouldn't put a lot of weight on it (it's a metric ton of images,
> loaded
> > > from
> > > > > the
> > > > > > cache so we're getting no overlap with network latency) but it does
> show
> > > the
> > > > > > potential current upside.
> > > > > > 
> > > > > > * Relationship to my work: The issue here is that I'm planning to
run
> a
> > > > field
> > > > > > trial to evaluate the splitting of the body tag reprioritization
into
> > > > network
> > > > > > and blink (see
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://docs.google.com/document/d/1p-XGz-BFFehs5j2olbkDkjrXzxzT3n45PVcLSZzOV...
> > > > > > ; this is part of the "Switch ResourceScheduler to, instead of doing
> the
> > > > > current
> > > > > > pre-body throttling, passing those requests through (respecting the
> > global
> > > > > > throttle) but changing the priority to THROTTLED. " item.  I expect
> that
> > > > > change
> > > > > > will have some performance issues, both positive and negative, on
its
> > own
> > > > > (just
> > > > > > because of the changes in priority handling).  If we don't batch
> > > > > prioritization
> > > > > > before I start that field trial, it's going to be laboring under a
> known
> > > > > > regression (because I'll be sending a lot more IPCs in the enabled
> > trial),
> > > > > which
> > > > > > will make it harder for me to figure out if the change is ok or not.
 
> > > > > 
> > > > > I see. If this is the case I think I'm fine with landing this, but
would
> > > like
> > > > us
> > > > > to leave a very explicit TODO comment to revisit if we could
generalize
> > this
> > > > > kind of batching (you could add my ldap there).  I've just filed a
meta
> > bug
> > > > > (crbug.com/672370) to track this kind of issues.
> > > > > 
> > > > > > Having said that, I suspect if the IPCs cause a major problem I can
> see
> > > that
> > > > > in
> > > > > > local testing (pre-field trial) and can revisit this CL at that
point.
> 
> > So
> > > > if
> > > > > > you'd like me to keep this CL on ice until I do local testing of my
> next
> > > > > change,
> > > > > > I'm willing--it would just make my life simpler to push it through
> now. 
> > > > > 
> > > > > This also sounds good :) I plan to do some investigation around IPC
this
> > > month
> > > > > (while everyone's quiet :)) so if you could hold off a bit it could
work
> > > > better
> > > > > for me.
> > > > > 
> > > > > > WDYT?
> > > > 
> > > > I think given what you're saying I'll hold off for a bit, at least until
I
> > > have
> > > > the next CL working and have done local tests.  I'll ping you back at
that
> > > > point, and we'll decide what the right next step is, before I run the
> field
> > > > trial.  Please let me know what you find out in your IPC investigation
:-}
> > > :-|.
> > > > 
> > > > Thanks!
> > > 
> > > SGTM. Please feel free to ping me again anytime!
> > 
> > By the way-- let me make sure, you're worrying about the case for
> > ResourceFetcher::updateAllImageResourcePriorities() where bunch of
> > didChangePriorities requests could be created in a tight loop, am I right?
> 
> Yes, that's the primary existing use case this CL targets.  I'm amused that
that
> isn't clear from the CL given that that's the function I traced code paths
from,
> but that's just because after hitting my head against the problem of plumbing
> down a batch message from that level, I threw up my hands and just coded the
> queue&post solution at the ResourceDispatcher.

Gotcha. Thanks for confirming!

Powered by Google App Engine
This is Rietveld 408576698