ADPF adaptive mode [chromium/src : main]
Richard Townsend (Gerrit)
Richard Townsend added 5 comments![]( "Open in Gerrit")
PS9: doesn't unconditionally request efficiency - just for a short period. Improves semantics of the hint_session.
Achieves a good speedometer score. Also seems to work well when scrolling Naver.com. https://ui.perfetto.dev/#!/?s=ead14eb14dce841ee535df0afa4cff678349532f
Next patchset will remove the MainThreadSchedulerImpl loading detection (solely rely on WebContentsImpl to kick).
out_frame.metadata.power_efficient_scheduling = power_efficient_scheduling_;Richard TownsendUnsure if this is required.
Done
set_power_efficient_scheduling(commit_state.power_efficient_scheduling);Richard TownsendTrace this
Done
void VizCompositorThreadRunnerImpl::SetPowerEfficientScheduling(bool enabled) {Richard TownsendIrrelevant
Not irrelevant!
// navigations only. This a trade off between latency and power - we don't
// want to do it for every navigation.
if (navigation_handle->IsInMainFrame()) {Richard TownsendI guess the argument is that you're saving much more power so we should do it everywhere? We can probably iterate a bunch here on heuristics but can start wtih this
Richard TownsendI think we can actually save a bit more power:
- Boost is requested
- If we miss the frame deadline => actually boost
This should strike a good balance between scroll smoothness and energy savings. It seems that loadline doesn't move much under the always efficient mode, so this is basically required only for Speeedometer.
Draft implementation in next patchset.
Related details
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 3 comments![]( "Open in Gerrit")
* Latest version requests a boost, fulfills the boost if a frame failsThis is no longer true
// navigations only. This a trade off between latency and power - we don't
// want to do it for every navigation.
if (navigation_handle->IsInMainFrame()) {Richard TownsendI guess the argument is that you're saving much more power so we should do it everywhere? We can probably iterate a bunch here on heuristics but can start wtih this
Richard TownsendI think we can actually save a bit more power:
- Boost is requested
- If we miss the frame deadline => actually boost
This should strike a good balance between scroll smoothness and energy savings. It seems that loadline doesn't move much under the always efficient mode, so this is basically required only for Speeedometer.
Draft implementation in next patchset.
OK, the "miss the frame deadline => actually boost" implementation is perceptibly less smooth, so will leave as-is for now.
if (!performance_scenarios::CurrentScenariosMatch(Richard TownsendI guess this is global state so it would know if compositor was handling a fast-path scroll and not visible (yet) on main?
Richard TownsendkGlobal was to try and resolve some bugginess, but it seems that this policy inconsistently detects whether something is loading or not - sometimes it works really well, sometimes it does not, hence the double-kick from WebContentsImpl. I think we me might be able to drop it back to single-kick.
Done
Related details
- Yaron Friedman
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. |
Yaron Friedman (Gerrit)
Yaron Friedman added 1 comment![]( "Open in Gerrit")
- Richard Townsend
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. |
Richard Townsend (Gerrit)
Richard Townsend voted and added 1 comment![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
1 comment
is this ready for review?
Yep, just manually validated it and it seems to be doing the right thing.
Related details
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Hi Igor, could you PTAL at this from the ADPF perspective?
Best
Richard
Related details
- Igor Kraskevich
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Hi Jon, could you PTAL at this power change from a viz/ + cc/ perspective?
Best
Richard
Related details
- Igor Kraskevich
- Jonathan Ross
- Yaron Friedman
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. |
Yaron Friedman (Gerrit)
Yaron Friedman added 5 comments![]( "Open in Gerrit")
just some minor commenst. probably best to have Igor/Jon look?
RequestForceSendMetadata();I thought this form of metadata was renderer->browser. Is it needed?
The rest is just side-car on the messages that would already be pushed renderer->viz so no force needed?
is_page_load ? base::Seconds(5) : base::Seconds(1);make field trial params?
// Notify the OS that the workload is about to increase for main frameupdate and flag guard
// We only need to ask a single widget scheduler because (at the moment)but you're asking all of them? i don't understand the comment
Related details
- Igor Kraskevich
- Jonathan Ross
- Richard Townsend
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. |
Yaron Friedman (Gerrit)
Yaron Friedman added 1 comment![]( "Open in Gerrit")
just some minor commenst. probably best to have Igor/Jon look?
lol, glad we thought of the same reviewers 😊
Richard Townsend (Gerrit)
Richard Townsend added 2 comments![]( "Open in Gerrit")
RequestForceSendMetadata();I thought this form of metadata was renderer->browser. Is it needed?
The rest is just side-car on the messages that would already be pushed renderer->viz so no force needed?
It's renderer to GPU process. I can try removing this and check it still works.
// We only need to ask a single widget scheduler because (at the moment)but you're asking all of them? i don't understand the comment
Yeah, we only need to ask a single one, but the `for (..) { ws -> RequestSchedulerBoost(); break; }` pattern is blocked by the Blink style plugin. Ultimately, they all set the the same flag somewhere in `cc`, and usually there seems to be only one WidgetScheduler.
Related details
- Igor Kraskevich
- Jonathan Ross
- Yaron Friedman
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. |
Igor Kraskevich (Gerrit)
Igor Kraskevich added 7 comments![]( "Open in Gerrit")
High level question - have we tested this for:
1. Speedometer 3 (I presume yes looking at the patch description/comments etc.)
2. Loadline (https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline2/, https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline)
If so, is there any impact on the scores?
Unfortunately we don't have any reliable local benchmarks for interactions or scrolling, the best we can do is grab some local traces from live websites and check that the new signal is doing something reasonable - we'll have to wait until we get field data to get any trustworthy numbers.
result == 0, "session", (void*)this);Why do we need this? Or more like - how is this supposed to be used?
const bool efficiency_preferred,
const bool is_page_load) {Maybe make this an enum? Like `{ kEfficiency, kPageLoad, kUserInput }`
The pair of bools allows passing combinations like "prefer efficiency + is page load" - I don't think we intend to use those?
// Default behaviour is 5 seconds for a page load, and 1 for a scroll.What about interactions? If a boost triggers only for page loads and scrolls, I'd expect a big INP regression.
P.S. Looking at the code below it's not just scroll but rather any user input. In which case let's just update the comment.
// Notify the OS that the workload is about to increase for main frameupdate and flag guard
+1, this shouldn't be an unconditional change.
The NotifyWorkloadIncrease logic was originally implemented for sending CPU_LOAD_UP hint on page load - https://chromium-review.googlesource.com/c/chromium/src/+/7062031, b/443653027. It is intentionally limited to main frames only to balance perf boost vs power drain.
We will run a field experiment for that one after the next Android release reaches most Pixel users.
As an option we could also plumb the information "is this page load for main frame" through NotifyWorkloadIncrease to viz and decide how to boost inside the hint session.
// N.B. we don't try to detect loading here because it doesn't seem
// effective.What does "doesn't seem effective" mean exactly? I.e. if the detection broken here, is it too slow for some reason, or something else?
// We only need to ask a single widget scheduler because (at the moment)Richard Townsendbut you're asking all of them? i don't understand the comment
Yeah, we only need to ask a single one, but the `for (..) { ws -> RequestSchedulerBoost(); break; }` pattern is blocked by the Blink style plugin. Ultimately, they all set the the same flag somewhere in `cc`, and usually there seems to be only one WidgetScheduler.
but the for (..) { ws -> RequestSchedulerBoost(); break; } pattern is blocked by the Blink style pluginIs there any other way to achieve the same? I understand that you want to notify only one widget scheduler, but at the moment the code and the comment diverge and cause some confusion.
Related details
- Jonathan Ross
- Richard Townsend
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend voted and added 9 comments![]( "Open in Gerrit")
Votes added by Richard Townsend
| Auto-Submit | +1 |
High level question - have we tested this for:
1. Speedometer 3 (I presume yes looking at the patch description/comments etc.)
2. Loadline (https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline2/, https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline)If so, is there any impact on the scores?
Unfortunately we don't have any reliable local benchmarks for interactions or scrolling, the best we can do is grab some local traces from live websites and check that the new signal is doing something reasonable - we'll have to wait until we get field data to get any trustworthy numbers.
Here are my current results for LoadLine:
```
Never: 87.91
Adaptive (previous PS): 86.64
Always: 77.40 (-12%)
```
To be honest, I expected it to be much worse given that 50% of the CPU power vanishes.
Speedometer:
```
Never: 17.834
Adaptive: 14.62
Always: 7.76
```
Unfortunately I don't have a very large sample for either Loadline or Speedometer on this PS, a previous CL achieved basically an identical Speedometer score.
Incremental CL to respond to the comments so far
The duration of boosts is now configurable e.g. with:
```
./out/arm64/bin/chrome_public_apk run --args='--enable-features=EnableAdpfEfficiencyMode:mode/adaptive/page_load_boost_duration/1s/interaction_boost_duration/16ms --disable-features=RestrictMainThreadBigCoreAffinity' https://buzzfeed.com
```
What's not done yet is the re-plumbing of WebContentsImpl and/or MainThreadSchedulerImpl. @lizeb wants integration with the ThreadAffinityBoost system, but the threading model doesn't doesn't work for me (everything is frame-based and single-threaded) in this version. We still need to pass the input scenario down on CompositorFrameMetadata to let Viz make the decision, _or_ forward a main-frame annotation for WebContentsImpl, don't know which will work yet
Richard TownsendI thought this form of metadata was renderer->browser. Is it needed?
The rest is just side-car on the messages that would already be pushed renderer->viz so no force needed?
It's renderer to GPU process. I can try removing this and check it still works.
It does still work, so removed in next PS.
Why do we need this? Or more like - how is this supposed to be used?
Removed in next patch set
const bool efficiency_preferred,
const bool is_page_load) {Maybe make this an enum? Like `{ kEfficiency, kPageLoad, kUserInput }`
The pair of bools allows passing combinations like "prefer efficiency + is page load" - I don't think we intend to use those?
Done
// Default behaviour is 5 seconds for a page load, and 1 for a scroll.What about interactions? If a boost triggers only for page loads and scrolls, I'd expect a big INP regression.
P.S. Looking at the code below it's not just scroll but rather any user input. In which case let's just update the comment.
Acknowledged
is_page_load ? base::Seconds(5) : base::Seconds(1);Richard Townsendmake field trial params?
Done
// N.B. we don't try to detect loading here because it doesn't seem
// effective.What does "doesn't seem effective" mean exactly? I.e. if the detection broken here, is it too slow for some reason, or something else?
As in the kLoading scenario never seemed to match/fire - I'm not sure why.
// We only need to ask a single widget scheduler because (at the moment)Richard Townsendbut you're asking all of them? i don't understand the comment
Igor KraskevichYeah, we only need to ask a single one, but the `for (..) { ws -> RequestSchedulerBoost(); break; }` pattern is blocked by the Blink style plugin. Ultimately, they all set the the same flag somewhere in `cc`, and usually there seems to be only one WidgetScheduler.
but the for (..) { ws -> RequestSchedulerBoost(); break; } pattern is blocked by the Blink style pluginIs there any other way to achieve the same? I understand that you want to notify only one widget scheduler, but at the moment the code and the comment diverge and cause some confusion.
So there is a case where we have more than one WidgetScheduler, and that's when you have more than one OOPIF. This seems not to practically matter since all the renderers I think (?) end up in the same ADPF session, but since that might change in future, it might be worth informing all of them
Related details
- Benoit Lize
- Igor Kraskevich
- Jonathan Ross
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend voted![]( "Open in Gerrit")
| Auto-Submit | +0 |
| Commit-Queue | +1 |
Igor Kraskevich (Gerrit)
Igor Kraskevich added 1 comment![]( "Open in Gerrit")
Richard TownsendHigh level question - have we tested this for:
1. Speedometer 3 (I presume yes looking at the patch description/comments etc.)
2. Loadline (https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline2/, https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline)If so, is there any impact on the scores?
Unfortunately we don't have any reliable local benchmarks for interactions or scrolling, the best we can do is grab some local traces from live websites and check that the new signal is doing something reasonable - we'll have to wait until we get field data to get any trustworthy numbers.
Here are my current results for LoadLine:
```
Never: 87.91
Adaptive (previous PS): 86.64
Always: 77.40 (-12%)
```To be honest, I expected it to be much worse given that 50% of the CPU power vanishes.
Speedometer:
```
Never: 17.834
Adaptive: 14.62
Always: 7.76
```Unfortunately I don't have a very large sample for either Loadline or Speedometer on this PS, a previous CL achieved basically an identical Speedometer score.
Loadline looks OK to me, Never vs Adaptive can be just noise.
The speedometer drop is very big though. I don't think we can ship it with such a regression (assuming that it's real but speedometer is usually much more consistent than loadline so like 5 local runs for each config should give a good picture).
Related details
- Benoit Lize
- Jonathan Ross
- Richard Townsend
- Yaron Friedman
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Richard TownsendHigh level question - have we tested this for:
1. Speedometer 3 (I presume yes looking at the patch description/comments etc.)
2. Loadline (https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline2/, https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline)If so, is there any impact on the scores?
Unfortunately we don't have any reliable local benchmarks for interactions or scrolling, the best we can do is grab some local traces from live websites and check that the new signal is doing something reasonable - we'll have to wait until we get field data to get any trustworthy numbers.
Igor KraskevichHere are my current results for LoadLine:
```
Never: 87.91
Adaptive (previous PS): 86.64
Always: 77.40 (-12%)
```To be honest, I expected it to be much worse given that 50% of the CPU power vanishes.
Speedometer:
```
Never: 17.834
Adaptive: 14.62
Always: 7.76
```Unfortunately I don't have a very large sample for either Loadline or Speedometer on this PS, a previous CL achieved basically an identical Speedometer score.
Loadline looks OK to me, Never vs Adaptive can be just noise.
The speedometer drop is very big though. I don't think we can ship it with such a regression (assuming that it's real but speedometer is usually much more consistent than loadline so like 5 local runs for each config should give a good picture).
This device can be a bit noisy due to thermals, so I wouldn't read into it too much - a previous run with 100 iterations did not display a statistically significant difference
Related details
- Benoit Lize
- Igor Kraskevich
- Jonathan Ross
- Yaron Friedman
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
* Latest version requests a boost, fulfills the boost if a frame failsThis is no longer true
Done
Igor Kraskevich (Gerrit)
Igor Kraskevich voted and added 2 comments![]( "Open in Gerrit")
Votes added by Igor Kraskevich
| Code-Review | +1 |
2 comments
LGTM modulo one thing - let's not just call `NotifyWorkloadIncrease` for all frame navigations in content/browser/web_contents/web_contents_impl.cc unconditionally as this will interfere with the upcoming CPU_LOAD_UP experiment.
kInteraction = 2,Nit: `kUserInput`?
Interaction is heavily associated with this: https://web.dev/articles/inp#whats_in_an_interaction
While we're "boosting" all kinds of inputs, including e.g. (some) scrolls.
The same for `kAdpfInteractionBoostParam` and related code comments.
Related details
- Benoit Lize
- Jonathan Ross
- Richard Townsend
- Yaron Friedman
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. |
Igor Kraskevich (Gerrit)
Igor Kraskevich voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Benoit Lize
- Jonathan Ross
- Richard Townsend
- Yaron Friedman
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. |
Jonathan Ross (Gerrit)
Jonathan Ross added 3 comments![]( "Open in Gerrit")
Conceptually SGTM. One question and general +1 to other comments/requests
if (hint != HintSession::Scenario::kNormal) {Would we ever want to `NotifyWorkloadReset` upon receiving `Normal`?
This could be a new frame arriving, without input, before we have completed the DrawAndSwap + Presentation of the frame we requested a boost for
// Notify the OS that the workload is about to increase for main frameIgor Kraskevichupdate and flag guard
+1, this shouldn't be an unconditional change.
The NotifyWorkloadIncrease logic was originally implemented for sending CPU_LOAD_UP hint on page load - https://chromium-review.googlesource.com/c/chromium/src/+/7062031, b/443653027. It is intentionally limited to main frames only to balance perf boost vs power drain.
We will run a field experiment for that one after the next Android release reaches most Pixel users.
As an option we could also plumb the information "is this page load for main frame" through NotifyWorkloadIncrease to viz and decide how to boost inside the hint session.
- Benoit Lize
- Richard Townsend
- Yaron Friedman
Richard Townsend (Gerrit)
Richard Townsend added 11 comments![]( "Open in Gerrit")
bool scheduler_boost_requested = true;Might be better to flip this over to "prefer_efficient_scheduling = false"
bool scheduler_boost_requested_ = false;Same, prefer_efficient_scheduling = false
kAdpfPageLoadBoostParam;These parameters are no longer required.
constexpr base::FeatureParam<base::TimeDelta> kAdpfPageLoadBoostParam{Parameters are no longer required.
bool scheduler_boost_requested = false;`prefer_efficient_scheduling = false`
!frame.metadata.scheduler_boost_requested);Remove not-inversion for clarity, rename to `prefer_efficient_scheduling`
Would we ever want to `NotifyWorkloadReset` upon receiving `Normal`?
This could be a new frame arriving, without input, before we have completed the DrawAndSwap + Presentation of the frame we requested a boost for
Approach has been superseded by a request from MainThreadSchedulerImpl
// Notify the OS that the workload is about to increase for main frameIgor Kraskevichupdate and flag guard
+1, this shouldn't be an unconditional change.
The NotifyWorkloadIncrease logic was originally implemented for sending CPU_LOAD_UP hint on page load - https://chromium-review.googlesource.com/c/chromium/src/+/7062031, b/443653027. It is intentionally limited to main frames only to balance perf boost vs power drain.
We will run a field experiment for that one after the next Android release reaches most Pixel users.
As an option we could also plumb the information "is this page load for main frame" through NotifyWorkloadIncrease to viz and decide how to boost inside the hint session.
Removed in next PS.
bool scheduler_boost_requested;rename to prefer_efficient_scheduling
Related details
- Benoit Lize
- Igor Kraskevich
- Jonathan Ross
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 10 comments![]( "Open in Gerrit")
PLATFORM_EXPORT BASE_DECLARE_FEATURE(kRestrictMainThreadBigCoreAffinity);Make sure to remove any tests
MakeNamedTrack("Scheduler.BoostPerformance", this),Flip to "PreferEfficiency"
void MainThreadSchedulerImpl::ApplyPerformanceState(const bool boost) {Flip over to "efficiency_preferred"
using UpdateStateCallback = base::RepeatingCallback<void(bool is_boosted)>;Flip to "prefer_efficiency"
void PerformanceHelper::Check(base::TimeTicks cur) {Flip logic over to "prefer_efficiency"
// Check whether we're ahead of the expiryNit: end comments with a period.
void OnMainThreadPriorityBoostChanged(bool is_boosted) const {Flip to "prefer_efficiency"
virtual void OnMainThreadPriorityBoostChanged(bool is_boosted) const = 0;Flip to "prefer_efficiency"
void OnMainThreadPriorityBoostChanged(bool is_boosted) const override;Flip to "prefer_efficiency"
void WidgetBase::OnMainThreadPriorityBoostChanged(const bool is_boosted) const {Flip to "prefer_efficiency"
Related details
- Benoit Lize
- Igor Kraskevich
- Jonathan Ross
- Yaron Friedman
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. |
Yaron Friedman (Gerrit)
Yaron Friedman added 3 comments![]( "Open in Gerrit")
bool scheduler_boost_requested = true;Might be better to flip this over to "prefer_efficient_scheduling = false"
sgtm
if (navigation_handle->IsInMainFrame()) {looks like this is still altered unconditionally
&kUsePerformanceHelper, "page_load_boost", base::Seconds(0.5)};copy-pasto? this was going to be higher, right? ~5
Also is 500ms sufficient?
Related details
- Benoit Lize
- Igor Kraskevich
- Jonathan Ross
- Richard Townsend
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
// Stores whether the session is current configured for efficiency. Used to
// de-bounce calls to setPreferPowerEfficiency.
bool will_prefer_efficiency_ = false;
bool prefer_efficiency_ = false;Nit: maybe rename `prefer_efficiency_` to `prefer_efficiency_applied_` ?
Update comment to reflect that `will_prefer_efficiency_` is the adaptive mode preference. While `prefer_efficiency_applied_` is the current sessions state
Related details
- Benoit Lize
- Igor Kraskevich
- Richard Townsend
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. |
Richard Townsend (Gerrit)
Richard Townsend added 20 comments![]( "Open in Gerrit")
bool scheduler_boost_requested = true;Might be better to flip this over to "prefer_efficient_scheduling = false"
Done
These parameters are no longer required.
Done
bool scheduler_boost_requested = false;Richard Townsend`prefer_efficient_scheduling = false`
Done
Nit: `kUserInput`?
Interaction is heavily associated with this: https://web.dev/articles/inp#whats_in_an_interaction
While we're "boosting" all kinds of inputs, including e.g. (some) scrolls.
The same for `kAdpfInteractionBoostParam` and related code comments.
Superseded by a less ambiguous enum `BoostType::kTapOrTyping` in the current PS
// Stores whether the session is current configured for efficiency. Used to
// de-bounce calls to setPreferPowerEfficiency.
bool will_prefer_efficiency_ = false;
bool prefer_efficiency_ = false;Nit: maybe rename `prefer_efficiency_` to `prefer_efficiency_applied_` ?
Update comment to reflect that `will_prefer_efficiency_` is the adaptive mode preference. While `prefer_efficiency_applied_` is the current sessions state
Done
if (navigation_handle->IsInMainFrame()) {looks like this is still altered unconditionally
Will be reverted in next PS.
bool scheduler_boost_requested;Richard Townsendrename to prefer_efficient_scheduling
Done
PLATFORM_EXPORT BASE_DECLARE_FEATURE(kRestrictMainThreadBigCoreAffinity);Make sure to remove any tests
Done
// rendererRichard TownsendCorrect line-wrapping
Done
&kUsePerformanceHelper, "page_load_boost", base::Seconds(0.5)};copy-pasto? this was going to be higher, right? ~5
Also is 500ms sufficient?
These values are "how long should we boost for after the UseCase has expired", so it's acceptable to have a short duration (I expect that we will tune this).
MakeNamedTrack("Scheduler.BoostPerformance", this),Richard TownsendFlip to "PreferEfficiency"
Done
void MainThreadSchedulerImpl::ApplyPerformanceState(const bool boost) {Richard TownsendFlip over to "efficiency_preferred"
Done
using UpdateStateCallback = base::RepeatingCallback<void(bool is_boosted)>;Richard TownsendFlip to "prefer_efficiency"
Done
void PerformanceHelper::Check(base::TimeTicks cur) {Flip logic over to "prefer_efficiency"
Done
Nit: end comments with a period.
Done
void OnMainThreadPriorityBoostChanged(bool is_boosted) const {Richard TownsendFlip to "prefer_efficiency"
Done
virtual void OnMainThreadPriorityBoostChanged(bool is_boosted) const = 0;Richard TownsendFlip to "prefer_efficiency"
Done
void OnMainThreadPriorityBoostChanged(bool is_boosted) const override;Richard TownsendFlip to "prefer_efficiency"
Done
void WidgetBase::OnMainThreadPriorityBoostChanged(const bool is_boosted) const {Richard TownsendFlip to "prefer_efficiency"
Done
Related details
- Benoit Lize
- Igor Kraskevich
- Jonathan Ross
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend voted and added 6 comments![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
6 comments
bool scheduler_boost_requested_ = false;Richard TownsendSame, prefer_efficient_scheduling = false
Done
constexpr base::FeatureParam<base::TimeDelta> kAdpfPageLoadBoostParam{Richard TownsendParameters are no longer required.
Done
!frame.metadata.scheduler_boost_requested);Richard TownsendRemove not-inversion for clarity, rename to `prefer_efficient_scheduling`
Done
if (navigation_handle->IsInMainFrame()) {Richard Townsendlooks like this is still altered unconditionally
Will be reverted in next PS.
Done
TODO: write some documentation here
Done
&kUsePerformanceHelper, "page_load_boost", base::Seconds(0.5)};Richard Townsendcopy-pasto? this was going to be higher, right? ~5
Also is 500ms sufficient?
These values are "how long should we boost for after the UseCase has expired", so it's acceptable to have a short duration (I expect that we will tune this).
Done
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Example session of Buzzfeed with the scheduler in place:
https://ui.perfetto.dev/#!/?s=e91780af6bb43d09e1ac9b55f786130c34a04ebe
Some interesting notes: there are cases where the frame metadata doesn't make it, and some frames seem to also trigger the boost, even though the UseCase doesn't change. Investigating.
Richard Townsend (Gerrit)
Richard Townsend voted and added 2 comments![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
LGTM modulo one thing - let's not just call `NotifyWorkloadIncrease` for all frame navigations in content/browser/web_contents/web_contents_impl.cc unconditionally as this will interfere with the upcoming CPU_LOAD_UP experiment.
Done
// CPU time.Richard Townsendnit: update this comment
Done
Related details
- Benoit Lize
- Igor Kraskevich
- Jonathan Ross
- Yaron Friedman
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. |
Jonathan Ross (Gerrit)
Jonathan Ross voted Code-Review+1![]( "Open in Gerrit")
- Benoit Lize
- Igor Kraskevich
- Richard Townsend
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend voted and added 1 comment![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
1 comment
Thanks Jon, continuing to fiddle until debug builds work...
Related details
- Benoit Lize
- Igor Kraskevich
- Yaron Friedman
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. |
Yaron Friedman (Gerrit)
Yaron Friedman added 2 comments![]( "Open in Gerrit")
BASE_FEATURE(kRestrictMainThreadBigCoreAffinity,this will break benoit's config, right? (that's ok, just a heads-up, as we'll need a new arm configured for that variant)
performance_helper_.Check();hmm, these checks are load-bearing now. if there's a failure/early-out case or we miss something, they never expire? Did you consider a timeut/timer based appraoch?
Related details
- Benoit Lize
- Igor Kraskevich
- Richard Townsend
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. |
Richard Townsend (Gerrit)
Richard Townsend added 2 comments![]( "Open in Gerrit")
BASE_FEATURE(kRestrictMainThreadBigCoreAffinity,this will break benoit's config, right? (that's ok, just a heads-up, as we'll need a new arm configured for that variant)
Hopefully, we won't have two slightly different affinity trials. Once we have some numbers back from Benoit's initial trial, we should start up a revised trial with three (or maybe four) arms:
- Baseline/Control
- Affinity
- ADPF efficiency
- Both
performance_helper_.Check();hmm, these checks are load-bearing now. if there's a failure/early-out case or we miss something, they never expire? Did you consider a timeut/timer based appraoch?
DeadlineTimer might be a good way to trigger the update, with a only a few extra moving parts. As this method ticks every frame, it seemed sensible to avoid this overhead. That said, it is a little irregular (since it's aligned to frame delivery).
If the method stops ticking, then we're either a) blocked or b) backgrounded. If we're blocked, this is likely due to user input (e.g. clicking a button that executes a few seconds of JS), we likely don't want to revise the throttling state in this case. If we're backgrounded, then we should be using very little CPU already.
Related details
- Benoit Lize
- Igor Kraskevich
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
performance_helper_.Check();Richard Townsendhmm, these checks are load-bearing now. if there's a failure/early-out case or we miss something, they never expire? Did you consider a timeut/timer based appraoch?
DeadlineTimer might be a good way to trigger the update, with a only a few extra moving parts. As this method ticks every frame, it seemed sensible to avoid this overhead. That said, it is a little irregular (since it's aligned to frame delivery).
If the method stops ticking, then we're either a) blocked or b) backgrounded. If we're blocked, this is likely due to user input (e.g. clicking a button that executes a few seconds of JS), we likely don't want to revise the throttling state in this case. If we're backgrounded, then we should be using very little CPU already.
Actually (just chasing a few bugs out), I think there might be a better/simpler way of doing this:
- Add to the timer at the start of every frame based on the current use_case
- Check the timer at the end of every frame and the start of any idle period
This seems to resolve the issue with the current patchset where the boost can toggle on and off within a given UseCase period.
Richard Townsend (Gerrit)
Richard Townsend added 3 comments![]( "Open in Gerrit")
PS21 centralizes and simplifies the logic for adding the boost, moving to a per-frame model.
// Start with a "boost", that is initially allow the renderer to run
// everywhere. This is meant to help with initialization. In the worst case,
// the current use case never changes, and the renderer is always allowed to
// run on all cores. But that would also mean a renderer that didn't load
// anything, ad was never interacted with, which then means it should not
// use a lot of resources.Nit: clarify and simplify this comment
constexpr bool ScrollNeedsMainThreadPrioritization(const UseCase use_case) {
switch (use_case) {
case UseCase::kTouchstart:
case UseCase::kSynchronizedGesture:
case UseCase::kMainThreadGesture:
case UseCase::kMainThreadCustomInputHandling:
return true;
default:
return false;
}
}Nit: remove this now-unnecessary function.
Related details
- Benoit Lize
- Igor Kraskevich
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
{PerformanceHintMode::kAffinity, "affinity"},I think this needs to be set with @Benoit's similarly named flag `RestrictBigCoreThreadAffinity`, otherwise it doesn't quite have quite the intended effect (e.g. the renderer's Compositor threads will escape onto the main core). This should be documented at minimum.
Richard Townsend (Gerrit)
Richard Townsend voted and added 5 comments![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
5 comments
I think this needs to be set with @Benoit's similarly named flag `RestrictBigCoreThreadAffinity`, otherwise it doesn't quite have quite the intended effect (e.g. the renderer's Compositor threads will escape onto the main core). This should be documented at minimum.
Done
// Start with a "boost", that is initially allow the renderer to run
// everywhere. This is meant to help with initialization. In the worst case,
// the current use case never changes, and the renderer is always allowed to
// run on all cores. But that would also mean a renderer that didn't load
// anything, ad was never interacted with, which then means it should not
// use a lot of resources.Nit: clarify and simplify this comment
Done
Richard Townsendhmm, these checks are load-bearing now. if there's a failure/early-out case or we miss something, they never expire? Did you consider a timeut/timer based appraoch?
Richard TownsendDeadlineTimer might be a good way to trigger the update, with a only a few extra moving parts. As this method ticks every frame, it seemed sensible to avoid this overhead. That said, it is a little irregular (since it's aligned to frame delivery).
If the method stops ticking, then we're either a) blocked or b) backgrounded. If we're blocked, this is likely due to user input (e.g. clicking a button that executes a few seconds of JS), we likely don't want to revise the throttling state in this case. If we're backgrounded, then we should be using very little CPU already.
Actually (just chasing a few bugs out), I think there might be a better/simpler way of doing this:
- Add to the timer at the start of every frame based on the current use_case
- Check the timer at the end of every frame and the start of any idle period
This seems to resolve the issue with the current patchset where the boost can toggle on and off within a given UseCase period.
PS21 seems to be significantly better behaved, so resolving this one (feel free to re-open).
constexpr bool ScrollNeedsMainThreadPrioritization(const UseCase use_case) {
switch (use_case) {
case UseCase::kTouchstart:
case UseCase::kSynchronizedGesture:
case UseCase::kMainThreadGesture:
case UseCase::kMainThreadCustomInputHandling:
return true;
default:
return false;
}
}Nit: remove this now-unnecessary function.
Done
Related details
- Benoit Lize
- Igor Kraskevich
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 9 comments![]( "Open in Gerrit")
PLATFORM_EXPORT BASE_DECLARE_FEATURE(kRestrictMainThreadBigCoreAffinity);It's clear I'm going to have to bring this back because fixes keep landing, making this change harder to rebase.
base::FEATURE_DISABLED_BY_DEFAULT);Bring back and add additional CHECKs to prevent them the new flag from being switched on at the same time.
// anything, ad was never interacted with, which then means it should notReturn this code.
previous_use_case == UseCase::kEarlyLoading;Return this code
// Checking early, as the forced update below will reset it.Return
[&](base::PlatformThreadId thread_id, bool allowed) {Return
Richard Townsend (Gerrit)
Richard Townsend added 9 comments![]( "Open in Gerrit")
// enabled.Richard TownsendBring back
Done
PLATFORM_EXPORT BASE_DECLARE_FEATURE(kRestrictMainThreadBigCoreAffinity);It's clear I'm going to have to bring this back because fixes keep landing, making this change harder to rebase.
Done
base::FEATURE_DISABLED_BY_DEFAULT);Bring back and add additional CHECKs to prevent them the new flag from being switched on at the same time.
Done
if (--depth_ == 0) {Richard TownsendReturn this
Done
// anything, ad was never interacted with, which then means it should notRichard TownsendReturn this code.
Done
previous_use_case == UseCase::kEarlyLoading;Richard TownsendReturn this code
Done
Richard TownsendReturn this code
Done
// Checking early, as the forced update below will reset it.Richard TownsendReturn
Done
[&](base::PlatformThreadId thread_id, bool allowed) {Richard TownsendReturn
Done
Related details
- Benoit Lize
- Igor Kraskevich
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
PS23 restores the original affinity helper and command line flags, ready for review
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Hi IPC reviewers, PTAL at this CL which adds an additional boolean flag on CompositorFrameMetadata.
Best
Richard
Related details
- Benoit Lize
- Chromium IPC Reviews
- Igor Kraskevich
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Hi Ian, PTAL at some changes to MainThreadSchedulerImpl, WidgetScheduler and associated machinery. These changes are to communicate a page's idle state to the compositor so appropriate power management can be applied on Android.
Best
Richard
Related details
- Benoit Lize
- Chromium IPC Reviews
- Ian Kilpatrick
- Igor Kraskevich
- Yaron Friedman
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ke...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ke...@chromium.org
Reviewer source(s):
ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Benoit Lize
- Ian Kilpatrick
- Igor Kraskevich
- Ken Buchanan
- Yaron Friedman
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
I'm probably not the best for this - maybe fdoray@ ?
Related details
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Ken Buchanan
- Richard Townsend
- Yaron Friedman
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. |
Ken Buchanan (Gerrit)
Ken Buchanan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ken Buchanan
| Code-Review | +1 |
1 comment
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Richard Townsend
- Yaron Friedman
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 12 comments![]( "Open in Gerrit")
void RequestEfficientScheduling(const bool prefer_efficiency) {Remove const.
Reason: "For a function parameter passed by value, const has no effect on the caller, thus is not recommended in function declarations. See TotW #109." https://google.github.io/styleguide/cppguide.html#Use_of_const
nit: Group overrides together:
```
// HintSession:
void NotifyWorkloadReset() override;
void NotifyWorkloadIncrease() override;
void SetPreferPowerEfficientScheduling(bool prefer_efficiency) final;
void WakeUp();
bool ShouldScheduleForEfficiency() const;
```
Reason: Readability.
bool ShouldScheduleForEfficiency() const;Keep this in the private section?
Reason: I couldn't find where it's used outside of this class?
void AdpfHintSession::SetPreferPowerEfficientScheduling(bool prefer_efficient) {Use the same name as in the declaration.
Reason: Readability.
// power during e.g. an idle period.Tip: If this is Android-specific, you can add `[EnableIf=is_android]` so it's not compiled on other platforms.
// Start in high-performance mode.
performance_helper_.Add(PerformanceHelper::BoostType::kPageLoad);Given that we have spare renderers (which remain unused for an unbounded amount of time), is this the right initial state?
// then (perhaps) uncap the CPU's performance for this and a few subsequentCapture time in a local variable and pass it as argument to Add() and Check(), to avoid sampling the time multiple times (and you already have a time argument on Add() and Check() to take an externally captured time).
What guarantees do we have that WillBeginFrame, BeginFrameNotExpectedSoon, BeginMainFrameNotExpectedUntil will be invoked ~soon after a boost expires? E.g. If:
- Frames are produced for a short amount of time
- BeginFrameNotExpectedSoon is invoked, because no frames are expected soon.
- The boost timeout expires
- No frames are ever produced again
What guarantees that we'll call `performance_helper_.Check()` and end the boost?
base::TimeDelta GetDuration(BoostType) const;Add a blank line here.
Reason: Consistency with surrounding code. Example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h;l=682;drc=b973733c8ce67cf9f8ea339c2abb85998e65b4a1
void PerformanceHelper::Configure(const Params&& params) {Change the type to `Params params`.
Reason:
- "Do not use rvalue references (or apply the && qualifier to methods), except as follows [...] if you want to "consume" an ordinary function parameter, prefer to pass it by value". https://google.github.io/styleguide/cppguide.html#Rvalue_references
- Because the argument is const, line 20 doesn't actually move the callback.
params_.loading_boost = params.loading_boost;Move the params struct, instead of individual members of the params struct?
Reason: Readability, avoid future errors.
[[likely]] now = base::TimeTicks::LowResolutionNow();Suggestion: Use default argument value instead. (You're already using a default argument value, it's a matter of changing `base::TimeTicks()` to `base::TimeTicks::LowResolutionNow()`.
Edit: It might not even be worthwhile to support passing a null time (see https://chromium-review.googlesource.com/c/chromium/src/+/7238073/23/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc#1052).
Related details
- Benoit Lize
Richard Townsend (Gerrit)
Richard Townsend voted and added 13 comments![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
13 comments
PS24/PS25 address review comments, correct a misnamed private variable, generally improve consistency and clarify inline documentation. Ready for review after dry-run. Undergoing testing:
- ./out/arm64/bin/chrome_public_apk run --args='EnableAdpfEfficiencyMode:mode/never,RestrictMainThreadBigCoreAffinity,RestrictBigCoreThreadAffinity' (check for no regressions)
- ./out/arm64/bin/chrome_public_apk run --args='EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:mode/affinity,RestrictBigCoreThreadAffinity' (check that the affinity performance management is working correctly)
- ./out/arm64/bin/chrome_public_apk run --args='EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:mode/compositor' (check that ADPF performance management is working)
- ./out/arm64/bin/chrome_public_apk run --args='EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:mode/both, RestrictBigCoreThreadAffinity' (check that the combined strategy is working)
- ./out/arm64.dchecks/bin/chrome_public_apk run --args='EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:mode/both, RestrictMainThreadBigCoreAffinity,RestrictBigCoreThreadAffinity' (check that the PerformanceHelper and @lizeb's approach cannot be accidentally enabled together).
void RequestEfficientScheduling(const bool prefer_efficiency) {Remove const.
Reason: "For a function parameter passed by value, const has no effect on the caller, thus is not recommended in function declarations. See TotW #109." https://google.github.io/styleguide/cppguide.html#Use_of_const
Done
nit: Group overrides together:
```
// HintSession:
void NotifyWorkloadReset() override;
void NotifyWorkloadIncrease() override;
void SetPreferPowerEfficientScheduling(bool prefer_efficiency) final;
void WakeUp();
bool ShouldScheduleForEfficiency() const;
```Reason: Readability.
Done
Keep this in the private section?
Reason: I couldn't find where it's used outside of this class?
Done
void AdpfHintSession::SetPreferPowerEfficientScheduling(bool prefer_efficient) {Use the same name as in the declaration.
Reason: Readability.
Done
Tip: If this is Android-specific, you can add `[EnableIf=is_android]` so it's not compiled on other platforms.
Unfortunately it doesn't seem to play well with cross-compilation, but I might be holding it wrong. I think that it's reasonably forseeable that other platforms could make use of it, but nothing's specifically planned at the moment.
// Start in high-performance mode.
performance_helper_.Add(PerformanceHelper::BoostType::kPageLoad);Given that we have spare renderers (which remain unused for an unbounded amount of time), is this the right initial state?
That's a good question. The spare renderer thing is a case where performance shouldn't matter in general, but maybe (?) does in practise and the idea is to keep the existing behaviour as close as possible. In my view: this is something that could be tweaked later in-field.
// then (perhaps) uncap the CPU's performance for this and a few subsequentCapture time in a local variable and pass it as argument to Add() and Check(), to avoid sampling the time multiple times (and you already have a time argument on Add() and Check() to take an externally captured time).
Done
What guarantees do we have that WillBeginFrame, BeginFrameNotExpectedSoon, BeginMainFrameNotExpectedUntil will be invoked ~soon after a boost expires? E.g. If:
- Frames are produced for a short amount of time
- BeginFrameNotExpectedSoon is invoked, because no frames are expected soon.
- The boost timeout expires
- No frames are ever produced again
What guarantees that we'll call `performance_helper_.Check()` and end the boost?
here is nothing that currently guarantees calling `performance_helper_.Check()` once we go idle, but I believe that is the correct trade-off.
My thinking is:
- During activity: We check on frame boundaries.
- During sleep: We let the boost hang. Waking the CPU from a deep C-state via a Timer just to clear a hint likely consumes more power than leaving the hint active on a sleeping thread.
- On wake-up: We catch the transition immediately.
However, having reviewed the code more carefully based on your comment, I think that relying on `UpdatePolicy` for the 'wake-up' check is insufficient, as it only runs when priorities change. In the next patch set, I've added an extra check on `MaybeUpdatePolicyOnTaskCompleted`. This guarantees that the very first task to run after a sleep will invalidate the boost.
Add a blank line here.
Reason: Consistency with surrounding code. Example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h;l=682;drc=b973733c8ce67cf9f8ea339c2abb85998e65b4a1
Done
void PerformanceHelper::Configure(const Params&& params) {Change the type to `Params params`.
Reason:
- "Do not use rvalue references (or apply the && qualifier to methods), except as follows [...] if you want to "consume" an ordinary function parameter, prefer to pass it by value". https://google.github.io/styleguide/cppguide.html#Rvalue_references
- Because the argument is const, line 20 doesn't actually move the callback.
Done
Move the params struct, instead of individual members of the params struct?
Reason: Readability, avoid future errors.
Done
[[likely]] now = base::TimeTicks::LowResolutionNow();Suggestion: Use default argument value instead. (You're already using a default argument value, it's a matter of changing `base::TimeTicks()` to `base::TimeTicks::LowResolutionNow()`.
Edit: It might not even be worthwhile to support passing a null time (see https://chromium-review.googlesource.com/c/chromium/src/+/7238073/23/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc#1052).
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend voted and added 1 comment![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +2 |
1 comment
Verified:
./out/arm64/bin/chrome_public_apk run --args='--enable-features=EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:mode/compositor' - works as expected, see all the relevant trace events
Richard Townsend (Gerrit)
Richard Townsend voted and added 1 comment![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
1 comment
Verified: ./out/arm64/bin/chrome_public_apk run --args='--enable-features=EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:mode/both,RestrictBigCoreThreadAffinity' - works
Related details
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Richard Townsend
- Yaron Friedman
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Verified: ./out/arm64/bin/chrome_public_apk run --args='--enable-features=EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:mode/both,RestrictBigCoreThreadAffinity' - works
Verified: ./out/arm64/bin/chrome_public_apk run --args='--enable-features=EnableAdpfEfficiencyMode:mode/never,RestrictMainThreadBigCoreAffinity,RestrictBigCoreThreadAffinity - CrRendererMain does not appear on the big core
Related details
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Yaron Friedman
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Verified: ./out/arm64.dchecks/bin/chrome_public_apk run --args='--enable-features=EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:mode/both,RestrictMainThreadBigCoreAffinity,RestrictBigCoreThreadAffinity' - stops the flags being enabled together
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Richard TownsendHigh level question - have we tested this for:
1. Speedometer 3 (I presume yes looking at the patch description/comments etc.)
2. Loadline (https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline2/, https://chromium.googlesource.com/crossbench/+/refs/heads/main/config/benchmark/loadline)If so, is there any impact on the scores?
Unfortunately we don't have any reliable local benchmarks for interactions or scrolling, the best we can do is grab some local traces from live websites and check that the new signal is doing something reasonable - we'll have to wait until we get field data to get any trustworthy numbers.
Igor KraskevichHere are my current results for LoadLine:
```
Never: 87.91
Adaptive (previous PS): 86.64
Always: 77.40 (-12%)
```To be honest, I expected it to be much worse given that 50% of the CPU power vanishes.
Speedometer:
```
Never: 17.834
Adaptive: 14.62
Always: 7.76
```Unfortunately I don't have a very large sample for either Loadline or Speedometer on this PS, a previous CL achieved basically an identical Speedometer score.
Richard TownsendLoadline looks OK to me, Never vs Adaptive can be just noise.
The speedometer drop is very big though. I don't think we can ship it with such a regression (assuming that it's real but speedometer is usually much more consistent than loadline so like 5 local runs for each config should give a good picture).
Richard TownsendThis device can be a bit noisy due to thermals, so I wouldn't read into it too much - a previous run with 100 iterations did not display a statistically significant difference
OK, test results for PS25:
Basically, LoadLine looks OK, but looks like there's a problem with Speedometer (about 10% down in terms of Geomean). I think the problem is that the page load clamp comes too soon. I'll experiment and address in PS26.
Richard Townsend (Gerrit)
Richard Townsend voted and added 2 comments![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
2 comments
PS26: corrects some inline documentation and formatting, adds a missing call inside `MainThreadSchedulerImpl::DidCommitProvisionalLoad` which corrects the Speedometer 3.1 performance regression.
void RequestSchedulingBoost(bool is_due_to_input);Remove
Related details
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 2 comments![]( "Open in Gerrit")
Test results for PS26, seems that everything is lined up for Speedometer 3.1:
https://colab.sandbox.google.com/drive/1iG4tpezeAnaWZLA-q9_Eh0wX53kGMcM9?resourcekey=0-HrdsF2raQEhuKn-heBCCeg&usp=sharing#scrollTo=aR6eepXNzBxK
And as the LoadLine results looked OK for PS25, I think we can say that all of the various MainThreadSchedulerImpl experiments are OK to ship from a Speedometer perspective
Related details
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Francois Pierre Doray (Gerrit)
Francois Pierre Doray voted and added 9 comments![]( "Open in Gerrit")
Votes added by Francois Pierre Doray
| Code-Review | +1 |
//third_party/blink/renderer/platform/scheduler/ LGTM, with a few suggestions for consistency of naming (I didn't put comments everywhere applicable)
void RequestEfficientScheduling(bool prefer_efficiency) {
pending_commit_state()->prefer_efficient_scheduling = prefer_efficiency;
}Suggestion for consistency of naming:
```suggestion
void RequestEfficientScheduling(bool prefer_efficient_scheduling) {
pending_commit_state()->prefer_efficient_scheduling = prefer_efficient_scheduling;
}
```
void SetPreferEfficientScheduling(bool efficiency_preferred) const;Suggestion for consistency of naming:
```suggestion
void SetPreferEfficientScheduling(bool prefer_efficient_scheduling) const;
```
void FrameSinkManagerImpl::SetPreferEfficientScheduling(
const bool prefer_efficiency) const {
if (hint_session_factory_) {
hint_session_factory_->SetPreferPowerEfficientScheduling(prefer_efficiency);
}
}Suggestion for consistency of naming:
```suggestion
void FrameSinkManagerImpl::SetPreferEfficientScheduling(
const bool prefer_efficient_scheduling) const {
if (hint_session_factory_) {
hint_session_factory_->SetPreferEfficientScheduling(
prefer_efficient_scheduling);
}
}
```
virtual void SetPreferPowerEfficientScheduling(bool prefer_efficiency) = 0;Suggestion for consistency of naming:
```suggestion
virtual void SetPreferEfficientScheduling(bool prefer_efficient_scheduling) = 0;
```
void AdpfHintSession::SetPreferPowerEfficientScheduling(
bool prefer_efficiency) {
DCHECK_CALLED_ON_VALID_THREAD(factory_->thread_checker_);
will_prefer_efficiency_ = prefer_efficiency;
}Suggestion for consistency of naming:
```suggestion
void AdpfHintSession::SetPreferEfficientScheduling(
bool prefer_efficient_scheduling) {
DCHECK_CALLED_ON_VALID_THREAD(factory_->thread_checker_);
will_prefer_efficiency_ = prefer_efficient_scheduling;
}
```
void HintSessionFactoryImpl::SetPreferPowerEfficientScheduling(
bool prefer_efficiency) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
for (auto& session : hint_sessions_) {
session->SetPreferPowerEfficientScheduling(prefer_efficiency);
}
}Suggestion for consistency of naming:
```suggestion
void HintSessionFactoryImpl::SetPreferEfficientScheduling(
bool prefer_efficient_scheduling) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
for (auto& session : hint_sessions_) {
session->SetPreferEfficientScheduling(prefer_efficient_scheduling);
}
}
```
performance_helper_.Check();Pass `time_now`.
void MainThreadSchedulerImpl::ApplyPerformanceState(
const bool prefer_efficiency) {
DCHECK(base::FeatureList::IsEnabled(kUsePerformanceHelper));
bool should_send_to_compositor = true;
#if BUILDFLAG(IS_ANDROID)
if (kUsePerformanceHelperParam.Get() >= PerformanceHintMode::kAffinity) {
base::SetCanRunOnBigCore(base::PlatformThread::CurrentId(),
!prefer_efficiency);
should_send_to_compositor =
kUsePerformanceHelperParam.Get() == PerformanceHintMode::kBoth;
}
#endif
// Emit for tracking
main_thread_only().restrict_cpu_performance = prefer_efficiency;
if (should_send_to_compositor) {
for (const auto& widget_scheduler : main_thread_only().widget_schedulers) {
widget_scheduler->RequestEfficientScheduling(prefer_efficiency);
}
}
}Suggestion for consistency of naming:
```suggestion
void MainThreadSchedulerImpl::ApplyPerformanceState(
const bool prefer_efficient_scheduling) {
DCHECK(base::FeatureList::IsEnabled(kUsePerformanceHelper));
bool should_send_to_compositor = true;
#if BUILDFLAG(IS_ANDROID)
if (kUsePerformanceHelperParam.Get() >= PerformanceHintMode::kAffinity) {
base::SetCanRunOnBigCore(base::PlatformThread::CurrentId(),
!prefer_efficient_scheduling);
should_send_to_compositor =
kUsePerformanceHelperParam.Get() == PerformanceHintMode::kBoth;
}
#endif
// Emit for tracking
main_thread_only().prefer_efficient_scheduling = prefer_efficient_scheduling;
if (should_send_to_compositor) {
for (const auto& widget_scheduler : main_thread_only().widget_schedulers) {
widget_scheduler->RequestEfficientScheduling(prefer_efficient_scheduling);
}
}
}
```
Related details
- Benoit Lize
- Igor Kraskevich
- Richard Townsend
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
Hi Vladmir, could you take a look at this CL from a WidgetScheduler perspective?
Best
Richard
Related details
- Benoit Lize
- Igor Kraskevich
- Vladimir Levin
- Yaron Friedman
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. |
Richard Townsend (Gerrit)
Richard Townsend voted and added 8 comments![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
8 comments
void RequestEfficientScheduling(bool prefer_efficiency) {
pending_commit_state()->prefer_efficient_scheduling = prefer_efficiency;
}Suggestion for consistency of naming:
```suggestion
void RequestEfficientScheduling(bool prefer_efficient_scheduling) {
pending_commit_state()->prefer_efficient_scheduling = prefer_efficient_scheduling;
}
```
Done
void SetPreferEfficientScheduling(bool efficiency_preferred) const;Suggestion for consistency of naming:
```suggestion
void SetPreferEfficientScheduling(bool prefer_efficient_scheduling) const;
```
Done
void FrameSinkManagerImpl::SetPreferEfficientScheduling(
const bool prefer_efficiency) const {
if (hint_session_factory_) {
hint_session_factory_->SetPreferPowerEfficientScheduling(prefer_efficiency);
}
}Suggestion for consistency of naming:
```suggestion
void FrameSinkManagerImpl::SetPreferEfficientScheduling(
const bool prefer_efficient_scheduling) const {
if (hint_session_factory_) {
hint_session_factory_->SetPreferEfficientScheduling(
prefer_efficient_scheduling);
}
}
```
Done
virtual void SetPreferPowerEfficientScheduling(bool prefer_efficiency) = 0;Suggestion for consistency of naming:
```suggestion
virtual void SetPreferEfficientScheduling(bool prefer_efficient_scheduling) = 0;
```
Done
void AdpfHintSession::SetPreferPowerEfficientScheduling(
bool prefer_efficiency) {
DCHECK_CALLED_ON_VALID_THREAD(factory_->thread_checker_);
will_prefer_efficiency_ = prefer_efficiency;
}Suggestion for consistency of naming:
```suggestion
void AdpfHintSession::SetPreferEfficientScheduling(
bool prefer_efficient_scheduling) {
DCHECK_CALLED_ON_VALID_THREAD(factory_->thread_checker_);
will_prefer_efficiency_ = prefer_efficient_scheduling;
}
```
Done
void HintSessionFactoryImpl::SetPreferPowerEfficientScheduling(
bool prefer_efficiency) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
for (auto& session : hint_sessions_) {
session->SetPreferPowerEfficientScheduling(prefer_efficiency);
}
}Suggestion for consistency of naming:
```suggestion
void HintSessionFactoryImpl::SetPreferEfficientScheduling(
bool prefer_efficient_scheduling) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
for (auto& session : hint_sessions_) {
session->SetPreferEfficientScheduling(prefer_efficient_scheduling);
}
}
```
Done
performance_helper_.Check();Richard TownsendPass `time_now`.
Done
void MainThreadSchedulerImpl::ApplyPerformanceState(
const bool prefer_efficiency) {
DCHECK(base::FeatureList::IsEnabled(kUsePerformanceHelper));
bool should_send_to_compositor = true;
#if BUILDFLAG(IS_ANDROID)
if (kUsePerformanceHelperParam.Get() >= PerformanceHintMode::kAffinity) {
base::SetCanRunOnBigCore(base::PlatformThread::CurrentId(),
!prefer_efficiency);
should_send_to_compositor =
kUsePerformanceHelperParam.Get() == PerformanceHintMode::kBoth;
}
#endif
// Emit for tracking
main_thread_only().restrict_cpu_performance = prefer_efficiency;
if (should_send_to_compositor) {
for (const auto& widget_scheduler : main_thread_only().widget_schedulers) {
widget_scheduler->RequestEfficientScheduling(prefer_efficiency);
}
}
}Suggestion for consistency of naming:
```suggestion
void MainThreadSchedulerImpl::ApplyPerformanceState(
const bool prefer_efficient_scheduling) {
DCHECK(base::FeatureList::IsEnabled(kUsePerformanceHelper));
bool should_send_to_compositor = true;
#if BUILDFLAG(IS_ANDROID)
if (kUsePerformanceHelperParam.Get() >= PerformanceHintMode::kAffinity) {
base::SetCanRunOnBigCore(base::PlatformThread::CurrentId(),
!prefer_efficient_scheduling);
should_send_to_compositor =
kUsePerformanceHelperParam.Get() == PerformanceHintMode::kBoth;
}
#endif
// Emit for tracking
main_thread_only().prefer_efficient_scheduling = prefer_efficient_scheduling;
if (should_send_to_compositor) {
for (const auto& widget_scheduler : main_thread_only().widget_schedulers) {
widget_scheduler->RequestEfficientScheduling(prefer_efficient_scheduling);
}
}
}
```
Done
Related details
- Benoit Lize
- Igor Kraskevich
- Vladimir Levin
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yaron Friedman (Gerrit)
Yaron Friedman voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yaron Friedman
| Code-Review | +1 |
2 comments
lgtm, just the one question but I believe is fine
performance_helper_.Check();is this sufficient for when app goes to background or tab switched? I guess it looks to be on _any_ task run here?
Historically, we've had bugs with things that rely on "the next frame" to get set correctly because in background there is no next frame...
So in current soln, if I start a load and am pumping frames, and then switch tabs - togglign visibility but no frame comes in, presumably unregistering for beginmainframeas and we don't have a racey frame come in, will we be left in whatever the last state was or whenever the main thread runs anything this is checked?
Related details
- Benoit Lize
- Igor Kraskevich
- Richard Townsend
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
performance_helper_.Check();is this sufficient for when app goes to background or tab switched? I guess it looks to be on _any_ task run here?
Historically, we've had bugs with things that rely on "the next frame" to get set correctly because in background there is no next frame...
So in current soln, if I start a load and am pumping frames, and then switch tabs - togglign visibility but no frame comes in, presumably unregistering for beginmainframeas and we don't have a racey frame come in, will we be left in whatever the last state was or whenever the main thread runs anything this is checked?
It's a good question and this might be a situation where affinity + timeout could be better. Right now, we either:
1) A global ADPF session including the compositor and all renderers
2) One ADPF session for compositors and viz, one ADPF session for all renderers.
Basically, the last frame submitted wins and controls the global state.
So what might happen in this case:
1. Start the load. BOOST
2. Open another tab. BOOST
3. Other tab completes the load.
4. Wait a few hundred ms. EFFICIENCY
This may cause some... interesting behaviour in multi-window scenarios (and is one of the reasons I wanted to do the state tracking purely on the compositor side), so some additional de-bouncing may have to be done if this is perceptible, but eventually the everything will trend to efficiency.
Related details
- Benoit Lize
- Igor Kraskevich
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Vladimir Levin (Gerrit)
Vladimir Levin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vladimir Levin
| Code-Review | +1 |
1 comment
[[likely]] LayerTreeHost()->RequestEfficientScheduling(style nit: I think we typically put the likely/unlikely attributes after the if conditional before the brace
```
if (LayerTreeHost()) [[likely]] {
...
}
```
Related details
- Benoit Lize
- Igor Kraskevich
- Richard Townsend
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 4 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
4 comments
A few drive-by comments, but LGTM.
case UseCase::kDiscreteInputResponse:Note: this is used to detect that we're waiting for the paint/frame after input, and
- doesn't cover the full input event (only after the input task finishes), which means JS event handlers aren't covered. kCustomInputHandling might handle more of the discrete input timespan, but I don't fully remember how those overlap.
- doesn't cover all discrete input events: input can also run during BeginMainFrame out of the CC task queue, although typically we'd have run another input task beforehand bc of task priorities, in which case the use case should be set.
- covers some time when task queues (most, not all) are paused while we're waiting for the paint.
So this is fine, but FYI there might room for improvement.
#ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_SCHEDULER_MAIN_THREAD_SCOPED_PERFORMANCE_HELPER_H_Old remnant, i.e. change to match the current file name?
Related details
- Benoit Lize
- Igor Kraskevich
- Richard Townsend
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. |
Richard Townsend (Gerrit)
Richard Townsend voted and added 5 comments![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
5 comments
PS28--PS29:
- Experiment configuration added to fieldtrial_testing_config.json. Since there doesn't seem to be an obvious way of scoping "params" within this file, the UsePerformanceHelper's "mode" param has been renamed to "helper_param".
- Addressing Scott and @vmp...@chromium.org's comments
@yfri...@chromium.org: I think I might need a rubber-stamp for the fieldtrial config changes.
// PerformanceRichard TownsendIs this incomplete?
Removed in PS29
Note: this is used to detect that we're waiting for the paint/frame after input, and
- doesn't cover the full input event (only after the input task finishes), which means JS event handlers aren't covered. kCustomInputHandling might handle more of the discrete input timespan, but I don't fully remember how those overlap.
- doesn't cover all discrete input events: input can also run during BeginMainFrame out of the CC task queue, although typically we'd have run another input task beforehand bc of task priorities, in which case the use case should be set.
- covers some time when task queues (most, not all) are paused while we're waiting for the paint.
So this is fine, but FYI there might room for improvement.
Ack: I'm interested primarily into whether this intervention really saves power before fine-tuning the heuristics, but doing more on this seems important.
#ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_SCHEDULER_MAIN_THREAD_SCOPED_PERFORMANCE_HELPER_H_Old remnant, i.e. change to match the current file name?
Actually, unsure why the presubmit doesn't catch that. Addressed in PS29
[[likely]] LayerTreeHost()->RequestEfficientScheduling(style nit: I think we typically put the likely/unlikely attributes after the if conditional before the brace
```
if (LayerTreeHost()) [[likely]] {
...
}
```
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Jonathan Ross
- Ken Buchanan
- Scott Haseley
- Vladimir Levin
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yaron Friedman (Gerrit)
Yaron Friedman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yaron Friedman
| Code-Review | +1 |
1 comment
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Jonathan Ross
- Ken Buchanan
- Richard Townsend
- Scott Haseley
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Richard Townsend (Gerrit)
Richard Townsend voted and added 1 comment![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +1 |
1 comment
PS30: implements additional hysterisis on the Viz side, the minimum boost is now 4 frames. This should better handle mixing efficiency vs performance hints from different MainThreadSchedulerImpl's: the behaviour is that if any of them requests a boost, we now do it for a minimum of 4 frames. During this time, the MainThreadSchedulerImpl should issue another frame, implementing the de-bounce.
Verified - seems to be working as intended: https://ui.perfetto.dev/#!/?s=950a7cb802f366eb2482157b6ab6efeee750de38
Related details
- Benoit Lize
- Francois Pierre Doray
- Igor Kraskevich
- Jonathan Ross
- Ken Buchanan
- Scott Haseley
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Kraskevich (Gerrit)
Igor Kraskevich voted Code-Review+1![]( "Open in Gerrit")
- Benoit Lize
- Francois Pierre Doray
- Jonathan Ross
- Ken Buchanan
- Richard Townsend
- Scott Haseley
- Vladimir Levin
Richard Townsend (Gerrit)
Richard Townsend voted and added 1 comment![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +2 |
1 comment
Thanks for the reviews all! Sending to CQ.
Best
Richard
Related details
- Benoit Lize
- Francois Pierre Doray
- Jonathan Ross
- Ken Buchanan
- Scott Haseley
- Vladimir Levin
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
ADPF adaptive mode
* Boosts via MainThreadScheduler's WillBeginFrame regular tick
* PerformanceHelper can set affinity or drive ADPF efficiency mode (or
both!)
Example command lines:
--enable-features=EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:helper_mode/compositor
--enable-features=EnableAdpfEfficiencyMode:mode/adaptive,UsePerformanceHelper:helper_mode/both
--enable-features=UsePerformanceHelper:mode/affinity
For testing/perf notes, see the CL.
- M cc/trees/commit_state.cc
- M cc/trees/commit_state.h
- M cc/trees/layer_tree_host.h
- M cc/trees/layer_tree_host_impl.cc
- M cc/trees/layer_tree_host_impl.h
- M components/viz/common/quads/compositor_frame_metadata.h
- M components/viz/service/frame_sinks/compositor_frame_sink_support.cc
- M components/viz/service/frame_sinks/frame_sink_manager_impl.cc
- M components/viz/service/frame_sinks/frame_sink_manager_impl.h
- M components/viz/service/performance_hint/hint_session.cc
- M components/viz/service/performance_hint/hint_session.h
- M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.cc
- M services/viz/public/cpp/compositing/compositor_frame_metadata_mojom_traits.h
- M services/viz/public/mojom/compositing/compositor_frame_metadata.mojom
- M testing/variations/fieldtrial_testing_config.json
- M third_party/blink/renderer/platform/scheduler/BUILD.gn
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl_unittest.cc
- A third_party/blink/renderer/platform/scheduler/main_thread/performance_helper.cc
- A third_party/blink/renderer/platform/scheduler/main_thread/performance_helper.h
- A third_party/blink/renderer/platform/scheduler/main_thread/performance_helper_unittest.cc
- M third_party/blink/renderer/platform/scheduler/main_thread/widget_scheduler_impl.h
- M third_party/blink/renderer/platform/scheduler/public/widget_scheduler.h
- M third_party/blink/renderer/platform/widget/widget_base.cc
- M third_party/blink/renderer/platform/widget/widget_base.h
Code-Review: +1 by Yaron Friedman, +1 by Igor Kraskevich
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
TRACE_EVENT_ASYNC_BEGIN1("android.adpf", "AdpfHintSession::Boost", this,
"session", (uintptr_t)this);Drive-by: TRACE_EVENT_ASYNC_* is banned by PRESUBMIT.py [1], does the presubmit not work?
https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;l=1774?q=TRACE_EVENT_ASYNC&ss=chromium
Related details
- Richard Townsend
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard Townsend (Gerrit)
Richard Townsend added 1 comment![]( "Open in Gerrit")
TRACE_EVENT_ASYNC_BEGIN1("android.adpf", "AdpfHintSession::Boost", this,
"session", (uintptr_t)this);Drive-by: TRACE_EVENT_ASYNC_* is banned by PRESUBMIT.py [1], does the presubmit not work?
https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;l=1774?q=TRACE_EVENT_ASYNC&ss=chromium
It looks like... yes the presubmit worked, but because it's a warning and not an error, the bot went green and I didn't think to check the log:
I will do a follow-up CL to bring the code into compliance.
Related details
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
TRACE_EVENT_ASYNC_BEGIN1("android.adpf", "AdpfHintSession::Boost", this,
"session", (uintptr_t)this);Richard TownsendDrive-by: TRACE_EVENT_ASYNC_* is banned by PRESUBMIT.py [1], does the presubmit not work?
https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;l=1774?q=TRACE_EVENT_ASYNC&ss=chromium
It looks like... yes the presubmit worked, but because it's a warning and not an error, the bot went green and I didn't think to check the log:
I will do a follow-up CL to bring the code into compliance.
Ah ok thanks!
I'll see if this can show red on bots.