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

Issue 12874003: Limit to only 10 image requests per page in ResourceScheduler. (Closed)

Created:
7 years, 9 months ago by James Simonsen
Modified:
7 years, 9 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Visibility:
Public.

Description

Limit to only 10 image requests per page in ResourceScheduler. The pending requests are now kept in a net::PriorityQueue. Priority changes affect the order of the queue. After painting or network idle, we'll load up to 10 images simultaneously in PriorityQueue order. This is a 2% improvement in Speed Index, 4% in first paint, and 5% in DOMContentLoaded. There is a 0.5% regression in onload. BUG=None Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190201

Patch Set 1 #

Total comments: 36

Patch Set 2 : Use custom priority queue #

Patch Set 3 : Remove unnecessary friend line #

Patch Set 4 : Move inner classes to .cc #

Total comments: 17

Patch Set 5 : Stop using high/low priority #

Total comments: 4

Patch Set 6 : Remove unnecessary includes #

Patch Set 7 : Fix sign comparison #

Patch Set 8 : Fix content exports #

Patch Set 9 : Fix constructor #

Patch Set 10 : Rebase #

Patch Set 11 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+405 lines, -83 lines) Patch
M content/browser/loader/resource_request_info_impl.h View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -3 lines 0 comments Download
M content/browser/loader/resource_scheduler.h View 1 2 3 4 5 4 chunks +33 lines, -16 lines 0 comments Download
M content/browser/loader/resource_scheduler.cc View 1 2 3 4 5 6 7 8 9 10 9 chunks +155 lines, -51 lines 0 comments Download
M content/browser/loader/resource_scheduler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +214 lines, -13 lines 0 comments Download

Messages

Total messages: 30 (0 generated)
James Simonsen
Will, please review this. Darin, please rubber stamp when he's done. It's purely a scheduling ...
7 years, 9 months ago (2013-03-14 21:38:58 UTC) #1
willchan no longer on Chromium
Sorry, I wasn't able to fit this in today due to IETF. Maybe I'll get ...
7 years, 9 months ago (2013-03-15 20:28:35 UTC) #2
willchan no longer on Chromium
I didn't get a chance to review the test yet. https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resource_scheduler.cc#newcode82 ...
7 years, 9 months ago (2013-03-18 07:30:33 UTC) #3
James Simonsen
https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resource_scheduler.cc#newcode82 content/browser/loader/resource_scheduler.cc:82: if (new_priority != old_priority) { On 2013/03/18 07:30:33, willchan ...
7 years, 9 months ago (2013-03-18 23:50:13 UTC) #4
willchan no longer on Chromium
https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resource_scheduler.cc#newcode185 content/browser/loader/resource_scheduler.cc:185: // cross-renderer navigation. On 2013/03/18 23:50:13, James Simonsen wrote: ...
7 years, 9 months ago (2013-03-19 20:53:52 UTC) #5
James Simonsen
On 2013/03/19 20:53:52, willchan wrote: > https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resource_scheduler.cc > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/12874003/diff/1/content/browser/loader/resource_scheduler.cc#newcode185 > ...
7 years, 9 months ago (2013-03-19 23:00:20 UTC) #6
James Simonsen
https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/resource_scheduler.cc File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/12874003/diff/13001/content/browser/loader/resource_scheduler.cc#newcode143 content/browser/loader/resource_scheduler.cc:143: for (ClientMap::iterator it ALLOW_UNUSED = client_map_.begin(); On 2013/03/19 20:53:53, ...
7 years, 9 months ago (2013-03-19 23:00:46 UTC) #7
willchan no longer on Chromium
LGTM. I still don't like the RenderViewHost signal, but that's more Darin's call if anything. ...
7 years, 9 months ago (2013-03-19 23:08:32 UTC) #8
James Simonsen
On 2013/03/19 23:08:32, willchan wrote: > LGTM. I still don't like the RenderViewHost signal, but ...
7 years, 9 months ago (2013-03-20 00:31:54 UTC) #9
James Simonsen
https://codereview.chromium.org/12874003/diff/18001/content/browser/loader/resource_scheduler.h File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/12874003/diff/18001/content/browser/loader/resource_scheduler.h#newcode13 content/browser/loader/resource_scheduler.h:13: #include "base/memory/linked_ptr.h" On 2013/03/19 23:08:32, willchan wrote: > delete? ...
7 years, 9 months ago (2013-03-20 00:32:03 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/24017
7 years, 9 months ago (2013-03-20 02:40:58 UTC) #11
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 9 months ago (2013-03-20 03:08:48 UTC) #12
darin (slow to review)
To answer Will's question, note that sometimes we nuke the renderer process in which case ...
7 years, 9 months ago (2013-03-20 06:54:23 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
7 years, 9 months ago (2013-03-20 22:13:14 UTC) #14
willchan no longer on Chromium
The yuckiness is that RVH can die on the UI thread while IPCs may be ...
7 years, 9 months ago (2013-03-20 22:23:54 UTC) #15
James Simonsen
On Wed, Mar 20, 2013 at 3:23 PM, <willchan@chromium.org> wrote: > The yuckiness is that ...
7 years, 9 months ago (2013-03-20 22:34:08 UTC) #16
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=26049
7 years, 9 months ago (2013-03-21 00:32:17 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
7 years, 9 months ago (2013-03-21 00:41:52 UTC) #18
commit-bot: I haz the power
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&number=22083
7 years, 9 months ago (2013-03-21 12:13:59 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
7 years, 9 months ago (2013-03-21 17:39:40 UTC) #20
commit-bot: I haz the power
Failed to request the patch to try. Please note that binary files are still unsupported ...
7 years, 9 months ago (2013-03-21 17:40:06 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
7 years, 9 months ago (2013-03-21 19:37:53 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/11005
7 years, 9 months ago (2013-03-22 19:00:35 UTC) #23
commit-bot: I haz the power
Failed to apply patch for content/browser/loader/resource_scheduler.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 9 months ago (2013-03-22 19:00:37 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/68001
7 years, 9 months ago (2013-03-22 20:36:26 UTC) #25
commit-bot: I haz the power
Failed to apply patch for content/browser/loader/resource_scheduler.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 9 months ago (2013-03-22 20:36:28 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/71001
7 years, 9 months ago (2013-03-22 23:25:05 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/12874003/71001
7 years, 9 months ago (2013-03-23 15:43:16 UTC) #28
commit-bot: I haz the power
Change committed as 190201
7 years, 9 months ago (2013-03-24 04:58:23 UTC) #29
willchan no longer on Chromium
7 years, 9 months ago (2013-03-24 06:29:23 UTC) #30
Took long enough :)
On Mar 23, 2013 9:58 PM, <commit-bot@chromium.org> wrote:

> Change committed as 190201
>
>
https://chromiumcodereview.**appspot.com/12874003/<https://chromiumcodereview...
>

Powered by Google App Engine
This is Rietveld 408576698