Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 2 days ago by Benoit L
Modified:
1 week, 2 days 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
Trybot results:  linux_chromium_asan_rel_ng   win_chromium_rel_ng   linux_chromium_tsan_rel_ng   ios-simulator-xcode-clang   android_arm64_dbg_recipe   chromeos_daisy_chromium_compile_only_ng   cast_shell_linux   linux_chromium_chromeos_ozone_rel_ng   cast_shell_android   ios-device   linux_android_rel_ng   mac_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   android_compile_dbg   android_cronet   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   mac_chromium_rel_ng   linux_chromium_asan_rel_ng   ios-simulator   win_clang   android_n5x_swarming_rel   android_clang_dbg_recipe   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   chromium_presubmit   linux_chromium_chromeos_rel_ng   ios-device-xcode-clang   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_compile_dbg_ng   ios-simulator-xcode-clang   ios-simulator   ios-device   ios-device-xcode-clang   linux_chromium_rel_ng   linux_chromium_tsan_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_asan_rel_ng   chromeos_daisy_chromium_compile_only_ng   chromium_presubmit   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   linux_android_rel_ng   android_n5x_swarming_rel   cast_shell_android   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe 
Commit queue not available (can’t edit this change).

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. ...
2 weeks, 2 days 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 week, 5 days 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 week, 5 days 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 week, 5 days 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 week, 5 days 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 week, 4 days 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 week, 3 days 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 week, 3 days ago (2017-02-15 09:55:34 UTC) #22
Ilya Sherman
Metrics LGTM, thanks.
1 week, 2 days 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 week, 2 days ago (2017-02-16 09:17:06 UTC) #26
commit-bot: I haz the power
1 week, 2 days 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd