Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399973003:
prerender: Don't set the priority to net::IDLE on Android. (Closed)
Descriptionprerender: Don't set the priority to net::IDLE on Android.
Resource requests issued by prerendering renderers have their priority
set to net::IDLE. This causes issues with content::ResourceScheduler,
leading to very poor loading performance for prerendered content.
This simply disables the prioritization on Android, as this is where the
priority lowering is the most problematic, and where potential downsides
from this patch are the most limited. This is a temporary fix.
BUG=652746
Committed: https://crrev.com/915cfe1d23cf249cff0b51b6f2a7434035d5e6c5
Cr-Commit-Position: refs/heads/master@{#424131}
Patch Set 1 #
Total comments: 6
Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : Rebase + Address comment. #Messages
Total messages: 39 (19 generated)
The CQ bit was checked by lizeb@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...
lizeb@chromium.org changed reviewers: + droger@chromium.org
lgtm https://codereview.chromium.org/2399973003/diff/1/chrome/browser/loader/chrom... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2399973003/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:426: // disabled only on Android, as the prerenders are not likely to compete wuth s/wuth/with/ https://codereview.chromium.org/2399973003/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:429: // TODO(lizeb,droger): Remove this hack, and fix the issue on all platforms. Nit: We're not quite sure that this is really a hack, it may be the right fix for all platforms. I would drop the first part of the comment and just put: // TODO(lizeb,droger): Fix the issue on all platforms. https://codereview.chromium.org/2399973003/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:493: if (!is_prerendering) { FYI: |is_prerendering| is used here, compile might fail if NACL gets enabled on android somehow. Not sure this can ever be a problem though.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Thanks! https://codereview.chromium.org/2399973003/diff/1/chrome/browser/loader/chrom... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2399973003/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:426: // disabled only on Android, as the prerenders are not likely to compete wuth On 2016/10/07 09:08:23, droger wrote: > s/wuth/with/ Done. https://codereview.chromium.org/2399973003/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:429: // TODO(lizeb,droger): Remove this hack, and fix the issue on all platforms. On 2016/10/07 09:08:23, droger wrote: > Nit: > > We're not quite sure that this is really a hack, it may be the right fix for all > platforms. > I would drop the first part of the comment and just put: > > // TODO(lizeb,droger): Fix the issue on all platforms. Done. https://codereview.chromium.org/2399973003/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:493: if (!is_prerendering) { On 2016/10/07 09:08:23, droger wrote: > FYI: > |is_prerendering| is used here, compile might fail if NACL gets enabled on > android somehow. > > Not sure this can ever be a problem though. Done.
The CQ bit was checked by lizeb@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lizeb@chromium.org changed reviewers: + mmenke@chromium.org
Hi mmenke, Please take a look at this patch. For more detail, you can refer to the two latest update to the linked bug. The tl;dr is that loading a 8kB blocking script in <head> takes ~7s on Android without this patch, due to undue stalling of the request. This makes android.com load in >8s when prerendered, vs ~2s otherwise (and with this patch). Thanks!
CC csharrison@ for pre-render weirdness. What's the long-term plan for a fix? (Naively, I'd think it would be increasing the priority of requests dispatched from a prerender when the prerender becomes visible?)
Arggh; +csharrison for real this time.
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
I'm also surprised that this fix won't cause other, fairly major, performance problems with pre-renders strongly interfering with regular loads. I presume you've confirmed that that doesn't happen for some reason?
On 2016/10/07 13:19:01, Randy Smith - Not in Mondays wrote: > I'm also surprised that this fix won't cause other, fairly major, performance > problems with pre-renders strongly interfering with regular loads. I presume > you've confirmed that that doesn't happen for some reason? The vast majority of prerenders on Android come from either the omnibox or external requests (through Custom Tabs). In both cases, it does not make sense to have the prerender at a lower priority than any other tab, so that's why the change is Android-only for now. (see the bug for more details). A longer-term fix could be to distinguish between prerenders depending on their origin. When it's external or coming from the omnibox, then the priority should be the same as a regular page load. In other cases we may want to throttle it more aggressively.
On 2016/10/07 13:25:16, Benoit L wrote: > On 2016/10/07 13:19:01, Randy Smith - Not in Mondays wrote: > > I'm also surprised that this fix won't cause other, fairly major, performance > > problems with pre-renders strongly interfering with regular loads. I presume > > you've confirmed that that doesn't happen for some reason? > > The vast majority of prerenders on Android come from either the omnibox or > external requests (through Custom Tabs). In both cases, it does not make sense > to have the prerender at a lower priority than any other tab, so that's why the > change is Android-only for now. (see the bug for more details). > > A longer-term fix could be to distinguish between prerenders depending on their > origin. When it's external or coming from the omnibox, then the priority should > be the same as a regular page load. In other cases we may want to throttle it > more aggressively. I'm going to defer to Randy on this one (He's not an owner, but he knows more about prioritization than I do). What metrics are you planning to watch as this rolls out? Have you thought about a field trial?
On 2016/10/07 15:38:37, mmenke wrote: > On 2016/10/07 13:25:16, Benoit L wrote: > > On 2016/10/07 13:19:01, Randy Smith - Not in Mondays wrote: > > > I'm also surprised that this fix won't cause other, fairly major, > performance > > > problems with pre-renders strongly interfering with regular loads. I > presume > > > you've confirmed that that doesn't happen for some reason? > > > > The vast majority of prerenders on Android come from either the omnibox or > > external requests (through Custom Tabs). In both cases, it does not make sense > > to have the prerender at a lower priority than any other tab, so that's why > the > > change is Android-only for now. (see the bug for more details). > > > > A longer-term fix could be to distinguish between prerenders depending on > their > > origin. When it's external or coming from the omnibox, then the priority > should > > be the same as a regular page load. In other cases we may want to throttle it > > more aggressively. > > I'm going to defer to Randy on this one (He's not an owner, but he knows more > about prioritization than I do). > > What metrics are you planning to watch as this rolls out? Have you thought > about a field trial? I cannot copy-past actual numbers from UMA, but as you can see them here: https://uma.googleplex.com/p/chrome/histograms/?endDate=10-05-2016&dayCount=1... On Android, at least 90% of prerenders should have the same priority as a normal page load, because they are initiated when Chrome is in the background already (external requests), or from the omnibox. The problem that we want to solve here is that by prerendering a link, you are quite likely to actually increasing the total PLT vs not doing anything. This is a problem for AMP articles loaded from GSA on Android, but also for links loaded from G+ or twitter. We agree that the fix here is not necessarily ideal, but since it solves a real-world issue, I think it should land sooner rather than later. What do you think?
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
drive-by: I'm happy with this patch, but could you refactor the logic a bit? It is already rather complicated. Maybe: - Thread the ResourceRequestInfo into AppendComponentUpdaterThrottles and checked for prerendering there. - Add a separate method to ReprioritizePrerenderRequests() with the #if ANDROID logic isolated to that method.
On 2016/10/07 15:59:10, Benoit L wrote: > On 2016/10/07 15:38:37, mmenke wrote: > > On 2016/10/07 13:25:16, Benoit L wrote: > > > On 2016/10/07 13:19:01, Randy Smith - Not in Mondays wrote: > > > > I'm also surprised that this fix won't cause other, fairly major, > > performance > > > > problems with pre-renders strongly interfering with regular loads. I > > presume > > > > you've confirmed that that doesn't happen for some reason? > > > > > > The vast majority of prerenders on Android come from either the omnibox or > > > external requests (through Custom Tabs). In both cases, it does not make > sense > > > to have the prerender at a lower priority than any other tab, so that's why > > the > > > change is Android-only for now. (see the bug for more details). > > > > > > A longer-term fix could be to distinguish between prerenders depending on > > their > > > origin. When it's external or coming from the omnibox, then the priority > > should > > > be the same as a regular page load. In other cases we may want to throttle > it > > > more aggressively. > > > > I'm going to defer to Randy on this one (He's not an owner, but he knows more > > about prioritization than I do). > > > > What metrics are you planning to watch as this rolls out? Have you thought > > about a field trial? > > I cannot copy-past actual numbers from UMA, but as you can see them here: > https://uma.googleplex.com/p/chrome/histograms/?endDate=10-05-2016&dayCount=1... > > On Android, at least 90% of prerenders should have the same priority as a normal > page load, because they are initiated when Chrome is in the background already > (external requests), or from the omnibox. > > The problem that we want to solve here is that by prerendering a link, you are > quite likely to actually increasing the total PLT vs not doing anything. This is > a problem for AMP articles loaded from GSA on Android, but also for links loaded > from G+ or twitter. > We agree that the fix here is not necessarily ideal, but since it solves a > real-world issue, I think it should land sooner rather than later. > > What do you think? * Looking for some background. Can you help me understand *why* Omnibox/Instant Search/External request goes through the pre-render path? At a surface level, that seems silly; they should be normal navigations. I assume there's some context about cross-process intent handling on Android I don't have. * Why can't we shift the priorities just for the pre-renders with origins that we know should have the top level priority, as opposed to all pre-renders? How hard would that plumbing be? * I think the metrics you are pointing at are intended to show that the large majority of pre-renderers are sourced from origins that should have the regular priority; please let me know if there's some point to those metrics that I've missed. I'd feel more comfortable if we also tracked overall PLT metrics as this was rolled out, as I remain concerned about prerenders from origins that should have an IDLE priority interfering with other page loads. I'm happy to believe that this is a real, and bad, real world problem. But I think we should be tracking the cost of the cure. Charles, can you comment on what metrics would be good to track as this rolls out?
On 2016/10/07 16:14:58, Randy Smith - Not in Mondays wrote: > On 2016/10/07 15:59:10, Benoit L wrote: > > On 2016/10/07 15:38:37, mmenke wrote: > > > On 2016/10/07 13:25:16, Benoit L wrote: > > > > On 2016/10/07 13:19:01, Randy Smith - Not in Mondays wrote: > > > > > I'm also surprised that this fix won't cause other, fairly major, > > > performance > > > > > problems with pre-renders strongly interfering with regular loads. I > > > presume > > > > > you've confirmed that that doesn't happen for some reason? > > > > > > > > The vast majority of prerenders on Android come from either the omnibox or > > > > external requests (through Custom Tabs). In both cases, it does not make > > sense > > > > to have the prerender at a lower priority than any other tab, so that's > why > > > the > > > > change is Android-only for now. (see the bug for more details). > > > > > > > > A longer-term fix could be to distinguish between prerenders depending on > > > their > > > > origin. When it's external or coming from the omnibox, then the priority > > > should > > > > be the same as a regular page load. In other cases we may want to throttle > > it > > > > more aggressively. > > > > > > I'm going to defer to Randy on this one (He's not an owner, but he knows > more > > > about prioritization than I do). > > > > > > What metrics are you planning to watch as this rolls out? Have you thought > > > about a field trial? > > > > I cannot copy-past actual numbers from UMA, but as you can see them here: > > > https://uma.googleplex.com/p/chrome/histograms/?endDate=10-05-2016&dayCount=1... > > > > On Android, at least 90% of prerenders should have the same priority as a > normal > > page load, because they are initiated when Chrome is in the background already > > (external requests), or from the omnibox. > > > > The problem that we want to solve here is that by prerendering a link, you are > > quite likely to actually increasing the total PLT vs not doing anything. This > is > > a problem for AMP articles loaded from GSA on Android, but also for links > loaded > > from G+ or twitter. > > We agree that the fix here is not necessarily ideal, but since it solves a > > real-world issue, I think it should land sooner rather than later. > > > > What do you think? > > * Looking for some background. Can you help me understand *why* Omnibox/Instant > Search/External request goes through the pre-render path? At a surface level, > that seems silly; they should be normal navigations. I assume there's some > context about cross-process intent handling on Android I don't have. > Sorry, I wasn't clear. I'm only referring to prerenders here. So it's not about all navigations, merely the ones that trigger prerenders. Some omnibox completions trigger a prerender. The external part is when an application tells Chrome of a likely future navigation to a URL. They still should be prerenders, because at that point they are speculative, and we don't want these page loads to start playing audio, for instance. > * Why can't we shift the priorities just for the pre-renders with origins that > we know should have the top level priority, as opposed to all pre-renders? How > hard would that plumbing be? That's the better solution, indeed. However we would like to keep the patch as small as possible to cherry-pick it to M55. But you're absolutely right, this is likely the right long-term solution. > > * I think the metrics you are pointing at are intended to show that the large > majority of pre-renderers are sourced from origins that should have the regular > priority; please let me know if there's some point to those metrics that I've > missed. I'd feel more comfortable if we also tracked overall PLT metrics as > this was rolled out, as I remain concerned about prerenders from origins that > should have an IDLE priority interfering with other page loads. I'm happy to > believe that this is a real, and bad, real world problem. But I think we should > be tracking the cost of the cure. > Tracking this would be hard, as prerenders are only a small fraction of navigations. So seeing the overall effect on normal page load would be difficult without introducing complex new metrics (Time to Contentful Paint for pages loading while a prerender is in progress?) > Charles, can you comment on what metrics would be good to track as this rolls > out?
On 2016/10/07 16:21:56, Benoit L wrote: > On 2016/10/07 16:14:58, Randy Smith - Not in Mondays wrote: > > On 2016/10/07 15:59:10, Benoit L wrote: > > > On 2016/10/07 15:38:37, mmenke wrote: > > > > On 2016/10/07 13:25:16, Benoit L wrote: > > > > > On 2016/10/07 13:19:01, Randy Smith - Not in Mondays wrote: > > > > > > I'm also surprised that this fix won't cause other, fairly major, > > > > performance > > > > > > problems with pre-renders strongly interfering with regular loads. I > > > > presume > > > > > > you've confirmed that that doesn't happen for some reason? > > > > > > > > > > The vast majority of prerenders on Android come from either the omnibox > or > > > > > external requests (through Custom Tabs). In both cases, it does not make > > > sense > > > > > to have the prerender at a lower priority than any other tab, so that's > > why > > > > the > > > > > change is Android-only for now. (see the bug for more details). > > > > > > > > > > A longer-term fix could be to distinguish between prerenders depending > on > > > > their > > > > > origin. When it's external or coming from the omnibox, then the priority > > > > should > > > > > be the same as a regular page load. In other cases we may want to > throttle > > > it > > > > > more aggressively. > > > > > > > > I'm going to defer to Randy on this one (He's not an owner, but he knows > > more > > > > about prioritization than I do). > > > > > > > > What metrics are you planning to watch as this rolls out? Have you > thought > > > > about a field trial? > > > > > > I cannot copy-past actual numbers from UMA, but as you can see them here: > > > > > > https://uma.googleplex.com/p/chrome/histograms/?endDate=10-05-2016&dayCount=1... > > > > > > On Android, at least 90% of prerenders should have the same priority as a > > normal > > > page load, because they are initiated when Chrome is in the background > already > > > (external requests), or from the omnibox. > > > > > > The problem that we want to solve here is that by prerendering a link, you > are > > > quite likely to actually increasing the total PLT vs not doing anything. > This > > is > > > a problem for AMP articles loaded from GSA on Android, but also for links > > loaded > > > from G+ or twitter. > > > We agree that the fix here is not necessarily ideal, but since it solves a > > > real-world issue, I think it should land sooner rather than later. > > > > > > What do you think? > > > > * Looking for some background. Can you help me understand *why* > Omnibox/Instant > > Search/External request goes through the pre-render path? At a surface level, > > that seems silly; they should be normal navigations. I assume there's some > > context about cross-process intent handling on Android I don't have. > > > > Sorry, I wasn't clear. I'm only referring to prerenders here. So it's not about > all navigations, merely the ones that trigger prerenders. Some omnibox > completions trigger a prerender. The external part is when an application tells > Chrome of a likely future navigation to a URL. > They still should be prerenders, because at that point they are speculative, and > we don't want these page loads to start playing audio, for instance. > > > * Why can't we shift the priorities just for the pre-renders with origins that > > we know should have the top level priority, as opposed to all pre-renders? > How > > hard would that plumbing be? > > That's the better solution, indeed. However we would like to keep the patch as > small as possible to cherry-pick it to M55. But you're absolutely right, this is > likely the right long-term solution. Actually, I think the right long-term solution is reprioritizing the requests associated with a pre-render when it becomes an actual navigation (especially since these navigations really are pre-renders, just pre-renders that may be more likely to turn into actual navigations and less likely to compete with other work Chrome's trying to do). That might show up bugs in the netstack around reprioritization, but that's ok; I'd like to fix those too :-}. > > > > > * I think the metrics you are pointing at are intended to show that the large > > majority of pre-renderers are sourced from origins that should have the > regular > > priority; please let me know if there's some point to those metrics that I've > > missed. I'd feel more comfortable if we also tracked overall PLT metrics as > > this was rolled out, as I remain concerned about prerenders from origins that > > should have an IDLE priority interfering with other page loads. I'm happy to > > believe that this is a real, and bad, real world problem. But I think we > should > > be tracking the cost of the cure. > > > > Tracking this would be hard, as prerenders are only a small fraction of > navigations. So seeing the overall effect on normal page load would be difficult > without introducing complex new metrics (Time to Contentful Paint for pages > loading while a prerender is in progress?) Fair, and I'm not looking for that, but I'd still be grateful if you can track 95%l on PLT as this rolls out. Keep me updated as to the long-term work in this area? With all that LGTM (I'll let you and Charles evaluate the refactor). But please don't drop the long-term fix on the floor. > > > Charles, can you comment on what metrics would be good to track as this rolls > > out?
On 2016/10/07 16:37:10, Randy Smith - Not in Mondays wrote: > On 2016/10/07 16:21:56, Benoit L wrote: > > On 2016/10/07 16:14:58, Randy Smith - Not in Mondays wrote: > > > On 2016/10/07 15:59:10, Benoit L wrote: > > > > On 2016/10/07 15:38:37, mmenke wrote: > > > > > On 2016/10/07 13:25:16, Benoit L wrote: > > > > > > On 2016/10/07 13:19:01, Randy Smith - Not in Mondays wrote: > > > > > > > I'm also surprised that this fix won't cause other, fairly major, > > > > > performance > > > > > > > problems with pre-renders strongly interfering with regular loads. > I > > > > > presume > > > > > > > you've confirmed that that doesn't happen for some reason? > > > > > > > > > > > > The vast majority of prerenders on Android come from either the > omnibox > > or > > > > > > external requests (through Custom Tabs). In both cases, it does not > make > > > > sense > > > > > > to have the prerender at a lower priority than any other tab, so > that's > > > why > > > > > the > > > > > > change is Android-only for now. (see the bug for more details). > > > > > > > > > > > > A longer-term fix could be to distinguish between prerenders depending > > on > > > > > their > > > > > > origin. When it's external or coming from the omnibox, then the > priority > > > > > should > > > > > > be the same as a regular page load. In other cases we may want to > > throttle > > > > it > > > > > > more aggressively. > > > > > > > > > > I'm going to defer to Randy on this one (He's not an owner, but he knows > > > more > > > > > about prioritization than I do). > > > > > > > > > > What metrics are you planning to watch as this rolls out? Have you > > thought > > > > > about a field trial? > > > > > > > > I cannot copy-past actual numbers from UMA, but as you can see them here: > > > > > > > > > > https://uma.googleplex.com/p/chrome/histograms/?endDate=10-05-2016&dayCount=1... > > > > > > > > On Android, at least 90% of prerenders should have the same priority as a > > > normal > > > > page load, because they are initiated when Chrome is in the background > > already > > > > (external requests), or from the omnibox. > > > > > > > > The problem that we want to solve here is that by prerendering a link, you > > are > > > > quite likely to actually increasing the total PLT vs not doing anything. > > This > > > is > > > > a problem for AMP articles loaded from GSA on Android, but also for links > > > loaded > > > > from G+ or twitter. > > > > We agree that the fix here is not necessarily ideal, but since it solves a > > > > real-world issue, I think it should land sooner rather than later. > > > > > > > > What do you think? > > > > > > * Looking for some background. Can you help me understand *why* > > Omnibox/Instant > > > Search/External request goes through the pre-render path? At a surface > level, > > > that seems silly; they should be normal navigations. I assume there's some > > > context about cross-process intent handling on Android I don't have. > > > > > > > Sorry, I wasn't clear. I'm only referring to prerenders here. So it's not > about > > all navigations, merely the ones that trigger prerenders. Some omnibox > > completions trigger a prerender. The external part is when an application > tells > > Chrome of a likely future navigation to a URL. > > They still should be prerenders, because at that point they are speculative, > and > > we don't want these page loads to start playing audio, for instance. > > > > > * Why can't we shift the priorities just for the pre-renders with origins > that > > > we know should have the top level priority, as opposed to all pre-renders? > > How > > > hard would that plumbing be? > > > > That's the better solution, indeed. However we would like to keep the patch as > > small as possible to cherry-pick it to M55. But you're absolutely right, this > is > > likely the right long-term solution. > > Actually, I think the right long-term solution is reprioritizing the requests > associated with a pre-render when it becomes an actual navigation (especially > since these navigations really are pre-renders, just pre-renders that may be > more likely to turn into actual navigations and less likely to compete with > other work Chrome's trying to do). That might show up bugs in the netstack > around reprioritization, but that's ok; I'd like to fix those too :-}. > > > > > > > > > * I think the metrics you are pointing at are intended to show that the > large > > > majority of pre-renderers are sourced from origins that should have the > > regular > > > priority; please let me know if there's some point to those metrics that > I've > > > missed. I'd feel more comfortable if we also tracked overall PLT metrics as > > > this was rolled out, as I remain concerned about prerenders from origins > that > > > should have an IDLE priority interfering with other page loads. I'm happy > to > > > believe that this is a real, and bad, real world problem. But I think we > > should > > > be tracking the cost of the cure. > > > > > > > Tracking this would be hard, as prerenders are only a small fraction of > > navigations. So seeing the overall effect on normal page load would be > difficult > > without introducing complex new metrics (Time to Contentful Paint for pages > > loading while a prerender is in progress?) > > Fair, and I'm not looking for that, but I'd still be grateful if you can track > 95%l on PLT as this rolls out. > > Keep me updated as to the long-term work in this area? > > With all that LGTM (I'll let you and Charles evaluate the refactor). But please > don't drop the long-term fix on the floor. > > > > > > Charles, can you comment on what metrics would be good to track as this > rolls > > > out? LGTM, deferring to rdsmith.
The CQ bit was checked by lizeb@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/07 16:37:10, Randy Smith - Not in Mondays wrote: > Actually, I think the right long-term solution is reprioritizing the requests > associated with a pre-render when it becomes an actual navigation (especially > since these navigations really are pre-renders, just pre-renders that may be > more likely to turn into actual navigations and less likely to compete with > other work Chrome's trying to do). That might show up bugs in the netstack > around reprioritization, but that's ok; I'd like to fix those too :-}. Doing this would certainly improve things, since it would avoid prerender to have worse performance than a normal load, and stop prerender from being actually harmful. But I am not sure this is enough, since prerender will remain very slow when not displayed, which kindof defeats the point of prerender.
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, rdsmith@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2399973003/#ps60001 (title: "Rebase + Address comment.")
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 ========== prerender: Don't set the priority to net::IDLE on Android. Resource requests issued by prerendering renderers have their priority set to net::IDLE. This causes issues with content::ResourceScheduler, leading to very poor loading performance for prerendered content. This simply disables the prioritization on Android, as this is where the priority lowering is the most problematic, and where potential downsides from this patch are the most limited. This is a temporary fix. BUG=652746 ========== to ========== prerender: Don't set the priority to net::IDLE on Android. Resource requests issued by prerendering renderers have their priority set to net::IDLE. This causes issues with content::ResourceScheduler, leading to very poor loading performance for prerendered content. This simply disables the prioritization on Android, as this is where the priority lowering is the most problematic, and where potential downsides from this patch are the most limited. This is a temporary fix. BUG=652746 Committed: https://crrev.com/915cfe1d23cf249cff0b51b6f2a7434035d5e6c5 Cr-Commit-Position: refs/heads/master@{#424131} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/915cfe1d23cf249cff0b51b6f2a7434035d5e6c5 Cr-Commit-Position: refs/heads/master@{#424131}
Message was sent while issue was closed.
On 2016/10/10 11:04:06, droger wrote: > On 2016/10/07 16:37:10, Randy Smith - Not in Mondays wrote: > > Actually, I think the right long-term solution is reprioritizing the requests > > associated with a pre-render when it becomes an actual navigation (especially > > since these navigations really are pre-renders, just pre-renders that may be > > more likely to turn into actual navigations and less likely to compete with > > other work Chrome's trying to do). That might show up bugs in the netstack > > around reprioritization, but that's ok; I'd like to fix those too :-}. > > Doing this would certainly improve things, since it would avoid prerender to > have worse performance than a normal load, and stop prerender from being > actually harmful. > But I am not sure this is enough, since prerender will remain very slow when not > displayed, which kindof defeats the point of prerender. We should have this debate on a bug that's tracking the long-term fix. Is the one attached to this CL a good one? |
