Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400123002:
Wait a frame before finishing navigation (Closed)
DescriptionWait a frame before finishing navigation
** PERF SHERIFF: this CL may cause a small regression to
telemetry benchmark but it's expected ***
When running a telemetry benchmark, we don't want to start
interaction gestures until the page is fully interactive. Currently,
the NavigationAction waits until document.readyState is at least
interactive. This isn't enough, since it only indicates that Blink
is ready; the page's layers may not yet have made it through the
compositor's commit cycle yet. We need to wait until the compositor
is ready since the gestures may well be handled on the compositor
thread.
BUG=chromium:652905
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/584a67ecbf203fd00b785dc76fcfeb964c09c664
Patch Set 1 #Patch Set 2 : Add method to FakeTab to fix tests #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== Wait a frame before finishing navigation BUG=652905 ========== to ========== Wait a frame before finishing navigation When running a telemetry benchmark, we don't want to start interaction gestures until the page is fully interactive. Currently, the NavigationAction waits until document.readyState is at least interactive. This isn't enough, since it only indicates that Blink is ready; the page's layers may not yet have made it through the compositor's commit cycle yet. We need to wait until the compositor is ready since the gestures may well be handled on the compositor thread. BUG=652905 ==========
bokan@chromium.org changed reviewers: + nednguyen@google.com
Hi Ned, Does this look like an ok change? I've tested and it fixes the issue I was having with scheduler.tough_scheduling_cases trying to scroll before the viewport layers have been pushed to the compositor's "impl" thread. I think waiting one frame should be enough since, if the page is ready in Blink but not CC, we need to commit everything in order produce the next frame. At least that's my understanding.
On 2016/10/07 02:06:40, bokan wrote: > Hi Ned, > > Does this look like an ok change? I've tested and it fixes the issue I was > having with scheduler.tough_scheduling_cases trying to scroll before the > viewport layers have been pushed to the compositor's "impl" thread. I think > waiting one frame should be enough since, if the page is ready in Blink but not > CC, we need to commit everything in order produce the next frame. At least > that's my understanding. In general I think this is a good change to stabilize all telemetry benchmarks. Though the impact of this can be non-trivial. Could you send out an announcement on telemetry-announce@chromium.org mailing list to see if there is any push back from people? +Kouhei, Ksakamoto: how do you think this would affect the wait time of loading & page cycler?
nednguyen@google.com changed reviewers: + kouhei@chromium.org, ksakamoto@chromium.org
On 2016/10/07 10:35:50, nednguyen wrote: > On 2016/10/07 02:06:40, bokan wrote: > > Hi Ned, > > > > Does this look like an ok change? I've tested and it fixes the issue I was > > having with scheduler.tough_scheduling_cases trying to scroll before the > > viewport layers have been pushed to the compositor's "impl" thread. I think > > waiting one frame should be enough since, if the page is ready in Blink but > not > > CC, we need to commit everything in order produce the next frame. At least > > that's my understanding. > > In general I think this is a good change to stabilize all telemetry benchmarks. > Though the impact of this can be non-trivial. Could you send out an announcement > on mailto:telemetry-announce@chromium.org mailing list to see if there is any push back > from people? > > +Kouhei, Ksakamoto: how do you think this would affect the wait time of loading > & page cycler? Sure, sent a message to the list.
On 2016/10/07 17:17:29, bokan wrote: > On 2016/10/07 10:35:50, nednguyen wrote: > > On 2016/10/07 02:06:40, bokan wrote: > > > Hi Ned, > > > > > > Does this look like an ok change? I've tested and it fixes the issue I was > > > having with scheduler.tough_scheduling_cases trying to scroll before the > > > viewport layers have been pushed to the compositor's "impl" thread. I think > > > waiting one frame should be enough since, if the page is ready in Blink but > > not > > > CC, we need to commit everything in order produce the next frame. At least > > > that's my understanding. > > > > In general I think this is a good change to stabilize all telemetry > benchmarks. > > Though the impact of this can be non-trivial. Could you send out an > announcement > > on mailto:telemetry-announce@chromium.org mailing list to see if there is any > push back > > from people? > > > > +Kouhei, Ksakamoto: how do you think this would affect the wait time of > loading > > & page cycler? > > Sure, sent a message to the list. sgtm. If no one objects, we can land this next week
On 2016/10/07 19:15:56, nednguyen wrote: > On 2016/10/07 17:17:29, bokan wrote: > > On 2016/10/07 10:35:50, nednguyen wrote: > > > On 2016/10/07 02:06:40, bokan wrote: > > > > Hi Ned, > > > > > > > > Does this look like an ok change? I've tested and it fixes the issue I was > > > > having with scheduler.tough_scheduling_cases trying to scroll before the > > > > viewport layers have been pushed to the compositor's "impl" thread. I > think > > > > waiting one frame should be enough since, if the page is ready in Blink > but > > > not > > > > CC, we need to commit everything in order produce the next frame. At least > > > > that's my understanding. > > > > > > In general I think this is a good change to stabilize all telemetry > > benchmarks. > > > Though the impact of this can be non-trivial. Could you send out an > > announcement > > > on mailto:telemetry-announce@chromium.org mailing list to see if there is > any > > push back > > > from people? > > > > > > +Kouhei, Ksakamoto: how do you think this would affect the wait time of > > loading > > > & page cycler? > > > > Sure, sent a message to the list. > > sgtm. > If no one objects, we can land this next week I think loading / page cycler are fine, as PageCyclerStory already waits until document.readyState is complete.
lgtm
On 2016/10/11 06:22:11, Kunihiko Sakamoto wrote: > On 2016/10/07 19:15:56, nednguyen wrote: > > On 2016/10/07 17:17:29, bokan wrote: > > > On 2016/10/07 10:35:50, nednguyen wrote: > > > > On 2016/10/07 02:06:40, bokan wrote: > > > > > Hi Ned, > > > > > > > > > > Does this look like an ok change? I've tested and it fixes the issue I > was > > > > > having with scheduler.tough_scheduling_cases trying to scroll before the > > > > > viewport layers have been pushed to the compositor's "impl" thread. I > > think > > > > > waiting one frame should be enough since, if the page is ready in Blink > > but > > > > not > > > > > CC, we need to commit everything in order produce the next frame. At > least > > > > > that's my understanding. > > > > > > > > In general I think this is a good change to stabilize all telemetry > > > benchmarks. > > > > Though the impact of this can be non-trivial. Could you send out an > > > announcement > > > > on mailto:telemetry-announce@chromium.org mailing list to see if there is > > any > > > push back > > > > from people? > > > > > > > > +Kouhei, Ksakamoto: how do you think this would affect the wait time of > > > loading > > > > & page cycler? > > > > > > Sure, sent a message to the list. > > > > sgtm. > > If no one objects, we can land this next week > > I think loading / page cycler are fine, as PageCyclerStory already waits until > document.readyState is complete. document.readyState is not enough. That's a signal that Blink has finished loading the page but there's still a race between sending gestures and Blink uploading the new layers to the compositor.
The CQ bit was checked by bokan@chromium.org
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
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
Description was changed from ========== Wait a frame before finishing navigation When running a telemetry benchmark, we don't want to start interaction gestures until the page is fully interactive. Currently, the NavigationAction waits until document.readyState is at least interactive. This isn't enough, since it only indicates that Blink is ready; the page's layers may not yet have made it through the compositor's commit cycle yet. We need to wait until the compositor is ready since the gestures may well be handled on the compositor thread. BUG=652905 ========== to ========== Wait a frame before finishing navigation ** PERF SHERIFF: this CL may cause a small regression to telemetry benchmark but it's expected *** When running a telemetry benchmark, we don't want to start interaction gestures until the page is fully interactive. Currently, the NavigationAction waits until document.readyState is at least interactive. This isn't enough, since it only indicates that Blink is ready; the page's layers may not yet have made it through the compositor's commit cycle yet. We need to wait until the compositor is ready since the gestures may well be handled on the compositor thread. BUG=652905 ==========
On 2016/10/11 14:23:02, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...) I added a note for perf sheriff
Description was changed from ========== Wait a frame before finishing navigation ** PERF SHERIFF: this CL may cause a small regression to telemetry benchmark but it's expected *** When running a telemetry benchmark, we don't want to start interaction gestures until the page is fully interactive. Currently, the NavigationAction waits until document.readyState is at least interactive. This isn't enough, since it only indicates that Blink is ready; the page's layers may not yet have made it through the compositor's commit cycle yet. We need to wait until the compositor is ready since the gestures may well be handled on the compositor thread. BUG=652905 ========== to ========== Wait a frame before finishing navigation ** PERF SHERIFF: this CL may cause a small regression to telemetry benchmark but it's expected *** When running a telemetry benchmark, we don't want to start interaction gestures until the page is fully interactive. Currently, the NavigationAction waits until document.readyState is at least interactive. This isn't enough, since it only indicates that Blink is ready; the page's layers may not yet have made it through the compositor's commit cycle yet. We need to wait until the compositor is ready since the gestures may well be handled on the compositor thread. BUG=chromium:652905 ==========
The CQ bit was checked by bokan@chromium.org
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
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2400123002/#ps20001 (title: "Add method to FakeTab to fix tests")
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.
Description was changed from ========== Wait a frame before finishing navigation ** PERF SHERIFF: this CL may cause a small regression to telemetry benchmark but it's expected *** When running a telemetry benchmark, we don't want to start interaction gestures until the page is fully interactive. Currently, the NavigationAction waits until document.readyState is at least interactive. This isn't enough, since it only indicates that Blink is ready; the page's layers may not yet have made it through the compositor's commit cycle yet. We need to wait until the compositor is ready since the gestures may well be handled on the compositor thread. BUG=chromium:652905 ========== to ========== Wait a frame before finishing navigation ** PERF SHERIFF: this CL may cause a small regression to telemetry benchmark but it's expected *** When running a telemetry benchmark, we don't want to start interaction gestures until the page is fully interactive. Currently, the NavigationAction waits until document.readyState is at least interactive. This isn't enough, since it only indicates that Blink is ready; the page's layers may not yet have made it through the compositor's commit cycle yet. We need to wait until the compositor is ready since the gestures may well be handled on the compositor thread. BUG=chromium:652905 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
