Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2275933002:
Experiment to disable Prerendering with fast wind-down (Closed)
|
Created:
4 years, 4 months ago by pasko Modified:
4 years, 3 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. |
DescriptionExperiment to disable Prerendering with fast wind-down
This adds the experiment "PrerenderSilence" to disable all performance-related
uses of prerendering. The group name encodes a timestamp. After reaching this
timesamp, prerendering should continue as normal.
The expiration timestamp should be in GMT, since all Field Trial logic both on
the server and on the client are also in GMT.
Note: This makes FieldTrial metrics incorrect for the days shortly after the
expiration timestamp because the experiment is sticky to the session.
BUG=640592
Committed: https://crrev.com/ad807fa355b06cfa4e4447f0ba8c8403aaaa8d2a
Cr-Commit-Position: refs/heads/master@{#414847}
Patch Set 1 #Patch Set 2 : move re-enabling to another test #Patch Set 3 : rebase #
Total comments: 8
Patch Set 4 : Added comments describing the new tests #
Total comments: 4
Patch Set 5 : Malformed expiration time falls back to normal operation. #
Total comments: 2
Patch Set 6 : DLOG(ERROR) #
Total comments: 8
Patch Set 7 : remove FRIEND_TEST_ALL_PREFIXES #
Total comments: 3
Messages
Total messages: 52 (30 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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...
Description was changed from ========== Experiment to disable Prerendering with fast wind-down This adds the experiment "PrerenderSilence" to disable all performance-related uses of prerendering. The group name encodes a timestamp. After reaching this timesamp, prerendering should continue as normal. The expiration timestamp should be in GMT, since all Field Trial logic both on the server and on the client are also in GMT. Note: This makes FieldTrial metrics incorrect for the days shortly after the expiration timestamp because the experiment is sticky to the session. BUG=none ========== to ========== Experiment to disable Prerendering with fast wind-down This adds the experiment "PrerenderSilence" to disable all performance-related uses of prerendering. The group name encodes a timestamp. After reaching this timesamp, prerendering should continue as normal. The expiration timestamp should be in GMT, since all Field Trial logic both on the server and on the client are also in GMT. Note: This makes FieldTrial metrics incorrect for the days shortly after the expiration timestamp because the experiment is sticky to the session. BUG=none ==========
pasko@chromium.org changed reviewers: + droger@chromium.org, mattcary@chromium.org
mattcary, droger: PTaL I am going to create a detailed bug for the experiment and send this review out to mmenke@ and rkaplow@.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/24 09:30:29, pasko wrote: > mattcary, droger: PTaL > > I am going to create a detailed bug for the experiment and send this review out > to mmenke@ and rkaplow@. friendly ping :)
lgtm https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1111: TEST_F(PrerenderTest, PrerenderSilenceAllowsOffline) { Can you add a comment like: // Checks that the prerender silence experiment does not disable offline prerendering. https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1129: TEST_F(PrerenderTest, PrerenderSilenceDisallowsNonOffline) { // Checks that the prerender silence experiment disables prerendering. https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1153: TEST_F(PrerenderTest, PrerenderSilenceAllowsAfterExpiration) { // Checks that prerendering is enabled after expiration of the experiment. https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1158: prerender_manager()->AdvanceTime(TimeDelta::FromSeconds(60)); Should we call AdvanceTimeticks too, so that times remain consistent?
The CQ bit was checked by pasko@chromium.org to run a CQ dry run
https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1111: TEST_F(PrerenderTest, PrerenderSilenceAllowsOffline) { On 2016/08/25 13:17:29, droger wrote: > Can you add a comment like: > // Checks that the prerender silence experiment does not disable offline > prerendering. Done this and other comments with slight modifications from your version, PTAL. https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1129: TEST_F(PrerenderTest, PrerenderSilenceDisallowsNonOffline) { On 2016/08/25 13:17:29, droger wrote: > // Checks that the prerender silence experiment disables prerendering. Done. https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1153: TEST_F(PrerenderTest, PrerenderSilenceAllowsAfterExpiration) { On 2016/08/25 13:17:28, droger wrote: > // Checks that prerendering is enabled after expiration of the experiment. Done. https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1158: prerender_manager()->AdvanceTime(TimeDelta::FromSeconds(60)); On 2016/08/25 13:17:29, droger wrote: > Should we call AdvanceTimeticks too, so that times remain consistent? TimeTicks should only be inspected after a prerender is added, so not touching it here makes it clearer that we don't depend on those parts. I think it's clearer ..
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/25 13:32:01, pasko wrote: > https://codereview.chromium.org/2275933002/diff/40001/chrome/browser/prerende... > chrome/browser/prerender/prerender_unittest.cc:1158: > prerender_manager()->AdvanceTime(TimeDelta::FromSeconds(60)); > On 2016/08/25 13:17:29, droger wrote: > > Should we call AdvanceTimeticks too, so that times remain consistent? > > TimeTicks should only be inspected after a prerender is added, so not touching > it here makes it clearer that we don't depend on those parts. > > I think it's clearer .. Ok, sgtm. Thanks.
lgtm https://codereview.chromium.org/2275933002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:628: return true; Do we really want a malformed experiment option to fail with prerender off? I can't imagine how we'd get a badly formed flag except through some kind of corruption, and so feel like we should return false in this case. https://codereview.chromium.org/2275933002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2275933002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_unittest.cc:1111: // Checks that the "PrerenderSilence experiment does not disable offline Lonley quote.
https://codereview.chromium.org/2275933002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:628: return true; On 2016/08/25 13:51:18, mattcary wrote: > Do we really want a malformed experiment option to fail with prerender off? I > can't imagine how we'd get a badly formed flag except through some kind of > corruption, and so feel like we should return false in this case. Probably same for line 634 below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
https://codereview.chromium.org/2275933002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:628: return true; On 2016/08/25 13:51:18, mattcary wrote: > Do we really want a malformed experiment option to fail with prerender off? I > can't imagine how we'd get a badly formed flag except through some kind of > corruption, and so feel like we should return false in this case. I wanted a mere "ExperimentYes" (without a date) to enable the experiment, sticky to the session. It might be that we'd want to try it on Beta? On 2016/08/25 13:56:24, droger wrote: > Probably same for line 634 below. Yeah, that one indeed is a malformed experiment, should disable. I added a few comments for clarity, PTAL.
https://codereview.chromium.org/2275933002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:636: // Malformed expiration time falls back to normal operation. I guess a DCHECK is not appropriate here, maybe a DLOG(ERROR)?
The CQ bit was checked by pasko@chromium.org to run a CQ dry run
https://codereview.chromium.org/2275933002/diff/80001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/80001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.cc:636: // Malformed expiration time falls back to normal operation. On 2016/08/25 15:23:24, droger wrote: > I guess a DCHECK is not appropriate here, maybe a DLOG(ERROR)? Good idea, done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:630: return true; Not to belabor the point, but won't we turn in the experiment on unconditionally if we have a flag like "ExperimentYes_until_2016-11-20"? That seems a little permissive. I was thinking it would be safer to do something like: if (group_name == kExperimentPrefix) return true; else if (!StartsWith(kExperimentPrefixWithExperiation)) return false;
https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:630: return true; On 2016/08/25 16:15:21, mattcary wrote: > Not to belabor the point, but won't we turn in the experiment on unconditionally > if we have a flag like "ExperimentYes_until_2016-11-20"? That seems a little > permissive. I was thinking it would be safer to do something like: > if (group_name == kExperimentPrefix) return true; > else if (!StartsWith(kExperimentPrefixWithExperiation)) return false; That's because we commonly like suffixing experiment groups: "ExperimentYes1", "ExperimentYes2" etc. In case we wanted to see on Beta, say, how much noise there is in the number of crashes per client. Not sure it is applicable here, just following the common practice to allow suffixes.
On 2016/08/25 16:25:39, pasko wrote: > https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... > File chrome/browser/prerender/prerender_manager.cc (right): > > https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... > chrome/browser/prerender/prerender_manager.cc:630: return true; > On 2016/08/25 16:15:21, mattcary wrote: > > Not to belabor the point, but won't we turn in the experiment on > unconditionally > > if we have a flag like "ExperimentYes_until_2016-11-20"? That seems a little > > permissive. I was thinking it would be safer to do something like: > > if (group_name == kExperimentPrefix) return true; > > else if (!StartsWith(kExperimentPrefixWithExperiation)) return false; > > That's because we commonly like suffixing experiment groups: "ExperimentYes1", > "ExperimentYes2" etc. In case we wanted to see on Beta, say, how much noise > there is in the number of crashes per client. Not sure it is applicable here, > just following the common practice to allow suffixes. Ah, I wasn't aware of that convention. Thanks, carry on!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Experiment to disable Prerendering with fast wind-down This adds the experiment "PrerenderSilence" to disable all performance-related uses of prerendering. The group name encodes a timestamp. After reaching this timesamp, prerendering should continue as normal. The expiration timestamp should be in GMT, since all Field Trial logic both on the server and on the client are also in GMT. Note: This makes FieldTrial metrics incorrect for the days shortly after the expiration timestamp because the experiment is sticky to the session. BUG=none ========== to ========== Experiment to disable Prerendering with fast wind-down This adds the experiment "PrerenderSilence" to disable all performance-related uses of prerendering. The group name encodes a timestamp. After reaching this timesamp, prerendering should continue as normal. The expiration timestamp should be in GMT, since all Field Trial logic both on the server and on the client are also in GMT. Note: This makes FieldTrial metrics incorrect for the days shortly after the expiration timestamp because the experiment is sticky to the session. BUG=640592 ==========
pasko@chromium.org changed reviewers: + mmenke@chromium.org, rkaplow@chromium.org
mmenke: PTaL (not super urgent) rkaplow: FYI (because you seem to be ooo)
https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:635: &expiration_time)) { Why is this needed? Experiments can have their expiration time on the server. https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:401: FRIEND_TEST_ALL_PREFIXES(PrerenderTest, PrerenderSilenceDisallowsNonOffline); I suggest adding a "IsPrerenderSilenceExperimentForTesting" method instead of friending the test. Makes what's exposed to tests clearer.
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...
https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:635: &expiration_time)) { On 2016/08/25 19:22:50, mmenke (busy) wrote: > Why is this needed? Experiments can have their expiration time on the server. Server-based experiments are sticky until Chrome is restarted. By encoding an additional expiration time here we can cut off the experiment faster. Not sure it really affects a large volume of users, but estimating the number was not easy. Also I am one of the users who does not like surprises to take effect for a long time, and restarts Chrome once every 3 weeks (huge mistake, I know), so this final straw convinced me that this extra small piece of code is handy to provide a fast experiment wind down. There are a few details in the doc linked from the bug. https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.h:401: FRIEND_TEST_ALL_PREFIXES(PrerenderTest, PrerenderSilenceDisallowsNonOffline); On 2016/08/25 19:22:50, mmenke (busy) wrote: > I suggest adding a "IsPrerenderSilenceExperimentForTesting" method instead of > friending the test. Makes what's exposed to tests clearer. Hm, eliminating FRIEND_TEST_ALL_PREFIXES is indeed a good idea, and it seems you are suggesting _not_ to call AddPrerender in the test (directly or indirectly). I am hesitating, a simple wrapper AddPrerenderForTesting would allow testing AddPrerender itself, and not just a method that it maybe-invokes. OK, I've done what I think you mean, can revert/rework if it reveals more problematic. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:635: &expiration_time)) { On 2016/08/26 11:22:12, pasko wrote: > On 2016/08/25 19:22:50, mmenke (busy) wrote: > > Why is this needed? Experiments can have their expiration time on the server. > > Server-based experiments are sticky until Chrome is restarted. By encoding an > additional expiration time here we can cut off the experiment faster. Not sure > it really affects a large volume of users, but estimating the number was not > easy. Also I am one of the users who does not like surprises to take effect for > a long time, and restarts Chrome once every 3 weeks (huge mistake, I know), so > this final straw convinced me that this extra small piece of code is handy to > provide a fast experiment wind down. > > There are a few details in the doc linked from the bug. I remain extremely skeptical this serves any useful purpose, but not going to fight over it. https://codereview.chromium.org/2275933002/diff/120001/chrome/browser/prerend... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2275933002/diff/120001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:1117: ASSERT_TRUE(prerender_manager()->SetTime("2016-12-20T00:00:30Z")); Do we really want a unit test with an expiration date? Remember that we run tests on stable channel, too.
https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2275933002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_manager.cc:635: &expiration_time)) { On 2016/08/26 16:44:47, mmenke (busy) wrote: > > There are a few details in the doc linked from the bug. > > I remain extremely skeptical this serves any useful purpose, but not going to > fight over it. Thank you. I am somewhat skeptical too, but figuring out how much skeptical I should be was not worth it. https://codereview.chromium.org/2275933002/diff/120001/chrome/browser/prerend... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2275933002/diff/120001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:1117: ASSERT_TRUE(prerender_manager()->SetTime("2016-12-20T00:00:30Z")); On 2016/08/26 16:44:47, mmenke (busy) wrote: > Do we really want a unit test with an expiration date? Remember that we run > tests on stable channel, too. I do not follow how release channel can be relevant to a unittest. Can you elaborate? The test itself does not expire even if it is run in 2020 because PrerenderManager::time_ is changed according to the test, not the current time.
LGTM https://codereview.chromium.org/2275933002/diff/120001/chrome/browser/prerend... File chrome/browser/prerender/prerender_unittest.cc (right): https://codereview.chromium.org/2275933002/diff/120001/chrome/browser/prerend... chrome/browser/prerender/prerender_unittest.cc:1117: ASSERT_TRUE(prerender_manager()->SetTime("2016-12-20T00:00:30Z")); On 2016/08/26 17:39:31, pasko wrote: > On 2016/08/26 16:44:47, mmenke (busy) wrote: > > Do we really want a unit test with an expiration date? Remember that we run > > tests on stable channel, too. > > I do not follow how release channel can be relevant to a unittest. Can you > elaborate? > > The test itself does not expire even if it is run in 2020 because > PrerenderManager::time_ is changed according to the test, not the current time. Ahh, I missed that. Channel is relevant because it means a test will live for months on even after it's been removed from Canary.
The CQ bit was checked by pasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, mattcary@chromium.org Link to the patchset: https://codereview.chromium.org/2275933002/#ps120001 (title: "remove FRIEND_TEST_ALL_PREFIXES")
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 ========== Experiment to disable Prerendering with fast wind-down This adds the experiment "PrerenderSilence" to disable all performance-related uses of prerendering. The group name encodes a timestamp. After reaching this timesamp, prerendering should continue as normal. The expiration timestamp should be in GMT, since all Field Trial logic both on the server and on the client are also in GMT. Note: This makes FieldTrial metrics incorrect for the days shortly after the expiration timestamp because the experiment is sticky to the session. BUG=640592 ========== to ========== Experiment to disable Prerendering with fast wind-down This adds the experiment "PrerenderSilence" to disable all performance-related uses of prerendering. The group name encodes a timestamp. After reaching this timesamp, prerendering should continue as normal. The expiration timestamp should be in GMT, since all Field Trial logic both on the server and on the client are also in GMT. Note: This makes FieldTrial metrics incorrect for the days shortly after the expiration timestamp because the experiment is sticky to the session. BUG=640592 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Experiment to disable Prerendering with fast wind-down This adds the experiment "PrerenderSilence" to disable all performance-related uses of prerendering. The group name encodes a timestamp. After reaching this timesamp, prerendering should continue as normal. The expiration timestamp should be in GMT, since all Field Trial logic both on the server and on the client are also in GMT. Note: This makes FieldTrial metrics incorrect for the days shortly after the expiration timestamp because the experiment is sticky to the session. BUG=640592 ========== to ========== Experiment to disable Prerendering with fast wind-down This adds the experiment "PrerenderSilence" to disable all performance-related uses of prerendering. The group name encodes a timestamp. After reaching this timesamp, prerendering should continue as normal. The expiration timestamp should be in GMT, since all Field Trial logic both on the server and on the client are also in GMT. Note: This makes FieldTrial metrics incorrect for the days shortly after the expiration timestamp because the experiment is sticky to the session. BUG=640592 Committed: https://crrev.com/ad807fa355b06cfa4e4447f0ba8c8403aaaa8d2a Cr-Commit-Position: refs/heads/master@{#414847} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ad807fa355b06cfa4e4447f0ba8c8403aaaa8d2a Cr-Commit-Position: refs/heads/master@{#414847} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
