Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1905293003:
Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. (Closed)
|
Created:
4 years, 8 months ago by Patrick Monette Modified:
4 years, 7 months ago CC:
chromium-reviews, grt+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks.
These tasks were previously SKIP_ON_SHUTDOWN and were sometime causing
a shutdown hang.
This change is behind an experiment called BrowserHangFixesExperiment.
BUG=592728, 478209
Committed: https://crrev.com/3a8ee8feb1c8f8a30191145cd6e13189bfa2d199
Cr-Commit-Position: refs/heads/master@{#390431}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Changed to feature #
Depends on Patchset: Messages
Total messages: 25 (11 generated)
Patchset #1 (id:1) has been deleted
pmonette@chromium.org changed reviewers: + manzagop@chromium.org
Take a first look. WDYT?
lgtm % seeking review from grt and someone from metrics https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_hangs_experiment.cc (right): https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.cc:17: // UMA reports the correct group. For my benefit, what gets reported in the case of an argument override? The non-overridden value, right? https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.cc:20: if (CommandLine::ForCurrentProcess()->HasSwitch( Need to check InitializedForCurrentProcess first? https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.cc:27: return base::StartsWith(group_name, "Enabled", base::CompareCase::SENSITIVE); Where does "Enabled" come from? Is it a standard group prefix? Might it be reused, eg in a test? Worth having a variable for it (constexpr)? https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_hangs_experiment.h (right): https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.h:10: // experiment. Reference the bug number? https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.h:13: bool IsBrowserHangsExperimentEnabled(); Wdyt of IsBrowserHangFixesExperimentEnabled to avoid misinterpretation? Please push back if you think it's overboard.
grt@chromium.org changed reviewers: + grt@chromium.org
https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_hangs_experiment.cc (right): https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.cc:10: #include "base/metrics/field_trial.h" use base/feature_list.h instead of field_trial.h? the docs say that the former supersedes the latter. it's a lot less typing (or copying-and-pasting, as the case may be): return base::FeatureList::IsEnabled(kYourFeature); https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_hangs_experiment.h (right): https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.h:8: // In order to demonstrate the impact of fixing hang bugs in relation to the nit: Fixes for browser hang bugs are deployed in a field trial in order to measure their impact.
Description was changed from ========== Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. These tasks were previously SKIP_ON_SHUTDOWN and were something causing a shutdown hang. This change is behind an experiment called BrowserHangsExperiment. BUG=592728, 478209 ========== to ========== Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. These tasks were previously SKIP_ON_SHUTDOWN and were something causing a shutdown hang. This change is behind an experiment called BrowserHangFixesExperiment. BUG=592728, 478209 ==========
ptal grt@ https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_hangs_experiment.cc (right): https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.cc:10: #include "base/metrics/field_trial.h" On 2016/04/25 13:37:53, grt wrote: > use base/feature_list.h instead of field_trial.h? the docs say that the former > supersedes the latter. it's a lot less typing (or copying-and-pasting, as the > case may be): > > return base::FeatureList::IsEnabled(kYourFeature); Ooohh shiny! https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.cc:17: // UMA reports the correct group. On 2016/04/25 13:37:39, manzagop wrote: > For my benefit, what gets reported in the case of an argument override? The > non-overridden value, right? The whole experiment is set server-side. There is a concept of "forcing_flag" while creating the experiment that will tell finch to consider people with those flags on their command line as a separate group. https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.cc:20: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2016/04/25 13:37:39, manzagop wrote: > Need to check InitializedForCurrentProcess first? No longer using CommandLine::ForCurrentProcess(). https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.cc:27: return base::StartsWith(group_name, "Enabled", base::CompareCase::SENSITIVE); On 2016/04/25 13:37:39, manzagop wrote: > Where does "Enabled" come from? Is it a standard group prefix? Might it be > reused, eg in a test? Worth having a variable for it (constexpr)? The only place it'll be reuse is in the finch config server-side. https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... File chrome/browser/browser_hangs_experiment.h (right): https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.h:8: // In order to demonstrate the impact of fixing hang bugs in relation to the On 2016/04/25 13:37:53, grt wrote: > nit: > > Fixes for browser hang bugs are deployed in a field trial in order to measure > their impact. Done. https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.h:10: // experiment. On 2016/04/25 13:37:39, manzagop wrote: > Reference the bug number? Done. https://codereview.chromium.org/1905293003/diff/20001/chrome/browser/browser_... chrome/browser/browser_hangs_experiment.h:13: bool IsBrowserHangsExperimentEnabled(); On 2016/04/25 13:37:39, manzagop wrote: > Wdyt of IsBrowserHangFixesExperimentEnabled to avoid misinterpretation? Please > push back if you think it's overboard. Done.
pmonette@chromium.org changed reviewers: + asvitkine@chromium.org
Hey asvitkine@ Take a look at chrome_features. Should you be an owner for this file? or should it get the same treatment as chrome_switches, url_constants and pref_names, where everyone is an owner? (chrome\common\OWNERS)
lgtm I'm ok not being an owner for it as the long term hope is that it becomes familiar to everyone and is easy enough to use and not make mistakes (unlike the old field trial API).
pmonette@chromium.org changed reviewers: + thakis@chromium.org
Thanks. Thakis@ PTAL at chrome_features*
On 2016/04/25 21:39:03, Patrick Monette wrote: > Thanks. > > Thakis@ PTAL at chrome_features* Ping
lgtm
Description was changed from ========== Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. These tasks were previously SKIP_ON_SHUTDOWN and were something causing a shutdown hang. This change is behind an experiment called BrowserHangFixesExperiment. BUG=592728, 478209 ========== to ========== Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. These tasks were previously SKIP_ON_SHUTDOWN and were sometime causing a shutdown hang. This change is behind an experiment called BrowserHangFixesExperiment. BUG=592728, 478209 ==========
lgtm I always thought shut-down would end up as "just terminate process without shutdown process" (this can happen at any time due to crashes anyway, so we need to handle that anyways). I'm surprised we have so much machinery for this.
Thanks! IIUC, there are some things we want to make sure we've saved to disk before shutdown like history/cookies etc. They are saved regularly while Chrome is running so that if an unclean shutdown happens (lost of power or crash), we don't lose the whole browsing session data but the last few minutes may be lost. We want to transition to a state where most tasks are CONTINUE_ON_SHUTDOWN as they usually shouldn't block shutdown.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org Link to the patchset: https://codereview.chromium.org/1905293003/#ps40001 (title: "Changed to feature")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905293003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905293003/40001
Message was sent while issue was closed.
Description was changed from ========== Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. These tasks were previously SKIP_ON_SHUTDOWN and were sometime causing a shutdown hang. This change is behind an experiment called BrowserHangFixesExperiment. BUG=592728, 478209 ========== to ========== Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. These tasks were previously SKIP_ON_SHUTDOWN and were sometime causing a shutdown hang. This change is behind an experiment called BrowserHangFixesExperiment. BUG=592728, 478209 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3a8ee8feb1c8f8a30191145cd6e13189bfa2d199 Cr-Commit-Position: refs/heads/master@{#390431}
Message was sent while issue was closed.
Description was changed from ========== Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. These tasks were previously SKIP_ON_SHUTDOWN and were sometime causing a shutdown hang. This change is behind an experiment called BrowserHangFixesExperiment. BUG=592728, 478209 ========== to ========== Use CONTINUE_ON_SHUTDOWN for IncidentReportingService tasks. These tasks were previously SKIP_ON_SHUTDOWN and were sometime causing a shutdown hang. This change is behind an experiment called BrowserHangFixesExperiment. BUG=592728, 478209 Committed: https://crrev.com/3a8ee8feb1c8f8a30191145cd6e13189bfa2d199 Cr-Commit-Position: refs/heads/master@{#390431} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
