[battery] Enable heavy AlignWakeUps under battery saver mode [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 3 comments![]( "Open in Gerrit")
void SetBatterySaverMode(bool align_wake_ups) override {Francois Pierre DoraySo this is on a spearate mojo channel than the Renderer mojo interface so it could end up being in the wrong state. (see other comment).
If you feel it is necessary then I'd like the RenderThreadImpl to be able to call into this implemenetation (it is a subclass ChildThreadImpl so we should be able to get into a common annyomous function implemented in this cc file).
Dave Tapuska"Align wake ups" and "Blink Battery Saver Modes" are meant to save power, but they're completely independent so it's fine if they're not in sync.
This CL calls `ChildThreadImpl::IOThreadState::SetBatterySaverMode` (this method) for non-renderer child processes and `RenderThreadImpl::SetBatterySaverMode` for renderers, so there is no inconsistency. Would it be better if `ChildThreadImpl::IOThreadState::SetBatterySaverMode` was invoked for all child processes (renderers and non-renderers)? The implementation could set the "Align wake ups" states (applies to all process types) and delegate to a virtual method on ChildThreadImpl to apply policies specific to a process type? (note: RenderThreadImpl is a ChildThreadImpl).
Etienne Pierre-DorayCan we possibly just have the ChildThreadImpl::IOThreadState::SetBatterySaveMode and have it take a bool enabled param (it contains the state of battery saver mode, not the feature settings) and remove the Renderer mojom version.
Then the FeatureParams can be tested in the renderer (we might need to update content renderer DEPS to allow components/performance_manager/public/) we don't need to pass that via IPC.
This allows us to reduce to a single IPC to the IO thread so it is consistent, and then the IOThreadState can post task to call a virtual on ChildThreadImpl that RenderThreadImpl overrides to change the state on the blink isolates.
Done
SetBatterySaverMode(bool render_saver_mode, bool align_wake_ups);Etienne Pierre-DorayI wonder if we should just make this SetRenderSaverMode that adjusts only the saver mode and not the align_wake_ups.
This is because if we use two different mojo channels to adjust the setting it could end up being the wrong state in the renderer even though they were loggically set in the right order from the browser. Not passing align_wake_ups here fixes that (at the cost of sending both IPCs, but I think you already doing that).
Dave TapuskaIIUC BrowserChildProcessHostIterator (used to iterate child processes) excludes renderers, thus mutually exclusive with RenderProcessHost::iterator; I'm not sure we can send IPC to renderers through ChildProcessHost?
Etienne Pierre-DorayYes I missed that sorry for my understanding. fdoray@ set me correct. Yes you can send stuff to a renderer via ChildProcessHost. (see https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_process_host_impl.h;l=1445;drc=fa1e0cf5238e2600039f60af6909aefbd4cf003c;bpv=1;bpt=1)
Acknowledged
Related details
- Anthony Vallée-Dubois
- Dave Tapuska
- Francois Pierre Doray
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Anthony Vallée-Dubois
- Etienne Pierre-Doray
- Francois Pierre Doray
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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: dan...@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): dan...@chromium.org
Reviewer source(s):
dan...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Anthony Vallée-Dubois
- Dave Tapuska
- Francois Pierre Doray
- danakj
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dave Tapuska
| Code-Review | +1 |
1 comment
Related details
- Anthony Vallée-Dubois
- Etienne Pierre-Doray
- Francois Pierre Doray
- danakj
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray voted and added 3 comments![]( "Open in Gerrit")
Votes added by Francois Pierre Doray
| Code-Review | +1 |
3 comments
LGTM with comments.
if (!data.GetProcess().IsValid()) {
return;
}The process is guaranteed to be valid (see [comment](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/browser_child_process_observer.h;l=22-23;drc=4cc063ac39c4a0d1f6011421b259a9715bb16de1), my bad for renaming handle -> process without updating the comment [cl](https://chromium-review.googlesource.com/c/chromium/src/+/1298345), fixed [here](https://crrev.com/c/5717291)), so this should be a CHECK.
void BindHostReceiver(mojo::GenericPendingReceiver receiver) override;This overrides a method from content::ChildProcessHost (defined here: https://chromium-review.googlesource.com/c/chromium/src/+/5347739/16/content/public/browser/child_process_host.h#199), not a method from mojom::ChildProcessHost. Move it to the correct section (line 67-81).
Related details
- Anthony Vallée-Dubois
- Etienne Pierre-Doray
- danakj
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
if (!data.GetProcess().IsValid()) {
return;
}The process is guaranteed to be valid (see [comment](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/browser_child_process_observer.h;l=22-23;drc=4cc063ac39c4a0d1f6011421b259a9715bb16de1), my bad for renaming handle -> process without updating the comment [cl](https://chromium-review.googlesource.com/c/chromium/src/+/1298345), fixed [here](https://crrev.com/c/5717291)), so this should be a CHECK.
That being said, I wouldn't want your CL to be reverted if the guarantee turns out to be wrong... so it's fine to keep as-is with a TODO to convert to a CHECK in the future.
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 2 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +1 |
2 comments
if (!data.GetProcess().IsValid()) {
return;
}Francois Pierre DorayThe process is guaranteed to be valid (see [comment](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/browser_child_process_observer.h;l=22-23;drc=4cc063ac39c4a0d1f6011421b259a9715bb16de1), my bad for renaming handle -> process without updating the comment [cl](https://chromium-review.googlesource.com/c/chromium/src/+/1298345), fixed [here](https://crrev.com/c/5717291)), so this should be a CHECK.
That being said, I wouldn't want your CL to be reverted if the guarantee turns out to be wrong... so it's fine to keep as-is with a TODO to convert to a CHECK in the future.
Done
void BindHostReceiver(mojo::GenericPendingReceiver receiver) override;This overrides a method from content::ChildProcessHost (defined here: https://chromium-review.googlesource.com/c/chromium/src/+/5347739/16/content/public/browser/child_process_host.h#199), not a method from mojom::ChildProcessHost. Move it to the correct section (line 67-81).
- Anthony Vallée-Dubois
- danakj
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthony Vallée-Dubois (Gerrit)
Anthony Vallée-Dubois voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anthony Vallée-Dubois
| Code-Review | +1 |
1 comment
Related details
- Etienne Pierre-Doray
- danakj
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
danakj (Gerrit)
danakj voted and added 1 comment![]( "Open in Gerrit")
Votes added by danakj
| Code-Review | +1 |
1 comment
LGTM for ipc security
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
base::MessagePump::OverrideAlignWakeUpsState(true, base::Milliseconds(32));Francois Pierre Doraynit: maybe add a comment explaining why this is 32ms?
Etienne Pierre-DoraySuggestion: In a follow-up, change to ActivateHeavyWakeUpAlignment() and hide the constant in the implementation. Document the constant choice in the implementation.
Reason: Avoid duplicating the constant here and in content/renderer/render_thread_impl.cc
Acknowledged
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[battery] Enable heavy AlignWakeUps under battery saver mode
- Add a feature kBatterySaverModeAlignWakeUps
- Add a signal to all child processes
- Call OverrideAlignWakeUpsState in browser,
renderers and child processes
- M chrome/browser/metrics/power/battery_discharge_reporter_unittest.cc
- M chrome/browser/performance_manager/public/user_tuning/battery_saver_mode_manager.h
- A chrome/browser/performance_manager/test_support/fake_child_process_tuning_delegate.cc
- A chrome/browser/performance_manager/test_support/fake_child_process_tuning_delegate.h
- D chrome/browser/performance_manager/test_support/fake_render_tuning_delegate.cc
- D chrome/browser/performance_manager/test_support/fake_render_tuning_delegate.h
- M chrome/browser/performance_manager/test_support/test_user_performance_tuning_manager_environment.cc
- M chrome/browser/performance_manager/test_support/test_user_performance_tuning_manager_environment.h
- M chrome/browser/performance_manager/user_tuning/battery_saver_mode_manager.cc
- M chrome/browser/performance_manager/user_tuning/battery_saver_mode_manager_unittest.cc
- M chrome/test/BUILD.gn
- M components/performance_manager/features.cc
- M components/performance_manager/public/features.h
- M content/browser/child_process_host_impl.cc
- M content/browser/child_process_host_impl.h
- M content/browser/child_process_task_port_provider_mac_unittest.cc
- M content/browser/renderer_host/render_process_host_impl.cc
- M content/child/child_thread_impl.cc
- M content/child/child_thread_impl.h
- M content/common/child_process.mojom
- M content/common/renderer.mojom
- M content/public/browser/child_process_host.h
- M content/public/common/content_features.cc
- M content/public/common/content_features.h
- M content/renderer/render_thread_impl.cc
- M content/renderer/render_thread_impl.h
Code-Review: +1 by danakj, +1 by Dave Tapuska, +1 by Anthony Vallée-Dubois, +1 by Francois Pierre Doray
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |