Commit aaf738a2 authored by gab's avatar gab Committed by Commit bot

Handle RenderProcessHostImpl::SetBackgrounded on Windows

This was disabled on Windows because:

 - http://crrev.com/274071 made it such that it was asynchronous via the renderer (as only
the process itself can set itself in background mode on Windows).

 - And then this was removed for Windows as part of issue 381820 as true background mode on
Windows was achieved by having the renderer itself change its process priority (which
doesn't work as the unbackgrounding request can be stuck behind other priority tasks
running in background mode).

Also, the test for this was disabled in between (see issue 394368) -- the asynchronicity
introduced in r274071 probably causing the flakes.

Overall there is no need to have the renderer enter PROCESS_MODE_BACKGROUND_BEGIN itself,
IDLE_PRIORITY_CLASS should be sufficient as its the same as far as CPU priority is
concerned and IO is irrelevant in the renderers.

Thus we can get background mode for hidden renderers without running into issue 381820.

Experiment with IDLE_PRIORITY_CLASS/BELOW_NORMAL_PRIORITY_CLASS vs the status quo.

Watching the relevant perf bots (tab_switching.top_10 -> MPArch.RWH_TabSwitchPaintDuration)
and UMA metrics MPArch.RWH_TabSwitchPaintDuration which shouldn't regress.
And others (e.g., SessionRestore related) which will hopefully improve.

Enable the experiment by default in the absence of Finch to get perf waterfall coverage.

The experiment will be called "BackgroundRendererProcesses" and have 4 groups:
 ["Disallow", "AllowBelowNormalFromBrowser",
  "AllowIdleFromBrowser", "AllowBackgroundModeFromRenderer"]

It will be a per-session (rather than per-profile) experiment, randomly assigning a user
to a group every new Chrome session.

BUG=394368, 458594

Review URL: https://codereview.chromium.org/926663002

