Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399233002:
Add histograms to the rendez-vous process (ProcessSingleton). (Closed)
DescriptionAdd histograms to the rendez-vous process (ProcessSingleton).
These will, among other things, help understand the potential impact of
a faster shutdown path.
Such histograms have tried to exist in the past and failed
(http://crrev.com/254112) but persistent UMA makes this possible again!
BUG=653647
Committed: https://crrev.com/439f54f40ed4c48703e517e5c140c4b6090ebdcc
Cr-Commit-Position: refs/heads/master@{#423987}
Patch Set 1 #Patch Set 2 : +comment about lacking fast notification path timing #
Total comments: 5
Patch Set 3 : comment on NotifyResult #
Total comments: 4
Patch Set 4 : merge up to r423894 #Patch Set 5 : no extra enum value for count #Patch Set 6 : fix compile #
Messages
Total messages: 37 (19 generated)
The CQ bit was checked by gab@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 ========== Add histograms to the rendez-vous process (ProcessSingleton). These will, among other things, help understand the potential impact of a faster shutdown path. BUG=653647 ========== to ========== Add histograms to the rendez-vous process (ProcessSingleton). These will, among other things, help understand the potential impact of a faster shutdown path. Such histograms have tried to exist in the past and failed (http://crrev.com/254112) but persistent UMA makes this possible again! BUG=653647 ==========
gab@chromium.org changed reviewers: + asvitkine@chromium.org
Alexei PTAL. I didn't find all the knobs to tickle to make persistent UMA work locally but I think this should give us data from the wild :)
manzagop@chromium.org changed reviewers: + manzagop@chromium.org
lgtm https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/process_... chrome/browser/process_singleton.h:52: enum NotifyResult { Add comment that these are logged as histograms and shouldn't be changed lightly?
lgtm % manzagop's comments Thanks!
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2399233002/#ps40001 (title: "comment on NotifyResult")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/process_... File chrome/browser/process_singleton.h (right): https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/process_... chrome/browser/process_singleton.h:52: enum NotifyResult { On 2016/10/07 14:23:29, manzagop wrote: > Add comment that these are logged as histograms and shouldn't be changed > lightly? Done.
bcwhite@chromium.org changed reviewers: + bcwhite@chromium.org
I don't think that histogram persistence isn't going to help you here because it doesn't get enabled until the field trials are evaluated and I'm pretty sure that has not happened by this point. If no rendezvous is done and this process continues to become a full-fledged browser, the histogram will be recorded, though not persistently... but that shouldn't be a problem since it'll get sent with the first metrics upload at the 1-minute mark. If a rendezvous is successful, though, and this process is exiting, we're going to have to be more clever... or do the recording on the side that received the rendezvous. https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1685: case ProcessSingleton::NUM_NOTIFY_RESULTS: Why this change?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/10/07 16:17:44, bcwhite wrote: > I don't think that histogram persistence isn't going to help you here because it > doesn't get enabled until the field trials are evaluated and I'm pretty sure > that has not happened by this point. It is I think. SetupMetrics() is done in PreCreateThreadsImpl [1] whereas the process singleton code is in PreMainMessageLoopRunImpl() [2] which happens after [3]. [1] https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?rc... [2] https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?rc... [3] https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?q=Pr... > > If no rendezvous is done and this process continues to become a full-fledged > browser, the histogram will be recorded, though not persistently... but that > shouldn't be a problem since it'll get sent with the first metrics upload at the > 1-minute mark. > > If a rendezvous is successful, though, and this process is exiting, we're going > to have to be more clever... or do the recording on the side that received the > rendezvous. https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1685: case ProcessSingleton::NUM_NOTIFY_RESULTS: On 2016/10/07 16:17:44, bcwhite wrote: > Why this change? Because it's better to enumerate all cases than to have a default switch as it allows clang to warn when a switch doesn't handle a new enum value.
gab@chromium.org changed reviewers: + sky@chromium.org
+sky for chrome/browser OWNER
https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2399233002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1685: case ProcessSingleton::NUM_NOTIFY_RESULTS: On 2016/10/07 16:34:14, gab wrote: > On 2016/10/07 16:17:44, bcwhite wrote: > > Why this change? > > Because it's better to enumerate all cases than to have a default switch as it > allows clang to warn when a switch doesn't handle a new enum value. Acknowledged.
https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1685: case ProcessSingleton::NUM_NOTIFY_RESULTS: Having to delete with fake enum values like this is awkward. What about a constant for the num results? e.g. class ProcessSingleton { ... LAST_VALUE = LOCK_ERROR, ... constexpr kNumNotifyResults = LAST_VALUE; https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:928: UMA_HISTOGRAM_MEDIUM_TIMES("Chrome.ProcessSingleton.TimeToNotify", Are you sure you don't want to differentiate these two vs 903-909? They are slightly different.
The CQ bit was checked by gab@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 Fri, Oct 7, 2016 at 12:47 PM, <sky@chromium.org> wrote: > > https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:1685: case > ProcessSingleton::NUM_NOTIFY_RESULTS: > Having to delete with fake enum values like this is awkward. What about I have to read what I write. 'delete' should be 'deal'. -Scott > a constant for the num results? e.g. > > class ProcessSingleton { > ... > LAST_VALUE = LOCK_ERROR, > ... > constexpr kNumNotifyResults = LAST_VALUE; > > https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/process_... > File chrome/browser/process_singleton_posix.cc (right): > > https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/process_... > chrome/browser/process_singleton_posix.cc:928: > UMA_HISTOGRAM_MEDIUM_TIMES("Chrome.ProcessSingleton.TimeToNotify", > Are you sure you don't want to differentiate these two vs 903-909? They > are slightly different. > > https://codereview.chromium.org/2399233002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1685: case ProcessSingleton::NUM_NOTIFY_RESULTS: On 2016/10/07 19:47:30, sky wrote: > Having to delete with fake enum values like this is awkward. What about a > constant for the num results? e.g. > > class ProcessSingleton { > ... > LAST_VALUE = LOCK_ERROR, > ... > constexpr kNumNotifyResults = LAST_VALUE; I like that a lot actually, done. https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/process_... File chrome/browser/process_singleton_posix.cc (right): https://codereview.chromium.org/2399233002/diff/40001/chrome/browser/process_... chrome/browser/process_singleton_posix.cc:928: UMA_HISTOGRAM_MEDIUM_TIMES("Chrome.ProcessSingleton.TimeToNotify", On 2016/10/07 19:47:30, sky wrote: > Are you sure you don't want to differentiate these two vs 903-909? They are > slightly different. Well it's still the time it took to achieve a result. The fact that it took longer because it was conflating with another Chrome is exactly the kind of thing that we're trying to measure for both 903 and 909.
LGTM
The CQ bit was unchecked by gab@chromium.org
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2399233002/#ps80001 (title: "no extra enum value for count")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, asvitkine@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2399233002/#ps100001 (title: "fix compile")
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 ========== Add histograms to the rendez-vous process (ProcessSingleton). These will, among other things, help understand the potential impact of a faster shutdown path. Such histograms have tried to exist in the past and failed (http://crrev.com/254112) but persistent UMA makes this possible again! BUG=653647 ========== to ========== Add histograms to the rendez-vous process (ProcessSingleton). These will, among other things, help understand the potential impact of a faster shutdown path. Such histograms have tried to exist in the past and failed (http://crrev.com/254112) but persistent UMA makes this possible again! 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 ========== Add histograms to the rendez-vous process (ProcessSingleton). These will, among other things, help understand the potential impact of a faster shutdown path. Such histograms have tried to exist in the past and failed (http://crrev.com/254112) but persistent UMA makes this possible again! BUG=653647 ========== to ========== Add histograms to the rendez-vous process (ProcessSingleton). These will, among other things, help understand the potential impact of a faster shutdown path. Such histograms have tried to exist in the past and failed (http://crrev.com/254112) but persistent UMA makes this possible again! BUG=653647 Committed: https://crrev.com/439f54f40ed4c48703e517e5c140c4b6090ebdcc Cr-Commit-Position: refs/heads/master@{#423987} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/439f54f40ed4c48703e517e5c140c4b6090ebdcc Cr-Commit-Position: refs/heads/master@{#423987} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
