Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2203263002:
History: Add load-time metric for non-MD and MD History WebUI (Closed)
|
Created:
4 years, 4 months ago by tsergeant Modified:
4 years, 4 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, stevenjb Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHistory: Add load-time metric for non-MD and MD History WebUI
This adds a new metric, History.ResultsRenderedTime, which is measured
on both versions of the history page. The metric measures the time taken
to render results onto the history page, which roughly corresponds to
'time to first meaningful paint'.
This will allow us to measure the relative performance of MD History in
the wild.
BUG=633477
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6b0157c3df09662bb47a98c508ff1ab9f1b3335a
Cr-Commit-Position: refs/heads/master@{#409983}
Patch Set 1 : JSDoc #Patch Set 2 : #
Total comments: 12
Patch Set 3 : Inline history method #Patch Set 4 : Fix closure #
Total comments: 7
Patch Set 5 : isherman@ review #
Dependent Patchsets: Messages
Total messages: 41 (23 generated)
Description was changed from ========== History: Add load-time metrics for non-MD and MD History WebUI This change adds two new metrics which are collected on both versions of the history page: * History.LoadDocumentTime, which measures the time taken to fire the 'load' event on the main document. This roughly corresponds to 'time to first paint'. * History.ResultsRenderedTime, which measures the time taken to render results onto the history page. This roughly corresponds to 'time to first meaningful paint'. These metrics will allow us to measure the relative performance of MD History in the wild. BUG=633477 ========== to ========== History: Add load-time metrics for non-MD and MD History WebUI This change adds two new metrics which are collected on both versions of the history page: * History.LoadDocumentTime, which measures the time taken to fire the 'load' event on the main document. This roughly corresponds to 'time to first paint'. * History.ResultsRenderedTime, which measures the time taken to render results onto the history page. This roughly corresponds to 'time to first meaningful paint'. These metrics will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== History: Add load-time metrics for non-MD and MD History WebUI This change adds two new metrics which are collected on both versions of the history page: * History.LoadDocumentTime, which measures the time taken to fire the 'load' event on the main document. This roughly corresponds to 'time to first paint'. * History.ResultsRenderedTime, which measures the time taken to render results onto the history page. This roughly corresponds to 'time to first meaningful paint'. These metrics will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== History: Add load-time metric for non-MD and MD History WebUI This adds a new metric, History.ResultsRenderedTime, which is measured on both versions of the history page. The metric measures the time taken to render results onto the history page, which roughly corresponds to 'time to first meaningful paint'. This will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== History: Add load-time metric for non-MD and MD History WebUI This adds a new metric, History.ResultsRenderedTime, which is measured on both versions of the history page. The metric measures the time taken to render results onto the history page, which roughly corresponds to 'time to first meaningful paint'. This will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== History: Add load-time metric for non-MD and MD History WebUI This adds a new metric, History.ResultsRenderedTime, which is measured on both versions of the history page. The metric measures the time taken to render results onto the history page, which roughly corresponds to 'time to first meaningful paint'. This will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tsergeant@chromium.org changed reviewers: + calamity@chromium.org
https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/history.js:1137: requestAnimationFrame(function() { I have two main options here: * requestAnimationFrame, which will always fire just before the next repaint. That means it doesn't take into account the time taken for paint itself. * setTimeout 0, which seems to be -after- the next paint but before the iron-list renders more items. However, I'm really not sure what kind of guarantees there are on this -- particularly since the rest of the iron-list rendering is also scheduled with setTimeout. I've gone with rAF, since it's the one which I'm more certain about. WDYT?
https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/history.js:82: function recordTimeHistogram(histogram) { Param no longer needed? https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/history.js:82: function recordTimeHistogram(histogram) { Is this worth moving into cr_util.js? https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/history.js:1137: requestAnimationFrame(function() { On 2016/08/03 07:37:04, tsergeant wrote: > I have two main options here: > > * requestAnimationFrame, which will always fire just before the next repaint. > That means it doesn't take into account the time taken for paint itself. > > * setTimeout 0, which seems to be -after- the next paint but before the > iron-list renders more items. However, I'm really not sure what kind of > guarantees there are on this -- particularly since the rest of the iron-list > rendering is also scheduled with setTimeout. > > I've gone with rAF, since it's the one which I'm more certain about. WDYT? rAF sgtm. https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/metrics_handler.cc (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/metrics_handler.cc:100: counter->AddTime(time_value); Why not just UMA_HISTOGRAM_TIMES?
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/externs.js:86: performance.now = function() {}; i haz a sad https://github.com/google/closure-compiler/blob/8a468ed4abb4ececc5f24928aac2f... https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/history.js:82: function recordTimeHistogram(histogram) { why not just inline this method? it's only used once
https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/externs.js:86: performance.now = function() {}; On 2016/08/03 21:05:17, Dan Beam wrote: > i haz a sad > https://github.com/google/closure-compiler/blob/8a468ed4abb4ececc5f24928aac2f... Is there any particular reason why these aren't pulled into Chrome? https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/history.js:82: function recordTimeHistogram(histogram) { On 2016/08/03 21:05:17, Dan Beam wrote: > why not just inline this method? it's only used once Sure, inlined. https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/metrics_handler.cc (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/metrics_handler.cc:100: counter->AddTime(time_value); On 2016/08/03 08:23:23, calamity wrote: > Why not just UMA_HISTOGRAM_TIMES? Histogram names passed into the macros have to be static -- you can't pass in a variable. Instead, we have to expand the macro manually, which is also what happens in HandleRecordInHistogram above.
The CQ bit was checked by tsergeant@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.
lgtm
https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/externs.js:86: performance.now = function() {}; On 2016/08/03 22:53:56, tsergeant wrote: > On 2016/08/03 21:05:17, Dan Beam wrote: > > i haz a sad > > > https://github.com/google/closure-compiler/blob/8a468ed4abb4ececc5f24928aac2f... > > Is there any particular reason why these aren't pulled into Chrome? it should be. have you tried window.performance.now?
https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... File chrome/browser/resources/history/externs.js (right): https://codereview.chromium.org/2203263002/diff/40001/chrome/browser/resource... chrome/browser/resources/history/externs.js:86: performance.now = function() {}; On 2016/08/04 03:51:33, Dan Beam wrote: > On 2016/08/03 22:53:56, tsergeant wrote: > > On 2016/08/03 21:05:17, Dan Beam wrote: > > > i haz a sad > > > > > > https://github.com/google/closure-compiler/blob/8a468ed4abb4ececc5f24928aac2f... > > > > Is there any particular reason why these aren't pulled into Chrome? > > it should be. have you tried window.performance.now? Ah, that works 🙌 (that doesn't show up in a code search, which was why I was confused)
lgtm
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
tsergeant@chromium.org changed reviewers: + isherman@chromium.org
+isherman@ for histograms.xml
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.
When looking at the metrics, how will you determine whether it is the MD or non-MD history page that's being recorded from? https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/metrics_handler.cc (right): https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/metrics_handler.cc:96: base::HistogramBase* counter = base::LinearHistogram::FactoryTimeGet( Hmm, I don't think you want a linear histogram -- I think you want a histogram with exponentially spaced buckets. I kind of suspect that this isn't the right handler to add this code to... though I'm not sure where else the code might live. Basically, I'm surprised that there isn't already code to handle recording non-linear histograms from WebUI pages. https://codereview.chromium.org/2203263002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2203263002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18308: +<histogram name="History.ResultsRenderedTime"> nit: Please add units="ms"
For determining the difference between the two pages, we plan on running a field trial and would be able to split the results based on that. I can also split the metrics apart in the source code if you prefer. https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/metrics_handler.cc (right): https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/metrics_handler.cc:96: base::HistogramBase* counter = base::LinearHistogram::FactoryTimeGet( On 2016/08/04 07:13:56, Ilya Sherman wrote: > Hmm, I don't think you want a linear histogram -- I think you want a histogram > with exponentially spaced buckets. I kind of suspect that this isn't the right > handler to add this code to... though I'm not sure where else the code might > live. Basically, I'm surprised that there isn't already code to handle > recording non-linear histograms from WebUI pages. Done, I've switched to base::Histogram. I think that this is the best place we have for this code right now, and there's actually an old bug (crbug.com/104338) to add this, among other things. https://codereview.chromium.org/2203263002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2203263002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18308: +<histogram name="History.ResultsRenderedTime"> On 2016/08/04 07:13:56, Ilya Sherman wrote: > nit: Please add units="ms" Done.
Okay, using a field trial to analyze the data makes sense. Thanks. https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/metrics_handler.cc (right): https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/metrics_handler.cc:96: base::HistogramBase* counter = base::LinearHistogram::FactoryTimeGet( On 2016/08/04 07:46:55, tsergeant wrote: > On 2016/08/04 07:13:56, Ilya Sherman wrote: > > Hmm, I don't think you want a linear histogram -- I think you want a histogram > > with exponentially spaced buckets. I kind of suspect that this isn't the > right > > handler to add this code to... though I'm not sure where else the code might > > live. Basically, I'm surprised that there isn't already code to handle > > recording non-linear histograms from WebUI pages. > > Done, I've switched to base::Histogram. > > I think that this is the best place we have for this code right now, and there's > actually an old bug (crbug.com/104338) to add this, among other things. Did you mean to upload a newer patch set?
https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/metrics_handler.cc (right): https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/metrics_handler.cc:96: base::HistogramBase* counter = base::LinearHistogram::FactoryTimeGet( On 2016/08/04 20:01:10, Ilya Sherman wrote: > On 2016/08/04 07:46:55, tsergeant wrote: > > On 2016/08/04 07:13:56, Ilya Sherman wrote: > > > Hmm, I don't think you want a linear histogram -- I think you want a > histogram > > > with exponentially spaced buckets. I kind of suspect that this isn't the > > right > > > handler to add this code to... though I'm not sure where else the code might > > > live. Basically, I'm surprised that there isn't already code to handle > > > recording non-linear histograms from WebUI pages. > > > > Done, I've switched to base::Histogram. > > > > I think that this is the best place we have for this code right now, and > there's > > actually an old bug (crbug.com/104338) to add this, among other things. > > Did you mean to upload a newer patch set? Oops, done
https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/metrics_handler.cc (right): https://codereview.chromium.org/2203263002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/metrics_handler.cc:96: base::HistogramBase* counter = base::LinearHistogram::FactoryTimeGet( On 2016/08/04 07:46:55, tsergeant wrote: > On 2016/08/04 07:13:56, Ilya Sherman wrote: > > Hmm, I don't think you want a linear histogram -- I think you want a histogram > > with exponentially spaced buckets. I kind of suspect that this isn't the > right > > handler to add this code to... though I'm not sure where else the code might > > live. Basically, I'm surprised that there isn't already code to handle > > recording non-linear histograms from WebUI pages. > > Done, I've switched to base::Histogram. > > I think that this is the best place we have for this code right now, and there's > actually an old bug (crbug.com/104338) to add this, among other things. yeahhhh, but that bug (and its filer) are silly
The CQ bit was checked by tsergeant@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.
LGTM, thanks.
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2203263002/#ps100001 (title: "isherman@ review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== History: Add load-time metric for non-MD and MD History WebUI This adds a new metric, History.ResultsRenderedTime, which is measured on both versions of the history page. The metric measures the time taken to render results onto the history page, which roughly corresponds to 'time to first meaningful paint'. This will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== History: Add load-time metric for non-MD and MD History WebUI This adds a new metric, History.ResultsRenderedTime, which is measured on both versions of the history page. The metric measures the time taken to render results onto the history page, which roughly corresponds to 'time to first meaningful paint'. This will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== History: Add load-time metric for non-MD and MD History WebUI This adds a new metric, History.ResultsRenderedTime, which is measured on both versions of the history page. The metric measures the time taken to render results onto the history page, which roughly corresponds to 'time to first meaningful paint'. This will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== History: Add load-time metric for non-MD and MD History WebUI This adds a new metric, History.ResultsRenderedTime, which is measured on both versions of the history page. The metric measures the time taken to render results onto the history page, which roughly corresponds to 'time to first meaningful paint'. This will allow us to measure the relative performance of MD History in the wild. BUG=633477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6b0157c3df09662bb47a98c508ff1ab9f1b3335a Cr-Commit-Position: refs/heads/master@{#409983} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6b0157c3df09662bb47a98c508ff1ab9f1b3335a Cr-Commit-Position: refs/heads/master@{#409983} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
