Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
393753002:
mac: Add a profiler for startup time. (Closed)
Descriptionmac: Add a profiler for startup time.
The profiler records UMA metrics for several key phases in Chrome startup.
BUG=393819
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283974
Patch Set 1 #Patch Set 2 : test #
Total comments: 19
Patch Set 3 : Respond to comments from isherman. #
Total comments: 8
Patch Set 4 : Comments from isherman, round 2. #
Total comments: 18
Patch Set 5 : Remove unused comment. #Patch Set 6 : Send histogram to UMA. #
Total comments: 12
Patch Set 7 : Comments from jeremy. #
Total comments: 4
Patch Set 8 : Comments from jeremy, round two. #Messages
Total messages: 21 (0 generated)
mark: Please review.
I’m delegating this one to Jeremy (for Mac startup stuff) and isherman (for UMA stuff and also a Mac guy).
Thanks for putting this together Erik! I'll take a look tomorrow morning my time.
You're adding a lot of metrics here. I bet that you'll find that only one or two are useful in the long-run. Please file a bug to come back to this and see if this can be cleaned up, or whether all seven histograms will be useful long-term. (Note: One advantage of having fewer histograms is that people looking at the dashboard will have a better chance of knowing which data they should be looking at.) https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:29: map_.insert(std::make_pair(location, base::Time::Now())); nit: IMO "map_[location] = base::Time::Now();" is clearer. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:38: for (std::map<Location, base::Time>::iterator it = map_.begin(); nit: Can this be a const_iterator? https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:48: #define HISTOGRAM_PREFIX "OSX.Startup." Please return a string and use normal string concatenation operations rather than defining a preprocessor macro. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:73: int buckets = 100; Are you sure that the parameters for, say, UMA_HISTOGRAM_MEDIUM_TIMES aren't matched closely enough to your needs? https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:76: // each different name. Given that you do this below, why not just inline the HistogramName() method? Alternately, you could avoid using the macro to avoid needing the switch stmt below. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... File chrome/browser/mac/mac_startup_profiler.h (right): https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.h:57: std::map<Location, base::Time> map_; nit: Please give this a more descriptive name, e.g. profiled_times_. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.h:60: bool launch_time_valid_; nit: Please document all of the member variables. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.h:60: bool launch_time_valid_; nit: Can you omit launch_time_valid_, and just check launch_time_.is_null()? https://codereview.chromium.org/393753002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/393753002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18927: + The amount of time that elapses between the launch of Chrome and the Optional nit: I'd change the tense from present ("elapses") to past ("elapsed")
isherman: PTAL Filed bug to examine histograms to determine which ones are useful here: https://code.google.com/p/chromium/issues/detail?id=394087 https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:29: map_.insert(std::make_pair(location, base::Time::Now())); On 2014/07/15 18:14:24, Ilya Sherman wrote: > nit: IMO "map_[location] = base::Time::Now();" is clearer. Done. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:38: for (std::map<Location, base::Time>::iterator it = map_.begin(); On 2014/07/15 18:14:24, Ilya Sherman wrote: > nit: Can this be a const_iterator? yes, done https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:48: #define HISTOGRAM_PREFIX "OSX.Startup." On 2014/07/15 18:14:24, Ilya Sherman wrote: > Please return a string and use normal string concatenation operations rather > than defining a preprocessor macro. The preprocessor macro uses fewer lines of code, and is more performant. *shrug* I've done as you've suggested. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:73: int buckets = 100; On 2014/07/15 18:14:24, Ilya Sherman wrote: > Are you sure that the parameters for, say, UMA_HISTOGRAM_MEDIUM_TIMES aren't > matched closely enough to your needs? This is no longer relevant, since I'm not using a MACRO anymore. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:76: // each different name. On 2014/07/15 18:14:24, Ilya Sherman wrote: > Given that you do this below, why not just inline the HistogramName() method? > Alternately, you could avoid using the macro to avoid needing the switch stmt > below. inlining HistogramName() does not actually work (there's still only 1 pointer made from the UMA_HISTOGRAM_CUSTOM_TIMES macro). I've gone ahead and removed the macros entirely. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... File chrome/browser/mac/mac_startup_profiler.h (right): https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.h:57: std::map<Location, base::Time> map_; On 2014/07/15 18:14:24, Ilya Sherman wrote: > nit: Please give this a more descriptive name, e.g. profiled_times_. Done. https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.h:60: bool launch_time_valid_; On 2014/07/15 18:14:24, Ilya Sherman wrote: > nit: Can you omit launch_time_valid_, and just check launch_time_.is_null()? yes, done https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.h:60: bool launch_time_valid_; On 2014/07/15 18:14:24, Ilya Sherman wrote: > nit: Please document all of the member variables. Done. https://codereview.chromium.org/393753002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/393753002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18927: + The amount of time that elapses between the launch of Chrome and the On 2014/07/15 18:14:24, Ilya Sherman wrote: > Optional nit: I'd change the tense from present ("elapses") to past ("elapsed") Done.
Metrics LGTM % nits: https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/20001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:76: // each different name. On 2014/07/15 21:39:42, erikchen1 wrote: > On 2014/07/15 18:14:24, Ilya Sherman wrote: > > Given that you do this below, why not just inline the HistogramName() method? > > Alternately, you could avoid using the macro to avoid needing the switch stmt > > below. > > inlining HistogramName() does not actually work (there's still only 1 pointer > made from the UMA_HISTOGRAM_CUSTOM_TIMES macro). I've gone ahead and removed the > macros entirely. FWIW, by inlining HistogramName(), I meant not having a |name| variable, and instead just listing the exact histogram name at each spot. But, yes, this is fine too. https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:47: std::string base("OSX.Startup."); Optional nit: Perhaps |prefix| would be a slightly clearer name for this variable? https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:48: const char* extension; Hmm, why did you decide to declare a variable for this, rather than keeping the pattern of |return base + "AwakeFromNib";| from the previous version? https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:73: return base; nit: If you keep this structure, please shorten these two lines to "return base + extension;" https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:78: const char* name = HistogramName(location).c_str(); nit: Why are you extracting out the C-string? FactoryTimeGet accepts an std::string.
https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:47: std::string base("OSX.Startup."); On 2014/07/15 22:42:00, Ilya Sherman wrote: > Optional nit: Perhaps |prefix| would be a slightly clearer name for this > variable? Done. https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:48: const char* extension; On 2014/07/15 22:42:00, Ilya Sherman wrote: > Hmm, why did you decide to declare a variable for this, rather than keeping the > pattern of |return base + "AwakeFromNib";| from the previous version? Good question. I changed to the previous pattern. https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:73: return base; On 2014/07/15 22:42:00, Ilya Sherman wrote: > nit: If you keep this structure, please shorten these two lines to "return base > + extension;" N/A after previous comment. https://codereview.chromium.org/393753002/diff/80001/chrome/browser/mac/mac_s... chrome/browser/mac/mac_startup_profiler.cc:78: const char* name = HistogramName(location).c_str(); On 2014/07/15 22:42:00, Ilya Sherman wrote: > nit: Why are you extracting out the C-string? FactoryTimeGet accepts an > std::string. Right you are.
On the assumption that the plan is to use the stats collected in this CL to analyze things locally, does using Trace events rather than histograms make more sense? There's a long tail of slow startups on end user machines, is that something you're looking at or are you looking at local profiles at the moment? https://codereview.chromium.org/393753002/diff/100001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:368: // Nothing here right now. nit: Remove this comment ? https://codereview.chromium.org/393753002/diff/100001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_mac.mm (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_mac.mm:159: MacStartupProfiler::GetInstance()->RecordLaunchTime(); Is this recording something different than startup_metric_utils::RecordMainEntryPointTime() ? https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:71: int bucket_count = 100; The histogram times/bucket count were a big issue when I was looking at startup time. There is a long tail on these numbers on end user machines, and most of the buckets will likely be lower than 10 milliseconds. I ended up having to tweak these a few times, what are your thoughts? To be clear, I have no issues with this - just something I ran into... https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:77: name, min, max, bucket_count, base::HistogramBase::kNoFlags); This is just a local histogram and doesn't go to UMA, right? https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.h (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:15: class MacStartupProfiler { This seems to parallel the code in components/startup_metric_utils/ - do you think this should stay separate or should we merge the two and adding the calls for the non-Mac-specific parts to cross-platform code so we're recording these stats for all platforms where it makes sense? https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:39: // has launched. We currently record Startup.BrowserMessageLoopStartTimeFromMainEntry and Startup.BrowserMessageLoopStartTime , the difference between these is when we start the clock - at process creation time or main entry. The first one is to try to reduce noise by removing the time it takes to load the Chrome binary.
jeremy: PTAL These stats are not for local analysis. They are intended to provide more details on Mac startup times in the wild. I've looked at the existing UMA stats for startup times and seen the long tails. Has recording the extra information actually been useful? If startup is taking more than 1 minute, Chrome has horribly failed and the difference between 1 minute and 60 minutes doesn't matter. https://codereview.chromium.org/393753002/diff/100001/chrome/browser/app_cont... File chrome/browser/app_controller_mac.mm (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/app_cont... chrome/browser/app_controller_mac.mm:368: // Nothing here right now. On 2014/07/16 08:19:23, jeremy wrote: > nit: Remove this comment ? Done. https://codereview.chromium.org/393753002/diff/100001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_mac.mm (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_mac.mm:159: MacStartupProfiler::GetInstance()->RecordLaunchTime(); On 2014/07/16 08:19:23, jeremy wrote: > Is this recording something different than > startup_metric_utils::RecordMainEntryPointTime() ? No. They are functionally equivalent, with the difference that this reduces the number of files that touch mac_startup_profiler. https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:71: int bucket_count = 100; On 2014/07/16 08:19:24, jeremy wrote: > The histogram times/bucket count were a big issue when I was looking at startup > time. There is a long tail on these numbers on end user machines, and most of > the buckets will likely be lower than 10 milliseconds. I ended up having to > tweak these a few times, what are your thoughts? > To be clear, I have no issues with this - just something I ran into... I prefer to restrict my histogram ranges to the values that I care about. There is no effective difference between 1 minute and 30. Something has gone horribly wrong. https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:77: name, min, max, bucket_count, base::HistogramBase::kNoFlags); On 2014/07/16 08:19:23, jeremy wrote: > This is just a local histogram and doesn't go to UMA, right? That is not correct. These metrics are going to UMA. https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.h (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:15: class MacStartupProfiler { On 2014/07/16 08:19:24, jeremy wrote: > This seems to parallel the code in components/startup_metric_utils/ - do you > think this should stay separate or should we merge the two and adding the calls > for the non-Mac-specific parts to cross-platform code so we're recording these > stats for all platforms where it makes sense? I like to only record histograms that are intended to be used. Of the seven histograms, 4 of them are platform-independent. If you intend to use them (or know someone who does), I will gladly add them to the other platforms, and port this code into components/startup_metric_utils. Otherwise, I will not move this class. https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:39: // has launched. On 2014/07/16 08:19:24, jeremy wrote: > We currently record Startup.BrowserMessageLoopStartTimeFromMainEntry and > Startup.BrowserMessageLoopStartTime , the difference between these is when we > start the clock - at process creation time or main entry. > The first one is to try to reduce noise by removing the time it takes to load > the Chrome binary. Ah, that is good to know. Thanks!
https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:77: name, min, max, bucket_count, base::HistogramBase::kNoFlags); On 2014/07/16 22:00:33, erikchen1 wrote: > On 2014/07/16 08:19:23, jeremy wrote: > > This is just a local histogram and doesn't go to UMA, right? > > That is not correct. These metrics are going to UMA. Jeremy noticed something important that I didn't: You're not setting the UMA flag, so these histograms won't be uploaded.
https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:77: name, min, max, bucket_count, base::HistogramBase::kNoFlags); On 2014/07/16 22:02:40, Ilya Sherman wrote: > On 2014/07/16 22:00:33, erikchen1 wrote: > > On 2014/07/16 08:19:23, jeremy wrote: > > > This is just a local histogram and doesn't go to UMA, right? > > > > That is not correct. These metrics are going to UMA. > > Jeremy noticed something important that I didn't: You're not setting the UMA > flag, so these histograms won't be uploaded. Ah, I see. I've fixed this.
https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:77: name, min, max, bucket_count, base::HistogramBase::kNoFlags); On 2014/07/16 22:12:18, erikchen1 wrote: > On 2014/07/16 22:02:40, Ilya Sherman wrote: > > On 2014/07/16 22:00:33, erikchen1 wrote: > > > On 2014/07/16 08:19:23, jeremy wrote: > > > > This is just a local histogram and doesn't go to UMA, right? > > > > > > That is not correct. These metrics are going to UMA. > > > > Jeremy noticed something important that I didn't: You're not setting the UMA > > flag, so these histograms won't be uploaded. > > Ah, I see. I've fixed this. Thanks, LG.
(please ignore duplicate comments below) OK, thanks for the clarifications, the piece I was missing was the inclusion of the UMA flag. Another important bit of information is that we can very easily add tracking and alerting on histograms in the perf dashboard so if you end up optimizing these and want to prevent regressions that's super easy. sidenote: if you haven't talked to leng@ and aiolos@, might be good to do so - I think they are also working on things connected to this space. > Has recording the extra information actually been useful? The way I tried to tackle this was to try to figure out what parts of startup were slower on the long tail. I never got the chance to finish analyzing it, the place I left off was recording stats for "long startups". The part I missed was the need to record the same stats I did for long startups, for regular startups so I could compare the distribution of events between the 2 groups. Just recording numbers for long startups didn't give me enough information. https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:22: launch_time_ = base::Time::Now(); startup_metric_utils also captures the process start time. Can you expose the timestamp used there (startup_metric_utils::MainEntryPointTimeInternal() and access that here). If nothing else, this means our startup metrics are using the same reference time so we can compare them easily if need be. https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:47: std::string prefix("OSX.Startup."); Any reason not to call these Startup.OSX so that the various startup UMAs are clustered together? https://codereview.chromium.org/393753002/diff/100001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:71: int bucket_count = 100; On an IO bound machine you may see some of these take over a minute, as long as you only care about this range, that's fine. https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:47: std::string prefix("OSX.Startup."); Startup.OSX so this is clustered together with other startup histograms ? https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.h (right): https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:40: void RecordLaunchTime(); Can you remove this and instead use the timestamp calculated in startup_metric_utils::MainEntryPointTimeInternal() ? That way our startup stats will be comparable to each ohter. https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:42: // Saves the current time for later processing. nit: this comment isn't very clear, how about something like: Record timestamp for the given location event ? https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:46: void RecordMetrics(); nit: // Call once to record all UMA metrics for all profiled locations ? https://codereview.chromium.org/393753002/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/393753002/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18927: + The amount of time that elapsed between the launch of Chrome and the "launch of Chrome" -> "main entry" here and below. https://codereview.chromium.org/393753002/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18932: +<histogram name="OSX.Startup.DidFinishLaunching" units="milliseconds"> How about calling this .DockIconHasFinishedBouncing or somesuch, that seems to be the metric people are interested in and will make it easier for the uninitiated to find ?
jeremy: PTAL I've sent email to leng@ and aiolos@ to make sure that we coordinate our efforts if we're all working on Mac startup performance. Regarding the long tails on the existing startup metrics: My first thought was that the 10min+ entries probably come from system clock jumps? base::Time::Now() makes no guarantees about this. (For Startup.BrowserMessageLoopStartTime, 0.01% of the entries are 0ms. This is almost certainly from clock jumps. Assuming that clock jumps are time symmetric, that implies that ~0.01% of the long tail is also from clock jumps. I guess that doesn't account for the ~1% of launches that takes 40+ seconds) https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:47: std::string prefix("OSX.Startup."); On 2014/07/17 11:10:18, jeremy wrote: > Startup.OSX so this is clustered together with other startup histograms ? Done. https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.h (right): https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:40: void RecordLaunchTime(); On 2014/07/17 11:10:18, jeremy wrote: > Can you remove this and instead use the timestamp calculated in > startup_metric_utils::MainEntryPointTimeInternal() ? > > That way our startup stats will be comparable to each ohter. Done. https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:42: // Saves the current time for later processing. On 2014/07/17 11:10:18, jeremy wrote: > nit: this comment isn't very clear, how about something like: > > Record timestamp for the given location event ? Used your suggested comment. https://codereview.chromium.org/393753002/diff/140001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.h:46: void RecordMetrics(); On 2014/07/17 11:10:18, jeremy wrote: > nit: > // Call once to record all UMA metrics for all profiled locations ? Used your suggested comment. https://codereview.chromium.org/393753002/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/393753002/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18927: + The amount of time that elapsed between the launch of Chrome and the On 2014/07/17 11:10:18, jeremy wrote: > "launch of Chrome" -> "main entry" > here and below. Done. https://codereview.chromium.org/393753002/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18932: +<histogram name="OSX.Startup.DidFinishLaunching" units="milliseconds"> On 2014/07/17 11:10:18, jeremy wrote: > How about calling this .DockIconHasFinishedBouncing or somesuch, that seems to > be the metric people are interested in and will make it easier for the > uninitiated to find ? I changed it to Startup.OSX.DockIconWillFinishBouncing, and updated the comment to be more accurate.
LGTM! https://codereview.chromium.org/393753002/diff/180001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/180001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:27: const base::Time* launch_time = startup_metric_utils::MainEntryPointTime(); nit: launch_time -> main_entry_time or somesuch https://codereview.chromium.org/393753002/diff/180001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/393753002/diff/180001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:30449: +<histogram name="Startup.OSX.DockIconWillFinishBouncing" units="milliseconds"> DidFinishBouncing ?
https://codereview.chromium.org/393753002/diff/180001/chrome/browser/mac/mac_... File chrome/browser/mac/mac_startup_profiler.cc (right): https://codereview.chromium.org/393753002/diff/180001/chrome/browser/mac/mac_... chrome/browser/mac/mac_startup_profiler.cc:27: const base::Time* launch_time = startup_metric_utils::MainEntryPointTime(); On 2014/07/17 20:58:02, jeremy wrote: > nit: launch_time -> main_entry_time or somesuch I used your suggestion of main_entry_time. https://codereview.chromium.org/393753002/diff/180001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/393753002/diff/180001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:30449: +<histogram name="Startup.OSX.DockIconWillFinishBouncing" units="milliseconds"> On 2014/07/17 20:58:02, jeremy wrote: > DidFinishBouncing ? I changed the name to WillFinishBouncing as its more accurate: the current bounce animation is still allowed to continue. If we measured DidFinishBouncing, all our times would be rounded up to the nearest 0.3 seconds or so.
sky: Looking for an owner review of chrome/browser/*
LGTM
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/393753002/200001
Message was sent while issue was closed.
Change committed as 283974 |
