Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121863002:
Separate deferred startup into tasks. (Closed)
DescriptionSeparate deferred startup into tasks.
Correct UMA timing to be based on foreground start rather than
application initialization, as the application is often initialized in
the background, causing misleading stats.
By separating one single task into multiple tasks queued by the idle
handler, we don't block UI thread for a long time. We used to do
~100ms of blocking work for deferred startup, now it is reduced to
around ~20ms for the longest task. Tested on an N6, so on slower
devices this takes much longer.
BUG=614452
Committed: https://crrev.com/b17794608da80ccba64795658fac6239bdae6510
Cr-Commit-Position: refs/heads/master@{#409251}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review comments. #Patch Set 3 : Create new UMA stats. #
Total comments: 16
Patch Set 4 : Split up everything to the idle handler. #
Total comments: 16
Patch Set 5 : Fix per review. #
Total comments: 4
Patch Set 6 : Fix histogram. #Patch Set 7 : Fix per review. #Patch Set 8 : Preserve ordering in ChromeTabbedActivity. #Patch Set 9 : Rebase #
Total comments: 6
Patch Set 10 : fix per review #Patch Set 11 : Account for multiple activities running deferred startup. #Patch Set 12 : Fix visibility. #Patch Set 13 : Fix ToolbarTest. #Patch Set 14 : Longer timeout. #Messages
Total messages: 59 (30 generated)
Description was changed from ========== Split up deferred startup tasks. Correct UMA timing to be based on foreground start rather than application initialization, as the application is often initialized in the background, causing misleading stats. BUG=614452 ========== to ========== Separate deferred startup into tasks. Correct UMA timing to be based on foreground start rather than application initialization, as the application is often initialized in the background, causing misleading stats. By separating one single task into multiple tasks queued by the idle handler, we don't block UI thread for a long time. We used to do ~100ms of blocking work for deferred startup, now it is reduced to around ~50ms at once. This will be further reduced when the TODO is fixed. This is tested on an N6, so on slower devices this is much worse. BUG=614452 ==========
wnwen@chromium.org changed reviewers: + agrieve@chromium.org
🍲
https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:86: mDeferredTasks = new ConcurrentLinkedQueue<Runnable>(); This looks to be only read & written on the UI thread. Probably don't need a concurrent queue? https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:87: mDeferredStartupDuration = 0; nit: don't bother initialization to default values. https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:114: * These tasks should be at most 50ms each. 50ms why? on which device? Might just be worth saying "keep these quick since they run on the UI thread" https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:78: return sForegroundStartTimeMs; nit: Maybe add an assert here that sForegroundStartTimeMs != 0? (They still do work on kk bots)
On 2016/07/05 00:42:40, agrieve wrote: > https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java > (right): > > https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:86: > mDeferredTasks = new ConcurrentLinkedQueue<Runnable>(); > This looks to be only read & written on the UI thread. Probably don't need a > concurrent queue? > > https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:87: > mDeferredStartupDuration = 0; > nit: don't bother initialization to default values. > > https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:114: > * These tasks should be at most 50ms each. > 50ms why? on which device? > > Might just be worth saying "keep these quick since they run on the UI thread" > > https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java > (right): > > https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:78: > return sForegroundStartTimeMs; > nit: Maybe add an assert here that sForegroundStartTimeMs != 0? (They still do > work on kk bots) lgtm
wnwen@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine@ for new UMA stat and changing meaning of UMA.Debug.EnableCrashUpload.*StartUptime. Now these startup times are counted from first foreground Chrome rather than the first instance of ChromeApplication. ChromeApplication gets started even on install, so we were counting time from user install to first page load, which could be huge and not relevant. https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:86: mDeferredTasks = new ConcurrentLinkedQueue<Runnable>(); On 2016/07/05 00:42:40, agrieve wrote: > This looks to be only read & written on the UI thread. Probably don't need a > concurrent queue? Done. https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:87: mDeferredStartupDuration = 0; On 2016/07/05 00:42:40, agrieve wrote: > nit: don't bother initialization to default values. Done. https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:114: * These tasks should be at most 50ms each. On 2016/07/05 00:42:40, agrieve wrote: > 50ms why? on which device? > > Might just be worth saying "keep these quick since they run on the UI thread" Done. https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2121863002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:78: return sForegroundStartTimeMs; On 2016/07/05 00:42:40, agrieve wrote: > nit: Maybe add an assert here that sForegroundStartTimeMs != 0? (They still do > work on kk bots) Done.
wnwen@chromium.org changed reviewers: + yfriedman@chromium.org
+yfriedman@ for chrome/android OWNERS. Now these startup times are counted from first foreground Chrome rather than the first instance of ChromeApplication. ChromeApplication gets started even on install, so we were counting time from user install to first page load, which could be huge and not relevant.
If you're making a change to the meaning of some metrics, I suggest either renaming them and marking the other ones as obsolete in the xml, or at least adding a note to their descriptions in the XML about this change and when it took place (M54).
Done, made new histograms and made old ones obsolete as the numbers and data points will be drastically different. Don't want results to be polluted by old data. PTAL. 🚥
https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:240: RecordHistogram.recordTimesHistogram( Suggest using recordLongTimes function which has a longer range and still offers reasonable precision at lower levels. Note: The other metrics use that one.
yfriedman@chromium.org changed reviewers: + pasko@chromium.org
https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:694: onDeferredStartupForActivity(); Hmm - this changed (unintentionally?). Previously this waited for idle but now the activity-based loads are immediate. This is also true for ChromeTabbedActivity.onDeferredStartup. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1650: }, DEFERRED_STARTUP_DELAY_MS); truthfully, this delay seems a bit arbitrary at this point. It seems like everything should wait for an idle queue and then get admitted when possible. I think the extra delay is unnecessary if you switch everything to the queue. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (left): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:186: "UMA.Debug.EnableCrashUpload.DeferredStartUpDuration", Damn, the median for this was 140ms. :( https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:175: }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); The timing of this also changed and now competes with when we're potentially quite active with application load. I guess it depends on fairness of scheduling on android so perhaps is ok but was that intentional? https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:177: mDeferredTasks.add(new Runnable() { how did you come up with the current divisions? https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:240: RecordHistogram.recordTimesHistogram( i wonder if it's also worth grabbing walltime at this point? It would give us an idea of how much time it still takes for the full initialization - do we care? Presumably we wouldn't want this to take too long and right now because it's synchronous we know when it starts and completes. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:73: public static long getMainEntryPointTime() { I think we should probably remove the last remaining caller and switch it to the new function. It seems like this was accidentally the wrong measure. +pasko
I may move around some tasks when I go over timing again tomorrow for the initializeSharedClasses call. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:694: onDeferredStartupForActivity(); On 2016/07/05 19:29:05, Yaron wrote: > Hmm - this changed (unintentionally?). Previously this waited for idle but now > the activity-based loads are immediate. This is also true for > ChromeTabbedActivity.onDeferredStartup. Agreed, I'll move all of those into the idle queue. Since we're using isActivityDestroyed() anyways we have to keep a reference to the activity. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1650: }, DEFERRED_STARTUP_DELAY_MS); On 2016/07/05 19:29:05, Yaron wrote: > truthfully, this delay seems a bit arbitrary at this point. It seems like > everything should wait for an idle queue and then get admitted when possible. I > think the extra delay is unnecessary if you switch everything to the queue. +1, done. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (left): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:186: "UMA.Debug.EnableCrashUpload.DeferredStartUpDuration", On 2016/07/05 19:29:05, Yaron wrote: > Damn, the median for this was 140ms. :( Split up initializeSharedClasses, so should be much better now. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:175: }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); On 2016/07/05 19:29:05, Yaron wrote: > The timing of this also changed and now competes with when we're potentially > quite active with application load. I guess it depends on fairness of scheduling > on android so perhaps is ok but was that intentional? This one wasn't intentional, I'll create the AsyncTask as one of the idle tasks, so it likely runs when we're completely idle. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:177: mDeferredTasks.add(new Runnable() { On 2016/07/05 19:29:05, Yaron wrote: > how did you come up with the current divisions? Timing, all tasks on average <10ms on my N6 except the long one with the TODO. This keeps us from having too many tasks and still responsive according to RAIL guidelines. (adding it to comment). https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:240: RecordHistogram.recordTimesHistogram( On 2016/07/05 18:40:16, Alexei Svitkine (very slow) wrote: > Suggest using recordLongTimes function which has a longer range and still offers > reasonable precision at lower levels. Note: The other metrics use that one. Done. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:240: RecordHistogram.recordTimesHistogram( On 2016/07/05 19:29:05, Yaron wrote: > i wonder if it's also worth grabbing walltime at this point? It would give us an > idea of how much time it still takes for the full initialization - do we care? > Presumably we wouldn't want this to take too long and right now because it's > synchronous we know when it starts and completes. Done. https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2121863002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:73: public static long getMainEntryPointTime() { On 2016/07/05 19:29:05, Yaron wrote: > I think we should probably remove the last remaining caller and switch it to the > new function. It seems like this was accidentally the wrong measure. +pasko Acknowledged.
lgtm
lgtm, thank you! (I am not super familiar with all deferred startup jobs, so it is not an in depth review, hopefully Yaron will have a closer look) non-aschii characters in commit messages are (still) not friendly to read from some terminals, please remove before committing. https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:192: // TODO(aruslan): http://b/6397072 This will be moved elsewhere While we are here, let's remove the TODO? It was moved to http://crbug.com/149569 and wontfix there.
Description was changed from ========== Separate deferred startup into tasks. Correct UMA timing to be based on foreground start rather than application initialization, as the application is often initialized in the background, causing misleading stats. By separating one single task into multiple tasks queued by the idle handler, we don't block UI thread for a long time. We used to do ~100ms of blocking work for deferred startup, now it is reduced to around ~50ms at once. This will be further reduced when the TODO is fixed. This is tested on an N6, so on slower devices this is much worse. BUG=614452 ========== to ========== Separate deferred startup into tasks. Correct UMA timing to be based on foreground start rather than application initialization, as the application is often initialized in the background, causing misleading stats. By separating one single task into multiple tasks queued by the idle handler, we don't block UI thread for a long time. We used to do ~100ms of blocking work for deferred startup, now it is reduced to around ~50ms at once. This will be further reduced when the TODO is fixed. This is tested on an N6, so on slower devices this is much worse. BUG=614452 ==========
https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:968: super.onDeferredStartup(); the ordering of this chnaged? https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:141: public void addDeferredTask(Runnable deferredTask) { Use varags: (i.e. Runnable ... ) and you can drop the other method https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:142: mDeferredTasks.add(deferredTask); this should error if mDeferredStartupCompleted is set to avoid accidentally scheduling a task that won't ever run. also, assert ui thread (to avoid sync issues) https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:225: ChromeApplication application = (ChromeApplication) mAppContext; this is done a few times. just do: final ChromeApplication application = ... above https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:190: DeferredStartupHandler.getInstance().runDeferredTasks(); does this work? previously this was kicked off in ChromeActivity but I don't think everything subclasses this activity. I think the asyncness of "AsyncInitializationActivity" is "async library load" and not whether it uses deferredstartuphandler https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:74: return sApplicationStartUptimeMs; so remove this now?
Thanks for the reviews, added new histogram Startup.FirstCommitNavigationTime2 as we are changing to first foreground time and removing the application onCreate timing (which is unreliable and meaningless on install). Removed emoji from subject, as though it is not in this commit message, it does get added to reverts. :( https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:968: super.onDeferredStartup(); On 2016/07/06 14:27:49, Yaron wrote: > the ordering of this chnaged? Yes, do you think that will cause a problem? This is a pretty quick task, basically 0ms. https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:141: public void addDeferredTask(Runnable deferredTask) { On 2016/07/06 14:27:49, Yaron wrote: > Use varags: (i.e. Runnable ... ) and you can drop the other method Done. https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:142: mDeferredTasks.add(deferredTask); On 2016/07/06 14:27:50, Yaron wrote: > this should error if mDeferredStartupCompleted is set to avoid accidentally > scheduling a task that won't ever run. > also, assert ui thread (to avoid sync issues) Done. https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:192: // TODO(aruslan): http://b/6397072 This will be moved elsewhere On 2016/07/06 13:03:31, pasko wrote: > While we are here, let's remove the TODO? It was moved to > http://crbug.com/149569 and wontfix there. Done. https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:225: ChromeApplication application = (ChromeApplication) mAppContext; On 2016/07/06 14:27:50, Yaron wrote: > this is done a few times. just do: > final ChromeApplication application = ... > above Done. https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:190: DeferredStartupHandler.getInstance().runDeferredTasks(); On 2016/07/06 14:27:50, Yaron wrote: > does this work? previously this was kicked off in ChromeActivity but I don't > think everything subclasses this activity. I think the asyncness of > "AsyncInitializationActivity" is "async library load" and not whether it uses > deferredstartuphandler Moved this to ChromeActivity. Only ChromeActivity and its subclass ChromeTabbedActivity use this method. https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:74: return sApplicationStartUptimeMs; On 2016/07/06 14:27:50, Yaron wrote: > so remove this now? Done.
still lgtm % comment https://codereview.chromium.org/2121863002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2121863002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55099: +<histogram name="Startup.FirstCommitNavigationTime2" units="ms"> Please mark the previous one as <obsolete>.
Thanks for noticing, Alexei! https://codereview.chromium.org/2121863002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2121863002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:55099: +<histogram name="Startup.FirstCommitNavigationTime2" units="ms"> On 2016/07/06 15:25:44, Alexei Svitkine (very slow) wrote: > Please mark the previous one as <obsolete>. Done.
ok this looks better but I feel like at least one of {Dan,Yusuf,Maria} ought to
review it too. There's some UI code that's touched and they've all made changes
to the delayed code running logic :)
Also, I'm a bit worried about the potential for subtle breakages. It's true that
only thanks to the bug Alexei discovered, did we realize that this sometimes
takes minutes to happen so this is probably no worse, but I'm wondering if
you've thought about how to improving testing of this? Everything is public on
DeferredStartupHandler, so presumably you can test that a registered task
executes.
https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):
https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:190:
DeferredStartupHandler.getInstance().runDeferredTasks();
On 2016/07/06 15:21:37, Peter Wen wrote:
> On 2016/07/06 14:27:50, Yaron wrote:
> > does this work? previously this was kicked off in ChromeActivity but I don't
> > think everything subclasses this activity. I think the asyncness of
> > "AsyncInitializationActivity" is "async library load" and not whether it
uses
> > deferredstartuphandler
>
> Moved this to ChromeActivity. Only ChromeActivity and its subclass
> ChromeTabbedActivity use this method.
Err, I was completely backwards on this :/
ChromeActivity directly subclasses asyncInitActivity
I still prefer it how you have it now
https://codereview.chromium.org/2121863002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):
https://codereview.chromium.org/2121863002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:686: *
Overriding methods should call super.onDeferredStartup() last, after queuing
their tasks in
this is debatable, right? it depends on whether they want to precede app
install?
Description was changed from ========== Separate deferred startup into tasks. Correct UMA timing to be based on foreground start rather than application initialization, as the application is often initialized in the background, causing misleading stats. By separating one single task into multiple tasks queued by the idle handler, we don't block UI thread for a long time. We used to do ~100ms of blocking work for deferred startup, now it is reduced to around ~50ms at once. This will be further reduced when the TODO is fixed. This is tested on an N6, so on slower devices this is much worse. BUG=614452 ========== to ========== Separate deferred startup into tasks. Correct UMA timing to be based on foreground start rather than application initialization, as the application is often initialized in the background, causing misleading stats. By separating one single task into multiple tasks queued by the idle handler, we don't block UI thread for a long time. We used to do ~100ms of blocking work for deferred startup, now it is reduced to around ~20ms for the longest task. Tested on an N6, so on slower devices this takes much longer. BUG=614452 ==========
Sounds good, thanks for the review! Will add tests when I come back on Friday
and send it to whichever of {Dan, Yusuf, Maria} seems to have the least review
load. :)
https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):
https://codereview.chromium.org/2121863002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:190:
DeferredStartupHandler.getInstance().runDeferredTasks();
On 2016/07/06 15:46:58, Yaron wrote:
> On 2016/07/06 15:21:37, Peter Wen wrote:
> > On 2016/07/06 14:27:50, Yaron wrote:
> > > does this work? previously this was kicked off in ChromeActivity but I
don't
> > > think everything subclasses this activity. I think the asyncness of
> > > "AsyncInitializationActivity" is "async library load" and not whether it
> uses
> > > deferredstartuphandler
> >
> > Moved this to ChromeActivity. Only ChromeActivity and its subclass
> > ChromeTabbedActivity use this method.
>
> Err, I was completely backwards on this :/
> ChromeActivity directly subclasses asyncInitActivity
> I still prefer it how you have it now
Acknowledged.
https://codereview.chromium.org/2121863002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):
https://codereview.chromium.org/2121863002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:686: *
Overriding methods should call super.onDeferredStartup() last, after queuing
their tasks in
On 2016/07/06 15:46:58, Yaron wrote:
> this is debatable, right? it depends on whether they want to precede app
> install?
That's true, since the idle handler should not preempt UI thread tasks. I'll
change the name of methods so it's clearer and change how the stats are reported
in DeferredStartupHandler.
wnwen@chromium.org changed reviewers: + yusufo@chromium.org
+yusufo@ for deferred startup tasks review, as Yusuf's worked with them before. Turns out deferred tasks are tested in ChromeActivityTestCaseBase and CustomTabActivityTestBase, ensuring that they all run to completion in a reasonable amount of time (couple seconds). The new RuntimeException should catch bad uses of DeferredStartupTask#addDeferredTask.
Thanks Peter! Two nits and a question about where we start recording the time. https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:701: final ChromeActivity activity = this; is this really needed? Why not call isActivityDestroyed() directly below? https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:267: UmaUtils.recordForegroundStartTime(); I would say this is a bit misleading. The previous value you are getting rid of is called from onCreate in application. We rename it here to be related with foreground and then call it in startwithnative which is called after all the native libraries have been loaded and onCreateWithNative in ChromeBrowserInitializer is completed. That means we have run through all UI initialization (with and without native) and completed all those tasks, by the time we are here. I agree that we are probably in the foreground by now, but we can't know how much time has passed since being in foreground (We have other stats around for first draw etc). Android makes the first draw after the first onResume call which may have happened way before this is called. I guess my question is what we are measuring here, is it going to be only related with deferred startup or will this starting time effect other historgrams related with startup as well? If latter, then we may want to rething where we are putting this. https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:473: GoogleServicesManager.get(getApplicationContext()) this can fit in one line?
PTAL. :) https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:701: final ChromeActivity activity = this; On 2016/07/12 18:00:19, Yusuf wrote: > is this really needed? Why not call isActivityDestroyed() directly below? Done. https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:267: UmaUtils.recordForegroundStartTime(); On 2016/07/12 18:00:19, Yusuf wrote: > I would say this is a bit misleading. The previous value you are getting rid of > is called from onCreate in application. We rename it here to be related with > foreground and then call it in startwithnative which is called after all the > native libraries have been loaded and onCreateWithNative in > ChromeBrowserInitializer is completed. That means we have run through all UI > initialization (with and without native) and completed all those tasks, by the > time we are here. I agree that we are probably in the foreground by now, but we > can't know how much time has passed since being in foreground (We have other > stats around for first draw etc). Android makes the first draw after the first > onResume call which may have happened way before this is called. > > I guess my question is what we are measuring here, is it going to be only > related with deferred startup or will this starting time effect other > historgrams related with startup as well? If latter, then we may want to rething > where we are putting this. This is currently used only by deferred startup and Startup.FirstCommitNavigationTime, which is owned by pasko@. Thus risk to startup metrics is low. The other reason to move this here is to have a more reliable baseline, as the previous time recorded in onCreate was very misleading due to it being recorded on app installation rather than app startup. Is there a better place for this concept? https://codereview.chromium.org/2121863002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:473: GoogleServicesManager.get(getApplicationContext()) On 2016/07/12 18:00:19, Yusuf wrote: > this can fit in one line? Done.
Got it, thank you! lgtm
The CQ bit was checked by wnwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wnwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wnwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by wnwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wnwen@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor@ for chrome/android and chrome/test OWNERS.
chrome/android and chrome/test lgtm! Sorry for the delay I was sick most of last week and this just fell off my radar!
On 2016/08/02 15:35:32, David Trainor wrote: > chrome/android and chrome/test lgtm! Sorry for the delay I was sick most of > last week and this just fell off my radar! No worries, thanks David. Hope you're feeling better now. I was just about to send a friendly ping. :)
The CQ bit was checked by wnwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wnwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org, pasko@chromium.org, asvitkine@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2121863002/#ps260001 (title: "Longer timeout.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Separate deferred startup into tasks. Correct UMA timing to be based on foreground start rather than application initialization, as the application is often initialized in the background, causing misleading stats. By separating one single task into multiple tasks queued by the idle handler, we don't block UI thread for a long time. We used to do ~100ms of blocking work for deferred startup, now it is reduced to around ~20ms for the longest task. Tested on an N6, so on slower devices this takes much longer. BUG=614452 ========== to ========== Separate deferred startup into tasks. Correct UMA timing to be based on foreground start rather than application initialization, as the application is often initialized in the background, causing misleading stats. By separating one single task into multiple tasks queued by the idle handler, we don't block UI thread for a long time. We used to do ~100ms of blocking work for deferred startup, now it is reduced to around ~20ms for the longest task. Tested on an N6, so on slower devices this takes much longer. BUG=614452 Committed: https://crrev.com/b17794608da80ccba64795658fac6239bdae6510 Cr-Commit-Position: refs/heads/master@{#409251} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b17794608da80ccba64795658fac6239bdae6510 Cr-Commit-Position: refs/heads/master@{#409251}
Message was sent while issue was closed.
On 2016/08/02 18:28:00, commit-bot: I haz the power wrote: > Patchset 14 (id:??) landed as > https://crrev.com/b17794608da80ccba64795658fac6239bdae6510 > Cr-Commit-Position: refs/heads/master@{#409251} Looks like this is failing an assert for CustomTabs: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... |
