Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1950423006:
Offline Pages: Add Model Load Time UMA. (Closed)
|
Created:
4 years, 7 months ago by dewittj Modified:
4 years, 7 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, petewil+watch_chromium.org, dewittj+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOffline Pages: Add Model Load Time UMA.
Since many offline pages API calls now wait for the model to be loaded,
we want to characterize the amount of time it can take.
BUG=607573
Committed: https://crrev.com/97d87bb593370ec0a9f92437bfe7c7ee08c8409b
Cr-Commit-Position: refs/heads/master@{#392237}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : Add units to metric descriptions. #
Total comments: 3
Patch Set 4 : Rename metrics for more precision. #
Messages
Total messages: 22 (8 generated)
Description was changed from ========== Offline Pages: Add Model Load Time UMA. Since many offline pages API calls now wait for the model to be loaded, we want to characterize the amount of time it can take. BUG=607573 ========== to ========== Offline Pages: Add Model Load Time UMA. Since many offline pages API calls now wait for the model to be loaded, we want to characterize the amount of time it can take. BUG=607573 ==========
dewittj@chromium.org changed reviewers: + mpearson@chromium.org, petewil@chromium.org
PTAL mpearson: histograms petewil: all
https://codereview.chromium.org/1950423006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1950423006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33945: +</histogram> Why would we measure the directory creation time? It seems like something that only happens once per user (unless they clear the cache), and should be relatively short. Also, I don't see any calls to collect this UMA.
https://codereview.chromium.org/1950423006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1950423006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33945: +</histogram> On 2016/05/06 16:57:14, Pete Williamson wrote: > Why would we measure the directory creation time? It seems like something that > only happens once per user (unless they clear the cache), and should be > relatively short. I expect that indeed it is, however I don't know for sure. So I thought I'd measure it. What if /all/ FS operations are super slow for crappy phones? > > Also, I don't see any calls to collect this UMA. line 579 of offline_page_model.cc
lgtm
https://codereview.chromium.org/1950423006/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1950423006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34109: + the offline page model. nit: The histogram name implies this is merely model load time. I think this is a bit misleading. If you think the time to create the archive directory is small relative to the total load time then this misleading name is probably okay. If not, you should change the name. Also, in general are you sure you want this histogram to include the earlier one as opposed to two separate histograms, one for creation and one for loading?
https://codereview.chromium.org/1950423006/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1950423006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34109: + the offline page model. On 2016/05/06 20:09:23, Mark P wrote: > nit: The histogram name implies this is merely model load time. I think this is > a bit misleading. If you think the time to create the archive directory is > small relative to the total load time then this misleading name is probably > okay. If not, you should change the name. I do expect that directory creation will be negligible. I'm not completely sure, though, so I'm measuring it. > Also, in general are you sure you want this histogram to include the earlier one > as opposed to two separate histograms, one for creation and one for loading? I'm most interested in the total time (dir + load), which is important for users of the model. I added the archive dir creation time as a way to see if that is a significant part of the model load time. Perhaps one way to make this a little more independent is to record the ratio of archive directory creation time to total time. Does that seem better to you?
https://codereview.chromium.org/1950423006/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1950423006/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34109: + the offline page model. On 2016/05/06 20:13:15, dewittj wrote: > On 2016/05/06 20:09:23, Mark P wrote: > > nit: The histogram name implies this is merely model load time. I think this > is > > a bit misleading. If you think the time to create the archive directory is > > small relative to the total load time then this misleading name is probably > > okay. If not, you should change the name. > > I do expect that directory creation will be negligible. I'm not completely > sure, though, so I'm measuring it. > > > Also, in general are you sure you want this histogram to include the earlier > one > > as opposed to two separate histograms, one for creation and one for loading? > > I'm most interested in the total time (dir + load), which is important for users > of the model. I added the archive dir creation time as a way to see if that is > a significant part of the model load time. Can you spent a couple minutes thinking about a better histogram name that reflects what it's actually measuring? > Perhaps one way to make this a little more independent is to record the ratio of > archive directory creation time to total time. Does that seem better to you? That seems worse because then you can't easily interpret a single number (say, 2.0) on its own. How long is that?
Updated to reflect a more precise definition of what we are measuring.
histograms.xml lgtm
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/1950423006/#ps60001 (title: "Rename metrics for more precision.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950423006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950423006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dewittj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950423006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950423006/60001
Message was sent while issue was closed.
Description was changed from ========== Offline Pages: Add Model Load Time UMA. Since many offline pages API calls now wait for the model to be loaded, we want to characterize the amount of time it can take. BUG=607573 ========== to ========== Offline Pages: Add Model Load Time UMA. Since many offline pages API calls now wait for the model to be loaded, we want to characterize the amount of time it can take. BUG=607573 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Offline Pages: Add Model Load Time UMA. Since many offline pages API calls now wait for the model to be loaded, we want to characterize the amount of time it can take. BUG=607573 ========== to ========== Offline Pages: Add Model Load Time UMA. Since many offline pages API calls now wait for the model to be loaded, we want to characterize the amount of time it can take. BUG=607573 Committed: https://crrev.com/97d87bb593370ec0a9f92437bfe7c7ee08c8409b Cr-Commit-Position: refs/heads/master@{#392237} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/97d87bb593370ec0a9f92437bfe7c7ee08c8409b Cr-Commit-Position: refs/heads/master@{#392237} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
