Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2197663002:
Prerender: Allow on low end devices for ORIGIN_OFFLINE (Closed)
|
Created:
4 years, 4 months ago by pasko Modified:
4 years, 4 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrerender: Allow on low end devices for ORIGIN_OFFLINE
TL;DR: Prerendering cannot keep track of all child processes, and only allows
prerendering when asked by the offliner.
Low end devices require (best effort at) keeping no more than one renderer
process at any time. Prerendering has been disabled on low end devices because
of this.
Offliner should be able to create prerenders, and it needs to ensure that
these prerenders are:
* not created when another renderer is alive
* killed when Chrome another renderer is created
We identified additional cases when to prevent offlining, among which:
* Chrome is in foreground (even though may not have a renderer)
* Battery level is low
Since the foreground checks should be in place anyway, we decided that while
Chrome is in background having two background renderers is OK, and we can rely
on one (or both) of them being killed by the system. For simplicity the
offliner does not actually watch for renderers specifically, only:
* Prevents offlining when any Chrome activity is visible
* Kills its prerenders when a transition to foreground happens
BUG=622508
Committed: https://crrev.com/63b429af6ac2e1a0835e30ddd962fc4ebad2cd1b
Cr-Commit-Position: refs/heads/master@{#409528}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Introduced PrerenderManager::IsLowEndDevice() #Patch Set 3 : git cl format #
Total comments: 7
Patch Set 4 : Addressed comments by mmenke@ #
Total comments: 1
Messages
Total messages: 37 (15 generated)
The CQ bit was checked by pasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pasko@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: please take a look Apologies for the document (go/background-loading-on-svelte) with details and discussion being internal, I asked for making it public.
Description was changed from ========== Prerender: Allow on low end devices for ORIGIN_OFFLINE TL;DR: Prerendering cannot keep track of all child processes, and only allows prerendering when asked by the offliner. Low end devices require (best effort at) keeping no more than one renderer process at any time. Prerendering has been disabled on low end devices because of this. Offliner should be able to create prerenders, and it needs to ensure that these prerenders are: * not created when another renderer is alive * killed when Chrome another renderer is created We identified additional cases when to prevent offlining, among which: * Chrome is in foreground (even though may not have a renderer) * Battery level is low Since the foreground checks should be in place anyway, we decided that while Chrome is in background having two background renderers is OK, and we can rely on one (or both) of them being killed by the system. For simplicity the offliner does not actually watch for renderers specifically, only: * Prevents offlining when any Chrome activity is visible * Kills its prerenders when a transition to foreground happens BUG=622508 ========== to ========== Prerender: Allow on low end devices for ORIGIN_OFFLINE TL;DR: Prerendering cannot keep track of all child processes, and only allows prerendering when asked by the offliner. Low end devices require (best effort at) keeping no more than one renderer process at any time. Prerendering has been disabled on low end devices because of this. Offliner should be able to create prerenders, and it needs to ensure that these prerenders are: * not created when another renderer is alive * killed when Chrome another renderer is created We identified additional cases when to prevent offlining, among which: * Chrome is in foreground (even though may not have a renderer) * Battery level is low Since the foreground checks should be in place anyway, we decided that while Chrome is in background having two background renderers is OK, and we can rely on one (or both) of them being killed by the system. For simplicity the offliner does not actually watch for renderers specifically, only: * Prevents offlining when any Chrome activity is visible * Kills its prerenders when a transition to foreground happens BUG=622508 ==========
pasko@chromium.org changed reviewers: + dougarnett@chromium.org
dougarnett: FYI
https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:450: EXPECT_FALSE(AddSimplePrerender(url)); I assume most of the other tests now fail on low end devices. We should make the other tests pass on them, right?
https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:450: EXPECT_FALSE(AddSimplePrerender(url)); On 2016/07/29 15:57:09, mmenke wrote: > I assume most of the other tests now fail on low end devices. We should make > the other tests pass on them, right? This test "fakes" running on low end (i.e. svelte) with the FieldTrial, it is not intended to run on svelte. I believe we do not run unit_tests on actual svelte devices. I know that we test the svelte configuration by modifying flags on non-svelte devices. Svelte perf bots were also removed not long ago. I think we do not run *anything* on svelte devices right now :( Could be my wrong interpretation.
https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:450: EXPECT_FALSE(AddSimplePrerender(url)); On 2016/07/29 16:10:00, pasko wrote: > On 2016/07/29 15:57:09, mmenke wrote: > > I assume most of the other tests now fail on low end devices. We should make > > the other tests pass on them, right? > > This test "fakes" running on low end (i.e. svelte) with the FieldTrial, it is > not intended to run on svelte. > > I believe we do not run unit_tests on actual svelte devices. I know that we test > the svelte configuration by modifying flags on non-svelte devices. Svelte perf > bots were also removed not long ago. I think we do not run *anything* on svelte > devices right now :( > > Could be my wrong interpretation. Just because none of the bots run on svelte devices surely doesn't mean we shouldn't support running unit tests on them, no?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_unittest.cc:450: EXPECT_FALSE(AddSimplePrerender(url)); On 2016/07/29 16:12:27, mmenke wrote: > On 2016/07/29 16:10:00, pasko wrote: > > On 2016/07/29 15:57:09, mmenke wrote: > > > I assume most of the other tests now fail on low end devices. We should > make > > > the other tests pass on them, right? > > > > This test "fakes" running on low end (i.e. svelte) with the FieldTrial, it is > > not intended to run on svelte. > > > > I believe we do not run unit_tests on actual svelte devices. I know that we > test > > the svelte configuration by modifying flags on non-svelte devices. Svelte perf > > bots were also removed not long ago. I think we do not run *anything* on > svelte > > devices right now :( > > > > Could be my wrong interpretation. > > Just because none of the bots run on svelte devices surely doesn't mean we > shouldn't support running unit tests on them, no? I disagree. Chrome is a large project such that when some configuration is not covered by regular testing, we are almost sure it is broken in numerous ways, many of these ways are usually unrelated to what the tests actually check for. Given that, the policy has been: "If there is a configuration we care about, it must be regularly tested on chrome infra bots." In particular, with testing on Svelte there would be flakiness problems caused by the system evicting our processes more aggressively, not all tests fitting in memory, etc. Deciding whether we need svelte bots "enough" is outside of the scope of this change, I argued for more svelte bots for perf in the past, and my arguments were not heard. Here I am not willing to fix the potential stack of problems with Svelte, and I do not want to add complexity to the code without being able to test it. Even if I do it properly with my local device and everything, it will rot faster than I will return to play with this test again.
On 2016/07/29 17:25:03, pasko wrote: > https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... > File chrome/browser/prerender/prerender_unittest.cc (right): > > https://codereview.chromium.org/2197663002/diff/1/chrome/browser/prerender/pr... > chrome/browser/prerender/prerender_unittest.cc:450: > EXPECT_FALSE(AddSimplePrerender(url)); > On 2016/07/29 16:12:27, mmenke wrote: > > On 2016/07/29 16:10:00, pasko wrote: > > > On 2016/07/29 15:57:09, mmenke wrote: > > > > I assume most of the other tests now fail on low end devices. We should > > make > > > > the other tests pass on them, right? > > > > > > This test "fakes" running on low end (i.e. svelte) with the FieldTrial, it > is > > > not intended to run on svelte. > > > > > > I believe we do not run unit_tests on actual svelte devices. I know that we > > test > > > the svelte configuration by modifying flags on non-svelte devices. Svelte > perf > > > bots were also removed not long ago. I think we do not run *anything* on > > svelte > > > devices right now :( > > > > > > Could be my wrong interpretation. > > > > Just because none of the bots run on svelte devices surely doesn't mean we > > shouldn't support running unit tests on them, no? > > I disagree. > > Chrome is a large project such that when some configuration is not covered by > regular testing, we are almost sure it is broken in numerous ways, many of these > ways are usually unrelated to what the tests actually check for. Given that, the > policy has been: "If there is a configuration we care about, it must be > regularly tested on chrome infra bots." > > In particular, with testing on Svelte there would be flakiness problems caused > by the system evicting our processes more aggressively, not all tests fitting in > memory, etc. > > Deciding whether we need svelte bots "enough" is outside of the scope of this > change, I argued for more svelte bots for perf in the past, and my arguments > were not heard. > > Here I am not willing to fix the potential stack of problems with Svelte, and I > do not want to add complexity to the code without being able to test it. Even if > I do it properly with my local device and everything, it will rot faster than I > will return to play with this test again. I'm not arguing we should set up svelte bots. I am arguing that developers with svelte devices should be able to run tests on them. I'm arguing we should not depend on which particular Android device we're running. I'm not comfortable signing off on tests that don't work on some arbitrary subset of devices.
On 2016/07/29 18:13:27, mmenke wrote: > On 2016/07/29 17:25:03, pasko wrote: > > Chrome is a large project such that when some configuration is not covered > > by regular testing, we are almost sure it is broken in numerous ways, many > > of these ways are usually unrelated to what the tests actually check for. > > Given that, the policy has been: "If there is a configuration we care about, > > it must be regularly tested on chrome infra bots." > > > > In particular, with testing on Svelte there would be flakiness problems > > caused by the system evicting our processes more aggressively, not all tests > > fitting in memory, etc. > > > > Deciding whether we need svelte bots "enough" is outside of the scope of > > this change, I argued for more svelte bots for perf in the past, and my > > arguments were not heard. > > > > Here I am not willing to fix the potential stack of problems with Svelte, > > and I do not want to add complexity to the code without being able to test > > it. Even if I do it properly with my local device and everything, it will > > rot faster than I will return to play with this test again. > > I'm not arguing we should set up svelte bots. I am arguing that developers > with svelte devices should be able to run tests on them. I'm arguing we > should not depend on which particular Android device we're running. I'm not > comfortable signing off on tests that don't work on some arbitrary subset of > devices. Do you have instructions on how to run prerender_unittest on Android? I could not find it on any of the waterfalls. Also, proper Svelte devices are hard to find for developers (10x harder than normal builds?). Given the above, it becomes really unlikely that any Chrome developer would hit this problem. So the benefit is low, how big is the effort? Let's see: * Flush the Svelte build to a Nexus device and wait, also flush it back at some point to something I can test performance with. (I don't think many external developers encountered these builds, they would likely need real Svelte devices to proceed) * Package PrerenderTest so that it can run on Android (probably with unit_tests, I am not sure), and probably *not* land the patch because cycling time on these bots is critical. * Make the new test run properly on Svelte * Fix other PrerenderTest cases Is the above correct? Given moderate effort and small benefits, this does not seem worth it.
On 2016/08/01 08:22:46, pasko wrote: > On 2016/07/29 18:13:27, mmenke wrote: > > On 2016/07/29 17:25:03, pasko wrote: > > > Chrome is a large project such that when some configuration is not covered > > > by regular testing, we are almost sure it is broken in numerous ways, many > > > of these ways are usually unrelated to what the tests actually check for. > > > Given that, the policy has been: "If there is a configuration we care about, > > > it must be regularly tested on chrome infra bots." > > > > > > In particular, with testing on Svelte there would be flakiness problems > > > caused by the system evicting our processes more aggressively, not all tests > > > fitting in memory, etc. > > > > > > Deciding whether we need svelte bots "enough" is outside of the scope of > > > this change, I argued for more svelte bots for perf in the past, and my > > > arguments were not heard. > > > > > > Here I am not willing to fix the potential stack of problems with Svelte, > > > and I do not want to add complexity to the code without being able to test > > > it. Even if I do it properly with my local device and everything, it will > > > rot faster than I will return to play with this test again. > > > > I'm not arguing we should set up svelte bots. I am arguing that developers > > with svelte devices should be able to run tests on them. I'm arguing we > > should not depend on which particular Android device we're running. I'm not > > comfortable signing off on tests that don't work on some arbitrary subset of > > devices. > > Do you have instructions on how to run prerender_unittest on Android? I could > not find it on any of the waterfalls. Also, proper Svelte devices are hard to > find for developers (10x harder than normal builds?). There is no prerender_unittest project. THey're part of Chrome's unit_tests project. https://chromium.googlesource.com/chromium/src/+/master/docs/android_test_ins... has instructions on running gtest tests (They look different from when I last ran net_unittests on Android. I've had to do this before to debug Android-only issues). > Given the above, it becomes really unlikely that any Chrome developer would hit > this problem. > > So the benefit is low, how big is the effort? Let's see: > * Flush the Svelte build to a Nexus device and wait, also flush it back at > some point to something I can test performance with. (I don't think many > external developers encountered these builds, they would likely need real > Svelte devices to proceed) I'm not following this. > * Package PrerenderTest so that it can run on Android (probably with unit_tests, > I am not sure), and probably *not* land the patch because cycling time on > these bots is critical. Per above, they're already built on Android. If you don't believe that, insert something that doesn't compile, and try to run it through the bots. Android one should fail like most of the others. > * Make the new test run properly on Svelte > * Fix other PrerenderTest cases None of the PrerenderTests current have an issue on Svelte. > Is the above correct? Given moderate effort and small benefits, this does not > seem worth it. Seems fairly small effort to not have an obvious issue with these devices - I assume there's some way to make SysInfo::IsLowEndDevice return false. Add that to the test fixture, and then have the test for the test that needs it to return true use a different test fixture that makes it return true. None of the prerender tests currently have a dependency on the SysInfo of the platform they're running on, and I'm loathe to introduce one, when there's an easy workaround.
On 2016/08/01 14:53:58, mmenke wrote: > On 2016/08/01 08:22:46, pasko wrote: > > On 2016/07/29 18:13:27, mmenke wrote: > > > On 2016/07/29 17:25:03, pasko wrote: > > > > Chrome is a large project such that when some configuration is not covered > > > > by regular testing, we are almost sure it is broken in numerous ways, many > > > > of these ways are usually unrelated to what the tests actually check for. > > > > Given that, the policy has been: "If there is a configuration we care > about, > > > > it must be regularly tested on chrome infra bots." > > > > > > > > In particular, with testing on Svelte there would be flakiness problems > > > > caused by the system evicting our processes more aggressively, not all > tests > > > > fitting in memory, etc. > > > > > > > > Deciding whether we need svelte bots "enough" is outside of the scope of > > > > this change, I argued for more svelte bots for perf in the past, and my > > > > arguments were not heard. > > > > > > > > Here I am not willing to fix the potential stack of problems with Svelte, > > > > and I do not want to add complexity to the code without being able to test > > > > it. Even if I do it properly with my local device and everything, it will > > > > rot faster than I will return to play with this test again. > > > > > > I'm not arguing we should set up svelte bots. I am arguing that developers > > > with svelte devices should be able to run tests on them. I'm arguing we > > > should not depend on which particular Android device we're running. I'm not > > > comfortable signing off on tests that don't work on some arbitrary subset of > > > devices. > > > > Do you have instructions on how to run prerender_unittest on Android? I could > > not find it on any of the waterfalls. Also, proper Svelte devices are hard to > > find for developers (10x harder than normal builds?). > > There is no prerender_unittest project. THey're part of Chrome's unit_tests > project. > https://chromium.googlesource.com/chromium/src/+/master/docs/android_test_ins... > has instructions on running gtest tests (They look different from when I last > ran net_unittests on Android. I've had to do this before to debug Android-only > issues). My point is that I do not see it run anywhere on the waterfall. For example, this link should show all tests being run on the device: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... I can easily find my favorite pieces from net_unittests there, but PrerenderTest is just not present. > > Given the above, it becomes really unlikely that any Chrome developer would > hit > > this problem. > > > > So the benefit is low, how big is the effort? Let's see: > > * Flush the Svelte build to a Nexus device and wait, also flush it back at > > some point to something I can test performance with. (I don't think many > > external developers encountered these builds, they would likely need real > > Svelte devices to proceed) > > I'm not following this. I am discussing the way to test it locally, do you think it is not required? > > * Package PrerenderTest so that it can run on Android (probably with > unit_tests, > > I am not sure), and probably *not* land the patch because cycling time on > > these bots is critical. > > Per above, they're already built on Android. If you don't believe that, insert > something that doesn't compile, and try to run it through the bots. Android one > should fail like most of the others. Android One is not Svelte. Android One have at least a GiB of RAM, while Svelte is restricted to 512MiB. Also, compiling and running are different things, my code compiles fine for Svelte. But that's a good idea, let's see if it gets tested somewhere. I sent a try here: https://codereview.chromium.org/2194233002/ > > * Make the new test run properly on Svelte > > * Fix other PrerenderTest cases > > None of the PrerenderTests current have an issue on Svelte. Do you believe this solely based on knowing the code or there is some evidence from running the tests? > > Is the above correct? Given moderate effort and small benefits, this does not > > seem worth it. > > Seems fairly small effort to not have an obvious issue with these devices - I > assume there's some way to make SysInfo::IsLowEndDevice return false. This might be complicated. Currently it calls into Java to determing LowEnd-ness from something non-mockable: https://codesearch.chromium.org/chromium/src/base/sys_info_android.cc?q=SysIn... (also note the inherent flakiness of the implementation returning false early) Possibilities: * Make it possible to mock out IsLowEndDevice (not sure good idea) * In the build configuration for testing provide an alternative implementation of IsLowEndDevice() for Android testing. If we do that, it will likely rot after seemingly unrelated changes in GN, so I am not eager to investigate this possibility. > Add that to the test fixture, and then have the test for the test that needs > it to return true use a different test fixture that makes it return true. > None of the prerender tests currently have a dependency on the SysInfo of the > platform they're running on, and I'm loathe to introduce one, when there's an > easy workaround. If the 'git cl try' mentioned above fails on some platform, I agree that it would make sense to investigate, in which case I will let you know. Otherwise doing it only for it to rot within a month is IMO not worth it.
On 2016/08/01 16:48:42, pasko wrote: > On 2016/08/01 14:53:58, mmenke wrote: > > On 2016/08/01 08:22:46, pasko wrote: > > > On 2016/07/29 18:13:27, mmenke wrote: > > > > On 2016/07/29 17:25:03, pasko wrote: > > > > > Chrome is a large project such that when some configuration is not > covered > > > > > by regular testing, we are almost sure it is broken in numerous ways, > many > > > > > of these ways are usually unrelated to what the tests actually check > for. > > > > > Given that, the policy has been: "If there is a configuration we care > > about, > > > > > it must be regularly tested on chrome infra bots." > > > > > > > > > > In particular, with testing on Svelte there would be flakiness problems > > > > > caused by the system evicting our processes more aggressively, not all > > tests > > > > > fitting in memory, etc. > > > > > > > > > > Deciding whether we need svelte bots "enough" is outside of the scope of > > > > > this change, I argued for more svelte bots for perf in the past, and my > > > > > arguments were not heard. > > > > > > > > > > Here I am not willing to fix the potential stack of problems with > Svelte, > > > > > and I do not want to add complexity to the code without being able to > test > > > > > it. Even if I do it properly with my local device and everything, it > will > > > > > rot faster than I will return to play with this test again. > > > > > > > > I'm not arguing we should set up svelte bots. I am arguing that > developers > > > > with svelte devices should be able to run tests on them. I'm arguing we > > > > should not depend on which particular Android device we're running. I'm > not > > > > comfortable signing off on tests that don't work on some arbitrary subset > of > > > > devices. > > > > > > Do you have instructions on how to run prerender_unittest on Android? I > could > > > not find it on any of the waterfalls. Also, proper Svelte devices are hard > to > > > find for developers (10x harder than normal builds?). > > > > There is no prerender_unittest project. THey're part of Chrome's unit_tests > > project. > > > https://chromium.googlesource.com/chromium/src/+/master/docs/android_test_ins... > > has instructions on running gtest tests (They look different from when I last > > ran net_unittests on Android. I've had to do this before to debug > Android-only > > issues). > > My point is that I do not see it run anywhere on the waterfall. For example, > this link should show all tests being run on the device: > > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... > > I can easily find my favorite pieces from net_unittests there, but PrerenderTest > is just not present. Looks like it's magically disabled through a sidechannel, for reasons that are inscrutable: https://cs.chromium.org/chromium/src/build/android/pylib/gtest/filter/unit_te... I'll see if I can remove it. Sad that we have so many tests still disabled for unclear reasons after all these years. > > > Given the above, it becomes really unlikely that any Chrome developer would > > hit > > > this problem. > > > > > > So the benefit is low, how big is the effort? Let's see: > > > * Flush the Svelte build to a Nexus device and wait, also flush it back at > > > some point to something I can test performance with. (I don't think many > > > external developers encountered these builds, they would likely need real > > > Svelte devices to proceed) > > > > I'm not following this. > > I am discussing the way to test it locally, do you think it is not required? Ah, I'm not too concerned about being able to test locally. Back when we supported XP and Vista, I certainly fixed a number of issues I couldn't reproduce locally, on the trybots, or on the waterfall, and wrote tests for the relevant conditions. Also note that by depending on it, you'd adding limitations on what SysInfo::IsLowEndDevice can do, and what devices can be on the waterfall. This seems a really weird limitation for the prerender code to force. I've had to go through and fix bizarre dependencies when trying to land what should be completely unrelated code (I'm doing it right not, actually - a bunch of tests implicitly depend on what we do when we get a 404 error page with an empty body, which is bizarre). Anyhow, I don't want the next person who mucks with SysInfo::IsLowEndDevice to potentially have to make prerender changes to do so. > > > * Package PrerenderTest so that it can run on Android (probably with > > unit_tests, > > > I am not sure), and probably *not* land the patch because cycling time on > > > these bots is critical. > > > > Per above, they're already built on Android. If you don't believe that, > insert > > something that doesn't compile, and try to run it through the bots. Android > one > > should fail like most of the others. > > Android One is not Svelte. Android One have at least a GiB of RAM, while Svelte > is restricted to 512MiB. Also, compiling and running are different things, my > code compiles fine for Svelte. > > But that's a good idea, let's see if it gets tested somewhere. I sent a try > here: > https://codereview.chromium.org/2194233002/ > > > > * Make the new test run properly on Svelte > > > * Fix other PrerenderTest cases > > > > None of the PrerenderTests current have an issue on Svelte. > > Do you believe this solely based on knowing the code or there is some evidence > from running the tests? I base this on knowing there are no dependencies on SysInfo, and no other probes on explicit amount of system memory available. > > > Is the above correct? Given moderate effort and small benefits, this does > not > > > seem worth it. > > > > Seems fairly small effort to not have an obvious issue with these devices - I > > assume there's some way to make SysInfo::IsLowEndDevice return false. > > This might be complicated. Currently it calls into Java to determing > LowEnd-ness from something non-mockable: > > https://codesearch.chromium.org/chromium/src/base/sys_info_android.cc?q=SysIn... > > (also note the inherent flakiness of the implementation returning false early) > > Possibilities: > * Make it possible to mock out IsLowEndDevice (not sure good idea) Note that we're already setting a command line flag to make this return true in the unit test this CL adds. Making it return false doesn't seem like a big change. > * In the build configuration for testing provide an alternative implementation > of IsLowEndDevice() for Android testing. If we do that, it will likely rot > after seemingly unrelated changes in GN, so I am not eager to investigate this > possibility. > > > Add that to the test fixture, and then have the test for the test that needs > > it to return true use a different test fixture that makes it return true. > > None of the prerender tests currently have a dependency on the SysInfo of the > > platform they're running on, and I'm loathe to introduce one, when there's an > > easy workaround. > > If the 'git cl try' mentioned above fails on some platform, I agree that it > would make sense to investigate, in which case I will let you know. Otherwise > doing it only for it to rot within a month is IMO not worth it.
On 2016/08/01 17:01:29, mmenke wrote: > > My point is that I do not see it run anywhere on the waterfall. For example, > > this link should show all tests being run on the device: > > > > > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... > > > > I can easily find my favorite pieces from net_unittests there, but > PrerenderTest > > is just not present. > > Looks like it's magically disabled through a sidechannel, for reasons that are > inscrutable: > https://cs.chromium.org/chromium/src/build/android/pylib/gtest/filter/unit_te... Ha, thanks for the pointer, I could look it up myself, but was not sure that unit_tests on android refers to the same unit_tests as on desktop. Wow, it may actually work out of the box after all these years, let me check in the next patchset to: https://codereview.chromium.org/2194233002/ > I'll see if I can remove it. Sad that we have so many tests still disabled for > unclear reasons after all these years. Good that we know more now. > > > > Given the above, it becomes really unlikely that any Chrome developer > would > > > hit > > > > this problem. > > > > > > > > So the benefit is low, how big is the effort? Let's see: > > > > * Flush the Svelte build to a Nexus device and wait, also flush it back at > > > > some point to something I can test performance with. (I don't think many > > > > external developers encountered these builds, they would likely need > real > > > > Svelte devices to proceed) > > > > > > I'm not following this. > > > > I am discussing the way to test it locally, do you think it is not required? > > Ah, I'm not too concerned about being able to test locally. Back when we > supported XP and Vista, I certainly fixed a number of issues I couldn't > reproduce locally, on the trybots, or on the waterfall, and wrote tests for the > relevant conditions. Yes, that is possible, but usualy beyond easy, as this needs multiple iterations and requires an hour to run all the tests each time (or perhaps several hours?). If necessary, we can do it, no doubt about that. > Also note that by depending on it, you'd adding limitations on what > SysInfo::IsLowEndDevice can do, and what devices can be on the waterfall. This > seems a really weird limitation for the prerender code to force. I've had to go > through and fix bizarre dependencies when trying to land what should be > completely unrelated code (I'm doing it right not, actually - a bunch of tests > implicitly depend on what we do when we get a 404 error page with an empty body, > which is bizarre). Anyhow, I don't want the next person who mucks with > SysInfo::IsLowEndDevice to potentially have to make prerender changes to do so. Thanks. I am getting closer to understand what your intentions are. Indeed forcing obvious constraints for waterfall configurations seems suboptimal. I was arguing about tradeoff between the more flexible waterfall (that we may never reach to) and effort+maintenance_of_this_extra_indirection. I think the balance would be good if we mock out SysInfo within prerendering. A SysInfoProvider can be injected by tests differently and can walk though all possibilities in tests perhaps. Mocking out the value in SysInfo itself, and dealing with Java<->Native initialization order would make me less happy, suspecting hidden barriers, dependencies on brittle build configuration. WDYT? > > > > * Package PrerenderTest so that it can run on Android (probably with > > > unit_tests, > > > > I am not sure), and probably *not* land the patch because cycling time > on > > > > these bots is critical. > > > > > > Per above, they're already built on Android. If you don't believe that, > > insert > > > something that doesn't compile, and try to run it through the bots. Android > > one > > > should fail like most of the others. > > > > Android One is not Svelte. Android One have at least a GiB of RAM, while > Svelte > > is restricted to 512MiB. Also, compiling and running are different things, my > > code compiles fine for Svelte. > > > > But that's a good idea, let's see if it gets tested somewhere. I sent a try > > here: > > https://codereview.chromium.org/2194233002/ > > > > > > * Make the new test run properly on Svelte > > > > * Fix other PrerenderTest cases > > > > > > None of the PrerenderTests current have an issue on Svelte. > > > > Do you believe this solely based on knowing the code or there is some evidence > > from running the tests? > > I base this on knowing there are no dependencies on SysInfo, and no other probes > on explicit amount of system memory available. Purely hypothetically: Android Framework imposes different constraints on apps in Svelte devices, and I am not confident that this would be non-flaky for our tests creating processes left and right. The good part is that we may discover real bugs this way, but that feels like a separate project to test properly on Svelte, and what we can do here realistically with PrerenderTest is a tiny drop in the ocean to help that project. > > > > Is the above correct? Given moderate effort and small benefits, this does > > not > > > > seem worth it. > > > > > > Seems fairly small effort to not have an obvious issue with these devices - > I > > > assume there's some way to make SysInfo::IsLowEndDevice return false. > > > > This might be complicated. Currently it calls into Java to determing > > LowEnd-ness from something non-mockable: > > > > > https://codesearch.chromium.org/chromium/src/base/sys_info_android.cc?q=SysIn... > > > > (also note the inherent flakiness of the implementation returning false early) > > > > Possibilities: > > * Make it possible to mock out IsLowEndDevice (not sure good idea) > > Note that we're already setting a command line flag to make this return true in > the unit test this CL adds. Making it return false doesn't seem like a big > change. Did you mean a FieldTrial? Yes, I went on with it because I could not find a more elegant way, and assumed that this FieldTrial is going to be needed for some time, this has a side effect of constraining whoever wants to change the trial name or even remove the trial. Adding more synthetic trials would be .. awkward IMO. Better probably to mock SysInfo out completely?
On 2016/08/02 08:43:44, pasko wrote: > On 2016/08/01 17:01:29, mmenke wrote: > > > My point is that I do not see it run anywhere on the waterfall. For example, > > > this link should show all tests being run on the device: > > > > > > > > > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... > > > > > > I can easily find my favorite pieces from net_unittests there, but > > PrerenderTest > > > is just not present. > > > > Looks like it's magically disabled through a sidechannel, for reasons that are > > inscrutable: > > > https://cs.chromium.org/chromium/src/build/android/pylib/gtest/filter/unit_te... > > Ha, thanks for the pointer, I could look it up myself, but was not sure that > unit_tests on android refers to the same unit_tests as on desktop. Wow, it may > actually work out of the box after all these years, let me check in the next > patchset to: > > https://codereview.chromium.org/2194233002/ > > > I'll see if I can remove it. Sad that we have so many tests still disabled > for > > unclear reasons after all these years. > > Good that we know more now. > > > > > > Given the above, it becomes really unlikely that any Chrome developer > > would > > > > hit > > > > > this problem. > > > > > > > > > > So the benefit is low, how big is the effort? Let's see: > > > > > * Flush the Svelte build to a Nexus device and wait, also flush it back > at > > > > > some point to something I can test performance with. (I don't think > many > > > > > external developers encountered these builds, they would likely need > > real > > > > > Svelte devices to proceed) > > > > > > > > I'm not following this. > > > > > > I am discussing the way to test it locally, do you think it is not required? > > > > Ah, I'm not too concerned about being able to test locally. Back when we > > supported XP and Vista, I certainly fixed a number of issues I couldn't > > reproduce locally, on the trybots, or on the waterfall, and wrote tests for > the > > relevant conditions. > > Yes, that is possible, but usualy beyond easy, as this needs multiple iterations > and requires an hour to run all the tests each time (or perhaps several hours?). > If necessary, we can do it, no doubt about that. > > > Also note that by depending on it, you'd adding limitations on what > > SysInfo::IsLowEndDevice can do, and what devices can be on the waterfall. > This > > seems a really weird limitation for the prerender code to force. I've had to > go > > through and fix bizarre dependencies when trying to land what should be > > completely unrelated code (I'm doing it right not, actually - a bunch of tests > > implicitly depend on what we do when we get a 404 error page with an empty > body, > > which is bizarre). Anyhow, I don't want the next person who mucks with > > SysInfo::IsLowEndDevice to potentially have to make prerender changes to do > so. > > Thanks. I am getting closer to understand what your intentions are. Indeed > forcing obvious constraints for waterfall configurations seems suboptimal. I was > arguing about tradeoff between the more flexible waterfall (that we may never > reach to) and effort+maintenance_of_this_extra_indirection. > > I think the balance would be good if we mock out SysInfo within prerendering. A > SysInfoProvider can be injected by tests differently and can walk though all > possibilities in tests perhaps. > > Mocking out the value in SysInfo itself, and dealing with Java<->Native > initialization order would make me less happy, suspecting hidden barriers, > dependencies on brittle build configuration. > > WDYT? I'm fine with mocking out SysInfo itself. I'm even fine with making a virtual PrerenderManager method called IsLowEndDevice, and mocking it out in tests. Basically, as long as we don't depend on the local device's configuration, I'm happy. > > > > > * Package PrerenderTest so that it can run on Android (probably with > > > > unit_tests, > > > > > I am not sure), and probably *not* land the patch because cycling time > > on > > > > > these bots is critical. > > > > > > > > Per above, they're already built on Android. If you don't believe that, > > > insert > > > > something that doesn't compile, and try to run it through the bots. > Android > > > one > > > > should fail like most of the others. > > > > > > Android One is not Svelte. Android One have at least a GiB of RAM, while > > Svelte > > > is restricted to 512MiB. Also, compiling and running are different things, > my > > > code compiles fine for Svelte. > > > > > > But that's a good idea, let's see if it gets tested somewhere. I sent a try > > > here: > > > https://codereview.chromium.org/2194233002/ > > > > > > > > * Make the new test run properly on Svelte > > > > > * Fix other PrerenderTest cases > > > > > > > > None of the PrerenderTests current have an issue on Svelte. > > > > > > Do you believe this solely based on knowing the code or there is some > evidence > > > from running the tests? > > > > I base this on knowing there are no dependencies on SysInfo, and no other > probes > > on explicit amount of system memory available. > > Purely hypothetically: Android Framework imposes different constraints on apps > in Svelte devices, and I am not confident that this would be non-flaky for our > tests creating processes left and right. The good part is that we may discover > real bugs this way, but that feels like a separate project to test properly on > Svelte, and what we can do here realistically with PrerenderTest is a tiny drop > in the ocean to help that project. > > > > > > Is the above correct? Given moderate effort and small benefits, this > does > > > not > > > > > seem worth it. > > > > > > > > Seems fairly small effort to not have an obvious issue with these devices > - > > I > > > > assume there's some way to make SysInfo::IsLowEndDevice return false. > > > > > > This might be complicated. Currently it calls into Java to determing > > > LowEnd-ness from something non-mockable: > > > > > > > > > https://codesearch.chromium.org/chromium/src/base/sys_info_android.cc?q=SysIn... > > > > > > (also note the inherent flakiness of the implementation returning false > early) > > > > > > Possibilities: > > > * Make it possible to mock out IsLowEndDevice (not sure good idea) > > > > Note that we're already setting a command line flag to make this return true > in > > the unit test this CL adds. Making it return false doesn't seem like a big > > change. > > Did you mean a FieldTrial? Yes, I went on with it because I could not find a > more elegant way, and assumed that this FieldTrial is going to be needed for > some time, this has a side effect of constraining whoever wants to change the > trial name or even remove the trial. Adding more synthetic trials would be .. > awkward IMO. Better probably to mock SysInfo out completely? Yes, I meant the field trial.
The CQ bit was checked by pasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/02 16:35:43, mmenke wrote: > I'm fine with mocking out SysInfo itself. I'm even fine with making a virtual > PrerenderManager method called IsLowEndDevice, and mocking it out in tests. > Basically, as long as we don't depend on the local device's configuration, I'm > happy. Went with PrerenderManager::IsLowEndDevice(), as it requires less boilerplate. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/03 13:33:33, pasko wrote: > On 2016/08/02 16:35:43, mmenke wrote: > > I'm fine with mocking out SysInfo itself. I'm even fine with making a virtual > > PrerenderManager method called IsLowEndDevice, and mocking it out in tests. > > Basically, as long as we don't depend on the local device's configuration, I'm > > happy. > > Went with PrerenderManager::IsLowEndDevice(), as it requires less boilerplate. > > PTAL. LGTM
Oops, forgot to publish my nits. https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:388: virtual bool IsLowEndDevice(); const? https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:224: bool IsLowEndDevice() override { return is_low_end_device_; } Doesn't really matter, but this can be private https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:333: void SetUp() override { prerender_manager()->SetIsLowEndDevice(false); } Can just put this in the constructor. Each test iteration uses its own instance of the test fixture (Which means SetUp is mostly just useful for interleaving subclass and superclass initialization)
mmenke: thank you for in-depth review! dougarnett: will you have a chance to look at it today? Otherwise I'd prefer to just land and address your comments in followup changes. https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:388: virtual bool IsLowEndDevice(); On 2016/08/03 15:10:58, mmenke wrote: > const? Oops, sure. Done. https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:224: bool IsLowEndDevice() override { return is_low_end_device_; } On 2016/08/03 15:10:58, mmenke wrote: > Doesn't really matter, but this can be private Done. https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:333: void SetUp() override { prerender_manager()->SetIsLowEndDevice(false); } On 2016/08/03 15:10:58, mmenke wrote: > Can just put this in the constructor. Each test iteration uses its own instance > of the test fixture (Which means SetUp is mostly just useful for interleaving > subclass and superclass initialization) Ah, there is some stuff already in the constructor, putting it there then, will look more consistent. I forgot that TEST_F re-creates the fixture (shame shame). Though, how does this align with general perception of doing work in constructors being more error prone? https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2197663002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:333: void SetUp() override { prerender_manager()->SetIsLowEndDevice(false); } On 2016/08/03 15:30:17, pasko wrote: > On 2016/08/03 15:10:58, mmenke wrote: > > Can just put this in the constructor. Each test iteration uses its own > instance > > of the test fixture (Which means SetUp is mostly just useful for interleaving > > subclass and superclass initialization) > > Ah, there is some stuff already in the constructor, putting it there then, will > look more consistent. I forgot that TEST_F re-creates the fixture (shame shame). > > Though, how does this align with general perception of doing work in > constructors being more error prone? > https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors We aren't doing any virtual calls to the test fixture, and we can't fail (If we could fail, EXPECTs work fine in the constructor, ASSERTs would have to go here, though). My feeling is that in test fixtures, it's best just to do all the work in the constructor/destructor, to have everything all in one place.
lgtm https://codereview.chromium.org/2197663002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2197663002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:388: virtual bool IsLowEndDevice() const; +1 for making this testable
The CQ bit was checked by pasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2197663002/#ps60001 (title: "Addressed comments by mmenke@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Prerender: Allow on low end devices for ORIGIN_OFFLINE TL;DR: Prerendering cannot keep track of all child processes, and only allows prerendering when asked by the offliner. Low end devices require (best effort at) keeping no more than one renderer process at any time. Prerendering has been disabled on low end devices because of this. Offliner should be able to create prerenders, and it needs to ensure that these prerenders are: * not created when another renderer is alive * killed when Chrome another renderer is created We identified additional cases when to prevent offlining, among which: * Chrome is in foreground (even though may not have a renderer) * Battery level is low Since the foreground checks should be in place anyway, we decided that while Chrome is in background having two background renderers is OK, and we can rely on one (or both) of them being killed by the system. For simplicity the offliner does not actually watch for renderers specifically, only: * Prevents offlining when any Chrome activity is visible * Kills its prerenders when a transition to foreground happens BUG=622508 ========== to ========== Prerender: Allow on low end devices for ORIGIN_OFFLINE TL;DR: Prerendering cannot keep track of all child processes, and only allows prerendering when asked by the offliner. Low end devices require (best effort at) keeping no more than one renderer process at any time. Prerendering has been disabled on low end devices because of this. Offliner should be able to create prerenders, and it needs to ensure that these prerenders are: * not created when another renderer is alive * killed when Chrome another renderer is created We identified additional cases when to prevent offlining, among which: * Chrome is in foreground (even though may not have a renderer) * Battery level is low Since the foreground checks should be in place anyway, we decided that while Chrome is in background having two background renderers is OK, and we can rely on one (or both) of them being killed by the system. For simplicity the offliner does not actually watch for renderers specifically, only: * Prevents offlining when any Chrome activity is visible * Kills its prerenders when a transition to foreground happens BUG=622508 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Prerender: Allow on low end devices for ORIGIN_OFFLINE TL;DR: Prerendering cannot keep track of all child processes, and only allows prerendering when asked by the offliner. Low end devices require (best effort at) keeping no more than one renderer process at any time. Prerendering has been disabled on low end devices because of this. Offliner should be able to create prerenders, and it needs to ensure that these prerenders are: * not created when another renderer is alive * killed when Chrome another renderer is created We identified additional cases when to prevent offlining, among which: * Chrome is in foreground (even though may not have a renderer) * Battery level is low Since the foreground checks should be in place anyway, we decided that while Chrome is in background having two background renderers is OK, and we can rely on one (or both) of them being killed by the system. For simplicity the offliner does not actually watch for renderers specifically, only: * Prevents offlining when any Chrome activity is visible * Kills its prerenders when a transition to foreground happens BUG=622508 ========== to ========== Prerender: Allow on low end devices for ORIGIN_OFFLINE TL;DR: Prerendering cannot keep track of all child processes, and only allows prerendering when asked by the offliner. Low end devices require (best effort at) keeping no more than one renderer process at any time. Prerendering has been disabled on low end devices because of this. Offliner should be able to create prerenders, and it needs to ensure that these prerenders are: * not created when another renderer is alive * killed when Chrome another renderer is created We identified additional cases when to prevent offlining, among which: * Chrome is in foreground (even though may not have a renderer) * Battery level is low Since the foreground checks should be in place anyway, we decided that while Chrome is in background having two background renderers is OK, and we can rely on one (or both) of them being killed by the system. For simplicity the offliner does not actually watch for renderers specifically, only: * Prevents offlining when any Chrome activity is visible * Kills its prerenders when a transition to foreground happens BUG=622508 Committed: https://crrev.com/63b429af6ac2e1a0835e30ddd962fc4ebad2cd1b Cr-Commit-Position: refs/heads/master@{#409528} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/63b429af6ac2e1a0835e30ddd962fc4ebad2cd1b Cr-Commit-Position: refs/heads/master@{#409528} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
