Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
926663002:
Handle RenderProcessHostImpl::SetBackgrounded on Windows (Closed)
|
Created:
5 years, 10 months ago by gab Modified:
5 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, cpu_(ooo_6.6-7.5), ananta, DaleCurtis Base URL:
https://chromium.googlesource.com/chromium/src.git@908473002_ps2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle 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}
Committed: https://crrev.com/d88d22aba23050a743d2dbbad57527433d504df7
Cr-Commit-Position: refs/heads/master@{#317241}
Patch Set 1 #Patch Set 2 : as an experiment #
Total comments: 2
Patch Set 3 : Add AllowBackgroundModeFromRenderer experiment group for regression testing #Patch Set 4 : merge up to r316866 #Patch Set 5 : Leave ChromeRenderProcessHostTest.Backgrounding disabled on Windows for now :-( #
Messages
Total messages: 55 (23 generated)
gab@chromium.org changed reviewers: + ananta@chromium.org
Ananta, PTAL. Seems pretty straight forward, any reason not to do this?!
On 2015/02/13 18:55:59, gab wrote: > Ananta, PTAL. Seems pretty straight forward, any reason not to do this?! +CC cpu@
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org, wfh@chromium.org
The vista process backgrounding was removed for http://crbug.com/381820 -- +wfh who can confirm if the current state is intended or not.
On 2015/02/13 19:24:06, DaleCurtis wrote: > The vista process backgrounding was removed for http://crbug.com/381820 -- +wfh > who can confirm if the current state is intended or not. FYI, discussion about this continued on other threads (e.g., http://crbug.com/458594). We'll begin with an experiment, I probably won't get to update this CL until next week unfortunately as it's already 18:30 here, so please hold on to the review. Thanks all and have a good week-end, Gab
gab@chromium.org changed reviewers: + jam@chromium.org - ananta@chromium.org, dalecurtis@chromium.org
Will please approve per discussion on http://crbug.com/458594. jam: PTAL as content/ owner. Thanks, Gab
lgtm but please monitor the waterfalls. I'd almost like to see a test with PROCESS_MODE_BACKGROUND_BEGIN to see if the perf bots correctly catch this. I'd really hate to regress this behavior for users. Not sure how to do this without having the ability to finch on the waterfall.
Interesting I could add an experiment bucket for calling SetProcessBackgrounded() from OnSetProcessBackgrounded() in the renderers which we know will regress tab switch load time (http://crbug.com/381820) in order to prove that the measurements we use would catch such a regression if there was one with the other groups. I can do that tomorrow if everybody is otherwise happy with this CL. Cheers, Gab Le mar. 17 févr. 2015 16:40, null <wfh@chromium.org> a écrit : > lgtm but please monitor the waterfalls. > > I'd almost like to see a test with PROCESS_MODE_BACKGROUND_BEGIN to see if > the > perf bots correctly catch this. I'd really hate to regress this behavior > for > users. Not sure how to do this without having the ability to finch on the > waterfall. > > https://codereview.chromium.org/926663002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/926663002/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/926663002/diff/20001/content/browser/renderer... content/browser/renderer_host/render_process_host_impl.cc:2220: // switfly scheduled by the OS per the low process priority nit: swiftly
One final nit/suggestion, could you add BUG=381820 to the CL as well? Then, in the unlikely even that this does regress anything, the "enthusastic" participants in that bug will, I'm sure, be very happy to tell you it's regressed :)
New patchsets have been uploaded after l-g-t-m from wfh@chromium.org,jam@chromium.org
On 2015/02/17 23:34:25, Will Harris wrote: > One final nit/suggestion, could you add BUG=381820 to the CL as well? Then, in > the unlikely even that this does regress anything, the "enthusastic" > participants in that bug will, I'm sure, be very happy to tell you it's > regressed :) As discussed, won't do that right away, will comment on the bug once the experiment is live. @jam/wfh: PTAL, as discussed, added an experiment group to trigger the old/undesired behaviour (i.e. which existed prior to https://codereview.chromium.org/421373002) and confirm that our latest metrics allow us to catch regressions in tab switch latency. Thanks, Gab
Oh and forgot to post it, but nit addressed. @jam: PTAL (and let me know if you prefer that I get a base/ reviewer or if you're okay with me ridding your /OWNERS presence for this CL). Thanks, Gab https://codereview.chromium.org/926663002/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/926663002/diff/20001/content/browser/renderer... content/browser/renderer_host/render_process_host_impl.cc:2220: // switfly scheduled by the OS per the low process priority On 2015/02/17 23:15:25, jam wrote: > nit: swiftly Done.
lgtm no need for other reviewer
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/926663002/40001
The CQ bit was unchecked by commit-bot@chromium.org
CQ experienced an internal error when committing your CL and the maintainers were notified. Sorry for the inconvenience.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/926663002/40001
The CQ bit was unchecked by commit-bot@chromium.org
CQ experienced an internal error when committing your CL and the maintainers were notified. Sorry for the inconvenience.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/926663002/40001
The CQ bit was unchecked by commit-bot@chromium.org
CQ experienced an internal error when committing your CL and the maintainers were notified. Sorry for the inconvenience.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/926663002/40001
The CQ bit was unchecked by commit-bot@chromium.org
CQ experienced an internal error when committing your CL and the maintainers were notified. Sorry for the inconvenience.
The CQ bit was unchecked by commit-bot@chromium.org
New patchsets have been uploaded after l-g-t-m from jam@chromium.org
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/926663002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/926663002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/926663002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aaf738a2cce69ce952386b48e3bc252bd307ef43 Cr-Commit-Position: refs/heads/master@{#317042}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/940993002/ by rdevlin.cronin@chromium.org. The reason for reverting is: 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/build... https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build....
On 2015/02/19 17:38:37, Devlin wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/940993002/ by https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=rdevlin.cronin@chromium.org. > > The reason for reverting is: 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/build... > https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build.... Relanding this CL without re-enabling the Backgrounding test I thought I could re-enable, here's the reasoning: - XP try bots are passing on patch set 3 - XP/Vista failures were during shutdown and although XP is missing symbols [1], I'm pretty sure it's the same shutdown error as Vista which I've analyzed and reported under http://crbug.com/460357 -- looks like an unrelated existing issue which is potentially simply more likely to be triggered by a test spawning multiple renderers (ChromeRenderProcessHostTest.ProcessOverflow which works the same way has seen similar flakes). [1] https://groups.google.com/a/chromium.org/d/topic/chromium-dev/kvjT8uYdBqQ/dis... Overall I'm confident that this CL is not introducing a regression that this test would have caught (the test passes on all try bots, it's just unhappy on the waterfall because of an existing issue).
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/926663002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d88d22aba23050a743d2dbbad57527433d504df7 Cr-Commit-Position: refs/heads/master@{#317241} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
