Skip to content

GitLab

T
tangled
Commit 126d77b6 authored by rdevlin.cronin's avatar rdevlin.cronin Committed by Commit bot
Browse files
Options

Revert of Handle RenderProcessHostImpl::SetBackgrounded on Windows (patchset... 

Revert of Handle RenderProcessHostImpl::SetBackgrounded on Windows (patchset #4 id:60001 of https://codereview.chromium.org/926663002/)

Reason for revert:
This patch set re-enabled ChromeRenderProcessHostTest.Backgrounding, which is now failing on XP bots.

Example logs:
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds/35740/steps/browser_tests%20on%20Windows-XP-SP3/logs/ChromeRenderProcessHostTest.Backgrounding
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds/35740/steps/browser_tests%20on%20Windows-XP-SP3/logs/ChromeRenderProcessHostTest.Backgrounding

Original issue's description:
> 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
>
> Committed: https://crrev.com/aaf738a2cce69ce952386b48e3bc252bd307ef43
> Cr-Commit-Position: refs/heads/master@{#317042}

TBR=wfh@chromium.org,jam@chromium.org,gab@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=394368, 458594

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

Cr-Commit-Position: refs/heads/master@{#317070}
parent 1860e5f6
Show whitespace changes
Inline Side-by-side
Showing with 17 additions and 54 deletions
base/process/process_win.cc
View file @ 126d77b6
......@@ -6,7 +6,6 @@
#include "base/logging.h"
#include "base/memory/scoped_ptr.h"
#include "base/metrics/field_trial.h"
#include "base/process/kill.h"
#include "base/win/windows_version.h"
......@@ -166,21 +165,7 @@ bool Process::SetProcessBackgrounded(bool value) {
priority = value ? PROCESS_MODE_BACKGROUND_BEGIN :
PROCESS_MODE_BACKGROUND_END;
} else {
// 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;
priority = value ? BELOW_NORMAL_PRIORITY_CLASS : NORMAL_PRIORITY_CLASS;
}
return (::SetPriorityClass(Handle(), priority) != 0);
......
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
View file @ 126d77b6
......@@ -291,7 +291,13 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, MAYBE_ProcessPerTab) {
// 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.
#if defined(OS_WIN) || defined(OS_LINUX)
IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, Backgrounding) {
#if defined(OS_WIN)
// 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()) {
LOG(ERROR) << "Can't background processes";
return;
......
content/browser/renderer_host/render_process_host_impl.cc
View file @ 126d77b6
......@@ -2207,27 +2207,17 @@ void RenderProcessHostImpl::SetBackgrounded(bool backgrounded) {
return;
#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.
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() {
......
content/child/child_thread_impl.cc
View file @ 126d77b6
......@@ -17,7 +17,6 @@
#include "base/logging.h"
#include "base/message_loop/message_loop.h"
#include "base/message_loop/timer_slack.h"
#include "base/metrics/field_trial.h"
#include "base/process/kill.h"
#include "base/process/process_handle.h"
#include "base/strings/string_number_conversions.h"
......@@ -650,23 +649,6 @@ void ChildThreadImpl::OnProcessBackgrounded(bool background) {
if (background)
timer_slack = base::TIMER_SLACK_MAXIMUM;
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
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