Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
989703002:
Add support for backgrounding processes on the Mac (Closed)
|
Created:
5 years, 9 months ago by shrike Modified:
5 years, 6 months ago Reviewers:
Ken Russell (switch to Gerrit), Avi (use Gerrit), gab, Nico, Alexei Svitkine (slow), Robert Sesek CC:
chromium-reviews, darin-cc_chromium.org, erikwright+watch_chromium.org, jam, pink (ping after 24hrs), Zhenyao Mo, Jamie Madill Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for backgrounding processes on the Mac
Added process_mac.cc with implementations of IsProcessBackgrounded() and
SetProcessBackgrounded().
BUG=460102
Originally Committed: https://crrev.com/e3bb10f7860a1d553c85293bd7d7615c0e7f0fd9
Reverted: https://crrev.com/ce6226a7ffe2c1cb7ac5f6cf34b56b8d217686b9
Second commit: https://crrev.com/0160d130f8a4462fa7bfb8a9924e476d31ba9a48
Second revert: https://crrev.com/93ef7cd278d450b06f4a95fad6577d05b67624aa
Committed: https://crrev.com/8fbe9d3eb4ab56b0be87c3c4e04bb544ede982c7
Cr-Commit-Position: refs/heads/master@{#332454}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix compile issue, wrap process backgrounding in an experiment #
Total comments: 28
Patch Set 3 : Fix typos, type casting, and repair broken unit tests #Patch Set 4 : Fix compilation problem on non-Mac systems #Patch Set 5 : Replace DPCHECK with DCHECK #Patch Set 6 : Fix merge conflicts #Patch Set 7 : Fix build conflicts #Patch Set 8 : Clean up field trial logic #
Total comments: 9
Patch Set 9 : Fix nits, add MACH_DCHECK calls. #
Total comments: 12
Patch Set 10 : Fix nits, refine Mach error reporting. #Patch Set 11 : Remove tab backgrounding from field trial #
Total comments: 2
Patch Set 12 : Rebased on tot, rewrapped backgrounging in an experiment. #
Total comments: 13
Patch Set 13 : Move experiment check to render_process_host_impl #Patch Set 14 : Rebase on tot #Patch Set 15 : Fix nits. #
Total comments: 5
Patch Set 16 : Remove unneeded includes. #
Total comments: 6
Patch Set 17 : Fix nits, add error checking. #
Total comments: 1
Patch Set 18 : Combine code enabling Mac and Windows experiments. #
Total comments: 4
Patch Set 19 : Change string compare to be case sensitive. #
Total comments: 2
Patch Set 20 : Edit a comment. #Patch Set 21 : Increase timeout in context_lost test suite #
Total comments: 1
Patch Set 22 : Fix incorrect QOS setting. #Patch Set 23 : Disable backgrounding during ExtensionApiNewTabTest.Tabs #
Total comments: 1
Patch Set 24 : Add switch to disable renderer process backgrounding #Patch Set 25 : Exclude more browser tests from renderer backgrounding #Patch Set 26 : Remove switch setting from browser test subclasses. #Patch Set 27 : Move switch from base to content. #
Total comments: 2
Patch Set 28 : Fix nit. #Messages
Total messages: 132 (24 generated)
shrike@chromium.org changed reviewers: + rsesek@chromium.org
gab@chromium.org changed reviewers: + gab@chromium.org
drive-by, thanks! https://codereview.chromium.org/989703002/diff/1/base/process/process_posix.cc File base/process/process_posix.cc (right): https://codereview.chromium.org/989703002/diff/1/base/process/process_posix.c... base/process/process_posix.cc:316: // priority. Update this comment? i.e. we do it on Linux, Chrome OS, and now Mac so this comment doesn't really make sense. (also consider adding a NOTIMPLEMENTED() macro to make it explicit that this could be enabled on other POSIX platforms, just isn't) https://codereview.chromium.org/989703002/diff/1/content/browser/child_proces... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/1/content/browser/child_proces... content/browser/child_process_launcher.cc:508: process.SetProcessBackgrounded(broker->TaskForPid(process.Pid()), background); To analyze the impact of this you'll want to put this behind an experiment. See https://codereview.chromium.org/926663002 for an example of how to set this up with experiments, you'll probably only care to have two modes (ON/OFF) in your case.
rsesek - I'm running into trouble now with the unit test for SetProcessBackgrounded(). That unit test is part of base, SetProcessBackgrounded() on the Mac now takes a mach port, but base has no access to MachBroker. Any suggestions? https://codereview.chromium.org/989703002/diff/1/base/process/process_posix.cc File base/process/process_posix.cc (right): https://codereview.chromium.org/989703002/diff/1/base/process/process_posix.c... base/process/process_posix.cc:316: // priority. On 2015/03/09 15:34:50, gab wrote: > Update this comment? i.e. we do it on Linux, Chrome OS, and now Mac so this > comment doesn't really make sense. > > (also consider adding a NOTIMPLEMENTED() macro to make it explicit that this > could be enabled on other POSIX platforms, just isn't) I added NOTIMPLEMENTED() to this method, but the unit test includes the following comment: // Note: a platform may not be willing or able to lower the priority of // a process. The calls to SetProcessBackground should be noops then. Is it the case that whenever the unit test runs on a platform that hits the NOTIMPLEMENTED() call, the unit test will fail?
https://codereview.chromium.org/989703002/diff/1/base/process/process_posix.cc File base/process/process_posix.cc (right): https://codereview.chromium.org/989703002/diff/1/base/process/process_posix.c... base/process/process_posix.cc:316: // priority. On 2015/03/10 00:34:44, shrike wrote: > On 2015/03/09 15:34:50, gab wrote: > > Update this comment? i.e. we do it on Linux, Chrome OS, and now Mac so this > > comment doesn't really make sense. > > > > (also consider adding a NOTIMPLEMENTED() macro to make it explicit that this > > could be enabled on other POSIX platforms, just isn't) > > I added NOTIMPLEMENTED() to this method, but the unit test includes the > following comment: > > // Note: a platform may not be willing or able to lower the priority of > // a process. The calls to SetProcessBackground should be noops then. > > Is it the case that whenever the unit test runs on a platform that hits the > NOTIMPLEMENTED() call, the unit test will fail? No, NOTIMPLEMENTED() simply results in an ERROR LOG [1] and is common practice [2] for cross-platform calls that are lacking an implementation on a given platform. i.e., yes, it's still a no-op implementation. The NOTIMPLEMENTED() macro serves both as a runtime hint that unimplemented paths are being hit when debugging and as an explicit code hint for things that are not intended to work a given way but are rather a default implementation pending a real one. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=N... [2] https://code.google.com/p/chromium/codesearch#search/&q=NOTIMPLEMENTED%5C(%5C...
https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... content/browser/child_process_launcher.cc:510: if (trial && trial->group_name() == "On") { I would suggest taking this path by default in the absence of field trials, i.e.: if (!trial || trial->group_name() == "On") {... This will make this the default behavior on bots (in particular perf bots). The only downside is that you'll have to make sure to have a Finch config initially explicitly disabling it on Beta/Stable.
https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... content/browser/child_process_launcher.cc:510: if (trial && trial->group_name() == "On") { On 2015/03/10 12:33:02, gab wrote: > I would suggest taking this path by default in the absence of field trials, > i.e.: > if (!trial || trial->group_name() == "On") {... > > This will make this the default behavior on bots (in particular perf bots). > > The only downside is that you'll have to make sure to have a Finch config > initially explicitly disabling it on Beta/Stable. OK. Will Finch flags be available soon enough after startup so that they'll exist before any render processes get backgrounded?
Nice! I'm excited to see the results of this experiment. On 2015/03/10 00:34:45, shrike wrote: > rsesek - I'm running into trouble now with the unit test for > SetProcessBackgrounded(). That unit test is part of base, > SetProcessBackgrounded() on the Mac now takes a mach port, but base has no > access to MachBroker. Any suggestions? For testing, there are two options: use mach_task_self() to set the priority of the current test process, or start a new process and adjust that task. The problem with the latter is there's no simple way to start a process and communicate over Mach IPC. I think doing the former is probably OK, though when you get a //base reviewer (I'd recommend Mark), they may have thoughts. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac.cc File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:5: #include "base/process/process.h" nit: blank line after https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:9: // The following was added to <mach/task_policy.h> after 10.8 nit: punctuation https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:56: return true; This isn't true unconditionally. This should probably return mac::IsOSMountainLionOrLater(), right? https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:68: (task_policy_t)&category_policy, C-style casts are banned by Google style guide. Use reinterpret_cast<>. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:70: DPCHECK(result == KERN_SUCCESS); This will fail on older kernels. PCHECK is also not the right one to use, since this won't set errno, but rather the kern_return_t will be set. We have base/mac/mach_logging.h to help here. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:82: if (!CanBackgroundProcesses() || process_port == 0) { TASK_NULL instead of 0. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:86: struct task_category_policy taskCatPolicy; Naming: use under_score variable names and spell out the full words like you do in IsProcessBackgrounded.
Alexei, see below. https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... content/browser/child_process_launcher.cc:510: if (trial && trial->group_name() == "On") { On 2015/03/10 18:00:33, shrike wrote: > On 2015/03/10 12:33:02, gab wrote: > > I would suggest taking this path by default in the absence of field trials, > > i.e.: > > if (!trial || trial->group_name() == "On") {... > > > > This will make this the default behavior on bots (in particular perf bots). > > > > The only downside is that you'll have to make sure to have a Finch config > > initially explicitly disabling it on Beta/Stable. > > OK. Will Finch flags be available soon enough after startup so that they'll > exist before any render processes get backgrounded? Yes, Finch is brought up before creating any threads/processes if I'm no mistaken. @asvitkine to confirm
shrike@chromium.org changed reviewers: + thakis@chromium.org
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... content/browser/child_process_launcher.cc:509: base::FieldTrialList::Find("BackgroundMacRendererProcesses"); Nit: You can just use FindFullName() and then the logic below becomes simpler. (It will return "" if no trial). https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... content/browser/child_process_launcher.cc:510: if (trial && trial->group_name() == "On") { On 2015/03/10 19:43:50, gab wrote: > On 2015/03/10 18:00:33, shrike wrote: > > On 2015/03/10 12:33:02, gab wrote: > > > I would suggest taking this path by default in the absence of field trials, > > > i.e.: > > > if (!trial || trial->group_name() == "On") {... > > > > > > This will make this the default behavior on bots (in particular perf bots). > > > > > > The only downside is that you'll have to make sure to have a Finch config > > > initially explicitly disabling it on Beta/Stable. > > > > OK. Will Finch flags be available soon enough after startup so that they'll > > exist before any render processes get backgrounded? > > Yes, Finch is brought up before creating any threads/processes if I'm no > mistaken. > > @asvitkine to confirm Correct.
PTAL Also, one of the Mac trybots is failing one test (context_lost on NVIDIA GPU on Mac Retina (with patch) on Mac-10.9 ). It could be a flaky test, or it could be the performance or timing of the browser is different now with my change and causing a failure. Anyone have advice on how to figure this out? https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac.cc File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/03/10 18:27:43, Robert Sesek wrote: > nit: no (c) Done (but FYI I just copy/pasted from process_linux.cc and changed the date). https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:5: #include "base/process/process.h" On 2015/03/10 18:27:43, Robert Sesek wrote: > nit: blank line after I added the blank line (what is the rule)? https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:9: // The following was added to <mach/task_policy.h> after 10.8 On 2015/03/10 18:27:43, Robert Sesek wrote: > nit: punctuation Done. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:56: return true; On 2015/03/10 18:27:43, Robert Sesek wrote: > This isn't true unconditionally. This should probably return > mac::IsOSMountainLionOrLater(), right? You can foreground/background a task on prior versions of the OS, it's just the quality of service settings that are only as of Mavericks. I have a check in SetProcessBackgrounded() to bail before the QoS code if you're running < Mavericks. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:68: (task_policy_t)&category_policy, On 2015/03/10 18:27:43, Robert Sesek wrote: > C-style casts are banned by Google style guide. Use reinterpret_cast<>. Done. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:70: DPCHECK(result == KERN_SUCCESS); On 2015/03/10 18:27:43, Robert Sesek wrote: > This will fail on older kernels. PCHECK is also not the right one to use, since > this won't set errno, but rather the kern_return_t will be set. We have > base/mac/mach_logging.h to help here. Looks like I borrowed some code from process_linux.cc instead of process_posix.cc. I have changed them to DCHECKs. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:82: if (!CanBackgroundProcesses() || process_port == 0) { On 2015/03/10 18:27:43, Robert Sesek wrote: > TASK_NULL instead of 0. Done. I know that TASK_NULL is 0 but I had a reason for saying 0 instead (not sure it's worth the typing to explain). https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:86: struct task_category_policy taskCatPolicy; On 2015/03/10 18:27:43, Robert Sesek wrote: > Naming: use under_score variable names and spell out the full words like you do > in IsProcessBackgrounded. Done. https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/20001/content/browser/child_pr... content/browser/child_process_launcher.cc:509: base::FieldTrialList::Find("BackgroundMacRendererProcesses"); On 2015/03/11 15:30:54, Alexei Svitkine (slow) wrote: > Nit: You can just use FindFullName() and then the logic below becomes simpler. > (It will return "" if no trial). Done.
experiment structure generally lgtm https://codereview.chromium.org/989703002/diff/140001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/140001/content/browser/child_p... content/browser/child_process_launcher.cc:508: std::string trialGroupName = nit: const
https://codereview.chromium.org/989703002/diff/140001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/140001/content/browser/child_p... content/browser/child_process_launcher.cc:508: std::string trialGroupName = hacker_style
https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac.cc File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/03/11 21:38:21, shrike wrote: > On 2015/03/10 18:27:43, Robert Sesek wrote: > > nit: no (c) > > Done (but FYI I just copy/pasted from process_linux.cc and changed the date). Yup, the copyright policy changed over time. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:5: #include "base/process/process.h" On 2015/03/11 21:38:21, shrike wrote: > On 2015/03/10 18:27:43, Robert Sesek wrote: > > nit: blank line after > > I added the blank line (what is the rule)? http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:56: return true; On 2015/03/11 21:38:21, shrike wrote: > On 2015/03/10 18:27:43, Robert Sesek wrote: > > This isn't true unconditionally. This should probably return > > mac::IsOSMountainLionOrLater(), right? > > You can foreground/background a task on prior versions of the OS, it's just the > quality of service settings that are only as of Mavericks. I have a check in > SetProcessBackgrounded() to bail before the QoS code if you're running < > Mavericks. Ah, my mistake. I missed that in the 10.6 kernel. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:70: DPCHECK(result == KERN_SUCCESS); On 2015/03/11 21:38:21, shrike wrote: > On 2015/03/10 18:27:43, Robert Sesek wrote: > > This will fail on older kernels. PCHECK is also not the right one to use, > since > > this won't set errno, but rather the kern_return_t will be set. We have > > base/mac/mach_logging.h to help here. > > Looks like I borrowed some code from process_linux.cc instead of > process_posix.cc. I have changed them to DCHECKs. I suggested you look at mach_logging.h because we have MACH_DCHECK: https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/mach_logg... https://codereview.chromium.org/989703002/diff/140001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/989703002/diff/140001/base/process/process.h#... base/process/process.h:119: bool IsProcessBackgrounded(mach_port_t process_port) const; naming: task_port, same in the comment, .cc file, and on line 126 https://codereview.chromium.org/989703002/diff/140001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/140001/base/process/process_ma... base/process/process_mac.cc:107: background ? THROUGHPUT_QOS_TIER_5 : THROUGHPUT_QOS_LAUNCH_DEFAULT_TIER}; nit: }; on its own line https://codereview.chromium.org/989703002/diff/140001/base/process/process_ma... base/process/process_mac.cc:109: (task_policy_t)&qosinfo, TASK_QOS_POLICY_COUNT); nit: reinterpret_cast
shrike@chromium.org changed reviewers: + avi@chromium.org
https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:74: MACH_DCHECK(result == KERN_SUCCESS, result); It may be more helpful to log this, rather than just DCHECK. Looking at other base/mac/ and Mach-related code, that seems to be the norm. Change this to: MACH_LOG_IF(ERROR, result != KERN_SUCCESS, result) << "task_policy_get TASK_CATEGORY_POLICY"; https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:89: struct task_category_policy task_category_policy; This function isn't consistently typed or named with IsProcessBackgrounded(). To make them consistent, use |task_category_policy| as the type (no |struct | required, since this is C++), but use the name above (category_poilcy). Use the same type and name in IsProcessBackgrounded(). https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:99: return false; Here, you could just do: MACH_LOG(ERROR, result) << "task_policy_set TASK_CATEGORY_POLICY" https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:105: // on foreground because precise timer firing isn't needed nit: punctuation https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:106: struct task_qos_policy qosinfo = { naming: qos_policy https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:113: MACH_DCHECK(result == KERN_SUCCESS, result); MACH_LOG_IF as above, or restructure: if (result != KERN_SUCCESS) { MACH_LOG(ERROR, result) << "task_policy_set TASK_OVERRIDE_QOS_POLICY"; return false; } return true;
After discussion with pinkerton and others on the Mac team I removed the field trial wrapper, so now tab backgrounding will be enabled for all users. The policy surrounding backgrounding a tab is set higher up - this code simply allows that policy to take effect on the Mac. Additionally it's not clear what performance stats the field trial would measure. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac.cc File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:5: #include "base/process/process.h" On 2015/03/12 16:00:23, Robert Sesek wrote: > On 2015/03/11 21:38:21, shrike wrote: > > On 2015/03/10 18:27:43, Robert Sesek wrote: > > > nit: blank line after > > > > I added the blank line (what is the rule)? > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... Thank you. https://codereview.chromium.org/989703002/diff/20001/base/process/process_mac... base/process/process_mac.cc:70: DPCHECK(result == KERN_SUCCESS); On 2015/03/12 16:00:23, Robert Sesek wrote: > On 2015/03/11 21:38:21, shrike wrote: > > On 2015/03/10 18:27:43, Robert Sesek wrote: > > > This will fail on older kernels. PCHECK is also not the right one to use, > > since > > > this won't set errno, but rather the kern_return_t will be set. We have > > > base/mac/mach_logging.h to help here. > > > > Looks like I borrowed some code from process_linux.cc instead of > > process_posix.cc. I have changed them to DCHECKs. > > I suggested you look at mach_logging.h because we have MACH_DCHECK: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/mach_logg... Thank you. I haven't done a lot of kernel work so I wasn't sure what to be on the lookout for. Done. https://codereview.chromium.org/989703002/diff/140001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/989703002/diff/140001/base/process/process.h#... base/process/process.h:119: bool IsProcessBackgrounded(mach_port_t process_port) const; On 2015/03/12 16:00:23, Robert Sesek wrote: > naming: task_port, same in the comment, .cc file, and on line 126 Done. https://codereview.chromium.org/989703002/diff/140001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/140001/base/process/process_ma... base/process/process_mac.cc:107: background ? THROUGHPUT_QOS_TIER_5 : THROUGHPUT_QOS_LAUNCH_DEFAULT_TIER}; On 2015/03/12 16:00:23, Robert Sesek wrote: > nit: }; on its own line Done. https://codereview.chromium.org/989703002/diff/140001/base/process/process_ma... base/process/process_mac.cc:109: (task_policy_t)&qosinfo, TASK_QOS_POLICY_COUNT); On 2015/03/12 16:00:23, Robert Sesek wrote: > nit: reinterpret_cast Done. https://codereview.chromium.org/989703002/diff/140001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/140001/content/browser/child_p... content/browser/child_process_launcher.cc:508: std::string trialGroupName = On 2015/03/12 15:50:09, gab wrote: > nit: const > hacker_style Done and done. https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:74: MACH_DCHECK(result == KERN_SUCCESS, result); On 2015/03/13 04:57:01, Robert Sesek wrote: > It may be more helpful to log this, rather than just DCHECK. Looking at other > base/mac/ and Mach-related code, that seems to be the norm. > > Change this to: > > MACH_LOG_IF(ERROR, result != KERN_SUCCESS, result) << "task_policy_get > TASK_CATEGORY_POLICY"; Thank you. Done. https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:89: struct task_category_policy task_category_policy; On 2015/03/13 04:57:01, Robert Sesek wrote: > This function isn't consistently typed or named with IsProcessBackgrounded(). To > make them consistent, use |task_category_policy| as the type (no |struct | > required, since this is C++), but use the name above (category_poilcy). Use the > same type and name in IsProcessBackgrounded(). Done. https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:99: return false; On 2015/03/13 04:57:01, Robert Sesek wrote: > Here, you could just do: > > MACH_LOG(ERROR, result) << "task_policy_set TASK_CATEGORY_POLICY" Done. https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:105: // on foreground because precise timer firing isn't needed On 2015/03/13 04:57:01, Robert Sesek wrote: > nit: punctuation (Not used to ending my comments with punctuation). Done. https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:106: struct task_qos_policy qosinfo = { On 2015/03/13 04:57:01, Robert Sesek wrote: > naming: qos_policy Done. https://codereview.chromium.org/989703002/diff/160001/base/process/process_ma... base/process/process_mac.cc:113: MACH_DCHECK(result == KERN_SUCCESS, result); On 2015/03/13 04:57:01, Robert Sesek wrote: > MACH_LOG_IF as above, or restructure: > > if (result != KERN_SUCCESS) { > MACH_LOG(ERROR, result) << "task_policy_set TASK_OVERRIDE_QOS_POLICY"; > return false; > } > > return true; Done.
avi@chromium.org changed reviewers: - avi@chromium.org
Nits. (Not officially reviewing.) https://codereview.chromium.org/989703002/diff/200001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/989703002/diff/200001/base/process/process.h#... base/process/process.h:116: // A process is backgrounded when it's priority is lower than normal. grammar nit: its https://codereview.chromium.org/989703002/diff/200001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/200001/base/process/process_ma... base/process/process_mac.cc:90: task_category_policy category_poilcy; typo in variable name
On 2015/03/18 16:20:23, shrike wrote: > After discussion with pinkerton and others on the Mac team I removed the field > trial wrapper, so now tab backgrounding will be enabled for all users. The > policy surrounding backgrounding a tab is set higher up - this code simply > allows that policy to take effect on the Mac. Additionally it's not clear what > performance stats the field trial would measure. I agree that controlling this experiment from child_process_launcher.cc may not have been the right approach, but this *needs* to be ran inside an experiment. Adding OS_MACOSX to this ifdef [1] may be good enough to get an enabled-by-default event controlled from Finch. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... As for which stats you would measure, have a look at this doc for the equivalent Windows analysis: https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... It's not even clear-cut that we should enable this on Windows yet (i.e. backgrounding may hurt synchronous IPCs made by Flash), having it as an experiment is definitely required. Lastly the only way to even analyze the gains brought by this change is to have it run as an A/B experiment on the same versions of Chrome and compare confidence intervals.
On 2015/03/18 19:13:29, gab wrote: > On 2015/03/18 16:20:23, shrike wrote: > > After discussion with pinkerton and others on the Mac team I removed the field > > trial wrapper, so now tab backgrounding will be enabled for all users. The > > policy surrounding backgrounding a tab is set higher up - this code simply > > allows that policy to take effect on the Mac. Additionally it's not clear what > > performance stats the field trial would measure. > > I agree that controlling this experiment from child_process_launcher.cc may not > have been the right approach, but this *needs* to be ran inside an experiment. > > Adding OS_MACOSX to this ifdef [1] may be good enough to get an > enabled-by-default event controlled from Finch. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > As for which stats you would measure, have a look at this doc for the equivalent > Windows analysis: > https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... > It's not even clear-cut that we should enable this on Windows yet (i.e. > backgrounding may hurt synchronous IPCs made by Flash), having it as an > experiment is definitely required. > > Lastly the only way to even analyze the gains brought by this change is to have > it run as an A/B experiment on the same versions of Chrome and compare > confidence intervals. As I read it, the experiment you are referring to has to do with a bug introduced in the Windows version (where background mode was set by the render process). There is no similar case to test against here, as that bug did not occur on the Mac. I don't know what we would actually measure on the Mac in any such experiment. I thought in chrome fast this morning someone said the tab backgrounding is wrapped in an experiment already? Seems like that would cover it.
I don't understand why we need separate experiments for windows and mac. Can't we just roll them together in the same finch experiment and look at the same metrics? As far as analyzing the gains for this CL, what specific metrics are we talking about that are (or would be) different from the other experiment?
On 2015/03/19 14:51:37, pinkerton1 wrote: > I don't understand why we need separate experiments for windows and mac. We don't. The problem is that the current experiment is Windows-only (see code reference mentioned above). I'm happy to make this a Mac experiment as well at the same place in the code (but the current CL doesn't do this and that's the issue I'm trying to point out). The Mac experiment itself should have its own release bug/timeline/finch_config and separate analysis as while this is the same code path (the OS-dependent behaviour I think is not at all comparable apples-to-apples). > Can't we just roll them together in the same finch experiment and look at the same > metrics? As far as analyzing the gains for this CL, what specific metrics are we > talking about that are (or would be) different from the other experiment? Yes we should do the same analysis as we did on Windows (modulo AllowBackgroundModeFromRenderer): https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... > As I read it, the experiment you are referring to has to do with a bug introduced in the Windows version (where background mode was set by the render process). Yes and no, one side of the experiment is indeed bringing back an old known Windows bug to make sure we now have coverage for what this was causing (and that we aren't regressing the same metrics with this new experiment). But the core of the experiment is just to: 1) Overall see how much this gains. 2) Make sure we don't regress anything else (i.e. right now we are looking to see whether synchronous flash IPCs would be hurt -- i.e. could essentially result in priority inversion). and we should do that experiment on Mac too (or we will have no idea what this change is helping and hurting).
On 2015/03/19 18:20:08, gab wrote: > But the core of the experiment is just to: > 1) Overall see how much this gains. What are you tracking in your experiment to measure this gain/loss? It's not clear to me from looking at the finch config file. > 2) Make sure we don't regress anything else (i.e. right now we are looking to > see whether synchronous flash IPCs would be hurt -- i.e. could essentially > result in priority inversion). Is this a measurement that's part of your experiment - if so, which item is it in the finch config file?
On 2015/03/19 18:54:26, shrike wrote: > On 2015/03/19 18:20:08, gab wrote: > > But the core of the experiment is just to: > > 1) Overall see how much this gains. > > What are you tracking in your experiment to measure this gain/loss? It's not > clear to me from looking at the finch config file. Startup, SessionRestore, TabSwitch timings All detailed in the analysis @ https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... > > > 2) Make sure we don't regress anything else (i.e. right now we are looking to > > see whether synchronous flash IPCs would be hurt -- i.e. could essentially > > result in priority inversion). > > Is this a measurement that's part of your experiment - if so, which item is it > in the finch config file? No, this is something which was brought to our attention after launching the initial experiment and which we will be adding soon.
On 2015/03/19 19:38:00, gab wrote: > On 2015/03/19 18:54:26, shrike wrote: > > On 2015/03/19 18:20:08, gab wrote: > > > But the core of the experiment is just to: > > > 1) Overall see how much this gains. > > > > What are you tracking in your experiment to measure this gain/loss? It's not > > clear to me from looking at the finch config file. > > Startup, SessionRestore, TabSwitch timings In downgrading the priority of background tab processes I would expect the foreground tab process to have more cycles available if needed. So if there's a processor-intensive task running in the foreground process it will get more cycles than backgrounded tabs, but if nothing much is going on in the foreground tab the background tab processes can still grab the majority of cycles if needed. You have said that I should be running an experiment to measure the gains of this backgrounding change on the Mac - I am hard pressed to think of a measurement that can capture the gains in this expected behavior. What metric do you think captures this? Re: your list of the measurements you're taking in your experiment, would you please explain why you believe each one is relevant to capturing the gains of this change, and the gains you expect to see in each case? > All detailed in the analysis @ > https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... This is my first experience with a Finch experiment document, but shouldn't the answers to my questions about relevance of the metrics and expected gains be included within it?
Sorry about the delay, see answers below. On 2015/03/20 15:59:35, shrike wrote: > On 2015/03/19 19:38:00, gab wrote: > > On 2015/03/19 18:54:26, shrike wrote: > > > On 2015/03/19 18:20:08, gab wrote: > > > > But the core of the experiment is just to: > > > > 1) Overall see how much this gains. > > > > > > What are you tracking in your experiment to measure this gain/loss? It's not > > > clear to me from looking at the finch config file. > > > > Startup, SessionRestore, TabSwitch timings > > In downgrading the priority of background tab processes I would expect the > foreground tab process to have more cycles available if needed. So if there's a > processor-intensive task running in the foreground process it will get more > cycles than backgrounded tabs, but if nothing much is going on in the foreground > tab the background tab processes can still grab the majority of cycles if > needed. You have said that I should be running an experiment to measure the > gains of this backgrounding change on the Mac - I am hard pressed to think of a > measurement that can capture the gains in this expected behavior. What metric do > you think captures this? > > Re: your list of the measurements you're taking in your experiment, would you > please explain why you believe each one is relevant to capturing the gains of > this change, and the gains you expect to see in each case? Startup/Session Restore of *foreground tab*: ideally completes faster per prioritisation. Tab Switch: We mostly wanted to make sure we weren't hurting this metric, but it turned out we actually helped it (probably because priority toggling happens immediately on tab switch, allowing the context switch to happen faster as the previous foreground tab is backgrounded (and also because the browser process has more cycle per the other renderers being backgrounded). > > > All detailed in the analysis @ > > > https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... > > This is my first experience with a Finch experiment document, but shouldn't the > answers to my questions about relevance of the metrics and expected gains be > included within it? Not sure I understand this question, what is "it"? Thanks, Gab
On 2015/03/24 20:36:21, gab wrote: > Sorry about the delay, see answers below. No problem. > Startup/Session Restore of *foreground tab*: ideally completes faster per > prioritisation. > Tab Switch: We mostly wanted to make sure we weren't hurting this metric, but it > turned out we actually helped it (probably because priority toggling happens > immediately on tab switch, allowing the context switch to happen faster as the > previous foreground tab is backgrounded (and also because the browser process > has more cycle per the other renderers being backgrounded). I guess I agree that these metrics might capture some portion of the after effects of this change, but I don't really agree beyond that. In other words, if we're concerned about this change causing bad things to happen, your measurements won't tell us anything beyond tab switching and session startup getting worse. I can;t think of a metric that will really measure this change. At best I think this experiment is a way to turn off the change if needed, and a way to correlate potential reports of strange behavior with users where the change has been activated. I will resubmit my cl with the experiment wrapper back in place. > > > > > All detailed in the analysis @ > > > > > > https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... > > > > This is my first experience with a Finch experiment document, but shouldn't > the > > answers to my questions about relevance of the metrics and expected gains be > > included within it? > > Not sure I understand this question, what is "it"? Sorry, the "it" is your report. I was saying that it seems like your Finch experiment document should explain why you are taking the measurements you are taking. Your document says: Investigated use cases are: startup, session restore, and tab switching. but that's all. There's nothing about why measuring these use cases is appropriate or relevant. I haven't had much experience with Finch experiment documents, but it seems like that would be a basic element of them?
On 2015/03/31 23:42:40, shrike wrote: > On 2015/03/24 20:36:21, gab wrote: > > Sorry about the delay, see answers below. > > No problem. > > > Startup/Session Restore of *foreground tab*: ideally completes faster per > > prioritisation. > > Tab Switch: We mostly wanted to make sure we weren't hurting this metric, but > it > > turned out we actually helped it (probably because priority toggling happens > > immediately on tab switch, allowing the context switch to happen faster as the > > previous foreground tab is backgrounded (and also because the browser process > > has more cycle per the other renderers being backgrounded). > > I guess I agree that these metrics might capture some portion of the after > effects of this change, but I don't really agree beyond that. In other words, if > we're concerned about this change causing bad things to happen, your > measurements won't tell us anything beyond tab switching and session startup > getting worse. I can;t think of a metric that will really measure this change. > At best I think this experiment is a way to turn off the change if needed, and a > way to correlate potential reports of strange behavior with users where the > change has been activated. > > I will resubmit my cl with the experiment wrapper back in place. Thanks, indeed there could be many more interesting metrics, but those are the ones we are explicitly looking at. As you say, the nice thing with an experiment is that we can always look at other metrics we hadn't originally thought of if anomalies pop out later. > > > > > > > > All detailed in the analysis @ > > > > > > > > > > https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... > > > > > > This is my first experience with a Finch experiment document, but shouldn't > > the > > > answers to my questions about relevance of the metrics and expected gains be > > > included within it? > > > > Not sure I understand this question, what is "it"? > > Sorry, the "it" is your report. I was saying that it seems like your Finch > experiment document should explain why you are taking the measurements you are > taking. Your document says: > > Investigated use cases are: startup, session restore, and tab switching. > > but that's all. There's nothing about why measuring these use cases is > appropriate or relevant. I haven't had much experience with Finch experiment > documents, but it seems like that would be a basic element of them? Ah, sorry if this wasn't clear. We are bringing back backgrounding specifically because we were hoping it would help startup and session restore (hence the analysis of the first two) and we know there was a form of background mode which was *hurting* tab switch performance on Windows before so we wanted to make sure ours wasn't. Note: we are also investigating whether Flash sync IPCs can be a problem (can in theory result in priority inversion, but does it matter in practice?), added Plugin.PpapiSyncIPCTime and potentially more metrics soon to take a look at this.
On 2015/04/01 12:46:22, gab wrote: > On 2015/03/31 23:42:40, shrike wrote: > > On 2015/03/24 20:36:21, gab wrote: > > > Sorry about the delay, see answers below. > > > > No problem. > > > > > Startup/Session Restore of *foreground tab*: ideally completes faster per > > > prioritisation. > > > Tab Switch: We mostly wanted to make sure we weren't hurting this metric, > but > > it > > > turned out we actually helped it (probably because priority toggling happens > > > immediately on tab switch, allowing the context switch to happen faster as > the > > > previous foreground tab is backgrounded (and also because the browser > process > > > has more cycle per the other renderers being backgrounded). > > > > I guess I agree that these metrics might capture some portion of the after > > effects of this change, but I don't really agree beyond that. In other words, > if > > we're concerned about this change causing bad things to happen, your > > measurements won't tell us anything beyond tab switching and session startup > > getting worse. I can;t think of a metric that will really measure this > change. > > At best I think this experiment is a way to turn off the change if needed, and > a > > way to correlate potential reports of strange behavior with users where the > > change has been activated. > > > > I will resubmit my cl with the experiment wrapper back in place. > > Thanks, indeed there could be many more interesting metrics, but those are the > ones we are explicitly looking at. As you say, the nice thing with an experiment > is that we can always look at other metrics we hadn't originally thought of if > anomalies pop out later. > > > > > > > > > > > > All detailed in the analysis @ > > > > > > > > > > > > > > > https://docs.google.com/document/d/1AbVdxNlOG4XFUsMDSi6VxlnJ0MDf-Fkw3XPLgv3CI... > > > > > > > > This is my first experience with a Finch experiment document, but > shouldn't > > > the > > > > answers to my questions about relevance of the metrics and expected gains > be > > > > included within it? > > > > > > Not sure I understand this question, what is "it"? > > > > Sorry, the "it" is your report. I was saying that it seems like your Finch > > experiment document should explain why you are taking the measurements you are > > taking. Your document says: > > > > Investigated use cases are: startup, session restore, and tab switching. > > > > but that's all. There's nothing about why measuring these use cases is > > appropriate or relevant. I haven't had much experience with Finch experiment > > documents, but it seems like that would be a basic element of them? > > Ah, sorry if this wasn't clear. > > We are bringing back backgrounding specifically because we were hoping it would > help startup and session restore (hence the analysis of the first two) and we > know there was a form of background mode which was *hurting* tab switch > performance on Windows before so we wanted to make sure ours wasn't. > > Note: we are also investigating whether Flash sync IPCs can be a problem (can in > theory result in priority inversion, but does it matter in practice?), added > Plugin.PpapiSyncIPCTime and potentially more metrics soon to take a look at > this. Oh also, I should say: Finch has a new notification system running in the background that will alert you if anything you're not actively looking at is different from one experiment group to the next (not 100% sure whether it looks at all histograms or only those you marked as histograms-of-interests in your config for now, but still very useful).
shrike@chromium.org changed reviewers: + avi@chromium.org
Added avi for review of changes in content/.... I have rewrapped the renderer process backgrounding in an experiment. The experiment is off by default - perf bots will test by supplying a command line argument to enable process backgrounding. PTAL
https://codereview.chromium.org/989703002/diff/220001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/989703002/diff/220001/base/process/process.h#... base/process/process.h:116: // A process is backgrounded when it's priority is lower than normal. its https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... base/process/process_mac.cc:54: #endif // TASK_OVERRIDE_QOS_POLICY This should go into base/mac/sdk_forward_declarations.h with all the other SDK declarations. https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... base/process/process_mac.cc:90: task_category_policy category_poilcy; typo: policy https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... content/browser/child_process_launcher.cc:23: #include "content/shell/common/shell_switches.h" This is the wrong file; you mean content/public/common/content_switches.h , although we're already including that file. This should fail DEPS checking; where are your trybots?
https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... content/browser/child_process_launcher.cc:276: base::FieldTrialList::FindFullName("BackgroundMacRendererProcesses"); As mentioned offline, I think we should use the same experiment entry point for all platforms (i.e. in render_process_host_impl.cc). I just wrote https://codereview.chromium.org/1089873002/ to get rid of the last Windows-specific chunk in there, after that it should just make sense to have the code be ifdef'ed to OS_WIN || OS_MAC (and even before that, it would have worked, only been slightly ackward since one of the comparions would have been against an impossible group name on Mac).
https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... content/browser/child_process_launcher.cc:23: #include "content/shell/common/shell_switches.h" On 2015/04/16 02:46:20, Avi wrote: > This is the wrong file; you mean content/public/common/content_switches.h , > although we're already including that file. > > This should fail DEPS checking; where are your trybots? Hi Avi, (not sure how I didn't fire up the trybots.) I saw the other location for content_switches - what is the difference between the two, and if I put command line switch in content/public/common/content_switches.cc will my code that looks for the flag still work (by content/shell/... I got the impression this was the way to pick up flags from the command line/shell)?
https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... content/browser/child_process_launcher.cc:278: base::CommandLine::ForCurrentProcess()->HasSwitch( Don't need a command-line switch either, it comes for free with the field trials: chrome.exe --force-fieldtrials=BackgroundRendererProcesses/Allow/
On 2015/04/16 17:31:33, shrike wrote: > I got the impression this was the way to pick > up flags from the command line/shell)? No, that's not it at all. Content is the module that renders web pages into a native view. The Content Shell is a basic shell around Content, useful to debug problems with web page rendering without spinning up all of Chrome. //content/shell/ is a basic browser built strictly on top of the rest of //content; you are never allowed to include from that directory outside of it. See https://www.chromium.org/developers/content-module
On 2015/04/16 18:47:35, Avi wrote: > On 2015/04/16 17:31:33, shrike wrote: > > I got the impression this was the way to pick > > up flags from the command line/shell)? > > No, that's not it at all. > > Content is the module that renders web pages into a native view. The Content > Shell is a basic shell around Content, useful to debug problems with web page > rendering without spinning up all of Chrome. //content/shell/ is a basic browser > built strictly on top of the rest of //content; you are never allowed to include > from that directory outside of it. Thanks for the explanation. Interesting though - seems like the DEPS checks aren't working because trybots are all green....
On 2015/04/16 19:07:50, shrike wrote: > On 2015/04/16 18:47:35, Avi wrote: > > On 2015/04/16 17:31:33, shrike wrote: > > > I got the impression this was the way to pick > > > up flags from the command line/shell)? > > > > No, that's not it at all. > > > > Content is the module that renders web pages into a native view. The Content > > Shell is a basic shell around Content, useful to debug problems with web page > > rendering without spinning up all of Chrome. //content/shell/ is a basic > browser > > built strictly on top of the rest of //content; you are never allowed to > include > > from that directory outside of it. > > Thanks for the explanation. Interesting though - seems like the DEPS checks > aren't working because trybots are all green.... Huh. Fix it anyway, though.
On 2015/04/16 19:13:50, Avi wrote: > > Thanks for the explanation. Interesting though - seems like the DEPS checks > > aren't working because trybots are all green.... > > Huh. Fix it anyway, though. Yes, of course.
PTAL. https://codereview.chromium.org/989703002/diff/220001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/989703002/diff/220001/base/process/process.h#... base/process/process.h:116: // A process is backgrounded when it's priority is lower than normal. On 2015/04/16 02:46:20, Avi wrote: > its Done. https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... base/process/process_mac.cc:54: #endif // TASK_OVERRIDE_QOS_POLICY On 2015/04/16 02:46:20, Avi wrote: > This should go into base/mac/sdk_forward_declarations.h with all the other SDK > declarations. The only thing is process_mac is a .cc file, not a .mm, so it won't compile if it tries to import sdk_forward_declarations.h. But once the builders move to 10.9 or higher there won't be any need for these declarations - I will add a TODO for that. https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... base/process/process_mac.cc:90: task_category_policy category_poilcy; On 2015/04/16 02:46:20, Avi wrote: > typo: policy Done. https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... content/browser/child_process_launcher.cc:276: base::FieldTrialList::FindFullName("BackgroundMacRendererProcesses"); On 2015/04/16 13:18:13, gab (OOO soon no new reviews) wrote: > As mentioned offline, I think we should use the same experiment entry point for > all platforms (i.e. in render_process_host_impl.cc). > > I just wrote https://codereview.chromium.org/1089873002/ to get rid of the last > Windows-specific chunk in there, after that it should just make sense to have > the code be ifdef'ed to OS_WIN || OS_MAC (and even before that, it would have > worked, only been slightly ackward since one of the comparions would have been > against an impossible group name on Mac). Done. https://codereview.chromium.org/989703002/diff/220001/content/browser/child_p... content/browser/child_process_launcher.cc:278: base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/04/16 18:42:42, gab (OOO soon no new reviews) wrote: > Don't need a command-line switch either, it comes for free with the field > trials: > > chrome.exe --force-fieldtrials=BackgroundRendererProcesses/Allow/ Done.
https://codereview.chromium.org/989703002/diff/280001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/280001/content/browser/child_p... content/browser/child_process_launcher.cc:12: #include "base/metrics/field_trial.h" Do you need this include? https://codereview.chromium.org/989703002/diff/280001/content/browser/child_p... content/browser/child_process_launcher.cc:23: #include "content/shell/common/shell_switches.h" You still need to remove this line.
PTAL. https://codereview.chromium.org/989703002/diff/280001/content/browser/child_p... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/989703002/diff/280001/content/browser/child_p... content/browser/child_process_launcher.cc:12: #include "base/metrics/field_trial.h" On 2015/04/21 20:59:45, Avi wrote: > Do you need this include? No - removed. https://codereview.chromium.org/989703002/diff/280001/content/browser/child_p... content/browser/child_process_launcher.cc:23: #include "content/shell/common/shell_switches.h" On 2015/04/21 20:59:45, Avi wrote: > You still need to remove this line. There was an existing ../content_switches.h include before my modifications - I thought this was it but I see it's not. Removed.
https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... base/process/process_mac.cc:54: #endif // TASK_OVERRIDE_QOS_POLICY On 2015/04/21 20:55:48, shrike wrote: > On 2015/04/16 02:46:20, Avi wrote: > > This should go into base/mac/sdk_forward_declarations.h with all the other SDK > > declarations. > > The only thing is process_mac is a .cc file, not a .mm, so it won't compile if > it tries to import sdk_forward_declarations.h. But once the builders move to > 10.9 or higher there won't be any need for these declarations - I will add a > TODO for that. FWIW I think it's fine to only declare these here, since the likelihood of another file needing them is very small, which is why I didn't suggest this. https://codereview.chromium.org/989703002/diff/280001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/280001/base/process/process_ma... base/process/process_mac.cc:88: if (!CanBackgroundProcesses() || task_port == TASK_NULL) { Why is |task_port| allowed to be TASK_NULL here but not in IsProcessBackgrounded()? They should both be consistent. Should it be a DCHECK_NE() instead?
lgtm
https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2260: } Why not just re-use the Windows code for both? As-is your experiment will not enabled by default on trunk (where there are no experiments) and thus will not get perf waterfall coverage. i.e. I'd say, just change the Windows ifdef to #if defined(OS_WIN) || defined(OS_MACOSX) instead of duplicating the code into a slightly different variant. (one thing to be careful with enabling by default on trunk is that you'll to create configs up to Stable right away to explicitly disable it there before it gets approval)
On 2015/04/21 23:02:38, Robert Sesek wrote: > https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... > File base/process/process_mac.cc (right): > > https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... > base/process/process_mac.cc:54: #endif // TASK_OVERRIDE_QOS_POLICY > On 2015/04/21 20:55:48, shrike wrote: > > On 2015/04/16 02:46:20, Avi wrote: > > > This should go into base/mac/sdk_forward_declarations.h with all the other > SDK > > > declarations. > > > > The only thing is process_mac is a .cc file, not a .mm, so it won't compile if > > it tries to import sdk_forward_declarations.h. But once the builders move to > > 10.9 or higher there won't be any need for these declarations - I will add a > > TODO for that. > > FWIW I think it's fine to only declare these here, since the likelihood of > another file needing them is very small, which is why I didn't suggest this. > > https://codereview.chromium.org/989703002/diff/280001/base/process/process_ma... > File base/process/process_mac.cc (right): > > https://codereview.chromium.org/989703002/diff/280001/base/process/process_ma... > base/process/process_mac.cc:88: if (!CanBackgroundProcesses() || task_port == > TASK_NULL) { > Why is |task_port| allowed to be TASK_NULL here but not in > IsProcessBackgrounded()? They should both be consistent. Should it be a > DCHECK_NE() instead? I agree they should be consistent. Re: DCHECK_NE() , I don't think so. The issue is that, as you know, the render process's port is not immediately available, so there's a chance that the MachBroker will return TASK_NULL when you ask for the port to pass to process.SetProcessBackgrounded(). Unless you feel that TASK_NULL should be an error and that callees should always check before passing it? I lean toward just ignoring any incoming TASK_NULL.
On 2015/04/22 13:11:34, gab (OOO soon no new reviews) wrote: > https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... > File content/browser/renderer_host/render_process_host_impl.cc (right): > > https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... > content/browser/renderer_host/render_process_host_impl.cc:2260: } > Why not just re-use the Windows code for both? > > As-is your experiment will not enabled by default on trunk (where there are no > experiments) and thus will not get perf waterfall coverage. > > i.e. I'd say, just change the Windows ifdef to > #if defined(OS_WIN) || defined(OS_MACOSX) > instead of duplicating the code into a slightly different variant. > > (one thing to be careful with enabling by default on trunk is that you'll to > create configs up to Stable right away to explicitly disable it there before it > gets approval) Hi Gab, I understand what you are saying about adding defined(OS_MACOSX) to your existing code, but I prefer the Mac experiment not be enabled by default. That way this change, which has taken forever to get right, can be landed separate from whatever it will take to get the Mac experiment approved and activated. Re: perf waterfall coverage, I asked sullivan about this and she said I could just create a telemetry benchmark with the flag enabled from the command line.
On 2015/04/22 21:50:54, shrike wrote: > On 2015/04/22 13:11:34, gab (OOO soon no new reviews) wrote: > > > https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... > > File content/browser/renderer_host/render_process_host_impl.cc (right): > > > > > https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... > > content/browser/renderer_host/render_process_host_impl.cc:2260: } > > Why not just re-use the Windows code for both? > > > > As-is your experiment will not enabled by default on trunk (where there are no > > experiments) and thus will not get perf waterfall coverage. > > > > i.e. I'd say, just change the Windows ifdef to > > #if defined(OS_WIN) || defined(OS_MACOSX) > > instead of duplicating the code into a slightly different variant. > > > > (one thing to be careful with enabling by default on trunk is that you'll to > > create configs up to Stable right away to explicitly disable it there before > it > > gets approval) > > Hi Gab, > > I understand what you are saying about adding defined(OS_MACOSX) to your > existing code, but I prefer the Mac experiment not be enabled by default. That > way this change, which has taken forever to get right, can be landed separate > from whatever it will take to get the Mac experiment approved and activated. Re: > perf waterfall coverage, I asked sullivan about this and she said I could just > create a telemetry benchmark with the flag enabled from the command line. Okay, up to you.
lgtm w/ two nits Thanks, Gab https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2256: const std::string trial_group_name = Add a comment above similar to the Windows one (plus launch bug link). https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2258: if (trial_group_name.length() > 0 && trial_group_name != "Disallow") { s/trial_group_name.length() > 0/!trial_group_name.empty()
On 2015/04/22 21:46:05, shrike wrote: > On 2015/04/21 23:02:38, Robert Sesek wrote: > > > https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... > > File base/process/process_mac.cc (right): > > > > > https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... > > base/process/process_mac.cc:54: #endif // TASK_OVERRIDE_QOS_POLICY > > On 2015/04/21 20:55:48, shrike wrote: > > > On 2015/04/16 02:46:20, Avi wrote: > > > > This should go into base/mac/sdk_forward_declarations.h with all the other > > SDK > > > > declarations. > > > > > > The only thing is process_mac is a .cc file, not a .mm, so it won't compile > if > > > it tries to import sdk_forward_declarations.h. But once the builders move to > > > 10.9 or higher there won't be any need for these declarations - I will add a > > > TODO for that. > > > > FWIW I think it's fine to only declare these here, since the likelihood of > > another file needing them is very small, which is why I didn't suggest this. > > > > > https://codereview.chromium.org/989703002/diff/280001/base/process/process_ma... > > File base/process/process_mac.cc (right): > > > > > https://codereview.chromium.org/989703002/diff/280001/base/process/process_ma... > > base/process/process_mac.cc:88: if (!CanBackgroundProcesses() || task_port == > > TASK_NULL) { > > Why is |task_port| allowed to be TASK_NULL here but not in > > IsProcessBackgrounded()? They should both be consistent. Should it be a > > DCHECK_NE() instead? > > I agree they should be consistent. Re: DCHECK_NE() , I don't think so. The issue > is that, as you know, the render process's port is not immediately available, so > there's a chance that the MachBroker will return TASK_NULL when you ask for the > port to pass to process.SetProcessBackgrounded(). Unless you feel that TASK_NULL > should be an error and that callees should always check before passing it? I > lean toward just ignoring any incoming TASK_NULL. I don't have a strong preference, but I think from an API perspective, IsProcessBackgrounded() returning false in the case where the port is NULL is a bit strange. It really should be reporting a failure, but it is would appear to be reporting that a process isn't backgrounded. Since there's only one caller here, I think having a precondition that the port be non-NULL is OK. But that's not a strong preference, though the two APIs should be consistent.
On 2015/04/22 21:59:52, Robert Sesek wrote: > On 2015/04/22 21:46:05, shrike wrote: > > On 2015/04/21 23:02:38, Robert Sesek wrote: > > > > > > https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... > > > File base/process/process_mac.cc (right): > > > > > > > > > https://codereview.chromium.org/989703002/diff/220001/base/process/process_ma... > > > base/process/process_mac.cc:54: #endif // TASK_OVERRIDE_QOS_POLICY > > > On 2015/04/21 20:55:48, shrike wrote: > > > > On 2015/04/16 02:46:20, Avi wrote: > > > > > This should go into base/mac/sdk_forward_declarations.h with all the > other > > > SDK > > > > > declarations. > > > > > > > > The only thing is process_mac is a .cc file, not a .mm, so it won't > compile > > if > > > > it tries to import sdk_forward_declarations.h. But once the builders move > to > > > > 10.9 or higher there won't be any need for these declarations - I will add > a > > > > TODO for that. > > > > > > FWIW I think it's fine to only declare these here, since the likelihood of > > > another file needing them is very small, which is why I didn't suggest this. > > > > > > > > > https://codereview.chromium.org/989703002/diff/280001/base/process/process_ma... > > > File base/process/process_mac.cc (right): > > > > > > > > > https://codereview.chromium.org/989703002/diff/280001/base/process/process_ma... > > > base/process/process_mac.cc:88: if (!CanBackgroundProcesses() || task_port > == > > > TASK_NULL) { > > > Why is |task_port| allowed to be TASK_NULL here but not in > > > IsProcessBackgrounded()? They should both be consistent. Should it be a > > > DCHECK_NE() instead? > > > > I agree they should be consistent. Re: DCHECK_NE() , I don't think so. The > issue > > is that, as you know, the render process's port is not immediately available, > so > > there's a chance that the MachBroker will return TASK_NULL when you ask for > the > > port to pass to process.SetProcessBackgrounded(). Unless you feel that > TASK_NULL > > should be an error and that callees should always check before passing it? I > > lean toward just ignoring any incoming TASK_NULL. > > I don't have a strong preference, but I think from an API perspective, > IsProcessBackgrounded() returning false in the case where the port is NULL is a > bit strange. It really should be reporting a failure, but it is would appear to > be reporting that a process isn't backgrounded. Since there's only one caller > here, I think having a precondition that the port be non-NULL is OK. But that's > not a strong preference, though the two APIs should be consistent. I agree with what you're saying. I have added DCHECK_NE()s and now check for TASK_NULL before calling the API. PTAL.
https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2256: const std::string trial_group_name = On 2015/04/22 21:57:38, gab (OOO soon no new reviews) wrote: > Add a comment above similar to the Windows one (plus launch bug link). Done. https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2258: if (trial_group_name.length() > 0 && trial_group_name != "Disallow") { On 2015/04/22 21:57:38, gab (OOO soon no new reviews) wrote: > s/trial_group_name.length() > 0/!trial_group_name.empty() Done.
LGTM!
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/989703002/#ps320001 (title: "Fix nits, add error checking.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/989703002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thakis, PTAL at and approve my changes to base/(when you get a chance - codereview says you're out sick).
not lgtm for experiment set up change https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/300001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2260: } On 2015/04/22 13:11:34, gab (OOO soon no new reviews) wrote: > Why not just re-use the Windows code for both? > > As-is your experiment will not enabled by default on trunk (where there are no > experiments) and thus will not get perf waterfall coverage. > > i.e. I'd say, just change the Windows ifdef to > #if defined(OS_WIN) || defined(OS_MACOSX) > instead of duplicating the code into a slightly different variant. > > (one thing to be careful with enabling by default on trunk is that you'll to > create configs up to Stable right away to explicitly disable it there before it > gets approval) Agreed, no sense in having two slightly different but logically equivalent codepaths right one after the other. https://codereview.chromium.org/989703002/diff/320001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/320001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2261: if (!trial_group_name.empty() && trial_group_name != "Disallow") { This pattern makes it impossible to have a Control group with the same behavior as the Disallow group. Experimentation best practice suggest having a Control group. This used to be handled by default when the advice was to be "off by default", but now that this is reversed by the new launch process, you need to be more careful when setting this up. Either: 1) Use experiment params rather than checking for the group name. 2) Or change the check to not rely on a specific group name - for example you can use StartsWith() Please make these changes to both the Windows and Mac experiment branches, since they suffer from the same problem.
On 2015/04/23 20:53:28, Alexei Svitkine wrote:
> not lgtm for experiment set up change
> Experimentation best practice suggest having a Control group. This used to be
> handled by default when the advice was to be "off by default", but now that
this
> is reversed by the new launch process, you need to be more careful when
setting
> this up.
According to hodie, default should be off. Same as what the Finch experiment
guide says ("Develop your feature default-off"). Where is what you're describing
documented?
On 2015/04/23 21:30:34, shrike wrote:
> On 2015/04/23 20:53:28, Alexei Svitkine wrote:
> > not lgtm for experiment set up change
> > Experimentation best practice suggest having a Control group. This used to
be
> > handled by default when the advice was to be "off by default", but now that
> this
> > is reversed by the new launch process, you need to be more careful when
> setting
> > this up.
>
> According to hodie, default should be off. Same as what the Finch experiment
> guide says ("Develop your feature default-off"). Where is what you're
describing
> documented?
Unfortunately the set of docs conflict a bit. go/newChromeFeature does currently
have a section that suggests "on by default" - whereas the guide says otherwise.
We need to update the docs and reconcile the two - but in the mean while, it's
important to ensure that whichever approach you take, it should allow for having
a Control group with the same behavior as the non-experimental group.
On 2015/04/23 21:47:00, Alexei Svitkine wrote: > Unfortunately the set of docs conflict a bit. go/newChromeFeature does currently > have a section that suggests "on by default" - whereas the guide says otherwise. > We need to update the docs and reconcile the two - but in the mean while, it's > important to ensure that whichever approach you take, it should allow for having > a Control group with the same behavior as the non-experimental group. I'm not getting your distinctions between experiment groups (Control, Disallow, etc.), and I don't see any documentation on it either. It's not part of the Finch experiment guide, which now states that it's up-to-date. I imagine I can ask hodie what you mean and he might be able to tell me how to structure the experiment. But: > Either: > 1) Use experiment params rather than checking for the group name. > 2) Or change the check to not rely on a specific group name - for example you > can use StartsWith() > > Please make these changes to both the Windows and Mac experiment branches, since > they suffer from the same problem. The Windows side of this experiment has been underway for quite some time, and I think even recently received final approval (for whatever experiments receive final approval for). You are telling me that I need to build out the Mac experiment to have Control/Disallow/blah blah groups, and also change the Windows experiment, which is almost finished, to include a Control group along with his existing four groups (that don't all match up to the Mac experiment)?
To avoid further back and forths: It should be as simple as changing group != "Disabled" to !StartsWith(group, "Disabled"). This allows "DisabledControl" group to have same behavior as "Disabled". On Apr 23, 2015 5:59 PM, <shrike@chromium.org> wrote: > On 2015/04/23 21:47:00, Alexei Svitkine wrote: > >> Unfortunately the set of docs conflict a bit. go/newChromeFeature >> <https://goto.google.com/newChromeFeature> does >> > currently > >> have a section that suggests "on by default" - whereas the guide says >> > otherwise. > >> We need to update the docs and reconcile the two - but in the mean while, >> it's >> important to ensure that whichever approach you take, it should allow for >> > having > >> a Control group with the same behavior as the non-experimental group. >> > > I'm not getting your distinctions between experiment groups (Control, > Disallow, > etc.), and I don't see any documentation on it either. It's not part of the > Finch experiment guide, which now states that it's up-to-date. I imagine I > can > ask hodie what you mean and he might be able to tell me how to structure > the > experiment. But: > > Either: >> 1) Use experiment params rather than checking for the group name. >> 2) Or change the check to not rely on a specific group name - for >> example >> > you > >> can use StartsWith() >> > > Please make these changes to both the Windows and Mac experiment branches, >> > since > >> they suffer from the same problem. >> > > The Windows side of this experiment has been underway for quite some time, > and I > think even recently received final approval (for whatever experiments > receive > final approval for). You are telling me that I need to build out the Mac > experiment to have Control/Disallow/blah blah groups, and also change the > Windows experiment, which is almost finished, to include a Control group > along > with his existing four groups (that don't all match up to the Mac > experiment)? > > https://codereview.chromium.org/989703002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Just wondering what's the status of this? I don't want progress to be blocked on due to the experiment set up code - so if that's currently the blocker, happy to work with you to get this resolved. As I mentioned in a previous comment, I think the change I'm suggesting should be as simple as changing the check: trial_group_name != "Disallow" to !StartsWith(trial_group_name, "Disallow")
Note that PK seemed to have issues with this on Win after it landed, pointing the figure at priority inversion. We should keep an eye out for Mac. On Sat, May 2, 2015 at 9:01 AM, <asvitkine@chromium.org> wrote: > Just wondering what's the status of this? > > I don't want progress to be blocked on due to the experiment set up code - > so if > that's currently the blocker, happy to work with you to get this resolved. > > As I mentioned in a previous comment, I think the change I'm suggesting > should > be as simple as changing the check: > > trial_group_name != "Disallow" > > to > > !StartsWith(trial_group_name, "Disallow") > > https://codereview.chromium.org/989703002/ > -- Mike Pinkerton Mac Weenie pinkerton@google.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/02 13:01:07, Alexei Svitkine wrote: > Just wondering what's the status of this? > > I don't want progress to be blocked on due to the experiment set up code - so if > that's currently the blocker, happy to work with you to get this resolved. > > As I mentioned in a previous comment, I think the change I'm suggesting should > be as simple as changing the check: > > trial_group_name != "Disallow" > > to > > !StartsWith(trial_group_name, "Disallow") Hello asvitkine, I will make this change, but I do not understand a lot of what you said about needing to do this to allow for a control group, etc., etc. It would be great to get a pointer to a document that expounds on it all (I am hoping my Finch ambassador will be able to answer my many questions about the experiment process and setup). That said, I am nervous about making this change as it will change code in an experiment that is actively running on a different platform (Windows). But if you are directing me to do so I will assume you see no risk with that. The real blocker, however, is your directive to combine my #if defined(OS_MACOSX) code with the #if defined(OS_WIN) code to create a single section of #if defined(OS_MACOSX) || defined(OS_WIN) code. The Windows code has the experiment default enabled; mine has the experiment default disabled. The combined version will have to have the experiment default enabled, which means I cannot land any part of this change until I get the Mac side of the experiment completely defined (so that it doesn't immediately go live when I land the cl). I have filed a bug to launch the Mac version of this experiment but didn't make much progress on it last week (I will work on pushing it forward today/this week).
Thanks for following up. I can be your ambassador and happy to chat or vc to clarify things. I can guarantee that the StartsWith() change is completely safe. It won't change any behavior for the current Windows experiments (if you look at the server-side configs, there's only a single "Disallow" group currently - so matching will still be the same). It just allows for more flexibility in the future. As for not combining the two parts - I buy your concerns about not wanting to turn this on by default on Mac at this time, so I'm OK with keeping them separate. But please at least use the same APIs in both code paths - currently one code path uses FindFullName() and the other one uses Find() - whereas the same logic can be easily expressed in either of the APIs. Generally, we recommend FindFullName() - so I suggest switching the Windows codepath to use it to (!trial in the old code is equivalent to trial_name == "" with FindFullName()). Like this StartsWith() suggestion, this change is safe to make - it won't change behavior of the Windows code. On Mon, May 4, 2015 at 2:03 PM, <shrike@chromium.org> wrote: > On 2015/05/02 13:01:07, Alexei Svitkine wrote: > >> Just wondering what's the status of this? >> > > I don't want progress to be blocked on due to the experiment set up code >> - so >> > if > >> that's currently the blocker, happy to work with you to get this resolved. >> > > As I mentioned in a previous comment, I think the change I'm suggesting >> should >> be as simple as changing the check: >> > > trial_group_name != "Disallow" >> > > to >> > > !StartsWith(trial_group_name, "Disallow") >> > > Hello asvitkine, > > I will make this change, but I do not understand a lot of what you said > about > needing to do this to allow for a control group, etc., etc. It would be > great to > get a pointer to a document that expounds on it all (I am hoping my Finch > ambassador will be able to answer my many questions about the experiment > process > and setup). > > That said, I am nervous about making this change as it will change code in > an > experiment that is actively running on a different platform (Windows). But > if > you are directing me to do so I will assume you see no risk with that. > > The real blocker, however, is your directive to combine my #if > defined(OS_MACOSX) code with the #if defined(OS_WIN) code to create a > single > section of #if defined(OS_MACOSX) || defined(OS_WIN) code. The Windows > code has > the experiment default enabled; mine has the experiment default disabled. > The > combined version will have to have the experiment default enabled, which > means I > cannot land any part of this change until I get the Mac side of the > experiment > completely defined (so that it doesn't immediately go live when I land the > cl). > I have filed a bug to launch the Mac version of this experiment but didn't > make > much progress on it last week (I will work on pushing it forward today/this > week). > > > > https://codereview.chromium.org/989703002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/04 18:12:59, Alexei Svitkine wrote: > Thanks for following up. I can be your ambassador and happy to chat or vc > to clarify things. I'd welcome your help (and I guess I should tell hodie that you've volunteered to be my ambassador?). You mention having a Control group - does that mean the experiment should divide the users into three buckets: Control, Disallow, and Allow? I understand why we need a Control group, but why isn't it the same as the Disallow group?
The best practice is to have the Control group be the same size as your
experiment group.
So (as an example) 5% Allow and 95% Disallow doesn't match that best
practice.
In such a case, you'd want 5% Allow, 5% Control and 90% Disallow. But your
code wouldn't support having a 5% Control that behaves the same as
Disallow. By changing to StartsWith("Disallow"), it lets you name your
control group Disallow_Control - which can support the above use case.
On Tue, May 5, 2015 at 2:59 PM, <shrike@chromium.org> wrote:
> On 2015/05/04 18:12:59, Alexei Svitkine wrote:
>
>> Thanks for following up. I can be your ambassador and happy to chat or vc
>> to clarify things.
>>
>
> I'd welcome your help (and I guess I should tell hodie that you've
> volunteered
> to be my ambassador?).
>
> You mention having a Control group - does that mean the experiment should
> divide
> the users into three buckets: Control, Disallow, and Allow? I understand
> why we
> need a Control group, but why isn't it the same as the Disallow group?
>
>
> https://codereview.chromium.org/989703002/
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
Hello asvitkine, I have combined the code that enables the experiment on Mac and on Windows in render_process_host_impl.cc. PTAL.
lgtm
Still lgtm, but two nits below. Thanks, Gab https://codereview.chromium.org/989703002/diff/340001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/340001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2230: if (!trial || !StartsWithASCII(trial->group_name(), "Disallow", false)) { Use case-sensitive? i.e. s/false/true? Case-sensitive string comparison is cheaper than non-case-sensitive and I don't see why we would need the flexibility of the latter. https://codereview.chromium.org/989703002/diff/340001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2232: } Why add the {}? It's still a single line conditional, single line body.
https://codereview.chromium.org/989703002/diff/340001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/340001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2230: if (!trial || !StartsWithASCII(trial->group_name(), "Disallow", false)) { On 2015/05/11 17:20:12, gab wrote: > Use case-sensitive? i.e. s/false/true? > > Case-sensitive string comparison is cheaper than non-case-sensitive and I don't > see why we would need the flexibility of the latter. Fixed. I wasn't actually sure what the right answer was here. I guess I was thinking that a case typo would enable the experiment, which is not what we want. https://codereview.chromium.org/989703002/diff/340001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2232: } On 2015/05/11 17:20:12, gab wrote: > Why add the {}? It's still a single line conditional, single line body. Yes, that is true (the style guide says either is OK). I was editing that if statement and I always include braces on if statements (I just think it's a safer way to code).
lgtm, thanks! Let's make sure to land this before the upcoming M44 branch point.
On 2015/05/14 16:12:27, gab (slow) wrote: > lgtm, thanks! > > Let's make sure to land this before the upcoming M44 branch point. Can you help me with next steps in the launch bug? I need a bunch of approvals to proceed.
Getting the approvals shouldn't block landing the CL, I think. On May 14, 2015 1:02 PM, <shrike@chromium.org> wrote: > On 2015/05/14 16:12:27, gab (slow) wrote: > >> lgtm, thanks! >> > > Let's make sure to land this before the upcoming M44 branch point. >> > > Can you help me with next steps in the launch bug? I need a bunch of > approvals > to proceed. > > https://codereview.chromium.org/989703002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/14 17:29:06, Alexei Svitkine wrote: > Getting the approvals shouldn't block landing the CL, I think. Correct, you can land the CL now (and follow-up with Finch configs to explicitly disable the experiment everywhere you don't have approval -- i.e., everywhere but Canary which can be 50/50 without approval). Making sure this CL lands for M44 is *much* more important than dealing with the approvals and configs at this point (this code is not about to hit Stable so we have time to get that right in the following days). Thanks, Gab > On May 14, 2015 1:02 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=shrike@chromium.org> wrote: > > > On 2015/05/14 16:12:27, gab (slow) wrote: > > > >> lgtm, thanks! > >> > > > > Let's make sure to land this before the upcoming M44 branch point. > >> > > > > Can you help me with next steps in the launch bug? I need a bunch of > > approvals > > to proceed. > > > > https://codereview.chromium.org/989703002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, avi@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/989703002/#ps360001 (title: "Change string compare to be case sensitive.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/989703002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I thought I had all the approvals I needed to land this cl. thakis, please approve my changes to base. Thank you.
base lgtm. Thanks for adding that comment to the .h file! https://codereview.chromium.org/989703002/diff/360001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/989703002/diff/360001/base/process/process.h#... base/process/process.h:112: // and there's no good way to get that from base:: given the pid. These s/:://
https://codereview.chromium.org/989703002/diff/360001/base/process/process.h File base/process/process.h (right): https://codereview.chromium.org/989703002/diff/360001/base/process/process.h#... base/process/process.h:112: // and there's no good way to get that from base:: given the pid. These On 2015/05/15 17:26:55, Nico wrote: > s/::// Done.
The CQ bit was checked by shrike@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, gab@chromium.org, thakis@chromium.org, asvitkine@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/989703002/#ps380001 (title: "Edit a comment.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/989703002/380001
The CQ bit was unchecked by shrike@chromium.org
The CQ bit was checked by shrike@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/989703002/380001
shrike@chromium.org changed reviewers: + kbr@chromium.org
Hello kbr, This change allows the browser to background renderer processes that are not in the foreground. I'm trying to get this change committed for M44 (as part of CP) but It is failing on one of the "context_lost" tests. The issue appears to be that the test is running in a tab in a window that is not the "key" window. Because the window is not key, the browser is backgrouding the process, and as a result the test is taking a little longer to complete (25s vs. the 20s timeout in the test). In fact, when I run the test on my machine and click the window to make it key during this test, the test succeeds. I propose increasing the timeout for context_lost tests to 27s (25s + a little cushion). Please let me know if this is OK, or suggest a different change. Thank you!
content/test/gpu/ lgtm with caveat. +zmo, current GPU pixel wrangler and jmadill, next week's wrangler as FYI. https://codereview.chromium.org/989703002/diff/400001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/context_lost.py (right): https://codereview.chromium.org/989703002/diff/400001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost.py:19: wait_timeout = 27 # seconds If the browser windows that Telemetry opens aren't made the key window, it sounds to me like all of the GPU tests will run more slowly. I also wonder whether other test suites that open windows may be affected. Could you please: - Watch this test suite and increase the timeout further (up to 60 seconds, say) if the test becomes flaky - File a follow-on bug to investigate why the newly opened window isn't being made key Thanks.
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, gab@chromium.org, thakis@chromium.org, asvitkine@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/989703002/#ps400001 (title: "Increase timeout in context_lost test suite")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/989703002/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:400001) has been created in https://codereview.chromium.org/1128183011/ by thakis@chromium.org. The reason for reverting is: Speculative; several browser_tests started failing on the Mac 10.9 bots, and this looks like the most likely culprit: http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/1929 http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29....
Message was sent while issue was closed.
(AutomationApiTest.Events is one of the failing tests; for debugging)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/e3bb10f7860a1d553c85293bd7d7615c0e7f0fd9 Cr-Commit-Position: refs/heads/master@{#330275}
Message was sent while issue was closed.
rsesak, this revision changes the QOS settings for the non-backgrounded state in base/process/process_mac.cc, from XXX_DEFAULT_TIER to XXX_TIER_UNSPECIFIED (in doing some debugging it appears that processes are given an "unspecified" QOS setting when created, not default as I had assumed). PTAL
LGTM still
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, avi@chromium.org, gab@chromium.org, thakis@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/989703002/#ps420001 (title: "Fix incorrect QOS setting.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/989703002/420001
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/0160d130f8a4462fa7bfb8a9924e476d31ba9a48 Cr-Commit-Position: refs/heads/master@{#330464}
Message was sent while issue was closed.
A revert of this CL (patchset #22 id:420001) has been created in https://codereview.chromium.org/1142913004/ by tnagel@chromium.org. The reason for reverting is: This CL seems to be the cause of ExtensionApiNewTabTest.Tabs failures on Mac. .
Message was sent while issue was closed.
On 2015/05/19 14:25:48, Thiemo Nagel wrote: > A revert of this CL (patchset #22 id:420001) has been created in > https://codereview.chromium.org/1142913004/ by https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=tnagel@chromium.org. > > The reason for reverting is: This CL seems to be the cause of > ExtensionApiNewTabTest.Tabs failures on Mac. > . ping, what's the status of this? Backgrounding appears to help quite a bit on Windows, can't wait to see this on Mac :-) Thanks, Gab
Message was sent while issue was closed.
So far this has landed twice and got reverted twice. On Thu, May 21, 2015 at 2:21 PM, <gab@chromium.org> wrote: > On 2015/05/19 14:25:48, Thiemo Nagel wrote: > >> A revert of this CL (patchset #22 id:420001) has been created in >> https://codereview.chromium.org/1142913004/ by >> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=tnagel@chromium.org. > > The reason for reverting is: This CL seems to be the cause of >> ExtensionApiNewTabTest.Tabs failures on Mac. >> . >> > > ping, what's the status of this? Backgrounding appears to help quite a bit > on > Windows, can't wait to see this on Mac :-) > > Thanks, > Gab > > https://codereview.chromium.org/989703002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/05/21 21:37:00, Nico wrote: > So far this has landed twice and got reverted twice. Indeed, but we should keep at it, I think this is a key feature. > > On Thu, May 21, 2015 at 2:21 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > > On 2015/05/19 14:25:48, Thiemo Nagel wrote: > > > >> A revert of this CL (patchset #22 id:420001) has been created in > >> https://codereview.chromium.org/1142913004/ by > >> > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=tnagel@chromium.org. > > > > The reason for reverting is: This CL seems to be the cause of > >> ExtensionApiNewTabTest.Tabs failures on Mac. > >> . > >> > > > > ping, what's the status of this? Backgrounding appears to help quite a bit > > on > > Windows, can't wait to see this on Mac :-) > > > > Thanks, > > Gab > > > > https://codereview.chromium.org/989703002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
Message was sent while issue was closed.
On Tue, May 26, 2015 at 12:22 PM, <gab@chromium.org> wrote: > On 2015/05/21 21:37:00, Nico wrote: > >> So far this has landed twice and got reverted twice. >> > > Indeed, but we should keep at it, I think this is a key feature. If you want to help debugging, I'm sure shrike would be glad :-) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/05/26 19:22:56, gab wrote: > Indeed, but we should keep at it, I think this is a key feature. I am keeping at it - I worked on it last week and again this morning to understand the scope of the problem and to come up with workaround options. The test that's failing is the browser test ExtensionApiNewTabTest.Tabs, which itself is divided up into subtests written in JavaScript. At the start of the subtests the extension receives a RenderWidgetHostImpl::WasHidden() message which backgrounds its renderer, and the renderer stays backgrounded for the majority of the test. The lowered priority causes the entire test to complete in about 35s vs. ~11s previously. The timeout is set at 30s, so the test fails. It doesn't appear possible to increase the timeout for this one test on the Mac - TestTimeouts::Initialize() gets called early in the test startup process, so by the time the browser test gets the chance to set a command line value for switches::kUiTestActionMaxTimeout it's too late. One option would be to change the timeout of all tests from 30s to 45s - but that would allow regressions in other tests to not get caught. Another option might be to disable process backgrounding within Process:: SetProcessBackgrounded() - seems kind of hacky, though. Any thoughts or suggestions?
Message was sent while issue was closed.
I think disabling backgrounding for this test (and filing a bug to track it) is an ok workaround. The test stays enabled and there's no blanket raising of the timeout. This will allow users to benefit without forcing a full-blown solution just because of one test. Does anyone think that's a bad plan? On Tue, May 26, 2015 at 5:41 PM, <shrike@chromium.org> wrote: > On 2015/05/26 19:22:56, gab wrote: > >> Indeed, but we should keep at it, I think this is a key feature. >> > > I am keeping at it - I worked on it last week and again this morning to > understand the scope of the problem and to come up with workaround options. > > The test that's failing is the browser test ExtensionApiNewTabTest.Tabs, > which > itself is divided up into subtests written in JavaScript. At the start of > the > subtests the extension receives a RenderWidgetHostImpl::WasHidden() message > which backgrounds its renderer, and the renderer stays backgrounded for the > majority of the test. The lowered priority causes the entire test to > complete in > about 35s vs. ~11s previously. The timeout is set at 30s, so the test > fails. > > It doesn't appear possible to increase the timeout for this one test on > the Mac > - TestTimeouts::Initialize() gets called early in the test startup > process, so > by the time the browser test gets the chance to set a command line value > for > switches::kUiTestActionMaxTimeout it's too late. > > One option would be to change the timeout of all tests from 30s to 45s - > but > that would allow regressions in other tests to not get caught. > > Another option might be to disable process backgrounding within Process:: > SetProcessBackgrounded() - seems kind of hacky, though. > > Any thoughts or suggestions? > > > https://codereview.chromium.org/989703002/ > -- Mike Pinkerton Mac Weenie pinkerton@google.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
That makes sense to me too. On May 27, 2015 6:40 AM, "Mike Pinkerton" <pinkerton@chromium.org> wrote: > I think disabling backgrounding for this test (and filing a bug to track > it) is an ok workaround. The test stays enabled and there's no blanket > raising of the timeout. > > This will allow users to benefit without forcing a full-blown solution > just because of one test. > > Does anyone think that's a bad plan? > > On Tue, May 26, 2015 at 5:41 PM, <shrike@chromium.org> wrote: > >> On 2015/05/26 19:22:56, gab wrote: >> >>> Indeed, but we should keep at it, I think this is a key feature. >>> >> >> I am keeping at it - I worked on it last week and again this morning to >> understand the scope of the problem and to come up with workaround >> options. >> >> The test that's failing is the browser test ExtensionApiNewTabTest.Tabs, >> which >> itself is divided up into subtests written in JavaScript. At the start of >> the >> subtests the extension receives a RenderWidgetHostImpl::WasHidden() >> message >> which backgrounds its renderer, and the renderer stays backgrounded for >> the >> majority of the test. The lowered priority causes the entire test to >> complete in >> about 35s vs. ~11s previously. The timeout is set at 30s, so the test >> fails. >> >> It doesn't appear possible to increase the timeout for this one test on >> the Mac >> - TestTimeouts::Initialize() gets called early in the test startup >> process, so >> by the time the browser test gets the chance to set a command line value >> for >> switches::kUiTestActionMaxTimeout it's too late. >> >> One option would be to change the timeout of all tests from 30s to 45s - >> but >> that would allow regressions in other tests to not get caught. >> >> Another option might be to disable process backgrounding within Process:: >> SetProcessBackgrounded() - seems kind of hacky, though. >> >> Any thoughts or suggestions? >> >> >> https://codereview.chromium.org/989703002/ >> > > > > -- > Mike Pinkerton > Mac Weenie > pinkerton@google.com > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+1
https://codereview.chromium.org/989703002/diff/440001/base/process/process_ma... File base/process/process_mac.cc (right): https://codereview.chromium.org/989703002/diff/440001/base/process/process_ma... base/process/process_mac.cc:101: "ExtensionApiNewTabTest.Tabs") { That's not a very common way to disable things for a test from my experience. Typically a switch is added to Chrome to generically toggle the feature off (I would recommend having that switch control the cross-platform code the same way the experiment does) and the test fixture adds that switch to itself by overidding SetupCommandLine().
On 2015/05/28 13:58:10, gab wrote: > https://codereview.chromium.org/989703002/diff/440001/base/process/process_ma... > File base/process/process_mac.cc (right): > > https://codereview.chromium.org/989703002/diff/440001/base/process/process_ma... > base/process/process_mac.cc:101: "ExtensionApiNewTabTest.Tabs") { > That's not a very common way to disable things for a test from my experience. > > Typically a switch is added to Chrome to generically toggle the feature off (I > would recommend having that switch control the cross-platform code the same way > the experiment does) and the test fixture adds that switch to itself by > overidding SetupCommandLine(). Thanks for the tip. PTAL. thakis - PTAL as well because of my changes to base (adding the switch; it governs code in process_mac.cc so I figure adding it to base_switches was the right thing to do).
lgtm++, thanks :-)! https://codereview.chromium.org/989703002/diff/520001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/520001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2212: } nit: Avoid {} (I know you like putting it all the time, but in this case it's actually inconsistent with surrounding code which is the main Chromium style guide rule)
base/ diff relative to patch set 22 lgtm
LGTM Good luck, this will be good.
https://codereview.chromium.org/989703002/diff/520001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/989703002/diff/520001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2212: } On 2015/06/01 19:34:20, gab wrote: > nit: Avoid {} (I know you like putting it all the time, but in this case it's > actually inconsistent with surrounding code which is the main Chromium style > guide rule) It feels wrong, but you are right. Fixed.
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, asvitkine@chromium.org, rsesek@chromium.org, gab@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/989703002/#ps540001 (title: "Fix nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/989703002/540001
Message was sent while issue was closed.
Committed patchset #28 (id:540001)
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/8fbe9d3eb4ab56b0be87c3c4e04bb544ede982c7 Cr-Commit-Position: refs/heads/master@{#332454} |
