Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2318373003:
Record shutdown type UMA (Closed)
DescriptionRecord shutdown type UMA
To investigate how frequently unclean shutdown is performed.
Start recording a new UMA Shutdown.ShutdownType.
Update prefs::kShutdownType when Chrome is performing unclean shutdown with SessionEnding().
BUG=644649
Committed: https://crrev.com/dc347b79180cd2880ee49067abb295a00ab3d7b6
Cr-Commit-Position: refs/heads/master@{#419123}
Patch Set 1 #Patch Set 2 : Record shutdown type UMA #
Total comments: 4
Patch Set 3 : Address comments #
Total comments: 4
Patch Set 4 : Clear kRestartLastSessionOnShutdown #
Total comments: 2
Patch Set 5 : Fix comment #
Total comments: 5
Patch Set 6 : Add description #
Dependent Patchsets: Messages
Total messages: 48 (32 generated)
Description was changed from ========== Record shutdown type UMA To investigate how frequently unclean shutdown is performed. BUG=644649 ========== to ========== Record shutdown type UMA To investigate how frequently unclean shutdown is performed. Start recording a new UMA Shutdown.ShutdownType. Update prefs::kShutdownType when Chrome is performing unclean shutdown with SessionEnding(). BUG=644649 ==========
The CQ bit was checked by hashimoto@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by hashimoto@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hashimoto@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.
hashimoto@chromium.org changed reviewers: + sky@chromium.org
sky@, could you review this change?
https://codereview.chromium.org/2318373003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_shutdown.h (right): https://codereview.chromium.org/2318373003/diff/20001/chrome/browser/browser_... chrome/browser/browser_shutdown.h:38: SHUTDOWN_TYPE_MAX_VALUE, I'm not a fan of this style. It means the enum has a fake value that isn't legal, yet has to be dealt with in switch statements because it's defined. How about a constant instead? constexpr kMaxShutdownValue = static_cast<int>(END_SESSION) + 1;? https://codereview.chromium.org/2318373003/diff/20001/chrome/browser/lifetime... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/2318373003/diff/20001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.cc:315: g_browser_process->local_state()->SetInteger(prefs::kShutdownType, There is a bunch of other stuff ShutdownPreThreadsStop() sets in prefs as well. Seems like both this and ShutdownPreThreadsStop should call a common function update prefs.
The CQ bit was checked by hashimoto@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.
PTAL https://codereview.chromium.org/2318373003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_shutdown.h (right): https://codereview.chromium.org/2318373003/diff/20001/chrome/browser/browser_... chrome/browser/browser_shutdown.h:38: SHUTDOWN_TYPE_MAX_VALUE, On 2016/09/09 15:40:56, sky wrote: > I'm not a fan of this style. It means the enum has a fake value that isn't > legal, yet has to be dealt with in switch statements because it's defined. How > about a constant instead? > > constexpr kMaxShutdownValue = static_cast<int>(END_SESSION) + 1;? Sounds good. Done. https://codereview.chromium.org/2318373003/diff/20001/chrome/browser/lifetime... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/2318373003/diff/20001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.cc:315: g_browser_process->local_state()->SetInteger(prefs::kShutdownType, On 2016/09/09 15:40:56, sky wrote: > There is a bunch of other stuff ShutdownPreThreadsStop() sets in prefs as well. > Seems like both this and ShutdownPreThreadsStop should call a common function > update prefs. Added a new function RecordShutdownInfoPrefs to browser_shutdown.
https://codereview.chromium.org/2318373003/diff/40001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2318373003/diff/40001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:177: if (prefs->HasPrefPath(prefs::kRestartLastSessionOnShutdown)) { Don't we want to clear this one too?
https://codereview.chromium.org/2318373003/diff/40001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2318373003/diff/40001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:177: if (prefs->HasPrefPath(prefs::kRestartLastSessionOnShutdown)) { On 2016/09/12 16:58:20, sky wrote: > Don't we want to clear this one too? I think kRestartLastSessionOnShutdown is different from kShutdownType, kShutdownNumProcesses, and kShutdownNumProcessesSlow. The former is used to control the behavior of the ongoing browser shutdown (btw I couldn't understand why pref is used for this). The latter is used to report stats about the shutdown on the next browser startup. IMHO mixing these two different groups of prefs makes the new function RecordShutdownInfoPrefs unnecessarily complicated. After all, SessionEnding() in application_lifetime.cc has been ignoring kRestartLastSessionOnShutdown for long, and I think there is no strong reason to change this. WDYT?
https://codereview.chromium.org/2318373003/diff/40001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2318373003/diff/40001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:177: if (prefs->HasPrefPath(prefs::kRestartLastSessionOnShutdown)) { On 2016/09/13 01:19:49, hashimoto wrote: > On 2016/09/12 16:58:20, sky wrote: > > Don't we want to clear this one too? > > I think kRestartLastSessionOnShutdown is different from kShutdownType, > kShutdownNumProcesses, and kShutdownNumProcessesSlow. > The former is used to control the behavior of the ongoing browser shutdown (btw > I couldn't understand why pref is used for this). > The latter is used to report stats about the shutdown on the next browser > startup. > IMHO mixing these two different groups of prefs makes the new function > RecordShutdownInfoPrefs unnecessarily complicated. > > After all, SessionEnding() in application_lifetime.cc has been ignoring > kRestartLastSessionOnShutdown for long, and I think there is no strong reason to > change this. > WDYT? My understanding is that if kRestartLastSessionOnShutdown is true, then when you exit the browser we restart it right away. kRestartLastSessionOnShutdown is set to true in AttemptRestart. If for whatever reason AttemptRestart fails, kRestartLastSessionOnShutdown is still true. If you don't change the common shutdown code to clear this pref, then we may attempt to restart chrome at a later date after the user logs off. That's why I think the session-end path should also clear this pref. I understand this is not an explicit part of the change you are after, but sometimes when you move code around you see bugs and other things that need to change. Since you're moving code a couple of lines above this it makes to fix it at the same time.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2318373003/diff/40001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2318373003/diff/40001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:177: if (prefs->HasPrefPath(prefs::kRestartLastSessionOnShutdown)) { On 2016/09/13 02:17:59, sky wrote: > On 2016/09/13 01:19:49, hashimoto wrote: > > On 2016/09/12 16:58:20, sky wrote: > > > Don't we want to clear this one too? > > > > I think kRestartLastSessionOnShutdown is different from kShutdownType, > > kShutdownNumProcesses, and kShutdownNumProcessesSlow. > > The former is used to control the behavior of the ongoing browser shutdown > (btw > > I couldn't understand why pref is used for this). > > The latter is used to report stats about the shutdown on the next browser > > startup. > > IMHO mixing these two different groups of prefs makes the new function > > RecordShutdownInfoPrefs unnecessarily complicated. > > > > After all, SessionEnding() in application_lifetime.cc has been ignoring > > kRestartLastSessionOnShutdown for long, and I think there is no strong reason > to > > change this. > > WDYT? > > My understanding is that if kRestartLastSessionOnShutdown is true, then when you > exit the browser we restart it right away. kRestartLastSessionOnShutdown is set > to true in AttemptRestart. If for whatever reason AttemptRestart fails, > kRestartLastSessionOnShutdown is still true. If you don't change the common > shutdown code to clear this pref, then we may attempt to restart chrome at a > later date after the user logs off. That's why I think the session-end path > should also clear this pref. > > I understand this is not an explicit part of the change you are after, but > sometimes when you move code around you see bugs and other things that need to > change. Since you're moving code a couple of lines above this it makes to fix it > at the same time. OK, done. Moved kRestartLastSessionOnShutdown related code to RecordShutdownInfoPrefs.
The CQ bit was checked by hashimoto@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.
Thanks! LGTM https://codereview.chromium.org/2318373003/diff/80001/chrome/browser/browser_... File chrome/browser/browser_shutdown.h (right): https://codereview.chromium.org/2318373003/diff/80001/chrome/browser/browser_... chrome/browser/browser_shutdown.h:57: // Records the shutdown related prefs, and returns true when we should restart How about: Returns true if the browser should be restarted on exit.
The CQ bit was checked by hashimoto@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...
hashimoto@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@, could you review tools/metrics/histograms/histograms.xml? https://codereview.chromium.org/2318373003/diff/80001/chrome/browser/browser_... File chrome/browser/browser_shutdown.h (right): https://codereview.chromium.org/2318373003/diff/80001/chrome/browser/browser_... chrome/browser/browser_shutdown.h:57: // Records the shutdown related prefs, and returns true when we should restart On 2016/09/13 13:21:14, sky wrote: > How about: Returns true if the browser should be restarted on exit. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:55511: +<histogram name="Shutdown.ShutdownType" enum="ShutdownType"> This seems to be adding a new top-level category ("Shutdown."). We generally advise against this, unless you're planning to add lots of histograms under this category. Otherwise, please re-use an existing prefix. https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:94514: + <int value="3" label="End session"/> It's not obvious to me from these what the differences between these are. What is end session vs. browser exit? How does browser exit compare to window close? Please add more information. You can expand on the description by putting text within the <int> </int> tags.
https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:55511: +<histogram name="Shutdown.ShutdownType" enum="ShutdownType"> On 2016/09/14 15:56:11, Alexei Svitkine (very slow) wrote: > This seems to be adding a new top-level category ("Shutdown."). We generally > advise against this, unless you're planning to add lots of histograms under this > category. > > Otherwise, please re-use an existing prefix. Actually, "Shutdown." is not new as there are already a number of histograms reported with this category. (https://cs.chromium.org/chromium/src/chrome/browser/browser_shutdown.cc?q=%5C...) The problem is that the existing histograms ("Shutdown.*.time", "Shutdown.*.time_per_process", "Shutdown.renderers.total", "Shutdown.renderers.slow") are not included in histograms.xml for some reason. I thought it's better to use "Shutdown." here to make it consistent with these existing histograms. If we are to avoid using "Shutdown.", the only category I find appropriate is "Chrome." If you think it's better, I'll upload a new change with "Chrome." WDYT? https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:94514: + <int value="3" label="End session"/> On 2016/09/14 15:56:11, Alexei Svitkine (very slow) wrote: > It's not obvious to me from these what the differences between these are. What > is end session vs. browser exit? How does browser exit compare to window close? > > Please add more information. You can expand on the description by putting text > within the <int> </int> tags. Done.
LGTM with a request for a follow-up change below. Thanks! https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:55511: +<histogram name="Shutdown.ShutdownType" enum="ShutdownType"> On 2016/09/15 06:38:37, hashimoto wrote: > On 2016/09/14 15:56:11, Alexei Svitkine (very slow) wrote: > > This seems to be adding a new top-level category ("Shutdown."). We generally > > advise against this, unless you're planning to add lots of histograms under > this > > category. > > > > Otherwise, please re-use an existing prefix. > > Actually, "Shutdown." is not new as there are already a number of histograms > reported with this category. > (https://cs.chromium.org/chromium/src/chrome/browser/browser_shutdown.cc?q=%5C...) > The problem is that the existing histograms ("Shutdown.*.time", > "Shutdown.*.time_per_process", "Shutdown.renderers.total", > "Shutdown.renderers.slow") are not included in histograms.xml for some reason. > > I thought it's better to use "Shutdown." here to make it consistent with these > existing histograms. > If we are to avoid using "Shutdown.", the only category I find appropriate is > "Chrome." > If you think it's better, I'll upload a new change with "Chrome." > WDYT? Ah, you are right. Looks like they're still listed in the internal histograms.xml file in google3 and don't have any owners. So keeping the Shutdown. prefix sounds good to me for this one. Given that you're looking at this area, do you mind taking a look at the existing metrics and a) moving their definitions to the open source histograms.xml and b) removing them if they're not useful - given that they have no owners I suspect no one has looked at them since we made the change to hide unowned histograms on the dashboards 2-3 years ago. So I suspect they're safe to simply be removed. (Unless you are interested in them since you're working in this area.)
On 2016/09/15 16:58:56, Alexei Svitkine (very slow) wrote: > LGTM with a request for a follow-up change below. Thanks! > > https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2318373003/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:55511: +<histogram > name="Shutdown.ShutdownType" enum="ShutdownType"> > On 2016/09/15 06:38:37, hashimoto wrote: > > On 2016/09/14 15:56:11, Alexei Svitkine (very slow) wrote: > > > This seems to be adding a new top-level category ("Shutdown."). We generally > > > advise against this, unless you're planning to add lots of histograms under > > this > > > category. > > > > > > Otherwise, please re-use an existing prefix. > > > > Actually, "Shutdown." is not new as there are already a number of histograms > > reported with this category. > > > (https://cs.chromium.org/chromium/src/chrome/browser/browser_shutdown.cc?q=%5C...) > > The problem is that the existing histograms ("Shutdown.*.time", > > "Shutdown.*.time_per_process", "Shutdown.renderers.total", > > "Shutdown.renderers.slow") are not included in histograms.xml for some reason. > > > > I thought it's better to use "Shutdown." here to make it consistent with these > > existing histograms. > > If we are to avoid using "Shutdown.", the only category I find appropriate is > > "Chrome." > > If you think it's better, I'll upload a new change with "Chrome." > > WDYT? > > Ah, you are right. Looks like they're still listed in the internal > histograms.xml file in google3 and don't have any owners. > > So keeping the Shutdown. prefix sounds good to me for this one. Given that > you're looking at this area, do you mind taking a look at the existing metrics > and a) moving their definitions to the open source histograms.xml and b) > removing them if they're not useful - given that they have no owners I suspect > no one has looked at them since we made the change to hide unowned histograms on > the dashboards 2-3 years ago. So I suspect they're safe to simply be removed. > (Unless you are interested in them since you're working in this area.) These existing histograms look useful to me. Made a CL to add them to histograms.xml. https://codereview.chromium.org/2348843002
The CQ bit was checked by hashimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2318373003/#ps120001 (title: "Add description")
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 ========== Record shutdown type UMA To investigate how frequently unclean shutdown is performed. Start recording a new UMA Shutdown.ShutdownType. Update prefs::kShutdownType when Chrome is performing unclean shutdown with SessionEnding(). BUG=644649 ========== to ========== Record shutdown type UMA To investigate how frequently unclean shutdown is performed. Start recording a new UMA Shutdown.ShutdownType. Update prefs::kShutdownType when Chrome is performing unclean shutdown with SessionEnding(). BUG=644649 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Record shutdown type UMA To investigate how frequently unclean shutdown is performed. Start recording a new UMA Shutdown.ShutdownType. Update prefs::kShutdownType when Chrome is performing unclean shutdown with SessionEnding(). BUG=644649 ========== to ========== Record shutdown type UMA To investigate how frequently unclean shutdown is performed. Start recording a new UMA Shutdown.ShutdownType. Update prefs::kShutdownType when Chrome is performing unclean shutdown with SessionEnding(). BUG=644649 Committed: https://crrev.com/dc347b79180cd2880ee49067abb295a00ab3d7b6 Cr-Commit-Position: refs/heads/master@{#419123} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/dc347b79180cd2880ee49067abb295a00ab3d7b6 Cr-Commit-Position: refs/heads/master@{#419123} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
