[cc/scheduler] Add SendEarlyLastBeginMainFrame [chromium/src : main]
Stacy Gaikovaia (Gerrit)
Stacy Gaikovaia added 1 comment![]( "Open in Gerrit")
This is the root cl for a chain which implements the performance optimization that Blink is looking for. Please perform a preliminary review. Patch #9 passes dry-run on CQ. Thanks!
Related details
- Jonathan Ross
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 3 comments![]( "Open in Gerrit")
viz::BeginFrameArgs args = last_dispatched_begin_main_frame_args_;We should exit if `!last_dispatched_begin_main_frame_args_.IsValid()`. As `interval` would be 0 for the division below.
Could occur after context loss, where the Renderer stays alive but we rebuild CC to reconnect to new Viz.
args.on_critical_path = !state_machine_->ImplLatencyTakesPriority();Would we want this feature to be `on_critical_path` by default?
urgent_begin_main_frame_pending_ = true;
SetNeedsBeginMainFrame(true);Could we reuse the `bool now` param of `SetNeedsBeginMainFrame` instead of a new `urgent_begin_main_frame_pending_`?
Related details
- Stacy Gaikovaia
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stacy Gaikovaia (Gerrit)
Stacy Gaikovaia added 3 comments![]( "Open in Gerrit")
viz::BeginFrameArgs args = last_dispatched_begin_main_frame_args_;We should exit if `!last_dispatched_begin_main_frame_args_.IsValid()`. As `interval` would be 0 for the division below.
Could occur after context loss, where the Renderer stays alive but we rebuild CC to reconnect to new Viz.
Sounds good, done.
args.on_critical_path = !state_machine_->ImplLatencyTakesPriority();Would we want this feature to be `on_critical_path` by default?
Sure - that's the default case, so I will remove those lines.
urgent_begin_main_frame_pending_ = true;
SetNeedsBeginMainFrame(true);Could we reuse the `bool now` param of `SetNeedsBeginMainFrame` instead of a new `urgent_begin_main_frame_pending_`?
- Jonathan Ross
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stacy Gaikovaia (Gerrit)
Stacy Gaikovaia added 1 comment![]( "Open in Gerrit")
urgent_begin_main_frame_pending_ = true;
SetNeedsBeginMainFrame(true);Stacy GaikovaiaCould we reuse the `bool now` param of `SetNeedsBeginMainFrame` instead of a new `urgent_begin_main_frame_pending_`?
Leaving this as a TODO for myself.
The answer is no, it does too many things and we can't just insert it in place of urgent_bmf_
Related details
- Jonathan Ross
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 5 comments![]( "Open in Gerrit")
Once subtle scheduling question. Overall approach SGTM
// in which we send a BeginMainFrame with the same args we just processedPlease fix this WARNING reported by autoreview issue finding: This comment is slightly misleading. The code actually increments the sequence number and adjusts the frame time and deadline to align with current time, rather than using the exact same args.
This comment is slightly misleading. The code actually increments the sequence number and adjusts the frame time and deadline to align with current time, rather than using the exact same args.
void Scheduler::SendEarlyLastBeginMainFrame() {We aren't invoking this in code in this patch, just tests for now?
state_machine_->SetUrgentBeginMainFramePending();We could set this after calling `FinishImplFrame`
Please fix this WARNING reported by autoreview issue finding: Setting this flag here is potentially problematic. `FinishImplFrame()` (called below) triggers `ProcessScheduledActions()`. If the scheduler is currently in a state that allows sending a BeginMainFrame, it will do so immediately using the *current* (stale) `begin_main_frame_args_`.
`OnBeginFrame(args)` is the call that updates `begin_main_frame_args_` to the new values calculated in this method. To ensure the "urgent" frame uses the correct, updated args, you should set this flag *after* the `OnBeginFrame(args)` call or ensure that no actions are processed during the intermediate `FinishImplFrame()`.
Setting this flag here is potentially problematic. `FinishImplFrame()` (called below) triggers `ProcessScheduledActions()`. If the scheduler is currently in a state that allows sending a BeginMainFrame, it will do so immediately using the *current* (stale) `begin_main_frame_args_`.
`OnBeginFrame(args)` is the call that updates `begin_main_frame_args_` to the new values calculated in this method. To ensure the "urgent" frame uses the correct, updated args, you should set this flag *after* the `OnBeginFrame(args)` call or ensure that no actions are processed during the intermediate `FinishImplFrame()`.
int64_t intervals = base::ClampRound(elapsed / args.interval);Please fix this WARNING reported by autoreview issue finding: `base::ClampRound` requires `#include \"base/numerics/safe_conversions.h\"`. Please add it to the includes section.
`base::ClampRound` requires `#include \"base/numerics/safe_conversions.h\"`. Please add it to the includes section.
Related details
- Stacy Gaikovaia
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |