Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773973002:
Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. (Closed)
DescriptionAdd Startup.BrowserView.FirstPaint / .CompositingEnded histograms.
Startup.BrowserView.FirstPaint measures how long does it take from browser
startup until BrowserView finished the initial paint of all its children.
Startup.BrowserView.CompositingEnded measures the time from browser startup
to the time the GPU has finished compositing after BrowserView has finished
painting its children. At this point the Browser interface is visible on
screen. Measures how much time does it take for GPU to actually paint the
first time.
BUG=704945, 589118
Review-Url: https://codereview.chromium.org/2773973002
Cr-Commit-Position: refs/heads/master@{#466009}
Committed: https://chromium.googlesource.com/chromium/src/+/3cfcb7cdbff23b208c96b7da2f42689421c4586e
Patch Set 1 #
Total comments: 12
Patch Set 2 : Add Cocoa "compatibility", fix remarks. #
Total comments: 2
Patch Set 3 : WasNonBrowserUIDisplayed --> WasMainWindowStartupInterrupted, combine the tests, report trace with … #
Total comments: 10
Patch Set 4 : Use more correct place to report timings in Cocoa, fix review remarks. #
Total comments: 20
Patch Set 5 : Rebase on recent Chromium, address sky's remarks. #
Total comments: 2
Patch Set 6 : BrowserView --> BrowserWindow, use factory method for class creation. #
Total comments: 24
Patch Set 7 : Fix gab's and asvitkine's remarks. #
Total comments: 3
Patch Set 8 : Don't use references for base::Time* classes. #
Total comments: 10
Patch Set 9 : Fix asvitkine's remarks. #
Total comments: 6
Patch Set 10 : Fix final review remarks. #Patch Set 11 : Rebase on top of master. #Patch Set 12 : Fix compilation on updated master. #Patch Set 13 : Fix compilation. #Patch Set 14 : OnCompositingStarted → OnCompositingEnded, as it's been restored and there are no plans to remove… #Patch Set 15 : Fix compilation error. #Patch Set 16 : Don't wait for histograms that are not reported during browser_tests. #Messages
Total messages: 52 (11 generated)
Description was changed from ========== Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. Startup.BrowserView.FirstPaint measures how long does it take from browser startup until BrowserView finished the initial paint of all its children. Startup.BrowserView.CompositingEnded measures the time from browser startup to the time the GPU has finished compositing after BrowserView has finished painting its children. At this point the Browser interface is visible on screen. Measures how much time does it take for GPU to actually paint the first time. BUG=None ========== to ========== Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. Startup.BrowserView.FirstPaint measures how long does it take from browser startup until BrowserView finished the initial paint of all its children. Startup.BrowserView.CompositingEnded measures the time from browser startup to the time the GPU has finished compositing after BrowserView has finished painting its children. At this point the Browser interface is visible on screen. Measures how much time does it take for GPU to actually paint the first time. BUG=None ==========
mblsha@yandex-team.ru changed reviewers: + pkasting@chromium.org
On 2017/03/24 15:12:22, themblsha wrote: > mailto:mblsha@yandex-team.ru changed reviewers: > + mailto:pkasting@chromium.org AFAIK currently there are Startup.BrowserWindowDisplay and Startup.FirstWebContents.NonEmptyPaint2, but they don't measure the moment when the actual Browser UI becomes visible for the user, we need to wait for the OnCompositingEnded() to actually measure that.
asvitkine@chromium.org changed reviewers: + gab@chromium.org
+gab who's a defacto owner of startup metrics Given the change is non-trivial this should probably have a crbug to and BUG= to track it.
Description was changed from ========== Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. Startup.BrowserView.FirstPaint measures how long does it take from browser startup until BrowserView finished the initial paint of all its children. Startup.BrowserView.CompositingEnded measures the time from browser startup to the time the GPU has finished compositing after BrowserView has finished painting its children. At this point the Browser interface is visible on screen. Measures how much time does it take for GPU to actually paint the first time. BUG=None ========== to ========== Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. Startup.BrowserView.FirstPaint measures how long does it take from browser startup until BrowserView finished the initial paint of all its children. Startup.BrowserView.CompositingEnded measures the time from browser startup to the time the GPU has finished compositing after BrowserView has finished painting its children. At this point the Browser interface is visible on screen. Measures how much time does it take for GPU to actually paint the first time. BUG=704945 ==========
On 2017/03/24 15:33:45, Alexei Svitkine (slow) wrote: > +gab who's a defacto owner of startup metrics > > Given the change is non-trivial this should probably have a crbug to and BUG= to > track it. Created https://bugs.chromium.org/p/chromium/issues/detail?id=704945
How is this different from Startup.FirstWebContents.NonEmptyPaint2?
On 2017/03/24 16:00:57, gab wrote: > How is this different from Startup.FirstWebContents.NonEmptyPaint2? Startup.FirstWebContents.NonEmptyPaint2 measures the first paint on the WebContents and is dependent on that page complexity, the new histograms measure when the actual Browser's UI (toolbar, tabs) are visible When restoring Chromium with a single about:blank page: Startup.BrowserWindowDisplay recorded 1 samples, mean = 901.0 Startup.FirstWebContents.NonEmptyPaint2 recorded 1 samples, mean = 1233.0 Startup.BrowserView.FirstPaint recorded 1 samples, mean = 1140.0 Startup.BrowserView.FirstPaint.CompositingEnded recorded 1 samples, mean = 1345.0 When restoring with chrome://ntp: Startup.BrowserWindowDisplay recorded 1 samples, mean = 786.0 Startup.FirstWebContents.NonEmptyPaint2 recorded 1 samples, mean = 1721.0 Startup.BrowserView.FirstPaint recorded 1 samples, mean = 1388.0 Startup.BrowserView.FirstPaint.CompositingEnded recorded 1 samples, mean = 1551.0
On 2017/03/24 16:25:53, themblsha wrote: > On 2017/03/24 16:00:57, gab wrote: > > How is this different from Startup.FirstWebContents.NonEmptyPaint2? > > Startup.FirstWebContents.NonEmptyPaint2 measures the first paint on the > WebContents and is dependent on that page complexity, the new histograms measure > when the actual Browser's UI (toolbar, tabs) are visible > > When restoring Chromium with a single about:blank page: > Startup.BrowserWindowDisplay recorded 1 samples, mean = 901.0 > Startup.FirstWebContents.NonEmptyPaint2 recorded 1 samples, mean = 1233.0 > Startup.BrowserView.FirstPaint recorded 1 samples, mean = 1140.0 > Startup.BrowserView.FirstPaint.CompositingEnded recorded 1 samples, mean = > 1345.0 > > When restoring with chrome://ntp: > Startup.BrowserWindowDisplay recorded 1 samples, mean = 786.0 > Startup.FirstWebContents.NonEmptyPaint2 recorded 1 samples, mean = 1721.0 > Startup.BrowserView.FirstPaint recorded 1 samples, mean = 1388.0 > Startup.BrowserView.FirstPaint.CompositingEnded recorded 1 samples, mean = > 1551.0 This cl needs a Mac implementation.
https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view_histogram_helper.cc (right): https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view_histogram_helper.cc:23: compositor->AddObserver(this); Nit: Prefer a ScopedObserver to manual add/remove calls. This will also allow removing the code from the destructor, and removing |compositor_|. (The existing DCHECKs that use it don't really buy much, I wouldn't bother trying to recreate them with the scoped observer.) https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view_histogram_helper.h:15: BrowserViewHistogramHelper(); Nit: Feel free to use =default https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view_histogram_helper.h:32: Nit: Blank line unnecessary https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:776: is_first_call = false; It seems like the only reason we need these guards is because the histogram helper is per-BrowserView rather than a singleton for the browser process. That also means that if multiple windows are opening, you'll really histogram the first paint times for the first window to paint, without capturing details of multiple windows. I don't know if this is a problem or not. What do you want to record when there are multiple windows? There are various solutions in this space, but it would be good to find something that lets us remove these statics and thus test this stuff from unit tests instead of browser tests. One such solution that preserves the existing behavior is to have the histogram helper object be a process singleton.
I expect this metric will not play well with background mode (chrome can be launched as a tray icon at which point the first window paint is meaningless -- could be days later...). See http://crbug.com/589118 for a similar issue. We should probably upgrade WasNonBrowserUIDisplayed() to WasMainWindowStartupInterrupted() or something and flag it when entering background mode as well (discarding all events that come after that). Also what's the difference between Startup.BrowserView.FirstPaint and Startup.BrowserWindowDisplay, should the latter be deprecated? https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:776: is_first_call = false; On 2017/03/24 22:39:18, Peter Kasting wrote: > It seems like the only reason we need these guards is because the histogram > helper is per-BrowserView rather than a singleton for the browser process. That > also means that if multiple windows are opening, you'll really histogram the > first paint times for the first window to paint, without capturing details of > multiple windows. I don't know if this is a problem or not. What do you want > to record when there are multiple windows? > > There are various solutions in this space, but it would be good to find > something that lets us remove these statics and thus test this stuff from unit > tests instead of browser tests. One such solution that preserves the existing > behavior is to have the histogram helper object be a process singleton. Not sure it makes sense to test this outside an integration (browser) test FWIW. It's based on process launch (which even if faked is a global variable). I don't think we test the other startup metrics FWIW -- i.e. they're tested by virtue of being collected in the wild and tracked via go/finch-chirp BrowserViewHistogramHelperTest.* as a unit test would merely be call X(), confirm X() was called... As a browser test at least it's an integration test that startup called X(). But overall I'd argue to not bother and remove BrowserViewHistogramHelperTest.* as we will see it in the histogram if it doesn't work (unless we're worried about the recording being flaky?). All of the other calls here have statics to only record the first call, some might be overkill and we probably could ensure each call is made exactly once but until someone tackles all of them, for consistency, it's better to have the statics in these calls as well? https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:789: UMA_HISTOGRAM_WITH_TEMPERATURE( Use UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE. Then could you grab a local trace by launching chrome via: chrome.exe --trace-startup --trace-startup-duration=10 --trace-stratup-file="mytrace.json" open the trace, zoom in on the registered startup events, and upload a screenshot to the bug to show how this metric's timing relates to the other existing startup metrics.
There are still some issues that nag me and I want to discuss with someone: 1. UI in Cocoa version is drawn on top of CALayers and they're composited using CoreAnimation. Chromium's GPU process is not used there at all, so this makes the ".CompositingEnded" histogram irrelevant (and I made them both to submit at the same time). 2. ChromeContentView seems to be a nice NSView that's always present in Chromium's windows, but there seems to be no way of knowing that all its children finished their -drawRect:'s as AppKit draws all the NSViews individually into their own CALayers. 3. https://bugs.chromium.org/p/chromium/issues/detail?id=671202 removed the OnCompositingEnded() so I switched to the OnCompositingStarted(). 4. Histogram names could probably be improved to reflect the intention without relying on the implementation details: Startup.BrowserWindow.FinishedPaintingChildren and Startup.BrowserWindow.FinishedPaintingChildren.ActuallyDrawnOnScreen? https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view_histogram_helper.cc (right): https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view_histogram_helper.cc:23: compositor->AddObserver(this); On 2017/03/24 22:39:17, Peter Kasting wrote: > Nit: Prefer a ScopedObserver to manual add/remove calls. > > This will also allow removing the code from the destructor, and removing > |compositor_|. (The existing DCHECKs that use it don't really buy much, I > wouldn't bother trying to recreate them with the scoped observer.) Done. https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view_histogram_helper.h:15: BrowserViewHistogramHelper(); On 2017/03/24 22:39:18, Peter Kasting wrote: > Nit: Feel free to use =default Seems to be no longer applicable with ScopedObserver in place. https://codereview.chromium.org/2773973002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view_histogram_helper.h:32: On 2017/03/24 22:39:18, Peter Kasting wrote: > Nit: Blank line unnecessary Done. https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:776: is_first_call = false; On 2017/03/27 15:21:43, gab wrote: > I don't think we test the other startup metrics FWIW -- i.e. they're tested by > virtue of being collected in the wild and tracked via go/finch-chirp We generally try test Yandex-specific histograms as it provides an early-warning system, as noticing that the histograms stopped being reported only after the release is sent to users could result in losing useful data. https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:789: UMA_HISTOGRAM_WITH_TEMPERATURE( On 2017/03/27 15:21:43, gab wrote: > Use UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE. > > Then could you grab a local trace by launching chrome via: > > chrome.exe --trace-startup --trace-startup-duration=10 > --trace-stratup-file="mytrace.json" > > open the trace, zoom in on the registered startup events, and upload a > screenshot to the bug to show how this metric's timing relates to the other > existing startup metrics. Done: https://bugs.chromium.org/p/chromium/issues/detail?id=704945#c2
On 2017/03/27 15:21:43, gab wrote: > I expect this metric will not play well with background mode (chrome can be > launched as a tray icon at which point the first window paint is meaningless -- > could be days later...). > > See http://crbug.com/589118 for a similar issue. We should probably upgrade > WasNonBrowserUIDisplayed() to WasMainWindowStartupInterrupted() or something and > flag it when entering background mode as well (discarding all events that come > after that). > > Also what's the difference between Startup.BrowserView.FirstPaint and > Startup.BrowserWindowDisplay, should the latter be deprecated? ping ^^^ ? https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/1/components/startup_metric_u... components/startup_metric_utils/browser/startup_metric_utils.cc:776: is_first_call = false; On 2017/03/31 14:32:31, themblsha wrote: > On 2017/03/27 15:21:43, gab wrote: > > I don't think we test the other startup metrics FWIW -- i.e. they're tested by > > virtue of being collected in the wild and tracked via go/finch-chirp > > We generally try test Yandex-specific histograms as it provides an early-warning > system, as noticing that the histograms stopped being reported only after the > release is sent to users could result in losing useful data. Okay then I would argue for a single browser test that ensures every startup metric monitored by startup_metric_utils has been logged (instead of one test per metric since the setup is the exact same and launching the browser once per metric is overkill).
On 2017/04/03 16:18:26, gab wrote: > > Also what's the difference between Startup.BrowserView.FirstPaint and > > Startup.BrowserWindowDisplay, should the latter be deprecated? > > ping ^^^ ? https://bugs.chromium.org/p/chromium/issues/attachment?aid=277888&inline=1 Startup.BrowserWindowDisplay marks the intent to show the first browser window, it's not visible at this point. Startup.BrowserView.FirstPaint marks the point of time when all the views had the chance to paint and we're waiting for the Compositor to swap the frame so it becomes visible. This is the time it takes for the main process to finish the initial first browser window paint. .CompositingEnded means that the user sees the painted browser window for the first time. There are two histograms because the GPU process overhead could change over time so it would become easier to guess what actually happened.
On 2017/04/04 17:31:33, themblsha wrote: > On 2017/04/03 16:18:26, gab wrote: > > > Also what's the difference between Startup.BrowserView.FirstPaint and > > > Startup.BrowserWindowDisplay, should the latter be deprecated? > > > > ping ^^^ ? > > https://bugs.chromium.org/p/chromium/issues/attachment?aid=277888&inline=1 > > Startup.BrowserWindowDisplay marks the intent to show the first browser window, > it's not visible at this point. Startup.BrowserView.FirstPaint marks the point > of time when all the views had the chance to paint and we're waiting for the > Compositor to swap the frame so it becomes visible. > > This is the time it takes for the main process to finish the initial first > browser window paint. .CompositingEnded means that the user sees the painted > browser window for the first time. > > There are two histograms because the GPU process overhead could change over time > so it would become easier to guess what actually happened. Thanks, still re. previous comments that (1) it won't play well with background mode as-is and (2) if we're going to have a browser test I'd rather we have a single one that confirms all startup metrics are recorded.
https://codereview.chromium.org/2773973002/diff/20001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/20001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:778: UMA_HISTOGRAM_WITH_TEMPERATURE(UMA_HISTOGRAM_LONG_TIMES_100, UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE for this one too
On 2017/04/04 20:44:08, gab wrote: > Thanks, still re. previous comments that (1) it won't play well with background > mode as-is Added WasMainWindowStartupInterrupted() which is triggered by either SetNonBrowserUIDisplayed() or the BackgroundModeManager::StartBackgroundMode() (if that's the background mode you meant). > (2) if we're going to have a browser test I'd rather we have a > single one that confirms all startup metrics are recorded. Done. https://codereview.chromium.org/2773973002/diff/20001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/20001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:778: UMA_HISTOGRAM_WITH_TEMPERATURE(UMA_HISTOGRAM_LONG_TIMES_100, On 2017/04/04 20:44:21, gab wrote: > UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE for this one too Done.
mblsha@yandex-team.ru changed reviewers: + tapted@chromium.org
+tapted Trent, maybe you could advice me on Cocoa Mac drawing: On Views (MacViews included) I'm measuring both the moment when all the children of the BrowserView finished painting for the first time, and the next CompositingEnded right after that. The intention is to monitor the moment in time when the Browser UI becomes visible for the first time after start (and the GPU process overhead, if any). Trouble points: 1. Monitoring something comparable to ComposingEnded on Cocoa seems unfeasible, as CoreAnimation doesn't seem to allow this sort of control. 2. Currently I'm monitoring the start of painting the children, as all the NSViews are painting into their own CALayers. I've thought of adding a dummy deeply-nested child that'll be painted last but this seems too hacky to be feasible. Or the Cocoa is going to be deprecated relatively soon and this shouldn't be a concern at all?
Awesome :), startup metrics lgtm w/ comment Please add bug 589118 to CL description as well. https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view_histogram_helper_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_histogram_helper_browsertest.cc:15: "Startup.BrowserView.FirstPaint.CompositingEnded"; Add all other metrics recorded from components/startup_metric_utils/browser/startup_metric_utils.cc here List them in a static constexpr char kStartupMetrics[][] = { "...", "...", "...", }; in the test's body directly (instead of anonymous namespace) and loop through the array in the test and move this test to chrome/browser/metrics/startup_metrics_browsertest.cc https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_histogram_helper_browsertest.cc:23: // process, and thus these tests cannot be unit_tests. Singular "test" now that we have just one.
On 2017/04/06 10:32:48, themblsha wrote: > +tapted > > Trent, maybe you could advice me on Cocoa Mac drawing: > > On Views (MacViews included) I'm measuring both the moment when all the children > of the BrowserView finished painting for the first time, and the next > CompositingEnded right after that. The intention is to monitor the moment in > time when the Browser UI becomes visible for the first time after start (and the > GPU process overhead, if any). > > Trouble points: > 1. Monitoring something comparable to ComposingEnded on Cocoa seems unfeasible, > as CoreAnimation doesn't seem to allow this sort of control. > 2. Currently I'm monitoring the start of painting the children, as all the > NSViews are painting into their own CALayers. I've thought of adding a dummy > deeply-nested child that'll be painted last but this seems too hacky to be > feasible. I think we want to hook in to BrowserWindowCocoa::Show() - when the initial makeKeyAndOrderFront returns is when Browser UI should be visible to a user. Details inline. > Or the Cocoa is going to be deprecated relatively soon and this shouldn't be a > concern at all? It would still be really nice to compare results :)
https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:71: std::unique_ptr<BrowserViewHistogramHelper> histogram_helper_; histogramHelper_ (since this is declared between @foo/@end). Also does this need to be unique_ptr? (i.e. why not just `BrowserViewHistogramHelper histogramHelper_;`). Then you don't need an initializer. https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:97: // non-hacky way of getting that notification. Does void BrowserWindowCocoa::Show() already record the value you want for Cocoa? Cocoa drawing is typically quite happy just to block the UI thread until essential drawing is done. In BrowserWindowCocoa::Show() we make the assumption that all drawing occurs under that `[window() makeKeyAndOrderFront:controller_];` call. When it returns, drawing is "done". (in MacViews we can probably cheat a little and fire off a compositing task before calling makeKeyAndOrderFront: so drawing is parallelised with the WindowServer IPC) I'd need to do some more research to see if there's an alternative hook that captures when drawing is finished versus when the window is mapped to the screen, but it's probably not worth collecting UMA separately since the user needs both to happen. (we may still want to capture it for analysis though). https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.h:719: std::unique_ptr<BrowserViewHistogramHelper> histogram_helper_; Same here - I don't think this needs to be a pointer. See, e.g., https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... which says "if it would otherwise make sense to use a type as a member by-value, don't convert it to a pointer just to be able to forward-declare the type."
Description was changed from ========== Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. Startup.BrowserView.FirstPaint measures how long does it take from browser startup until BrowserView finished the initial paint of all its children. Startup.BrowserView.CompositingEnded measures the time from browser startup to the time the GPU has finished compositing after BrowserView has finished painting its children. At this point the Browser interface is visible on screen. Measures how much time does it take for GPU to actually paint the first time. BUG=704945 ========== to ========== Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. Startup.BrowserView.FirstPaint measures how long does it take from browser startup until BrowserView finished the initial paint of all its children. Startup.BrowserView.CompositingEnded measures the time from browser startup to the time the GPU has finished compositing after BrowserView has finished painting its children. At this point the Browser interface is visible on screen. Measures how much time does it take for GPU to actually paint the first time. BUG=704945,589118 ==========
https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right): https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:71: std::unique_ptr<BrowserViewHistogramHelper> histogram_helper_; On 2017/04/07 00:49:34, tapted wrote: > histogramHelper_ (since this is declared between @foo/@end). Also does this > need to be unique_ptr? (i.e. why not just `BrowserViewHistogramHelper > histogramHelper_;`). Then you don't need an initializer. I'm using unique_ptr in browser_view.h so I can forward-declare the class without adding it as a dependency for all the units. Here however it's a good concern :) Decided to use it without an instance variable in BrowserWindowCocoa::Show(). https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:97: // non-hacky way of getting that notification. On 2017/04/07 00:49:34, tapted wrote: > Does void BrowserWindowCocoa::Show() already record the value you want for > Cocoa? > > Cocoa drawing is typically quite happy just to block the UI thread until > essential drawing is done. In BrowserWindowCocoa::Show() we make the assumption > that all drawing occurs under that `[window() > makeKeyAndOrderFront:controller_];` call. When it returns, drawing is "done". > > (in MacViews we can probably cheat a little and fire off a compositing task > before calling makeKeyAndOrderFront: so drawing is parallelised with the > WindowServer IPC) > > I'd need to do some more research to see if there's an alternative hook that > captures when drawing is finished versus when the window is mapped to the > screen, but it's probably not worth collecting UMA separately since the user > needs both to happen. (we may still want to capture it for analysis though). Thanks for the tip! Moved the code there. The BrowserView solution with ui::Compositor already compiles and works on MacViews. I've traced the BrowserView::Show() (around the frame_->Show(), which calls makeKeyAndOrderFront: inside), and frame_->Show() call by itself took 56ms on Debug build on my i7, Startup.BrowserView.FirstPaint was reported 420ms later, and compositing took 164ms according to my new histograms. https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.h:719: std::unique_ptr<BrowserViewHistogramHelper> histogram_helper_; On 2017/04/07 00:49:34, tapted wrote: > Same here - I don't think this needs to be a pointer. > > See, e.g., > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > which says "if it would otherwise make sense to use a type as a member by-value, > don't convert it to a pointer just to be able to forward-declare the type." Ok, removed unique_ptr :-) https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view_histogram_helper_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_histogram_helper_browsertest.cc:15: "Startup.BrowserView.FirstPaint.CompositingEnded"; On 2017/04/06 15:30:01, gab wrote: > Add all other metrics recorded from > components/startup_metric_utils/browser/startup_metric_utils.cc here > > List them in a > > static constexpr char kStartupMetrics[][] = { > "...", > "...", > "...", > }; > > in the test's body directly (instead of anonymous namespace) and loop through > the array in the test > > and move this test to chrome/browser/metrics/startup_metrics_browsertest.cc It wasn't compiling with '[][]' it worked only with 'constexpr const char* kStartupMetrics[]'. https://codereview.chromium.org/2773973002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_histogram_helper_browsertest.cc:23: // process, and thus these tests cannot be unit_tests. On 2017/04/06 15:30:01, gab wrote: > Singular "test" now that we have just one. Done.
cocoa stuff lgtm
mblsha@yandex-team.ru changed reviewers: + asvitkine@chromium.org, sky@chromium.org
+pkasting +sky +asvitkine Could you please review this? Also histograms.xml is still red even though Alexei is in src/tools/metrics/OWNERS :-/
https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/backgrou... File chrome/browser/background/background_mode_manager.cc (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/backgrou... chrome/browser/background/background_mode_manager.cc:721: startup_metric_utils::SetBackgroundModeEnabled(); How come you don't need a matching End? https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/startup_metrics_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/startup_metrics_browsertest.cc:30: while (!base::StatisticsRecorder::FindHistogram(histogram)) { no {} https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_view_histogram_helper.cc (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.cc:14: RemoveCompositorObserver(); Doesn't ~ScopedObserver take care of this for you? https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.cc:19: if (!did_first_paint_) { Early out rather than big conditional. https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_view_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.h:15: class BrowserViewHistogramHelper : public ui::CompositorObserver { This class is only used for metrics recording. I think it makes more sense to live in c/b/metrics? https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.h:18: ~BrowserViewHistogramHelper(); override https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.h:20: void OnDidPaintChildren(ui::Compositor* compositor); Document this. Also, that it happens to be called from PaintChildren is an implementation detail of views. How about naming this OnBrowserPainted()? https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.h:719: BrowserViewHistogramHelper histogram_helper_; Why do you need BrowserViewHistogramHelper per browser? From the description of the metrics it seems like we only need to record the histograms once, in which case this should be global. Am I missing something?
On 2017/04/11 12:16:31, themblsha wrote: > +pkasting +sky +asvitkine > > Could you please review this? When you add multiple reviewers, can you say what you want from each? I think most things I'm an OWNER of sky is also an OWNER of, so if he's looking already you likely don't need me, but let me know.
Will wait for sky@'s comments to be addressed before looking. Happy to review the histograms changes. On Tue, Apr 11, 2017 at 3:57 PM, <pkasting@chromium.org> wrote: > On 2017/04/11 12:16:31, themblsha wrote: > > +pkasting +sky +asvitkine > > > > Could you please review this? > > When you add multiple reviewers, can you say what you want from each? I > think > most things I'm an OWNER of sky is also an OWNER of, so if he's looking > already > you likely don't need me, but let me know. > > https://codereview.chromium.org/2773973002/ > -- 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/2773973002/diff/60001/chrome/browser/backgrou... File chrome/browser/background/background_mode_manager.cc (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/backgrou... chrome/browser/background/background_mode_manager.cc:721: startup_metric_utils::SetBackgroundModeEnabled(); On 2017/04/11 17:13:36, sky wrote: > How come you don't need a matching End? The intent is to set this once per startup, as it could delay the normal appearance of the first Browser window. The only way to 'end' this is to restart the browser. It would affect the unit_tests but they're out of scope of this problem anyway since startup_metric_utils.cc already rely on global singletons and require the tests to be inside browser_tests / interactive_ui_tests anyway. https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/startup_metrics_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/startup_metrics_browsertest.cc:30: while (!base::StatisticsRecorder::FindHistogram(histogram)) { On 2017/04/11 17:13:36, sky wrote: > no {} Done. https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_view_histogram_helper.cc (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.cc:14: RemoveCompositorObserver(); On 2017/04/11 17:13:36, sky wrote: > Doesn't ~ScopedObserver take care of this for you? Correct. Removed the call but the destructor still lives on because of 'Complex class/struct needs an explicit out-of-line destructor.' https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.cc:19: if (!did_first_paint_) { On 2017/04/11 17:13:36, sky wrote: > Early out rather than big conditional. Done. https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_view_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.h:15: class BrowserViewHistogramHelper : public ui::CompositorObserver { On 2017/04/11 17:13:36, sky wrote: > This class is only used for metrics recording. I think it makes more sense to > live in c/b/metrics? Done. But I'm not sure how to add it to the GN in that case: please look at the updated chrome/browser/ui/BUILD.gn (this file still makes sense only in ui) https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.h:18: ~BrowserViewHistogramHelper(); On 2017/04/11 17:13:36, sky wrote: > override Ah yes, I currently don't have the https://codereview.chromium.org/2791613002 that breaks compilation that I submitted last week :-) Done. https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.h:20: void OnDidPaintChildren(ui::Compositor* compositor); On 2017/04/11 17:13:36, sky wrote: > Document this. Also, that it happens to be called from PaintChildren is an > implementation detail of views. How about naming this OnBrowserPainted()? Better name, thanks! Renamed. https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.h:719: BrowserViewHistogramHelper histogram_helper_; On 2017/04/11 17:13:36, sky wrote: > Why do you need BrowserViewHistogramHelper per browser? From the description of > the metrics it seems like we only need to record the histograms once, in which > case this should be global. Am I missing something? It's placed here as an instance variable so it could wait for the first Compositor operation after the Browser UI was painted. Should I create a proper singleton instance instead?
https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_view_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.h:15: class BrowserViewHistogramHelper : public ui::CompositorObserver { On 2017/04/12 17:18:29, themblsha wrote: > On 2017/04/11 17:13:36, sky wrote: > > This class is only used for metrics recording. I think it makes more sense to > > live in c/b/metrics? > > Done. But I'm not sure how to add it to the GN in that case: please look at the > updated chrome/browser/ui/BUILD.gn (this file still makes sense only in ui) The new file should be in the target that contains the other files in c/b/metrics. That is, chrome/browser:browser. https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.h:719: BrowserViewHistogramHelper histogram_helper_; On 2017/04/12 17:18:29, themblsha wrote: > On 2017/04/11 17:13:36, sky wrote: > > Why do you need BrowserViewHistogramHelper per browser? From the description > of > > the metrics it seems like we only need to record the histograms once, in which > > case this should be global. Am I missing something? > > It's placed here as an instance variable so it could wait for the first > Compositor operation after the Browser UI was painted. Should I create a proper > singleton instance instead? How about calling a factory function for creation that internally maintains if the first logging happened. If it happened, null is returned. https://codereview.chromium.org/2773973002/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/browser_view_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/browser_view_histogram_helper.h:15: class BrowserViewHistogramHelper : public ui::CompositorObserver { BrowserView is specific to views, please name this BrowserWindowHistogramHelper.
https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_view_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_view_histogram_helper.h:15: class BrowserViewHistogramHelper : public ui::CompositorObserver { On 2017/04/12 19:34:08, sky wrote: > On 2017/04/12 17:18:29, themblsha wrote: > > On 2017/04/11 17:13:36, sky wrote: > > > This class is only used for metrics recording. I think it makes more sense > to > > > live in c/b/metrics? > > > > Done. But I'm not sure how to add it to the GN in that case: please look at > the > > updated chrome/browser/ui/BUILD.gn (this file still makes sense only in ui) > > The new file should be in the target that contains the other files in > c/b/metrics. That is, chrome/browser:browser. Thanks! Moved, also updated histogram names. https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/2773973002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.h:719: BrowserViewHistogramHelper histogram_helper_; On 2017/04/12 19:34:08, sky wrote: > On 2017/04/12 17:18:29, themblsha wrote: > > On 2017/04/11 17:13:36, sky wrote: > > > Why do you need BrowserViewHistogramHelper per browser? From the description > > of > > > the metrics it seems like we only need to record the histograms once, in > which > > > case this should be global. Am I missing something? > > > > It's placed here as an instance variable so it could wait for the first > > Compositor operation after the Browser UI was painted. Should I create a > proper > > singleton instance instead? > > How about calling a factory function for creation that internally maintains if > the first logging happened. If it happened, null is returned. Done. https://codereview.chromium.org/2773973002/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/browser_view_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/browser_view_histogram_helper.h:15: class BrowserViewHistogramHelper : public ui::CompositorObserver { On 2017/04/12 19:34:08, sky wrote: > BrowserView is specific to views, please name this BrowserWindowHistogramHelper. Done.
Pass on latest changes https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/startup_metrics_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:19: }; List all metrics in startup_metrics_utils.cc: constexpr const char* kStartupMetrics[] = { "Startup.BrowserMainToRendererMain", "Startup.BrowserMessageLoopStartTime", "Startup.BrowserMessageLoopStartTimeFromMainEntry2", "Startup.BrowserOpenTabs", "Startup.BrowserWindow.FirstPaint", "Startup.BrowserWindow.FirstPaint.CompositingEnded", "Startup.BrowserWindowDisplay", "Startup.FirstWebContents.MainFrameLoad2", "Startup.FirstWebContents.MainNavigationFinished", "Startup.FirstWebContents.MainNavigationStart", "Startup.FirstWebContents.NonEmptyPaint2", "Startup.LoadTime.ExeMainToDllMain2", "Startup.LoadTime.ProcessCreateToDllMain2", "Startup.LoadTime.ProcessCreateToExeMain2", "Startup.SystemUptime", #if defined(OS_WIN) "Startup.BrowserMessageLoopStartHardFaultCount", "Startup.Temperature", #endif // defined(OS_WIN) }; https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:26: // cannot be in unit_tests. Remove this paragraph (it's obvious now why it's an integration test) https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:28: // Tests that the specified startup-specific histograms are indeed reported. Change to: // Verify that startup histograms are logged on browser startup. https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:771: return; Add {} per conditional becoming multi-lines (here and elsewhere in this file)
https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/browser_window_histogram_helper.cc (right): https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.cc:17: // In Cocoa version of Chromium UI is rendered inside the main process using Nit: Add a , after Chromium https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.cc:33: BrowserWindowHistogramHelper::OnBrowserPainted(ui::Compositor* compositor) { Can this be named in some way that makes it clear that it returns the helper? e.g. MaybeCreateInstanceOnPaint() https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.cc:40: return std::unique_ptr<BrowserWindowHistogramHelper>( Nit: MakeUnique https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/browser_window_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.h:16: // Startup.BrowserWindow.Paint-histograms. Nit: Startup.BrowserWindow.FirstPaint* histograms. https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2005: if (!histogram_helper_.get()) Nit: {} Also, does if (!histogram_helper_) work? (without the .get()) https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:670: g_process_creation_ticks.Get().is_null()) Nit: Instead of checking for both conditions in a bunch of places, how about making the helper function check the second thing too? e.g. make the helper "ShouldLogProcessCreationTicksHistogram()" and have it check both startup interrupted and presence of procession creation ticks. This way, all of these can still be 1-liners. https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:781: void RecordBrowserWindowFirstPaint(const base::TimeTicks& ticks) { Nit: TimeTicks should be passed by value (see header comments). Please update the other functions while you're here.
https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/browser_window_histogram_helper.cc (right): https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.cc:17: // In Cocoa version of Chromium UI is rendered inside the main process using On 2017/04/18 16:01:31, Alexei Svitkine (slow) wrote: > Nit: Add a , after Chromium Done. https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.cc:33: BrowserWindowHistogramHelper::OnBrowserPainted(ui::Compositor* compositor) { On 2017/04/18 16:01:31, Alexei Svitkine (slow) wrote: > Can this be named in some way that makes it clear that it returns the helper? > > e.g. > > MaybeCreateInstanceOnPaint() Done. https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.cc:40: return std::unique_ptr<BrowserWindowHistogramHelper>( On 2017/04/18 16:01:32, Alexei Svitkine (slow) wrote: > Nit: MakeUnique I was unable to make it work with private constructor. Am I missing something here? https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/browser_window_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.h:16: // Startup.BrowserWindow.Paint-histograms. On 2017/04/18 16:01:32, Alexei Svitkine (slow) wrote: > Nit: Startup.BrowserWindow.FirstPaint* histograms. Done. https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/startup_metrics_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:19: }; On 2017/04/18 13:52:15, gab wrote: > List all metrics in startup_metrics_utils.cc: > > constexpr const char* kStartupMetrics[] = { > "Startup.BrowserMainToRendererMain", > "Startup.BrowserMessageLoopStartTime", > "Startup.BrowserMessageLoopStartTimeFromMainEntry2", > "Startup.BrowserOpenTabs", > "Startup.BrowserWindow.FirstPaint", > "Startup.BrowserWindow.FirstPaint.CompositingEnded", > "Startup.BrowserWindowDisplay", > "Startup.FirstWebContents.MainFrameLoad2", > "Startup.FirstWebContents.MainNavigationFinished", > "Startup.FirstWebContents.MainNavigationStart", > "Startup.FirstWebContents.NonEmptyPaint2", > "Startup.LoadTime.ExeMainToDllMain2", > "Startup.LoadTime.ProcessCreateToDllMain2", > "Startup.LoadTime.ProcessCreateToExeMain2", > "Startup.SystemUptime", > #if defined(OS_WIN) > "Startup.BrowserMessageLoopStartHardFaultCount", > "Startup.Temperature", > #endif // defined(OS_WIN) > }; Done. https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:26: // cannot be in unit_tests. On 2017/04/18 13:52:15, gab wrote: > Remove this paragraph (it's obvious now why it's an integration test) Done. https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:28: // Tests that the specified startup-specific histograms are indeed reported. On 2017/04/18 13:52:15, gab wrote: > Change to: > > // Verify that startup histograms are logged on browser startup. Done. https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2773973002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2005: if (!histogram_helper_.get()) On 2017/04/18 16:01:32, Alexei Svitkine (slow) wrote: > Nit: {} > > Also, does if (!histogram_helper_) work? (without the .get()) sky prefers the absence of curly brackets on single-line expressions :-) Yes, it works without .get(), but I thought that it added more clarity. I don't have a strong position here, so I yielded in both cases. https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:670: g_process_creation_ticks.Get().is_null()) On 2017/04/18 16:01:32, Alexei Svitkine (slow) wrote: > Nit: Instead of checking for both conditions in a bunch of places, how about > making the helper function check the second thing too? > > e.g. make the helper "ShouldLogProcessCreationTicksHistogram()" and have it > check both startup interrupted and presence of procession creation ticks. This > way, all of these can still be 1-liners. Good idea, thanks! https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:771: return; On 2017/04/18 13:52:15, gab wrote: > Add {} per conditional becoming multi-lines (here and elsewhere in this file) Alexei had a better idea :-) https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:781: void RecordBrowserWindowFirstPaint(const base::TimeTicks& ticks) { On 2017/04/18 16:01:32, Alexei Svitkine (slow) wrote: > Nit: TimeTicks should be passed by value (see header comments). > > Please update the other functions while you're here. Do you mean this? https://codereview.chromium.org/1425263003 It doesn't mention anything about passing by reference. Or am I missing something? https://codereview.chromium.org/2773973002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/startup_metrics_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:30: // Currently they're not reported at all during browser_tests. Should I create a new crbug for this? Fixing this in current CL seems to be out of scope.
https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:781: void RecordBrowserWindowFirstPaint(const base::TimeTicks& ticks) { On 2017/04/19 15:06:09, themblsha wrote: > On 2017/04/18 16:01:32, Alexei Svitkine (slow) wrote: > > Nit: TimeTicks should be passed by value (see header comments). > > > > Please update the other functions while you're here. > > Do you mean this? https://codereview.chromium.org/1425263003 > > It doesn't mention anything about passing by reference. Or am I missing > something? From base/time/time.h: // All time classes are copyable, assignable, and occupy 64-bits per instance. // As a result, prefer passing them by value: // void MyFunction(TimeDelta arg); // If circumstances require, you may also pass by const reference: // void MyFunction(const TimeDelta& arg); // Not preferred. //
https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:781: void RecordBrowserWindowFirstPaint(const base::TimeTicks& ticks) { On 2017/04/19 15:26:19, Alexei Svitkine (slow) wrote: > On 2017/04/19 15:06:09, themblsha wrote: > > On 2017/04/18 16:01:32, Alexei Svitkine (slow) wrote: > > > Nit: TimeTicks should be passed by value (see header comments). > > > > > > Please update the other functions while you're here. > > > > Do you mean this? https://codereview.chromium.org/1425263003 > > > > It doesn't mention anything about passing by reference. Or am I missing > > something? > > From base/time/time.h: > > // All time classes are copyable, assignable, and occupy 64-bits per instance. > // As a result, prefer passing them by value: > // void MyFunction(TimeDelta arg); > // If circumstances require, you may also pass by const reference: > // void MyFunction(const TimeDelta& arg); // Not preferred. > // Ahhar! Removed references from the whole file.
lgtm % comments https://codereview.chromium.org/2773973002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/browser_window_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.h:16: // Startup.BrowserWindow.FirstPaint* histograms. Add a sentence here to mention that this is not in components/startup_metrics_utils because of a dependency on ui/compositor. https://codereview.chromium.org/2773973002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.h:28: RecordValueAndMaybeCreateInstanceOnBrowserPainted(ui::Compositor* compositor); Nit: The "Record value" part is also under Maybe since you don't do it unless it's the first time - so put the Maybe at the front. MaybeRecordValueAndCreateInstanceOnBrowserPaint() (No need to use Painted - just Paint - since the name is long enough as it is and Paint is the name of the vent.) https://codereview.chromium.org/2773973002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.h:34: Nit: Remove empty line https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:668: void RecordBrowserWindowDisplay(const base::TimeTicks ticks) { Nit: We don't usually keep const in the qualifiers for non-ref parameters, so suggest removing them.
asvitkine: Strangely enough histograms.xml is still red even with your LGTM. Is it a bug or should I add someone else for that? https://codereview.chromium.org/2773973002/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/browser_window_histogram_helper.h (right): https://codereview.chromium.org/2773973002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.h:16: // Startup.BrowserWindow.FirstPaint* histograms. On 2017/04/19 16:52:16, Alexei Svitkine (slow) wrote: > Add a sentence here to mention that this is not in > components/startup_metrics_utils because of a dependency on ui/compositor. Done. https://codereview.chromium.org/2773973002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.h:28: RecordValueAndMaybeCreateInstanceOnBrowserPainted(ui::Compositor* compositor); On 2017/04/19 16:52:16, Alexei Svitkine (slow) wrote: > Nit: The "Record value" part is also under Maybe since you don't do it unless > it's the first time - so put the Maybe at the front. > > MaybeRecordValueAndCreateInstanceOnBrowserPaint() > > (No need to use Painted - just Paint - since the name is long enough as it is > and Paint is the name of the vent.) Good point, done. https://codereview.chromium.org/2773973002/diff/140001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.h:34: On 2017/04/19 16:52:16, Alexei Svitkine (slow) wrote: > Nit: Remove empty line Done. https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:668: void RecordBrowserWindowDisplay(const base::TimeTicks ticks) { On 2017/04/19 16:52:16, Alexei Svitkine (slow) wrote: > Nit: We don't usually keep const in the qualifiers for non-ref parameters, so > suggest removing them. I thought that it offers protection from unintended modification, and semantically suggests that it shouldn't be modified inside the function. const is already used inside several of the functions for local variables in this file and parameters are local variables as well.
https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:668: void RecordBrowserWindowDisplay(const base::TimeTicks ticks) { On 2017/04/19 17:27:41, themblsha wrote: > On 2017/04/19 16:52:16, Alexei Svitkine (slow) wrote: > > Nit: We don't usually keep const in the qualifiers for non-ref parameters, so > > suggest removing them. > > I thought that it offers protection from unintended modification, and > semantically suggests that it shouldn't be modified inside the function. > > const is already used inside several of the functions for local variables in > this file and parameters are local variables as well. const on locals is fine. const on params is more questionable: * It looks to readers as if this was intended to be a const ref, so people try to figure out what the intent was or waste time "fixing" things that weren't broken. * If the "const" is also used in the declaration, it exposes this implementation detail to callers. If it is not, then the declaration and definition have different qualifiers, which is legal in this case but surprising to readers. So even though I'm a big fan of local const, I suggest not using it on params.
On 2017/04/19 18:12:12, Peter Kasting wrote: > https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... > File components/startup_metric_utils/browser/startup_metric_utils.cc (right): > > https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... > components/startup_metric_utils/browser/startup_metric_utils.cc:668: void > RecordBrowserWindowDisplay(const base::TimeTicks ticks) { > On 2017/04/19 17:27:41, themblsha wrote: > > On 2017/04/19 16:52:16, Alexei Svitkine (slow) wrote: > > > Nit: We don't usually keep const in the qualifiers for non-ref parameters, > so > > > suggest removing them. > > > > I thought that it offers protection from unintended modification, and > > semantically suggests that it shouldn't be modified inside the function. > > > > const is already used inside several of the functions for local variables in > > this file and parameters are local variables as well. > > const on locals is fine. const on params is more questionable: > * It looks to readers as if this was intended to be a const ref, so people try > to figure out what the intent was or waste time "fixing" things that weren't > broken. > * If the "const" is also used in the declaration, it exposes this implementation > detail to callers. If it is not, then the declaration and definition have > different qualifiers, which is legal in this case but surprising to readers. > > So even though I'm a big fan of local const, I suggest not using it on params. +1 to Peter's reasoning. "asvitkine: Strangely enough histograms.xml is still red even with your LGTM. Is it a bug or should I add someone else for that?" This is just a problem with the extension that adds those colors to Chromium codereview - it can't parse the file:// include syntax in OWNERS files. The real presubmit tools work fine however. Also, with the move to gerrit in the future, that extension won't be used anymore (and hopefully corresponding functionality in gerrit UI will exist and won't have the issue.)
https://codereview.chromium.org/2773973002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/startup_metrics_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:30: // Currently they're not reported at all during browser_tests. On 2017/04/19 15:06:09, themblsha wrote: > Should I create a new crbug for this? Fixing this in current CL seems to be out > of scope. Ah, right, let's not bother. Leaving them commented out here is fine, make the above comments something like: // The following histograms depend on normal browser startup through BrowserMain // and are as such not caught by this browser test. https://codereview.chromium.org/2773973002/diff/160001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/160001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:551: bool ShouldLogProcessCreationTicksHistogram() { How about "ShouldLogStartupHistogram"? I find "ProcessCreationTicksHistogram" hard to parse. https://codereview.chromium.org/2773973002/diff/160001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.h (right): https://codereview.chromium.org/2773973002/diff/160001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.h:50: void RecordStartupProcessCreationTime(const base::Time time); nit: no "const" when passing by value
LGTM with the following change https://codereview.chromium.org/2773973002/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/browser_window_histogram_helper.cc (right): https://codereview.chromium.org/2773973002/diff/160001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.cc:10: BrowserWindowHistogramHelper::BrowserWindowHistogramHelper( Style guide says positions of definition and declaration should match.
Gerrit looks nice! Wonder how it compares to Bitbucket Server which we use locally at Yandex. https://codereview.chromium.org/2773973002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/startup_metrics_browsertest.cc (right): https://codereview.chromium.org/2773973002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/startup_metrics_browsertest.cc:30: // Currently they're not reported at all during browser_tests. On 2017/04/19 18:45:43, gab wrote: > On 2017/04/19 15:06:09, themblsha wrote: > > Should I create a new crbug for this? Fixing this in current CL seems to be > out > > of scope. > > Ah, right, let's not bother. Leaving them commented out here is fine, make the > above comments something like: > > // The following histograms depend on normal browser startup through BrowserMain > // and are as such not caught by this browser test. Done. Startup.BrowserMessageLoopStartHardFaultCount / Startup.Temperature could also be BrowserMainRunnerImpl-dependent (I was testing locally on MacViews), if so I'll comment them out as well. https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/140001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:668: void RecordBrowserWindowDisplay(const base::TimeTicks ticks) { On 2017/04/19 18:12:12, Peter Kasting (OOO until 4-24) wrote: > On 2017/04/19 17:27:41, themblsha wrote: > > On 2017/04/19 16:52:16, Alexei Svitkine (slow) wrote: > > > Nit: We don't usually keep const in the qualifiers for non-ref parameters, > so > > > suggest removing them. > > > > I thought that it offers protection from unintended modification, and > > semantically suggests that it shouldn't be modified inside the function. > > > > const is already used inside several of the functions for local variables in > > this file and parameters are local variables as well. > > const on locals is fine. const on params is more questionable: > * It looks to readers as if this was intended to be a const ref, so people try > to figure out what the intent was or waste time "fixing" things that weren't > broken. > * If the "const" is also used in the declaration, it exposes this implementation > detail to callers. If it is not, then the declaration and definition have > different qualifiers, which is legal in this case but surprising to readers. > > So even though I'm a big fan of local const, I suggest not using it on params. Thanks for explaining this point, now it's clear to me. https://codereview.chromium.org/2773973002/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/browser_window_histogram_helper.cc (right): https://codereview.chromium.org/2773973002/diff/160001/chrome/browser/metrics... chrome/browser/metrics/browser_window_histogram_helper.cc:10: BrowserWindowHistogramHelper::BrowserWindowHistogramHelper( On 2017/04/19 20:05:08, sky wrote: > Style guide says positions of definition and declaration should match. Done, thanks. Would be nice to have an automated check for this. https://codereview.chromium.org/2773973002/diff/160001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/2773973002/diff/160001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:551: bool ShouldLogProcessCreationTicksHistogram() { On 2017/04/19 18:45:43, gab wrote: > How about "ShouldLogStartupHistogram"? I find "ProcessCreationTicksHistogram" > hard to parse. Done. https://codereview.chromium.org/2773973002/diff/160001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.h (right): https://codereview.chromium.org/2773973002/diff/160001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.h:50: void RecordStartupProcessCreationTime(const base::Time time); On 2017/04/19 18:45:43, gab wrote: > nit: no "const" when passing by value pkasting did a great explanation why it should be done. Done.
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, tapted@chromium.org, sky@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2773973002/#ps290001 (title: "Don't wait for histograms that are not reported during browser_tests.")
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": 290001, "attempt_start_ts": 1492696633078690,
"parent_rev": "7db59fd0e7a25c9a92e80ac48913f2f8f35ed3f5", "commit_rev":
"3cfcb7cdbff23b208c96b7da2f42689421c4586e"}
Message was sent while issue was closed.
Description was changed from ========== Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. Startup.BrowserView.FirstPaint measures how long does it take from browser startup until BrowserView finished the initial paint of all its children. Startup.BrowserView.CompositingEnded measures the time from browser startup to the time the GPU has finished compositing after BrowserView has finished painting its children. At this point the Browser interface is visible on screen. Measures how much time does it take for GPU to actually paint the first time. BUG=704945,589118 ========== to ========== Add Startup.BrowserView.FirstPaint / .CompositingEnded histograms. Startup.BrowserView.FirstPaint measures how long does it take from browser startup until BrowserView finished the initial paint of all its children. Startup.BrowserView.CompositingEnded measures the time from browser startup to the time the GPU has finished compositing after BrowserView has finished painting its children. At this point the Browser interface is visible on screen. Measures how much time does it take for GPU to actually paint the first time. BUG=704945,589118 Review-Url: https://codereview.chromium.org/2773973002 Cr-Commit-Position: refs/heads/master@{#466009} Committed: https://chromium.googlesource.com/chromium/src/+/3cfcb7cdbff23b208c96b7da2f42... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:290001) as https://chromium.googlesource.com/chromium/src/+/3cfcb7cdbff23b208c96b7da2f42... |
