Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(451)

Issue 2688633002: predictors: Add prefetching hit/miss histograms. (Closed)

Created:
1 year, 3 months ago by Benoit L
Modified:
1 year, 3 months ago
Reviewers:
alexilin, Ilya Sherman
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

predictors: Add prefetching hit/miss histograms. When the resource_prefetch_predictor triggers a prefetch, record how many resources were used / not used (hits and misses), and the pattern of these (in cache, or not in cache). For the resources that are not in the cache, record the amount of data prefetched and wasted. This also assumes that a prefetch older than 5 minutes is wasted. BUG=680049 Review-Url: https://codereview.chromium.org/2688633002 Cr-Commit-Position: refs/heads/master@{#450935} Committed: https://chromium.googlesource.com/chromium/src/+/2e4491eac73425489384c1428b17415e7dcec8e2

Patch Set 1 #

Patch Set 2 : Now with a test. #

Total comments: 19

Patch Set 3 : Rebase and address comments. #

Patch Set 4 : Remove duplicated duplicated comment comment. #

Total comments: 4

Patch Set 5 : Histograms.xml and comments. #

Total comments: 2

Patch Set 6 : Add units. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+267 lines, -34 lines) Patch
M chrome/browser/predictors/resource_prefetch_predictor.h View 1 2 3 4 5 chunks +19 lines, -4 lines 0 comments Download
M chrome/browser/predictors/resource_prefetch_predictor.cc View 1 2 3 4 5 chunks +77 lines, -4 lines 2 comments Download
M chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc View 1 2 7 chunks +24 lines, -3 lines 0 comments Download
M chrome/browser/predictors/resource_prefetcher.h View 1 2 4 chunks +27 lines, -1 line 0 comments Download
M chrome/browser/predictors/resource_prefetcher.cc View 1 2 6 chunks +31 lines, -4 lines 0 comments Download
M chrome/browser/predictors/resource_prefetcher_manager.h View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/predictors/resource_prefetcher_manager.cc View 1 2 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/predictors/resource_prefetcher_unittest.cc View 1 2 3 4 5 chunks +25 lines, -15 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 2 chunks +57 lines, -0 lines 0 comments Download

Messages

Total messages: 29 (18 generated)
alexilin
https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predictors/resource_prefetch_predictor.cc File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode590 chrome/browser/predictors/resource_prefetch_predictor.cc:590: observer_->OnPrefetchingFinished(main_frame_url, stats); You don't use stats inside an observer. ...
1 year, 3 months ago (2017-02-09 15:24:18 UTC) #6
Benoit L
Thanks! https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predictors/resource_prefetch_predictor.cc File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode590 chrome/browser/predictors/resource_prefetch_predictor.cc:590: observer_->OnPrefetchingFinished(main_frame_url, stats); On 2017/02/09 15:24:17, alexilin wrote: > ...
1 year, 3 months ago (2017-02-13 16:13:15 UTC) #7
alexilin
P.S. don't forget about histograms.xml https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predictors/resource_prefetch_predictor.cc File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2688633002/diff/20001/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode704 chrome/browser/predictors/resource_prefetch_predictor.cc:704: auto it = prefetcher_stats_.find(main_frame_url); ...
1 year, 3 months ago (2017-02-13 17:07:06 UTC) #10
Benoit L
Thanks! https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predictors/resource_prefetch_predictor.h File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2688633002/diff/60001/chrome/browser/predictors/resource_prefetch_predictor.h#newcode305 chrome/browser/predictors/resource_prefetch_predictor.h:305: // inflight_navigations_ and inflight_prefetches_. On 2017/02/13 17:07:05, alexilin ...
1 year, 3 months ago (2017-02-13 17:32:51 UTC) #13
alexilin
lgtm, thank you! https://codereview.chromium.org/2688633002/diff/80001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2688633002/diff/80001/tools/metrics/histograms/histograms.xml#newcode55125 tools/metrics/histograms/histograms.xml:55125: +<histogram name="ResourcePrefetchPredictor.PrefetchHitsCount.Cached"> tiny nit: specify units ...
1 year, 3 months ago (2017-02-13 17:41:49 UTC) #14
Benoit L
isherman: Please review the histogram changes here, thanks! https://codereview.chromium.org/2688633002/diff/80001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2688633002/diff/80001/tools/metrics/histograms/histograms.xml#newcode55125 tools/metrics/histograms/histograms.xml:55125: +<histogram ...
1 year, 3 months ago (2017-02-14 08:40:16 UTC) #18
Ilya Sherman
https://codereview.chromium.org/2688633002/diff/100001/chrome/browser/predictors/resource_prefetch_predictor.cc File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2688633002/diff/100001/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode167 chrome/browser/predictors/resource_prefetch_predictor.cc:167: internal::kResourcePrefetchPredictorPrefetchHitsSize, hits_bytes / 1024); This goes up to about ...
1 year, 3 months ago (2017-02-15 00:07:41 UTC) #21
Benoit L
https://codereview.chromium.org/2688633002/diff/100001/chrome/browser/predictors/resource_prefetch_predictor.cc File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2688633002/diff/100001/chrome/browser/predictors/resource_prefetch_predictor.cc#newcode167 chrome/browser/predictors/resource_prefetch_predictor.cc:167: internal::kResourcePrefetchPredictorPrefetchHitsSize, hits_bytes / 1024); On 2017/02/15 00:07:41, Ilya Sherman ...
1 year, 3 months ago (2017-02-15 09:55:34 UTC) #22
Ilya Sherman
Metrics LGTM, thanks.
1 year, 3 months ago (2017-02-15 23:55:27 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2688633002/100001
1 year, 3 months ago (2017-02-16 09:17:06 UTC) #26
commit-bot: I haz the power
1 year, 3 months ago (2017-02-16 11:47:36 UTC) #29
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2e4491eac73425489384c1428b17...

Powered by Google App Engine
This is Rietveld 408576698