Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1165853002:
Pipe impl_latency_takes_priority_ to the RenderScheduler. (Closed)
|
Created:
5 years, 6 months ago by alex clarke (OOO till 29th) Modified:
5 years, 6 months ago CC:
chromium-reviews, cc-bugs_chromium.org, scheduler-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPipe impl_latency_takes_priority_ to the RenderScheduler.
The scheduler will use this as a signal to help determine when blink
timers can be limited to running only during idle time.
BUG=463143
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/c8ff6cd3e974e84ad82ea521d68b014961b04d24
Cr-Commit-Position: refs/heads/master@{#333740}
Patch Set 1 #Patch Set 2 : Various fixes #Patch Set 3 : Added a couple of tests to SchedulerTest #Patch Set 4 : Rebased #
Total comments: 7
Patch Set 5 : Made the flag default #
Total comments: 6
Patch Set 6 : Fix test #
Messages
Total messages: 47 (19 generated)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165853002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165853002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165853002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165853002/60001
alexclarke@chromium.org changed reviewers: + brianderson@chromium.org, skyostil@chromium.org
My knowledge of the compositor is limited, and I was surprised by how many places construct BeginFrameArgs. I'm not at all confident I got the on_critical_path flag right everywhere.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/06/03 16:03:36, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. This patch is missing something in scheduler.cc possibly in OnBeginFrameMixInDelegate to do the following adjusted_args.on_critical_path = MainThreadOnCriticalPath(); That seems to work on device, but I'm having trouble writing a test.
On 2015/06/03 17:22:32, alexclarke1 wrote: > On 2015/06/03 16:03:36, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > This patch is missing something in scheduler.cc possibly in > OnBeginFrameMixInDelegate to do the following > > adjusted_args.on_critical_path = MainThreadOnCriticalPath(); > > > That seems to work on device, but I'm having trouble writing a test. I got the test working.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165853002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I guess the overall question is whether this bit makes sense in all contexts where BeginFrames are being used or whether we should call it something else or do the same by tweaking the deadline. Brian, WDYT? https://codereview.chromium.org/1165853002/diff/100001/cc/output/begin_frame_... File cc/output/begin_frame_args.cc (right): https://codereview.chromium.org/1165853002/diff/100001/cc/output/begin_frame_... cc/output/begin_frame_args.cc:31: on_critical_path(false) { I'm wondering if we should default this to true and then have a setter that can be used to override it? Usually BeginFrames are urgent. https://codereview.chromium.org/1165853002/diff/100001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1165853002/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:356: adjusted_args.deadline -= EstimatedParentDrawTime(); Another option would be to communicate this through the deadline. For example if your deadline is zero then you won't be on the critical path. https://codereview.chromium.org/1165853002/diff/100001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/1165853002/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.h:155: bool MainThreadOnCriticalPath() const { This should probably mirror the setter (SetImplLatencyTakesPriority). https://codereview.chromium.org/1165853002/diff/100001/cc/trees/single_thread... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/1165853002/diff/100001/cc/trees/single_thread... cc/trees/single_thread_proxy.cc:615: ? scheduler_on_impl_thread_->MainThreadOnCriticalPath() I think this should default to true when we are using the single threaded compositor. The compositor can't really do anything before the BeginFrame completes.
https://codereview.chromium.org/1165853002/diff/100001/cc/output/begin_frame_... File cc/output/begin_frame_args.cc (right): https://codereview.chromium.org/1165853002/diff/100001/cc/output/begin_frame_... cc/output/begin_frame_args.cc:31: on_critical_path(false) { On 2015/06/08 11:47:53, Sami wrote: > I'm wondering if we should default this to true and then have a setter that can > be used to override it? Usually BeginFrames are urgent. Done. https://codereview.chromium.org/1165853002/diff/100001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/1165853002/diff/100001/cc/scheduler/scheduler... cc/scheduler/scheduler.h:155: bool MainThreadOnCriticalPath() const { On 2015/06/08 11:47:53, Sami wrote: > This should probably mirror the setter (SetImplLatencyTakesPriority). Done. https://codereview.chromium.org/1165853002/diff/100001/cc/trees/single_thread... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/1165853002/diff/100001/cc/trees/single_thread... cc/trees/single_thread_proxy.cc:615: ? scheduler_on_impl_thread_->MainThreadOnCriticalPath() On 2015/06/08 11:47:53, Sami wrote: > I think this should default to true when we are using the single threaded > compositor. The compositor can't really do anything before the BeginFrame > completes. Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165853002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I say go ahead and add this flag for all BeginFrames. It'll be a long time before we have strong deadline guarantees, so until we get there we could use this to fudge the compositor's deadline. It could be useful for the Browser to set for a Renderer that is either 1) low-latency 2) being resized or 3) in the foreground. If we need to differentiate between the cases, we can change it to an enum in the future. https://codereview.chromium.org/1165853002/diff/120001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1165853002/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:357: adjusted_args.on_critical_path = !ImplLatencyTakesPriority(); Depending on how the Renderer scheduler uses this flag, you may also want to make on_critical_path false when MainThreadIsInHighLatencyMode and !SkipNextBeginMainFrameToReduceLatency since there's no chance of running in low-latency anyway, but that can come in a followup patch. https://codereview.chromium.org/1165853002/diff/120001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1165853002/diff/120001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1657: scheduler_->WillBeginFrame(cc::BeginFrameArgs::Create( Do you need to add another argument to Create?
Thanks Brian. https://codereview.chromium.org/1165853002/diff/120001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1165853002/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:357: adjusted_args.on_critical_path = !ImplLatencyTakesPriority(); On 2015/06/09 01:12:42, brianderson wrote: > Depending on how the Renderer scheduler uses this flag, you may also want to > make on_critical_path false when MainThreadIsInHighLatencyMode and > !SkipNextBeginMainFrameToReduceLatency since there's no chance of running in > low-latency anyway, but that can come in a followup patch. When this flag is false, the renderer scheduler is going to allow some non-critical work (e.g., timers) to run with an equal priority to the BeginFrame. In other words the semantic is that this BeginFrame isn't really that urgent, so no rush. Do you think the combination high latency and !skip has that same meaning? https://codereview.chromium.org/1165853002/diff/120001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1165853002/diff/120001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1657: scheduler_->WillBeginFrame(cc::BeginFrameArgs::Create( On 2015/06/09 01:12:42, brianderson wrote: > Do you need to add another argument to Create? We did that in the first patch set but I was thinking it was cleaner to default to true. Dissenting opinions welcome :)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165853002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1165853002/diff/120001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/1165853002/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:357: adjusted_args.on_critical_path = !ImplLatencyTakesPriority(); On 2015/06/09 09:47:34, Sami wrote: > On 2015/06/09 01:12:42, brianderson wrote: > > Depending on how the Renderer scheduler uses this flag, you may also want to > > make on_critical_path false when MainThreadIsInHighLatencyMode and > > !SkipNextBeginMainFrameToReduceLatency since there's no chance of running in > > low-latency anyway, but that can come in a followup patch. > > When this flag is false, the renderer scheduler is going to allow some > non-critical work (e.g., timers) to run with an equal priority to the > BeginFrame. In other words the semantic is that this BeginFrame isn't really > that urgent, so no rush. Do you think the combination high latency and !skip has > that same meaning? Hmm, probably not. It's more of an in-between priority where the main thread has a little more time to respond than in low latency mode. A better way to communicate might be to use a later deadline. Anyways, no need to worry about this now. https://codereview.chromium.org/1165853002/diff/120001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/1165853002/diff/120001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1657: scheduler_->WillBeginFrame(cc::BeginFrameArgs::Create( On 2015/06/09 09:47:34, Sami wrote: > On 2015/06/09 01:12:42, brianderson wrote: > > Do you need to add another argument to Create? > > We did that in the first patch set but I was thinking it was cleaner to default > to true. Dissenting opinions welcome :) Ah ok. This is good then.
(lgtm)++
The CQ bit was checked by alexclarke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165853002/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c8ff6cd3e974e84ad82ea521d68b014961b04d24 Cr-Commit-Position: refs/heads/master@{#333740}
Message was sent while issue was closed.
This is kind of late because the patch has already landed but when is BeginMainFrame not on the critical path? Why do we ever want to run timers not in idle time?
Message was sent while issue was closed.
On 2015/06/12 13:35:34, mithro wrote: > This is kind of late because the patch has already landed but when is > BeginMainFrame not on the critical path? When we are compositor scrolling. Why do we ever want to run timers not > in idle time? It's the inverse, if BeginMainFrame is on the critical path, we want to limit timers to idle periods, provided the scheduler is in compositor priority mode and doing so won't unduly starve timers.
Message was sent while issue was closed.
Patchset #7 (id:160001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
