Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528173003:
Desktop fast shutdown experiment (Closed)
|
Created:
4 years ago by manzagop (departed) Modified:
4 years ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDesktop fast shutdown experiment
Set up an experiment to see the impact of always using the fast exit
path. For context, see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/AMB2Y7Sqj48/discussion
Testing: launch with --enable-features=DesktopFastShutdown
BUG=653647
Committed: https://crrev.com/011b6142b57f328cbeac1e7a903a36a15eba2cf7
Cr-Commit-Position: refs/heads/master@{#436328}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comment #
Total comments: 2
Patch Set 3 : Add a comment #Patch Set 4 : Add comment to features.h #
Total comments: 2
Patch Set 5 : Refine another comment #Patch Set 6 : Put features in 'features' namespace #
Messages
Total messages: 42 (23 generated)
manzagop@chromium.org changed reviewers: + gab@chromium.org
As discussed! Tell me what you think.
lgtm w/ comments https://codereview.chromium.org/2528173003/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2528173003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1987: if (base::FeatureList::IsEnabled(chrome::kDesktopFastShutdown)) { nit: no {} for single line condition/body https://codereview.chromium.org/2528173003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1988: chrome::SessionEnding(); Won't this result in recording the exits in UMA as though they were triggered by END_SESSION? Do we care? https://codereview.chromium.org/2528173003/diff/1/chrome/browser/features.h File chrome/browser/features.h (right): https://codereview.chromium.org/2528173003/diff/1/chrome/browser/features.h#n... chrome/browser/features.h:10: namespace chrome { FWIW (no action required): we don't typically use the chrome:: namespace but this might be a good usage according to https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RVps606IB_g/ZLes1...
The CQ bit was checked by manzagop@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...
manzagop@chromium.org changed reviewers: + hashimoto@chromium.org, rkaplow@chromium.org
Hi rkaplow@, hashimoto@! We're interested in measuring the stability impact of always using the quick exit path on Windows. Do you have any concerns? Note this will lead non-EndSession exits to be counted as EndSession exits wrt the Shutdown.ShutdownType metric. Cheers, Pierre https://codereview.chromium.org/2528173003/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2528173003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1987: if (base::FeatureList::IsEnabled(chrome::kDesktopFastShutdown)) { On 2016/11/28 16:05:05, gab wrote: > nit: no {} for single line condition/body Done. https://codereview.chromium.org/2528173003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1988: chrome::SessionEnding(); On 2016/11/28 16:05:05, gab wrote: > Won't this result in recording the exits in UMA as though they were triggered by > END_SESSION? Do we care? It will, but I don't think we care in the context of this experiment. We have Shutdown.ShutdownType, but I don't think it's really looked at or that anything else sits on top of it. I'll add rkaplow (from metrics) and hashimoto (the metric owner) to validate. https://codereview.chromium.org/2528173003/diff/1/chrome/browser/features.h File chrome/browser/features.h (right): https://codereview.chromium.org/2528173003/diff/1/chrome/browser/features.h#n... chrome/browser/features.h:10: namespace chrome { On 2016/11/28 16:05:05, gab wrote: > FWIW (no action required): we don't typically use the chrome:: namespace but > this might be a good usage according to > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/RVps606IB_g/ZLes1... Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
lgtm I guess there will be no new issue caused by this experiment. (SessionEnding corresponds to about 10% of all Windows shutdown already.) https://codereview.chromium.org/2528173003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2528173003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1988: chrome::SessionEnding(); How about having a comment which makes it clear that SessionEnding won't return because it terminates the current process?
Description was changed from ========== Desktop fast shutdown experiment Set up an experiment to see the impact of always using the fast exit path. Testing: launch with --enable-features=DesktopFastShutdown BUG=653647 ========== to ========== Desktop fast shutdown experiment Set up an experiment to see the impact of always using the fast exit path. For context, see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/AMB2Y7Sqj48/dis... Testing: launch with --enable-features=DesktopFastShutdown BUG=653647 ==========
manzagop@chromium.org changed reviewers: + jochen@chromium.org
Hi jochen@, Could you have an OWNERS' look for chrome/browser/*? Thank you! Pierre https://codereview.chromium.org/2528173003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2528173003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1988: chrome::SessionEnding(); On 2016/11/30 10:50:15, hashimoto wrote: > How about having a comment which makes it clear that SessionEnding won't return > because it terminates the current process? Done.
what about putting this into a sub directory with an OWNERS file?
Thanks for the quick reply! On 2016/11/30 15:29:39, jochen wrote: > what about putting this into a sub directory with an OWNERS file? This CL is all that's needed for running the experiment. I guess the features.h/cc could go in a 'desktop/' subdirectory but the owners for such a directory should probably be the same as chrome/browser?
The CQ bit was checked by manzagop@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/11/30 15:58:03, manzagop wrote: > Thanks for the quick reply! > > On 2016/11/30 15:29:39, jochen wrote: > > what about putting this into a sub directory with an OWNERS file? > > This CL is all that's needed for running the experiment. I guess the > features.h/cc could go in a 'desktop/' subdirectory but the owners for > such a directory should probably be the same as chrome/browser? Right, most Features should be in the anonymous namespace, features.h is really only for sharing Features that span multiple files like this one (which should be rare). Having it top-level makes sense to me since it's used in two top-level files right now. Maybe add a comment to features.h stating that it should only be used when the Feature needs to be shared across multiple .cc's?
On 2016/11/30 16:11:24, gab wrote: (snip) > Maybe add a comment to features.h stating that it should only be used when the > Feature needs to be shared across multiple .cc's? Done.
The CQ bit was checked by manzagop@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2528173003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/2528173003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials_desktop.cc:118: base::FeatureList::IsEnabled(kDesktopFastShutdown); I'm not entirely clear why this is needed - is the intention here for this to tag the initial stability proto? Is that because you're concerned there will be a crash after the isEnabled call?
Ugh, seems I forgot to send my reply - here is! https://codereview.chromium.org/2528173003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/2528173003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_field_trials_desktop.cc:118: base::FeatureList::IsEnabled(kDesktopFastShutdown); On 2016/11/30 21:15:36, rkaplow wrote: > I'm not entirely clear why this is needed - is the intention here for this to > tag the initial stability proto? Is that because you're concerned there will be > a crash after the isEnabled call? > Yes! Because we're specifically interested in the stability of this experiment, I want to make sure the experiment gets registered as early as possible so there's more chances of the crash key callback having run and the system profile proto serialization having occurred. Otherwise you could imagine shutdown being initiated, chrome entering the experiment but then being killed before it gets to write the experiment to the system profile on disk. I realize this is not how it usually goes. You could argue that a chrome that crashes mid-run never triggered the experiment and that we don't want to count it. In this case, we think it might have a large impact on overall stability, so I think this is the right call. Actually, there's a second reason . We need to have entered the experiment so the rendez-vous metrics (https://codereview.chromium.org/2399233002/) are assigned to the experiment. https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?sq...
Friendly ping. Does this look good?
lgtm
Hi Jochen, Are you ok without the subdirectory? Thanks, Pierre
sorry for the long delay. I'm ok without a sub directory. typically, features are defined inside a features namespace, see e.g. content_features.h, so I'd propose to use either chrome::features or just namespace features here with that, lgtm
The CQ bit was checked by manzagop@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...
Thanks for the review! > typically, features are defined inside a features namespace, see e.g. > content_features.h, so I'd propose to use either chrome::features or just > namespace features here Done.
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 manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, hashimoto@chromium.org, jochen@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2528173003/#ps100001 (title: "Put features in 'features' namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480957635602240,
"parent_rev": "57dd05c42bfe334ef8617736e8ff55968a5fa891", "commit_rev":
"8544945a0ff3924b72d5eddd32fc900daef1941f"}
Message was sent while issue was closed.
Description was changed from ========== Desktop fast shutdown experiment Set up an experiment to see the impact of always using the fast exit path. For context, see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/AMB2Y7Sqj48/dis... Testing: launch with --enable-features=DesktopFastShutdown BUG=653647 ========== to ========== Desktop fast shutdown experiment Set up an experiment to see the impact of always using the fast exit path. For context, see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/AMB2Y7Sqj48/dis... Testing: launch with --enable-features=DesktopFastShutdown BUG=653647 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Desktop fast shutdown experiment Set up an experiment to see the impact of always using the fast exit path. For context, see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/AMB2Y7Sqj48/dis... Testing: launch with --enable-features=DesktopFastShutdown BUG=653647 ========== to ========== Desktop fast shutdown experiment Set up an experiment to see the impact of always using the fast exit path. For context, see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/AMB2Y7Sqj48/dis... Testing: launch with --enable-features=DesktopFastShutdown BUG=653647 Committed: https://crrev.com/011b6142b57f328cbeac1e7a903a36a15eba2cf7 Cr-Commit-Position: refs/heads/master@{#436328} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/011b6142b57f328cbeac1e7a903a36a15eba2cf7 Cr-Commit-Position: refs/heads/master@{#436328} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
