Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2853263002:
SimpleCache: histogram queuing delays and I/O portion of more ops. (Closed)
|
Created:
3 years, 7 months ago by Maks Orlovich Modified:
3 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, asvitkine+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histograms for how execution of open + create in SimpleCache is
split between queuing delay and synchronous portion.
(there is already an end-to-end stat that covers both ops, and one for
sync portion of open only).
Made the sync portion of open histogram split by cache_type like every
other SimpleCache statistic.
Also add synchronous portion metrics for Write/Close as both of these
seem to have opportunities for improvement, so it makes sense to measure them directly, at least short-term.
BUG=
Review-Url: https://codereview.chromium.org/2853263002
Cr-Commit-Position: refs/heads/master@{#469038}
Committed: https://chromium.googlesource.com/chromium/src/+/5753d9910d1fe46387348c180b7212ca485df179
Patch Set 1 #
Total comments: 9
Patch Set 2 : Add a couple more sync metrics. #
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by morlovich@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...
morlovich@chromium.org changed reviewers: + xunjieli@chromium.org
I will take a look by EOD today. On choosing a histograms.xml reviewer, I normally would wait for the net reviewer to sign off and then loop in an OWNER from tools/metrics/OWNERS.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> 2) Add sync + queue delay stats for every other common op. That feels > like it might be excessive, but may be a good idea if I keep changing > them. (Sync portions would probably be more important) > Unfortunately, I don't really have a sense of how much a big deal adding > these things is (e.g. in terms of resource usage). Adding extra UMA histogram should be relatively cheap. However, I would avoid adding histograms that are noisy or just for the sake of completeness. Most of the times, we won't be able to detect any statistically significant differences in these histograms on Canary/Dev if your change is not drastic. If you think those will be helpful, I'd say go ahead and add those. https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.cc:279: UMA_HISTOGRAM_TIMES("SimpleCache.DiskOpenLatency", I think it makes sense for DiskOpenLatency to be split by cache subtype like other SimpleCache stats. One benefit is that on UMA dashboard, you could associate this with other histograms. If you do not split, you might need to use filters to make the data comparable, because the number of samples for the histograms (e.g. SimpleCache.DiskOpenLatency vs SimpleCache.Http.QueueLatency.CreateEntry) are not the same. Do we care about the failure case ("result != net::OK") ? https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.cc:311: base::TimeTicks::Now() - start_sync_create_entry); Same here. Do we care about the failure case? https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.h:120: // |had_index| is provided only for histograms. can you add a comment here on when |start_time| refers to? Could also consider renaming |start_time| to |queuing_start_time| to be more explicit that the timestamp is when we post the Open/CreateEntry task to the worker pool. https://codereview.chromium.org/2853263002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2853263002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:67423: + entry on disk. If we only care about the success case, we should mention it in the summary that this histogram is only reported on success.
The CQ bit was checked by morlovich@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...
Description was changed from ========== Add histograms for how execution of open + create in SimpleCache is split between queuing delay and synchronous portion. (there is already an end-to-end stat that covers both ops, and one for sync portion of open only) Q for reviewer: it might also make sense to: 1) Make DiskOpenLatency be split by cache subtype like every other SimpleCache stat? 2) Add sync + queue delay stats for every other common op. That feels like it might be excessive, but may be a good idea if I keep changing them. (Sync portions would probably be more important) Unfortunately, I don't really have a sense of how much a big deal adding these things is (e.g. in terms of resource usage). Also, how does one pick a histograms.xml reviewer? BUG= ========== to ========== Add histograms for how execution of open + create in SimpleCache is split between queuing delay and synchronous portion. (there is already an end-to-end stat that covers both ops, and one for sync portion of open only). Made the sync portion of open histogram split by cache_type like every other SimpleCache statistic. Also add synchronous portion metrics for Write/Close as both of these seem to have opportunities for improvement, so it makes sense to measure them directly, at least short-term. BUG= ==========
Thanks for the feedback. Added a couple other sync ones as well. https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.cc:279: UMA_HISTOGRAM_TIMES("SimpleCache.DiskOpenLatency", On 2017/05/02 18:21:29, xunjieli wrote: > I think it makes sense for DiskOpenLatency to be split by cache subtype like > other SimpleCache stats. One benefit is that on UMA dashboard, you could > associate this with other histograms. If you do not split, you might need to use > filters to make the data comparable, because the number of samples for the > histograms (e.g. SimpleCache.DiskOpenLatency vs > SimpleCache.Http.QueueLatency.CreateEntry) are not the same. This is convincing, thanks. (And suggests that I may be missing some functionality) > Do we care about the failure case ("result != net::OK") ? Those seem to happen a negligible amount of time. (Except creates on Android which... is probably worth following up on). https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.cc:311: base::TimeTicks::Now() - start_sync_create_entry); On 2017/05/02 18:21:29, xunjieli wrote: > Same here. Do we care about the failure case? Acknowledged. https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.h:120: // |had_index| is provided only for histograms. On 2017/05/02 18:21:29, xunjieli wrote: > can you add a comment here on when |start_time| refers to? > Could also consider renaming |start_time| to |queuing_start_time| to be more > explicit that the timestamp is when we post the Open/CreateEntry task to the > worker pool. Done. https://codereview.chromium.org/2853263002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2853263002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:67423: + entry on disk. On 2017/05/02 18:21:29, xunjieli wrote: > If we only care about the success case, we should mention it in the summary that > this histogram is only reported on success. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/2853263002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_synchronous_entry.cc:279: UMA_HISTOGRAM_TIMES("SimpleCache.DiskOpenLatency", On 2017/05/02 19:49:20, Maks Orlovich wrote: > On 2017/05/02 18:21:29, xunjieli wrote: > > I think it makes sense for DiskOpenLatency to be split by cache subtype like > > other SimpleCache stats. One benefit is that on UMA dashboard, you could > > associate this with other histograms. If you do not split, you might need to > use > > filters to make the data comparable, because the number of samples for the > > histograms (e.g. SimpleCache.DiskOpenLatency vs > > SimpleCache.Http.QueueLatency.CreateEntry) are not the same. > > This is convincing, thanks. (And suggests that I may be missing some > functionality) > > > Do we care about the failure case ("result != net::OK") ? > > Those seem to happen a negligible amount of time. (Except creates on Android > which... is probably worth following up on). > Acknowledged.
morlovich@chromium.org changed reviewers: + mpearson@google.com
+mpearson for the histograms.xml review (unless it already sends this when I edit the issue, if so, apologies for redundancy)
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
histograms.xml lgtm
The CQ bit was checked by morlovich@chromium.org
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": 20001, "attempt_start_ts": 1493835440483360,
"parent_rev": "ce159dee32a6cf18669950198af0efd9218393e8", "commit_rev":
"5753d9910d1fe46387348c180b7212ca485df179"}
Message was sent while issue was closed.
Description was changed from ========== Add histograms for how execution of open + create in SimpleCache is split between queuing delay and synchronous portion. (there is already an end-to-end stat that covers both ops, and one for sync portion of open only). Made the sync portion of open histogram split by cache_type like every other SimpleCache statistic. Also add synchronous portion metrics for Write/Close as both of these seem to have opportunities for improvement, so it makes sense to measure them directly, at least short-term. BUG= ========== to ========== Add histograms for how execution of open + create in SimpleCache is split between queuing delay and synchronous portion. (there is already an end-to-end stat that covers both ops, and one for sync portion of open only). Made the sync portion of open histogram split by cache_type like every other SimpleCache statistic. Also add synchronous portion metrics for Write/Close as both of these seem to have opportunities for improvement, so it makes sense to measure them directly, at least short-term. BUG= Review-Url: https://codereview.chromium.org/2853263002 Cr-Commit-Position: refs/heads/master@{#469038} Committed: https://chromium.googlesource.com/chromium/src/+/5753d9910d1fe46387348c180b72... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/5753d9910d1fe46387348c180b72... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
