Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629873002:
Do not create base::HighResolutionTimerManager in service utility process (Closed)
|
Created:
3 years, 11 months ago by leonhsl(Using Gerrit) Modified:
3 years, 11 months ago Reviewers:
Ken Rockot(use gerrit already), bajones, danakj, kinuko, Vitaly Buka (NO REVIEWS), blundell CC:
chromium-reviews, darin-cc_chromium.org, jam Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not create base::HighResolutionTimerManager in service utility process
Before mojofication of power monitor IPCs, although service utility process
would create base::PowerMonitor, it can't receive any IPC about power
monitor changes because corresponding ServiceUtilityProcessHost does not create
the PowerMonitorMessageBroadcaster instance sending power monitor changes. Then
service utility process enables HiRes-Timer no matter on battery or not, this
is supposed to be incorrect behavior, the expected behavior should be that
base::HighResolutionTimerManager can enable/disable HiRes-Timer correctly.
Currently power monitor has been mojofied and changed to be hosted in Device
Service, because service utility process has no connection to service manager,
it can not connect to Device Service, so base::PowerMonitor can not work, means
base::HighResolutionTimerManager still can not work.
This CL
- Avoids creating base::HighResolutionTimerManager in service utility process,
this will make HiRes-Timer always be disabled by default. As currently
service utiltiy process does not really use HiRes-Timer, this change has no
any bad effect.
- Adds a TODO to enable service utility process to have a service manager
connection, after that we can make base::HighResolutionTimerManager work
correctly for future possible usage of HiRes-Timer in service utility
process.
BUG=680400
Review-Url: https://codereview.chromium.org/2629873002
Cr-Commit-Position: refs/heads/master@{#445646}
Committed: https://chromium.googlesource.com/chromium/src/+/8f88196e6d6f9f81cb3bb05e3baca460a7df91b6
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fix #
Total comments: 4
Patch Set 3 : Address comments from danakj@ #Messages
Total messages: 53 (26 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Disable base::HighResolutionTimerManager for service utility process temporarily. BUG=680400 ========== to ========== Disable base::HighResolutionTimerManager for service utility process temporarily. Currently service utility process has no connection to service manager. However, service manager connection is necessary for instantiation of the process-wide base::PowerMonitor, which is further necessary to base::HighResolutionTimerManager. So this CL disables base::HighResolutionTimerManager in case of service utility process for now, until it is introduced into service manager world in future. BUG=680400 ==========
leon.han@intel.com changed reviewers: + blundell@chromium.org, rockot@chromium.org
Although this CL does not solve the problem completely, at least it can avoid the crash of chrome for now. WDYT? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
blundell@chromium.org changed reviewers: + bajones@chromium.org
Thanks for tracking this down! Hmm, I have no idea if it's OK for the service utility process to not have a HighResolutionTimerManager; if it's not OK, we'll need to revert the PowerMonitor port and block it on having a connection here. Brandon, do you know, or do you know who would know? It looks like vandebo@ used to be the expert (or one of them) here, but they're no longer on Chromium.
On 2017/01/16 13:14:21, blundell wrote: > Thanks for tracking this down! > > Hmm, I have no idea if it's OK for the service utility process to not have a > HighResolutionTimerManager; if it's not OK, we'll need to revert the > PowerMonitor port and block it on having a connection here. Brandon, do you > know, or do you know who would know? It looks like vandebo@ used to be the > expert (or one of them) here, but they're no longer on Chromium. If we need to go back, I'm afraid we must roll back to PowerMonitor legacy IPCs to enable base::PowerMonitor to work well in service utility process, because the interface provider returned by ChildThreadImpl::GetRemoteInterfaces() is also initiated from service manager connection, if no service manager connection, this interface provider will just be empty and can't do anything. That is to say, codes running in service utility process can't connect to any mojo interfaces outside. So maybe it'd better to have some proof indicating the HighResolutionTimerManager is not so important for service utility process :-)
On 2017/01/16 13:14:21, blundell wrote: > Thanks for tracking this down! > > Hmm, I have no idea if it's OK for the service utility process to not have a > HighResolutionTimerManager; if it's not OK, we'll need to revert the > PowerMonitor port and block it on having a connection here. Brandon, do you > know, or do you know who would know? It looks like vandebo@ used to be the > expert (or one of them) here, but they're no longer on Chromium. It's been a while since I touched this portion of the code, and unfortunately I don't really recall how the PowerMonitor related to the HighResolutionTimerManager. I have a fuzzy recollection that there was a battery efficiency fix put in place that would prevent the use of high res timers on windows when on battery power, but I'm failing to find a bug related to it.
leon.han@intel.com changed reviewers: + vitalybuka@chromium.org
+vitalybuka@, OWNER of chrome/service/, would you please help to clarify whether HighResolutionTimerManager is necessary for service utility process? I noticed that HighResolutionTimerManager is being added for all chrome processes to "Ensures that the Windows high resolution timer is only used when not running on battery power.", but does service utility process really use 'Windows high resolution timer' anywhere? Could we just disable it for service utility process for now? Thanks.
I don't work there any more, but I suspect Cloud Print itself should be OK without HiRes timer. https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... File content/utility/utility_main.cc (right): https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... content/utility/utility_main.cc:52: switches::kServiceRequestChannelToken)) Please add {}
On 2017/01/18 19:07:50, Vitaly Buka wrote: > I don't work there any more, but I suspect Cloud Print itself should be OK > without HiRes timer. > > https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... > File content/utility/utility_main.cc (right): > > https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... > content/utility/utility_main.cc:52: switches::kServiceRequestChannelToken)) > Please add {} Thank you for the information sharing! I found that all IPCs handled by ServiceUtilityProcessHost are for Cloud Print, so can we consider that service utility process is only for Cloud Print? If so, and given that Cloud Print should be OK without HiRes Timer, seems we can consider that service utility process should be OK without HiRes timer :)
https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... File content/utility/utility_main.cc (right): https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... content/utility/utility_main.cc:52: switches::kServiceRequestChannelToken)) Probably you can just base::HighResolutionTimerManager
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... File content/utility/utility_main.cc (right): https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... content/utility/utility_main.cc:52: switches::kServiceRequestChannelToken)) On 2017/01/19 04:25:35, Vitaly Buka wrote: > Probably you can just base::HighResolutionTimerManager I suppose the comment above means that we can just 'remove' base::HighResolutionTimerManager here? :) But I'm not sure whether utility process needs HiRes timer or not, seems utility process embeds several features.
I investigated, and here's the situation as I see it: - Before Mojo-ification, BrowserChildProcessHostImpl instantiated a PowerMonitorMessageBroadcaster, but ChildProcessHostImpl did not. So a child process whose host was a ChildProcessHostImpl would not receive IPC about power monitor changes. (cf. the changes in https://codereview.chromium.org/2433203003) - BrowserChildProcessHostImpl sets a kServiceRequestChannelToken, whereas ChildProcessHostImpl does not. The only wrinkle is that BrowserChildProcessHostImpl only sets this token if it was passed a service name in its constructor. AFAICT, this is currently always the case (i.e., we could just remove the codepath that allows for BrowserChildProcessHostImpl being passed an empty service name in its constructor). - PowerMonitorBroadcastSource defaults to saying that the device is *not* on battery power (both in the current and former implementations). - If the device is not on battery power, then HiResTimerManager calls base::EnableHighResolutionTimer(true) in *its* constructor. Thus, the way to strictly preserve the functionality pre-Mojoification would be to do: if (hasServiceToken) // instantiate HiResTimerManager else base::EnableHighResolutionTimer(true); However, it's not clear to me whether it's really intentional that the system behaved this way pre-Mojo-ification; perhaps it's a better idea to conservatively default to not using the high-resolution timer?
On 2017/01/19 07:06:44, leonhsl wrote: > https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... > File content/utility/utility_main.cc (right): > > https://codereview.chromium.org/2629873002/diff/1/content/utility/utility_mai... > content/utility/utility_main.cc:52: switches::kServiceRequestChannelToken)) > On 2017/01/19 04:25:35, Vitaly Buka wrote: > > Probably you can just base::HighResolutionTimerManager > > I suppose the comment above means that we can just 'remove' > base::HighResolutionTimerManager here? :) But I'm not sure whether utility > process needs HiRes timer or not, seems utility process embeds several features. Oh, my mistake, only service utility process does not need this.
Thanks Colin a lot for deep analysis! I suppose the behavior pre-Mojofication is incorrect,, all child processes(no matter it's launched by BrowserChildProcessHostImpl or ChildProcessHostImpl) should disable HiRes Timer on battery power on Windows, because of the comment at https://cs.chromium.org/chromium/src/base/timer/hi_res_timer_manager.h?rcl=14.... +danakj OWNER of base/, Would you please help to confirm this? Thanks. And I agree we could conservatively default to disable HiRes Timer for those child processes not able to get the information whether on battery or not for now.
leon.han@intel.com changed reviewers: + danakj@chromium.org
+danakj@, would you please help to confirm the question above? Thanks.
Could you update the CL description to reflect our current understanding? Thanks!
On 2017/01/20 02:58:31, leonhsl wrote: > Thanks Colin a lot for deep analysis! > I suppose the behavior pre-Mojofication is incorrect,, all child processes(no > matter it's launched by BrowserChildProcessHostImpl or ChildProcessHostImpl) > should disable HiRes Timer on battery power on Windows, because of the comment > at > https://cs.chromium.org/chromium/src/base/timer/hi_res_timer_manager.h?rcl=14.... > +danakj OWNER of base/, Would you please help to confirm this? Thanks. AFAIK you are correct. I don't know why any one process would want to use high-res timer on battery, it's just a power drain. > And I agree we could conservatively default to disable HiRes Timer for those > child processes not able to get the information whether on battery or not for > now. High res timer is important for things like graphics which need to run based on short internals. Do these processes have similar characteristics or can they default to low res?
https://codereview.chromium.org/2629873002/diff/20001/content/utility/utility... File content/utility/utility_main.cc (right): https://codereview.chromium.org/2629873002/diff/20001/content/utility/utility... content/utility/utility_main.cc:51: std::unique_ptr<base::HighResolutionTimerManager> hi_res_timer_manager; you could use Optional if u dont want to add malloc just to make this optionally exist https://codereview.chromium.org/2629873002/diff/20001/content/utility/utility... content/utility/utility_main.cc:52: if (base::CommandLine::ForCurrentProcess()->HasSwitch( It makes sense that if (!base::PowerMonitor::Get() && service_manager_connection_) didn't pass it didn't make a power monitor. But why is this based on a command line flag and not on if base::PowerMonitor::Get() is null or not then? It adds additional dependencies to the logic. It would be nice if this pointed to the ChildThreadImpl code more directly in the comment too, rather than only through the bug.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/20 16:37:27, danakj (slow) wrote: > On 2017/01/20 02:58:31, leonhsl wrote: > > Thanks Colin a lot for deep analysis! > > I suppose the behavior pre-Mojofication is incorrect,, all child processes(no > > matter it's launched by BrowserChildProcessHostImpl or ChildProcessHostImpl) > > should disable HiRes Timer on battery power on Windows, because of the comment > > at > > > https://cs.chromium.org/chromium/src/base/timer/hi_res_timer_manager.h?rcl=14.... > > +danakj OWNER of base/, Would you please help to confirm this? Thanks. > > AFAIK you are correct. I don't know why any one process would want to use > high-res timer on battery, it's just a power drain. > > > And I agree we could conservatively default to disable HiRes Timer for those > > child processes not able to get the information whether on battery or not for > > now. > > High res timer is important for things like graphics which need to run based on > short internals. Do these processes have similar characteristics or can they > default to low res? I checked around codebase and I think service utility process is the only one content child process which is using HiResTimerManager to enable/disable HiRes timer. Further, according from vitalybuka@'s comments, in fact service utility process(cloud print feature) is not really using any HiRes timer, so there would be no bad influence even if this CL now disables HiRes timer by default for it.
Description was changed from
==========
Disable base::HighResolutionTimerManager for service utility process
temporarily.
Currently service utility process has no connection to service manager.
However, service manager connection is necessary for instantiation of the
process-wide base::PowerMonitor, which is further necessary to
base::HighResolutionTimerManager.
So this CL disables base::HighResolutionTimerManager in case of service utility
process for now, until it is introduced into service manager world in future.
BUG=680400
==========
to
==========
Do not create base::HighResolutionTimerManager in service utility process
Before mojofication of power monitor IPCs, although service utility process
would create base::PowerMonitor, it can't receive any IPC about power
monitor changes because corresponding ServiceUtilityProcessHost does not create
the PowerMonitorMessageBroadcaster instance sending power monitor changes. Then
service utility process enables HiRes-Timer no matter on battery or not, this
is supposed to be incorrect behavior, the expected behavior should be that
base::HighResolutionTimerManager can enable/disable HiRes-Timer correctly.
Currently power monitor has been mojofied and changed to be hosted in Device
Service, because service utility process has no connection to service manager,
it can not connect to Device Service, so base::PowerMonitor can not work, means
base::HighResolutionTimerManager still can not work.
This CL
- Avoids creating base::HighResolutionTimerManager in service utility process,
this will make HiRes-Timer always be disabled by default. As currently
service utiltiy process does not really use HiRes-Timer, this change has no
any bad effect.
- Adds a TODO to enable service utility process to have a service manager
connection, after that we can make base::HighResolutionTimerManager work
correctly for future possible usage of HiRes-Timer in service utility
process.
BUG=680400
==========
https://codereview.chromium.org/2629873002/diff/20001/content/utility/utility... File content/utility/utility_main.cc (right): https://codereview.chromium.org/2629873002/diff/20001/content/utility/utility... content/utility/utility_main.cc:51: std::unique_ptr<base::HighResolutionTimerManager> hi_res_timer_manager; On 2017/01/20 16:37:33, danakj (slow) wrote: > you could use Optional if u dont want to add malloc just to make this optionally > exist Done and thanks! https://codereview.chromium.org/2629873002/diff/20001/content/utility/utility... content/utility/utility_main.cc:52: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/01/20 16:37:33, danakj (slow) wrote: > It makes sense that if (!base::PowerMonitor::Get() && > service_manager_connection_) didn't pass it didn't make a power monitor. But why > is this based on a command line flag and not on if base::PowerMonitor::Get() is > null or not then? It adds additional dependencies to the logic. It would be nice > if this pointed to the ChildThreadImpl code more directly in the comment too, > rather than only through the bug. Done.
On 2017/01/20 10:17:56, blundell wrote: > Could you update the CL description to reflect our current understanding? > Thanks! Done.
lgtm It seems like the information we have now is basically all we can gather about this issue. If this change turns out to be a problem, we can revert to the pre-Mojo-ification behavior of defaulting to hi-res timing if there's no info flowing in from the PowerMonitor.
LGTM
lgtm
leon.han@intel.com changed reviewers: + kinuko@chromium.org
+kinuko@, would you PTAL for OWNER review of content/? Thanks.
rs-ish lgtm
Thanks all! Landing..
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485228154745940,
"parent_rev": "91d7a77589d07404a5b46589f2d3df29e3e253b3", "commit_rev":
"8f88196e6d6f9f81cb3bb05e3baca460a7df91b6"}
Message was sent while issue was closed.
Description was changed from
==========
Do not create base::HighResolutionTimerManager in service utility process
Before mojofication of power monitor IPCs, although service utility process
would create base::PowerMonitor, it can't receive any IPC about power
monitor changes because corresponding ServiceUtilityProcessHost does not create
the PowerMonitorMessageBroadcaster instance sending power monitor changes. Then
service utility process enables HiRes-Timer no matter on battery or not, this
is supposed to be incorrect behavior, the expected behavior should be that
base::HighResolutionTimerManager can enable/disable HiRes-Timer correctly.
Currently power monitor has been mojofied and changed to be hosted in Device
Service, because service utility process has no connection to service manager,
it can not connect to Device Service, so base::PowerMonitor can not work, means
base::HighResolutionTimerManager still can not work.
This CL
- Avoids creating base::HighResolutionTimerManager in service utility process,
this will make HiRes-Timer always be disabled by default. As currently
service utiltiy process does not really use HiRes-Timer, this change has no
any bad effect.
- Adds a TODO to enable service utility process to have a service manager
connection, after that we can make base::HighResolutionTimerManager work
correctly for future possible usage of HiRes-Timer in service utility
process.
BUG=680400
==========
to
==========
Do not create base::HighResolutionTimerManager in service utility process
Before mojofication of power monitor IPCs, although service utility process
would create base::PowerMonitor, it can't receive any IPC about power
monitor changes because corresponding ServiceUtilityProcessHost does not create
the PowerMonitorMessageBroadcaster instance sending power monitor changes. Then
service utility process enables HiRes-Timer no matter on battery or not, this
is supposed to be incorrect behavior, the expected behavior should be that
base::HighResolutionTimerManager can enable/disable HiRes-Timer correctly.
Currently power monitor has been mojofied and changed to be hosted in Device
Service, because service utility process has no connection to service manager,
it can not connect to Device Service, so base::PowerMonitor can not work, means
base::HighResolutionTimerManager still can not work.
This CL
- Avoids creating base::HighResolutionTimerManager in service utility process,
this will make HiRes-Timer always be disabled by default. As currently
service utiltiy process does not really use HiRes-Timer, this change has no
any bad effect.
- Adds a TODO to enable service utility process to have a service manager
connection, after that we can make base::HighResolutionTimerManager work
correctly for future possible usage of HiRes-Timer in service utility
process.
BUG=680400
Review-Url: https://codereview.chromium.org/2629873002
Cr-Commit-Position: refs/heads/master@{#445646}
Committed:
https://chromium.googlesource.com/chromium/src/+/8f88196e6d6f9f81cb3bb05e3bac...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8f88196e6d6f9f81cb3bb05e3bac... |