Cr-Commit-Position: refs/heads/master@{#317042}
parent 9a5cfc93
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
#include "base/logging.h" #include "base/logging.h"
#include "base/memory/scoped_ptr.h" #include "base/memory/scoped_ptr.h"
#include "base/metrics/field_trial.h"
#include "base/process/kill.h" #include "base/process/kill.h"
#include "base/win/windows_version.h" #include "base/win/windows_version.h"
...@@ -165,7 +166,21 @@ bool Process::SetProcessBackgrounded(bool value) { ...@@ -165,7 +166,21 @@ bool Process::SetProcessBackgrounded(bool value) {
priority = value ? PROCESS_MODE_BACKGROUND_BEGIN : priority = value ? PROCESS_MODE_BACKGROUND_BEGIN :
PROCESS_MODE_BACKGROUND_END; PROCESS_MODE_BACKGROUND_END;
} else { } else {
priority = value ? BELOW_NORMAL_PRIORITY_CLASS : NORMAL_PRIORITY_CLASS; // Experiment (http://crbug.com/458594) with using IDLE_PRIORITY_CLASS as a
// background priority for background renderers (this code path is
// technically for more than just the renderers but they're the only use
// case in practice and experimenting here direclty is thus easier -- plus
// it doesn't really hurt as above we already state our intent of using
// PROCESS_MODE_BACKGROUND_BEGIN if available which is essentially
// IDLE_PRIORITY_CLASS plus lowered IO priority). Enabled by default in the
// asbence of field trials to get coverage on the perf waterfall.
DWORD background_priority = IDLE_PRIORITY_CLASS;
base::FieldTrial* trial =
base::FieldTrialList::Find("BackgroundRendererProcesses");
if (trial && trial->group_name() == "AllowBelowNormalFromBrowser")
background_priority = BELOW_NORMAL_PRIORITY_CLASS;
priority = value ? background_priority : NORMAL_PRIORITY_CLASS;
} }
return (::SetPriorityClass(Handle(), priority) != 0); return (::SetPriorityClass(Handle(), priority) != 0);
......
...@@ -291,13 +291,7 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, MAYBE_ProcessPerTab) { ...@@ -291,13 +291,7 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, MAYBE_ProcessPerTab) {
// We don't change process priorities on Mac or Posix because the user lacks the // We don't change process priorities on Mac or Posix because the user lacks the
// permission to raise a process' priority even after lowering it. // permission to raise a process' priority even after lowering it.
#if defined(OS_WIN) || defined(OS_LINUX) #if defined(OS_WIN) || defined(OS_LINUX)
#if defined(OS_WIN) IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, Backgrounding) {
// Flaky test: crbug.com/394368
#define MAYBE_Backgrounding DISABLED_Backgrounding
#else
#define MAYBE_Backgrounding Backgrounding
#endif
IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, MAYBE_Backgrounding) {
if (!base::Process::CanBackgroundProcesses()) { if (!base::Process::CanBackgroundProcesses()) {
LOG(ERROR) << "Can't background processes"; LOG(ERROR) << "Can't background processes";
return; return;
......
...@@ -2207,17 +2207,27 @@ void RenderProcessHostImpl::SetBackgrounded(bool backgrounded) { ...@@ -2207,17 +2207,27 @@ void RenderProcessHostImpl::SetBackgrounded(bool backgrounded) {
return; return;
#endif // OS_WIN #endif // OS_WIN
#if defined(OS_WIN)
// Same as below, but bound to an experiment (http://crbug.com/458594)
// initially on Windows. Enabled by default in the asbence of field trials to
// get coverage on the perf waterfall.
base::FieldTrial* trial =
base::FieldTrialList::Find("BackgroundRendererProcesses");
if (!trial || (trial->group_name() != "Disallow" &&
trial->group_name() != "AllowBackgroundModeFromRenderer")) {
child_process_launcher_->SetProcessBackgrounded(backgrounded);
}
#else
// Control the background state from the browser process, otherwise the task
// telling the renderer to "unbackground" itself may be preempted by other
// tasks executing at lowered priority ahead of it or simply by not being
// swiftly scheduled by the OS per the low process priority
// (http://crbug.com/398103).
child_process_launcher_->SetProcessBackgrounded(backgrounded);
#endif // OS_WIN
// Notify the child process of background state. // Notify the child process of background state.
Send(new ChildProcessMsg_SetProcessBackgrounded(backgrounded)); Send(new ChildProcessMsg_SetProcessBackgrounded(backgrounded));
#if !defined(OS_WIN)
// Backgrounding may require elevated privileges not available to renderer
// processes, so control backgrounding from the process host.
// Windows Vista+ has a fancy process backgrounding mode that can only be set
// from within the process.
child_process_launcher_->SetProcessBackgrounded(backgrounded);
#endif // !OS_WIN
} }
void RenderProcessHostImpl::OnProcessLaunched() { void RenderProcessHostImpl::OnProcessLaunched() {
......
...@@ -17,6 +17,7 @@ ...@@ -17,6 +17,7 @@
#include "base/logging.h" #include "base/logging.h"
#include "base/message_loop/message_loop.h" #include "base/message_loop/message_loop.h"
#include "base/message_loop/timer_slack.h" #include "base/message_loop/timer_slack.h"
#include "base/metrics/field_trial.h"
#include "base/process/kill.h" #include "base/process/kill.h"
#include "base/process/process_handle.h" #include "base/process/process_handle.h"
#include "base/strings/string_number_conversions.h" #include "base/strings/string_number_conversions.h"
...@@ -649,6 +650,23 @@ void ChildThreadImpl::OnProcessBackgrounded(bool background) { ...@@ -649,6 +650,23 @@ void ChildThreadImpl::OnProcessBackgrounded(bool background) {
if (background) if (background)
timer_slack = base::TIMER_SLACK_MAXIMUM; timer_slack = base::TIMER_SLACK_MAXIMUM;
base::MessageLoop::current()->SetTimerSlack(timer_slack); base::MessageLoop::current()->SetTimerSlack(timer_slack);
#ifdef OS_WIN
// Windows Vista+ has a fancy process backgrounding mode that can only be set
// from within the process. This used to be how chrome set its renderers into
// background mode on Windows but was removed due to http://crbug.com/398103.
// As we experiment with bringing back some other form of background mode for
// hidden renderers, add a bucket to allow us to trigger this undesired method
// of setting background state in order to confirm that the metrics which were
// added to prevent regressions on the aforementioned issue indeed catch such
// regressions and are thus a reliable way to confirm that our latest proposal
// doesn't cause such issues. TODO(gab): Remove this once the experiment is
// over (http://crbug.com/458594).
base::FieldTrial* trial =
base::FieldTrialList::Find("BackgroundRendererProcesses");
if (trial && trial->group_name() == "AllowBackgroundModeFromRenderer")
base::Process::Current().SetProcessBackgrounded(background);
#endif // OS_WIN
} }
} // namespace content } // namespace content
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment