Prerendered content in Custom Tabs sometimes show a blank page for a long time. |
||||||||||
Issue descriptionTest scenario: Using the test application in Chrome (tools/android/customtabs_benchmark), issue in order: warmup() -> (1s) -> mayLaunchUrl(url) -> (1s) -> Intent(url) Sometimes, the tab displays blank content for several seconds (~7-8s, in one case at least). Context: Nexus 4, Android L, chrome_apk, master as of 10/4. Attached are two traces, with and without the issue. These are attached through Drive, as they are >10MB. From looking at the two traces, the obvious difference is a ProxyMain::SetDeferCommits event in the renderer, lasting a total of 7.7s! From the code: https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebLayerTreeView.h?sq=package:chromium&dr=CSs&rcl=1475579939&l=142 // Prevents updates to layer tree from becoming visible.
,
Oct 5 2016
Here is a way to reproduce it. Unfortunately it is not reliable, and doesn't seem to happen on all the phone I've tested it with. It happens semi-reliably on a Nexus 4, running this command: python tools/android/customtabs_benchmark/scripts/customtabs_benchmark.py --once --delay_to_launch 1000 --delay_to_may_launch_url 1000 --cold --warmup --output_file - --wpr_archive android_com.wpr --wpr_log android_com.log --network_condition=Regular3G --prerender=enabled This requires CustomTabsBenchmark.apk to be installed on the device.
,
Oct 5 2016
So, state of the investigation: - When the navigation commits, Document::Shutdown is called, presumably to shut down the initial blank document (called from WebURLLoaderImpl::Context::OnReceivedData, before WebLocalFrameImpl::createFrameView and RenderFrameImpl::didCommitProvisionalLoad) - The document shutdown prevents the compositor from outputting new frames by posting SetDeferCommitsOnImpl on the Compositor thread (through Scheduler::SetDeferCommits). - Later, from StyleElement::processStyleSheet, beginLifecycleUpdates() is called. It posts a task to the Compositor thread, calling Scheduler::SetDeferCommits(false). The compositor can now display frames. This same sequence happens in both the good and the bad cases. The difference is that in the bad case, it happens multiple seconds too late. In the good case, it happens the first time a stylesheet is processed, not in the other case. Note that this seems a bit dubious anyway, since that means that we're not displaying the progress bar, even though the navigation has committed. beginLifecycleUpdates() is called from Document::beginLifecycleUpdatesIfRenderingReady(). That's where I'm currently at...
,
Oct 5 2016
Do you think this is only on 55 or do we see it on 54 as well?
,
Oct 6 2016
Here is what we discovered: - the page is actually very slow to load when prerendering, a lot slower than with a normal load. - there is probably a bug with the deferred commits (we should see the progress bar at least), but it is not the main reason why the page is slow. The most important issue is a bug in the way the resources are scheduled/prioritized: all resources from the prerender have IDLE priority, even after the prerender becomes visible. IDLE resources are heavily throttled. In fact IDLE requests are so slow that one small request from <head> takes 8s to complete and blocks all the rendering in the page (hence why it is blank). In more details: All requests from prerender are set to IDLE in chrome_resource_dispatcher_host_delegate.cc: https://cs.chromium.org/chromium/src/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc?rcl=1475738528&l=433 Then the ResourceScheduler thinks that all the requests are delayable, since they are very low priority. When the layout is blocked, it would let only let one go through at a time (kMaxNumDelayableWhileLayoutBlockingPerClient = 1), and in some random order (requests all have the same IDLE priority, any relative priority is lost). This potentially leads to the whole page being loaded sequentially. When the page becomes visible (when the prerender is swapped in), only image priorities seem to be updated, all other requests remain on IDLE, which does not unblock the layout. We believe the bug is quite old and very likely on stable. We expect that it causes prerendering performance to be very bad (impacting AMP and Android CustomTabs in particular).
,
Oct 7 2016
The simplest solution is to remove the lowering of request priorities described above. This means that from the point of view of content::ResourceScheduler, prerender loads are no longer different from normal ones. There are several ways a prerender can be triggered (see the prerender::Origin enum): - <link rel="prerender"> - Google Search (technically not different from the previous one) - Omnibox - Instant Search - External request (on Android) The largest contributors (by far) are (on Android): - External - Omnibox In both these cases, it makes sense to consider the prerender as a normal load. So removing the priority lowering on Android makes sense. This is a temporary solution, made to be easy to cherry-pick to M55. For context, there are a lot of "external" requests on Android, and we have had reports of poor performance in this case. This fixes a real issue, and quite a widespread one.
,
Oct 7 2016
,
Oct 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/915cfe1d23cf249cff0b51b6f2a7434035d5e6c5 commit 915cfe1d23cf249cff0b51b6f2a7434035d5e6c5 Author: lizeb <lizeb@chromium.org> Date: Mon Oct 10 11:14:07 2016 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 Review-Url: https://codereview.chromium.org/2399973003 Cr-Commit-Position: refs/heads/master@{#424131} [modify] https://crrev.com/915cfe1d23cf249cff0b51b6f2a7434035d5e6c5/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
,
Oct 11 2016
Requesting a merge to M55. Rationale: large performance issue affecting actual users, trivial patch.
,
Oct 11 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Oct 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f49bbc55974f5e09484a38917d37877c3ef71251 commit f49bbc55974f5e09484a38917d37877c3ef71251 Author: Benoit Lize <lizeb@chromium.org> Date: Tue Oct 11 11:35:25 2016 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 Review-Url: https://codereview.chromium.org/2399973003 Cr-Commit-Position: refs/heads/master@{#424131} (cherry picked from commit 915cfe1d23cf249cff0b51b6f2a7434035d5e6c5) Review URL: https://codereview.chromium.org/2411663002 . Cr-Commit-Position: refs/branch-heads/2883@{#33} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/f49bbc55974f5e09484a38917d37877c3ef71251/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
,
Oct 12 2016
Lets split the progress bar issues in another bug ( bug 655060 ) and keep this one about the priority issues.
,
Oct 17 2016
Marking as fixed, as the temporary fix has been merged to M55.
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f49bbc55974f5e09484a38917d37877c3ef71251 commit f49bbc55974f5e09484a38917d37877c3ef71251 Author: Benoit Lize <lizeb@chromium.org> Date: Tue Oct 11 11:35:25 2016 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 Review-Url: https://codereview.chromium.org/2399973003 Cr-Commit-Position: refs/heads/master@{#424131} (cherry picked from commit 915cfe1d23cf249cff0b51b6f2a7434035d5e6c5) Review URL: https://codereview.chromium.org/2411663002 . Cr-Commit-Position: refs/branch-heads/2883@{#33} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/f49bbc55974f5e09484a38917d37877c3ef71251/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840
,
Nov 8 2016
Just got interested in-- Do we have a plan or separate bug for fixing or addressing this issue other than on Android / how to solve this in longer term?
,
Nov 8 2016
Yes: remove prerender :)
,
Nov 8 2016
#18: the problem would also affect no-state prefetch
,
Nov 8 2016
there is also "fix loading prioritization" or .. "remove prioritization", where I guess kinuko@ suggests more coordination. Is there a bug for that? rdsmith@ may know
,
Nov 8 2016
Surfacing a discussion that took place in the code review for the Android CL: * One suggested fix would be to reset the priority of the requests when the pre-render becomes visible. This would make sure that pre-rendering does not actually hurt the page load time. The downside of this approach is that it does not address completely the problem: the performance of pre-rendering (or prefetching) will still be worse than what it could be. * Another suggested fix is to apply the Android fix to all platforms. This would improve prerendering/prefetching performance, but the issue is that prerendering could then start to compete with visible pages. This was deemed OK on Android, because on that platform prerendering is only enabled in a limited set of cases, for which the UX should not be affected too much if the prerenders are over-prioritized.
,
Nov 8 2016
> The downside of this approach is that it does not address completely the problem: the performance of pre-rendering (or prefetching) will still be worse than what it could be. So I want to understand better the reasons for this. Are you referring to the fact that, until the priority is raised, the pre-render will be moving fairly slowly, and that will delay the user seeing the page because less will have gotten done before the priority is raised? If so, I'm consider that WAI--if something's a pre-render, it should be behind most other stuff we're doing in the browser, and only when we're certain that it's going to be an actual navigation should it start taking a more fair share of the resources. But an area I could imagine a bug in is if the browser isn't doing anything other than the pre-render (which is how I imagine the Android use case, at least) and the pre-render is still slow. If that's your concern, I agree that's a bug; if the browser isn't doing anything other than IDLE requests, those IDLE requests should get all the resources. This isn't what currently happens due to the architecture of ResourceScheduler, but I'd be happy to move in that direction. The work I'm currently doing will help (because IDLE requests are throttled *drastically* before a certain point in HTML parsing, and if all requests are IDLE that means progress will be glacial to that point), but won't completely fix things (because there is a global limit on IDLE requests even after that point in HTML parsing). But one approach we might take is to see how much this is improved by the my change. Note that even in a perfecter (sic) world in which IDLE requests got all available resources in the absence of any higher priority requests, we'd still not have the ideal handling, because we would have lost the prioritization differentiation among the different requests involved in the pre-render. The right way to fix that is by a complete overhaul of the priority API, which at least for the moment I consider intractable. But let's not worry about that issue until we actually know it's a serious problem in the wild.
,
Nov 8 2016
> Are you referring to the fact that, until the priority is raised, the pre-render will be moving fairly slowly Yes, this is the concern. > But an area I could imagine a bug in is if the browser isn't doing anything other than the pre-render (which is how I imagine the Android use case, at least) and the pre-render is still slow. Yes, this is also what I meant. Prerender is still slow even if the browser does nothing. This is indeed what happened in my case. I agree with all what you said. Something else to consider that you did not mention: even if the browser is doing nothing, maybe we still don't want to use all the resources? Just in case the browser would suddenly need them. Do we need/have a way to pause low priority work to make room for high priority work?
,
Nov 8 2016
> Something else to consider that you did not mention: even if the browser is doing nothing, maybe we still don't want to use all the resources? Just in case the browser would suddenly need them. Do we need/have a way to pause low priority work to make room for high priority work? So yeah, this is an issue, and I've chewed on it some without feeling like I see a complete solution. At the most abstract level, a partial answer is that if we throttle before a resource bottleneck s.t. that bottleneck is only a little bit oversubscribed, if a higher priority request comes in, the resource utilization at the bottleneck should drop below the bottleneck "soon enough" to allow the high priority request to progress. The main place I'm aware of this breaking down is around the network bandwidth, where the client doesn't have a lot of control over bandwidth use once the request is sent, though it does have some at a RTT delay. At an immediate practical level, I"m experimenting with a new priority level under IDLE (THROTTLED) that specifically holds back requests even if there isn't anything else to do with the network resources. That level is intended to be used when a higher level expects that higher priority requests will be coming along fairly soon--specific use case is the preload scanner for web pages, which will be followed by actual prioritization after parsing. But it's not a question I think is solved by any means. Meta-question: Should we create a new bug around this question? It feels funny to be having debates around code choices going forward on a closed bug. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by yus...@chromium.org
, Oct 4 2016