Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(478)

Issue 2403193002: Precache: Optionally rank resources-to-precache globally. (Closed)

Created:
4 years, 2 months ago by twifkak
Modified:
3 years, 12 months ago
Reviewers:
bengr, jamartin, Raj
CC:
chromium-reviews, jam, darin-cc_chromium.org, wifiprefetch-reviews_google.com
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Precache: Optionally rank resources-to-precache globally. Added a new global_ranking varation param to the experiment, default false. If specified, the client will multiply each resource weight (a value [0,1]) from the PrecacheManifest by the number of visits to that host -- the result being an estimated likelihood of using that resource. The resources will then be fetched in decreasing order of likelihood. This allows hosts with vastly more visits to fetch more resources than hosts with vastly fewer visits. It also allows mostly-useless resources to bubble down and be replaced by resources for additional hosts. In addition, added a couple new fields to PrecacheConfigurationSettings to tweak its behavior: - total_resources_count is a cap on the total number of resource URLs to fetch - min_weight is a threshold on the minimum likelihood In addition, a few things changed in behavior: - manifests are fetched first, before any resources - the PercentCompleted UMA is measured in % of resource URLs complete rather than % of manifests complete - resource ordering is preserved across pause/resume BUG=654166 Committed: https://crrev.com/528a15b56652173bd822ea2a1123e6960ba4f255 Cr-Commit-Position: refs/heads/master@{#426350}

Patch Set 1 #

Total comments: 18

Patch Set 2 : Code readability improvements per bengr. #

Total comments: 10

Patch Set 3 : Extract QueueResourcesForFetch() and other cleanup. #

Patch Set 4 : Log 0/0 to overflow bucket instead of 0 bucket. #

Patch Set 5 : Move global_ranking from variation param to proto. #

Patch Set 6 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+641 lines, -211 lines) Patch
M components/precache/content/precache_manager.cc View 1 2 3 4 2 chunks +14 lines, -8 lines 0 comments Download
M components/precache/core/precache_fetcher.h View 1 2 3 4 7 chunks +46 lines, -7 lines 0 comments Download
M components/precache/core/precache_fetcher.cc View 1 2 3 4 20 chunks +127 lines, -88 lines 0 comments Download
M components/precache/core/precache_fetcher_unittest.cc View 1 2 3 4 32 chunks +430 lines, -107 lines 0 comments Download
M components/precache/core/proto/precache.proto View 1 2 3 4 5 2 chunks +16 lines, -0 lines 0 comments Download
M components/precache/core/proto/unfinished_work.proto View 2 chunks +8 lines, -1 line 0 comments Download

Messages

Total messages: 31 (15 generated)
twifkak
4 years, 2 months ago (2016-10-10 19:46:38 UTC) #4
jamartin
I'm not convinced we should do the host-level threshold on the % of visits. http://crbug.com/638423 ...
4 years, 2 months ago (2016-10-12 19:32:27 UTC) #8
bengr
On 2016/10/12 19:32:27, jamartin wrote: > I'm not convinced we should do the host-level threshold ...
4 years, 2 months ago (2016-10-14 21:22:18 UTC) #9
twifkak
On 2016/10/14 21:22:18, bengr wrote: > On 2016/10/12 19:32:27, jamartin wrote: > > I'm not ...
4 years, 2 months ago (2016-10-14 21:31:07 UTC) #11
bengr
https://codereview.chromium.org/2403193002/diff/1/components/precache/content/precache_manager.cc File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2403193002/diff/1/components/precache/content/precache_manager.cc#newcode151 components/precache/content/precache_manager.cc:151: unfinished_work->set_start_time(base::Time::Now().ToInternalValue()); Why is start time being set here too? ...
4 years, 2 months ago (2016-10-14 21:52:19 UTC) #12
twifkak
https://codereview.chromium.org/2403193002/diff/1/components/precache/content/precache_manager.cc File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2403193002/diff/1/components/precache/content/precache_manager.cc#newcode151 components/precache/content/precache_manager.cc:151: unfinished_work->set_start_time(base::Time::Now().ToInternalValue()); On 2016/10/14 21:52:19, bengr wrote: > Why is ...
4 years, 2 months ago (2016-10-14 22:41:45 UTC) #13
bengr
https://chromiumcodereview.appspot.com/2403193002/diff/1/components/precache/core/precache_fetcher.cc File components/precache/core/precache_fetcher.cc (right): https://chromiumcodereview.appspot.com/2403193002/diff/1/components/precache/core/precache_fetcher.cc#newcode380 components/precache/core/precache_fetcher.cc:380: VLOG(6) << "Percent completed: " << percent_completed; On 2016/10/14 ...
4 years, 2 months ago (2016-10-18 18:57:20 UTC) #14
twifkak
https://codereview.chromium.org/2403193002/diff/1/components/precache/core/precache_fetcher.cc File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2403193002/diff/1/components/precache/core/precache_fetcher.cc#newcode380 components/precache/core/precache_fetcher.cc:380: VLOG(6) << "Percent completed: " << percent_completed; On 2016/10/18 ...
4 years, 2 months ago (2016-10-18 19:52:27 UTC) #16
bengr
lgtm https://codereview.chromium.org/2403193002/diff/20001/components/precache/core/precache_fetcher.cc File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2403193002/diff/20001/components/precache/core/precache_fetcher.cc#newcode374 components/precache/core/precache_fetcher.cc:374: num_total_resources == 0 On 2016/10/18 19:52:27, twifkak wrote: ...
4 years, 2 months ago (2016-10-18 21:50:31 UTC) #17
twifkak
Thanks for the review! I'll send to CQ later today. https://codereview.chromium.org/2403193002/diff/20001/components/precache/core/precache_fetcher.cc File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/2403193002/diff/20001/components/precache/core/precache_fetcher.cc#newcode374 ...
4 years, 2 months ago (2016-10-18 22:55:48 UTC) #18
twifkak
Manual testing looks good. In patch set 5, made global_ranking a field in the config ...
4 years, 2 months ago (2016-10-19 22:23:04 UTC) #19
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/2403193002/80001
4 years, 2 months ago (2016-10-19 22:32:07 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/90504) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, ...
4 years, 2 months ago (2016-10-19 22:35:29 UTC) #24
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/2403193002/100001
4 years, 2 months ago (2016-10-19 22:47:58 UTC) #27
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 2 months ago (2016-10-20 00:22:28 UTC) #29
commit-bot: I haz the power
4 years, 2 months ago (2016-10-21 13:13:47 UTC) #31
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/528a15b56652173bd822ea2a1123e6960ba4f255
Cr-Commit-Position: refs/heads/master@{#426350}

Powered by Google App Engine
This is Rietveld 408576698