Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488323002:
Cache net::HostPortPair in ResourceScheduler::ScheduledResourceRequest (Closed)
|
Created:
4 years, 1 month ago by Charlie Harrison Modified:
4 years, 1 month ago CC:
chromium-reviews, loading-reviews_chromium.org, jam, darin-cc_chromium.org, Randy Smith (Not in Mondays), mmenke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache net::HostPortPair in ResourceScheduler::ScheduledResourceRequest
When loading http://cr.kungfoo.net/loading/1000spacers/ with resources
coming from the disk cache, responding to priority changes takes up
~26% of the CPU time spent on the IO thread. This change moves that
to ~7.7% (Linux).
The time is saved in EffectiveIntPort, which must do a lookup for the
scheme in a set (DoIsStandard).
There is a memory overhead, but since we are only storing host and port
this should not be too bad.
BUG=664174
Committed: https://crrev.com/8c2a381d0adbb6d791172cee1b311c1ad4f31e8e
Cr-Commit-Position: refs/heads/master@{#431339}
Patch Set 1 #Patch Set 2 : make it const #
Total comments: 7
Patch Set 3 : mmenke@ review #Patch Set 4 : +1 const #Messages
Total messages: 22 (12 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + mmenke@chromium.org
Matt, what do you think about this? I know ResourceScheduler is going away eventually but I'd like to make sure comparisons to the new system are fair. In particular, I am nervous about reprioritization which this CL makes a lot faster.
Oh I would also be happy with doing the strategy in https://codereview.chromium.org/2492753002/ which gives us a similar speed up in more places. This is just a much simpler change.
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
You didn't ask, but LGTM from me :-}.
LGTM, modulo comments. https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:247: const net::HostPortPair host_port_pair() { return host_port_pair_; } return by reference? https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:553: net::HostPortPair host_port_pair = (*it)->host_port_pair(); Store as a const ref, or just inline? Not sure how smart compilers are, but best to play it safe.
https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:247: const net::HostPortPair host_port_pair() { return host_port_pair_; } Also +1 const: const net::HostPortPair& host_port_pair() const {
Thanks https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:247: const net::HostPortPair host_port_pair() { return host_port_pair_; } On 2016/11/10 19:11:37, mmenke wrote: > return by reference? Oops, typo. Done :) https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:553: net::HostPortPair host_port_pair = (*it)->host_port_pair(); On 2016/11/10 19:11:37, mmenke wrote: > Store as a const ref, or just inline? Not sure how smart compilers are, but > best to play it safe. Done. https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:616: net::HostPortPair host_port_pair = request->host_port_pair(); I also made this const &.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2488323002/#ps40001 (title: "mmenke@ review")
The CQ bit was unchecked by csharrison@chromium.org
hehe, I was too fast https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/2488323002/diff/20001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:247: const net::HostPortPair host_port_pair() { return host_port_pair_; } On 2016/11/10 19:15:26, mmenke wrote: > Also +1 const: > > const net::HostPortPair& host_port_pair() const { Done.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2488323002/#ps60001 (title: "+1 const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Cache net::HostPortPair in ResourceScheduler::ScheduledResourceRequest When loading http://cr.kungfoo.net/loading/1000spacers/ with resources coming from the disk cache, responding to priority changes takes up ~26% of the CPU time spent on the IO thread. This change moves that to ~7.7% (Linux). The time is saved in EffectiveIntPort, which must do a lookup for the scheme in a set (DoIsStandard). There is a memory overhead, but since we are only storing host and port this should not be too bad. BUG=664174 ========== to ========== Cache net::HostPortPair in ResourceScheduler::ScheduledResourceRequest When loading http://cr.kungfoo.net/loading/1000spacers/ with resources coming from the disk cache, responding to priority changes takes up ~26% of the CPU time spent on the IO thread. This change moves that to ~7.7% (Linux). The time is saved in EffectiveIntPort, which must do a lookup for the scheme in a set (DoIsStandard). There is a memory overhead, but since we are only storing host and port this should not be too bad. BUG=664174 Committed: https://crrev.com/8c2a381d0adbb6d791172cee1b311c1ad4f31e8e Cr-Commit-Position: refs/heads/master@{#431339} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8c2a381d0adbb6d791172cee1b311c1ad4f31e8e Cr-Commit-Position: refs/heads/master@{#431339} |
