Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1221523003:
Add a SiteEngagementEvictionPolicy. (Closed)
DescriptionAdd a SiteEngagementEvictionPolicy.
This CL adds a TemporaryStorageEvictionPolicy interface to QuotaManager which
is implemented by a new SiteEngagementEvictionPolicy which picks the origin to
evict based on its site engagement score and usage.
This will then be integrated into the actual temporary storage eviction system
in a follow-up patch.
BUG=464234
TBR=jam@chromium.org
Committed: https://crrev.com/508c6fa442424455b9746ba81bc4b9b59a059187
Cr-Commit-Position: refs/heads/master@{#345813}
Patch Set 1 #
Total comments: 33
Patch Set 2 : address most comments #Patch Set 3 : refactor #
Total comments: 4
Patch Set 4 : refactor again #
Total comments: 14
Patch Set 5 : address comments #Patch Set 6 : fix test #
Total comments: 4
Patch Set 7 : move static method into anonymous namespace #Patch Set 8 : Extract SiteEngagementScoreProvider interface #
Total comments: 16
Patch Set 9 : address_comments_and_rebase #
Total comments: 10
Patch Set 10 : const ref #
Total comments: 4
Patch Set 11 : remove unnecessary destructor #Dependent Patchsets: Messages
Total messages: 41 (12 generated)
calamity@chromium.org changed reviewers: + raymes@chromium.org
This one's more interesting.
I just have to review the unittest still. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:26: for (auto usage : usage_map) { const auto& usage https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:35: return origin_to_evict; As discussed offline, this is a heuristic but I think it should make it clear what we're trying to optimise for (either by code or in comment) because then if someone wanted to write a better heuristic they could. I think there are 2 things we want to optimise: -evicting the sites that the user cares about least, first -evicting the smallest number of sites to get under the quota limit I think the current heuristic gives some combination of these 2 goals. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:45: base::Unretained(this), usage_map, global_quota), Are we sure this object will be alive for the life of the call? https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:55: return engagement_service_->GetTotalEngagementPoints(); Did you consider making these functions virtual in SiteEngagementService (or pulling them into an API which SiteEngagementService implements) https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:62: double quota_per_point = Out of curiosity, what units is quota? I would guess something like bytes or kb which should be fine. I was just worried that quota_per_point might be very small, in which case we could re-arrange the multiplications/divisions to get better accuracy, but I don't think that should be the case. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:64: std::max(kExpectedEngagementSites * SiteEngagementScore::kMaxPoints, Hmm, what is kExpectedEngagementSites exactly? Is this the max number of sites that we expect to have quota? https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:73: callback.Run(origin_to_evict); We should be able to pull this out into a function in the anonymous namespace above :) https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_eviction_policy.h (right): https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.h:18: // Returns the origin from |usage_map| to evict based on usage and site nit: Returns the next origin from |usage_map| to evict https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.h:21: int64 global_quota); nit: include gurl.h and <map> https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.h:46: DISALLOW_COPY_AND_ASSIGN(SiteEngagementEvictionPolicy); base/macros.h https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:1476: } Were these 2 functions just moved to fix ordering? https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.h:72: class STORAGE_EXPORT TemporaryStorageEvictionPolicy { Perhaps make a note that this isn't hooked up in the QuotaEvictionHandler yet? I'm guessing the LRU scheme would eventually become a policy? Is that right? https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.h:74: virtual void GetEvictionOrigin(const std::map<GURL, int64>& usage_map, We should document this. I think it's good to note that this returns the /next/ origin that should be evicted based on the inputs (could even name it GetNextEvictionOrigin or GetNextOriginToEvict). Also noting that the origin could be empty of no origins can be or need to be evicted.
Okay, I'll do that big refactor so don't re-review this quite yet. I'll ping again when it's done. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:26: for (auto usage : usage_map) { On 2015/07/08 04:43:29, raymes wrote: > const auto& usage Done. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:35: return origin_to_evict; On 2015/07/08 04:43:29, raymes wrote: > As discussed offline, this is a heuristic but I think it should make it clear > what we're trying to optimise for (either by code or in comment) because then if > someone wanted to write a better heuristic they could. I think there are 2 > things we want to optimise: > -evicting the sites that the user cares about least, first > -evicting the smallest number of sites to get under the quota limit > > I think the current heuristic gives some combination of these 2 goals. Done. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:45: base::Unretained(this), usage_map, global_quota), On 2015/07/08 04:43:29, raymes wrote: > Are we sure this object will be alive for the life of the call? As discussed this should be solved with a big refactor of this patch. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:55: return engagement_service_->GetTotalEngagementPoints(); On 2015/07/08 04:43:29, raymes wrote: > Did you consider making these functions virtual in SiteEngagementService (or > pulling them into an API which SiteEngagementService implements) I did. I'm not sure how much the site engagement service will change so I don't think it's worth pulling out a TestEngagementService quite yet. When there are more clients and the API is more stable, it's definitely something I'll do. Added a TODO to the header. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:62: double quota_per_point = On 2015/07/08 04:43:29, raymes wrote: > Out of curiosity, what units is quota? I would guess something like bytes or kb > which should be fine. I was just worried that quota_per_point might be very > small, in which case we could re-arrange the multiplications/divisions to get > better accuracy, but I don't think that should be the case. Bytes. Do you think that it's worth doing this anyway? https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:64: std::max(kExpectedEngagementSites * SiteEngagementScore::kMaxPoints, On 2015/07/08 04:43:29, raymes wrote: > Hmm, what is kExpectedEngagementSites exactly? Is this the max number of sites > that we expect to have quota? It's a reasonable minimum. This is there so that the overuse value is still meaningful if the user has only visited a few sites. Otherwise the soft quota would be very large and engagement wouldn't affect the overuse metric as much. We may just want a maximum and minimum for quota_per_point to achieve the same goal... https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:73: callback.Run(origin_to_evict); On 2015/07/08 04:43:29, raymes wrote: > We should be able to pull this out into a function in the anonymous namespace > above :) Done. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_eviction_policy.h (right): https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.h:18: // Returns the origin from |usage_map| to evict based on usage and site On 2015/07/08 04:43:29, raymes wrote: > nit: Returns the next origin from |usage_map| to evict Done. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.h:21: int64 global_quota); On 2015/07/08 04:43:29, raymes wrote: > nit: include gurl.h and <map> Done. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.h:46: DISALLOW_COPY_AND_ASSIGN(SiteEngagementEvictionPolicy); On 2015/07/08 04:43:29, raymes wrote: > base/macros.h Done. https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:1476: } On 2015/07/08 04:43:29, raymes wrote: > Were these 2 functions just moved to fix ordering? Yup. Shame there's no way to indicate that =( https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.h:72: class STORAGE_EXPORT TemporaryStorageEvictionPolicy { On 2015/07/08 04:43:29, raymes wrote: > Perhaps make a note that this isn't hooked up in the QuotaEvictionHandler yet? > > I'm guessing the LRU scheme would eventually become a policy? Is that right? Done. https://codereview.chromium.org/1221523003/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.h:74: virtual void GetEvictionOrigin(const std::map<GURL, int64>& usage_map, On 2015/07/08 04:43:29, raymes wrote: > We should document this. I think it's good to note that this returns the /next/ > origin that should be evicted based on the inputs (could even name it > GetNextEvictionOrigin or GetNextOriginToEvict). Also noting that the origin > could be empty of no origins can be or need to be evicted. Done.
Here's the majority of the changes. There will be a bit of tweaking for testing to work out, but probably worth taking a look now.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1221523003/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_eviction_policy.h (right): https://codereview.chromium.org/1221523003/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_eviction_policy.h:24: class GetEvictionOriginTask { This should be a private class, right? https://codereview.chromium.org/1221523003/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service_proxy.cc (right): https://codereview.chromium.org/1221523003/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service_proxy.cc:13: content::BrowserContext* browser_context, We can't know for sure whether the browser_context is valid here.
Updated. https://codereview.chromium.org/1221523003/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_eviction_policy.h (right): https://codereview.chromium.org/1221523003/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_eviction_policy.h:24: class GetEvictionOriginTask { On 2015/07/15 07:55:47, raymes wrote: > This should be a private class, right? Hmm. I originally did have it as an anonymous class in the .cc but exposed it here for testing. I guess I can make it private and friend the test. https://codereview.chromium.org/1221523003/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_service_proxy.cc (right): https://codereview.chromium.org/1221523003/diff/60001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_service_proxy.cc:13: content::BrowserContext* browser_context, On 2015/07/15 07:55:47, raymes wrote: > We can't know for sure whether the browser_context is valid here. We now check with the profile manager.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:27: int64 overuse = hmm overuse sort of implies that this will always be positive, but that's not the case. It's sort of the usage relative to the quota. But I can't think of better terminology here so overuse is probably fine :) Perhaps note this in a comment https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:55: return engagement_service_->GetTotalEngagementPoints(); Ok, sounds good. I didn't notice a TODO but maybe I'm missing it :) If the interface ends up staying the same then we should consider it later. It will just require adding a SiteEngementScoreMap (or something) which has those 2 functions (GetScore and GetTotal) and is implemented by the SiteEngagementService. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:64: std::max(kExpectedEngagementSites * SiteEngagementScore::kMaxPoints, Hmm say the number of points was 3 and site A had 2 points and site B had 1. Global quota = 100GB. That would mean that A would have a soft quota of 66GB and B of 33GB. Their usage wouldn't affect their overuse very much because it's presumably small. Is this what we're trying to protect against? The problem in my mind right now is that if there is sufficiently large disk space relative to the total number of points then we will always have this problem. So perhaps the minimum number of points should be relative to the global quota? (though I haven't done my maths on this so I don't know if it would fix all the problems :) What if we (for some reason) end up with a very large number of points relative to the global quota. I think this would mean that usage would have a relatively large affect on overuse (the reverse case) is that true? Is that something we want to protect against too? Hmm.. Were these cases considered in the design doc? It could be something worth documenting because it seems tricky! :) https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:55: base::Unretained(this))); I think we need to pass "this" rather than base::Unretained(this) to ensure that a reference gets held. Another option here, rather than making SiteEngagementEvictionPolicy refcounted would be to bind result_callback_ to GetEvictionOriginTask::ReplyWithEvictionOrigin (and make that function standalone as well). This is slightly safer because it means that if for some reason the other thread doesn't respond (e.g. if it has been shutdown for some reason) then we won't leak this object. Then you could remove the GetEvictionOriginTask class altogether and just inline this PostTaskAndReply call and all the other functions would be standalone in the anonymous namespace. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:112: } nit: I think we can make the above 2 functions standalone in the anonymous namespace https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:130: } nit: I think we should reorder this to match the header (SiteENgagementEvictionPolicy first) https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc (right): https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:53: CalculateEvictionOrigin(service_, usage, kGlobalQuota); Ohh I see, this one is static for testing. That seems ok then. If we do remove GetEvictionOriginTask perhaps we can make this a static function on SiteEngagementEvictionPolicy or else a little class SiteEngagementEvictionCalculator and make it clear that we're just testing that unit of functionality https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:85: // less usage. This comment is a bit hard to read in the context of the code. Perhaps: // Now |url2| has the most usage but |url3| has the least engagement score so one of them should be evicted. In this case the heuristic chooses |url3|. ... https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:89: // But exceeding allocated usage too much will still result in being evicted. result in being evicted even though the engagement with |url2| is higher. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:91: } I can think of at least one more test to add: All sites have the same score but different usage. Make sure that the highest usage is thrown away.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1221523003/120001
https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:27: int64 overuse = On 2015/07/21 03:16:00, raymes wrote: > hmm overuse sort of implies that this will always be positive, but that's not > the case. It's sort of the usage relative to the quota. But I can't think of > better terminology here so overuse is probably fine :) Perhaps note this in a > comment Done. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:55: return engagement_service_->GetTotalEngagementPoints(); On 2015/07/21 03:16:00, raymes wrote: > Ok, sounds good. I didn't notice a TODO but maybe I'm missing it :) > > If the interface ends up staying the same then we should consider it later. It > will just require adding a SiteEngementScoreMap (or something) which has those 2 > functions (GetScore and GetTotal) and is implemented by the > SiteEngagementService. This changed and there's a TestSiteEngagementService in the unit test now. https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:64: std::max(kExpectedEngagementSites * SiteEngagementScore::kMaxPoints, On 2015/07/21 03:16:00, raymes wrote: > Hmm say the number of points was 3 and site A had 2 points and site B had 1. > Global quota = 100GB. That would mean that A would have a soft quota of 66GB and > B of 33GB. Their usage wouldn't affect their overuse very much because it's > presumably small. Is this what we're trying to protect against? The problem in > my mind right now is that if there is sufficiently large disk space relative to > the total number of points then we will always have this problem. So perhaps the > minimum number of points should be relative to the global quota? (though I > haven't done my maths on this so I don't know if it would fix all the problems > :) > > What if we (for some reason) end up with a very large number of points relative > to the global quota. I think this would mean that usage would have a relatively > large affect on overuse (the reverse case) is that true? Is that something we > want to protect against too? Hmm.. > > Were these cases considered in the design doc? It could be something worth > documenting because it seems tricky! :) Yeah, this was definitely something I was thinking about a bit. I think we can just forge ahead with this basic algorithm for now and do a small amount of math later and account for these cases. I don't want to jam this largely structural CL chock full of heuristics and tests for those sorts of corner cases. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:55: base::Unretained(this))); On 2015/07/21 03:16:00, raymes wrote: > I think we need to pass "this" rather than base::Unretained(this) to ensure that > a reference gets held. > > Another option here, rather than making SiteEngagementEvictionPolicy refcounted > would be to bind result_callback_ to > GetEvictionOriginTask::ReplyWithEvictionOrigin (and make that function > standalone as well). This is slightly safer because it means that if for some > reason the other thread doesn't respond (e.g. if it has been shutdown for some > reason) then we won't leak this object. Then you could remove the > GetEvictionOriginTask class altogether and just inline this PostTaskAndReply > call and all the other functions would be standalone in the anonymous namespace. Done. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:112: } On 2015/07/21 03:16:00, raymes wrote: > nit: I think we can make the above 2 functions standalone in the anonymous > namespace Done. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:130: } On 2015/07/21 03:16:00, raymes wrote: > nit: I think we should reorder this to match the header > (SiteENgagementEvictionPolicy first) Done. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc (right): https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:53: CalculateEvictionOrigin(service_, usage, kGlobalQuota); On 2015/07/21 03:16:00, raymes wrote: > Ohh I see, this one is static for testing. That seems ok then. If we do remove > GetEvictionOriginTask perhaps we can make this a static function on > SiteEngagementEvictionPolicy or else a little class > SiteEngagementEvictionCalculator and make it clear that we're just testing that > unit of functionality Done. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:85: // less usage. On 2015/07/21 03:16:00, raymes wrote: > This comment is a bit hard to read in the context of the code. Perhaps: > // Now |url2| has the most usage but |url3| has the least engagement score so > one of them should be evicted. In this case the heuristic chooses |url3|. > ... Done. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:89: // But exceeding allocated usage too much will still result in being evicted. On 2015/07/21 03:16:00, raymes wrote: > result in being evicted even though the engagement with |url2| is higher. Done. https://codereview.chromium.org/1221523003/diff/100001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy_unittest.cc:91: } On 2015/07/21 03:16:00, raymes wrote: > I can think of at least one more test to add: All sites have the same score but > different usage. Make sure that the highest usage is thrown away. Done.
calamity@chromium.org changed reviewers: + michaeln@chromium.org
+michaeln for storage/ OWNERS.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1221523003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/1/chrome/browser/engagement/s... chrome/browser/engagement/site_engagement_eviction_policy.cc:55: return engagement_service_->GetTotalEngagementPoints(); I still think we should eventually remove the virtual functions in favour of an interface that gets implemented. Let's discuss tomorrow but this can be done in a followup CL. https://codereview.chromium.org/1221523003/diff/140001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/140001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:58: GURL SiteEngagementEvictionPolicy::GetSiteEngagementEvictionOriginOnUIThread( nit: I think we can make this a standalone function too?
https://codereview.chromium.org/1221523003/diff/140001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/140001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:58: GURL SiteEngagementEvictionPolicy::GetSiteEngagementEvictionOriginOnUIThread( On 2015/07/21 09:22:20, raymes wrote: > nit: I think we can make this a standalone function too? It won't be able to see the private static method below =(
Extracted interface.
Doing this by way of an abstraction to pick the victim is great. Please see the comments about SpecialStoragePolicy. https://codereview.chromium.org/1221523003/diff/140001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/140001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:54: base::Bind(&ReplyWithEvictionOrigin, callback)); would it work to use 'callback' here directly without the intermediary function? https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:42: // The heuristic for deciding the next eviction origin calculates a soft It's very hard to evaluate the specifics of this heuristic? Where can i learn about how the 'score' is computed? https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:45: // exceeds its soft quota is chosen. An input missing from this heuristic is whether the site is actually accessing its stored data. That's something that maybe could be put into the usage_map. https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:65: content::BrowserContext* browser_context, I don't think this is necessarily a problem but worth knowing, there can be mutliple storagepartitions and quotamanangers for each browsercontext. https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:90: void SiteEngagementEvictionPolicy::GetEvictionOrigin( I think this needs to be informed by the SpecialStoragePolicy too, IsStorageUnlimited() or IsStorageDurable() origins should probably be excluded. https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... File storage/browser/quota/quota_callbacks.h (right): https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... storage/browser/quota/quota_callbacks.h:34: typedef base::Callback<void(const GURL&)> GetEvictionOriginCallback; maybe make it completely generic GetOriginCallback since it's in the common.h file now https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... storage/browser/quota/quota_manager.h:79: virtual void GetEvictionOrigin(const std::map<GURL, int64>& usage_map, I think we need to add SpecialStoragePolicy* to the inputs here. And consider adding lastaccess times too?
https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... storage/browser/quota/quota_manager.h:75: class STORAGE_EXPORT TemporaryStorageEvictionPolicy { I'd vote for a simpler name, QuotaEvictionPolicy.
https://codereview.chromium.org/1221523003/diff/140001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/140001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:54: base::Bind(&ReplyWithEvictionOrigin, callback)); On 2015/07/23 02:22:32, michaeln wrote: > would it work to use 'callback' here directly without the intermediary function? Done. https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:42: // The heuristic for deciding the next eviction origin calculates a soft On 2015/07/23 02:22:32, michaeln wrote: > It's very hard to evaluate the specifics of this heuristic? Where can i learn > about how the 'score' is computed? Here's the doc I've been working from. https://docs.google.com/document/d/1GQE9gguqVMgXvR68jZyIsBhHVoAJb9m4fTn6omfqj... It's worth noting that this is still highly experimental so we'll be iterating on the heuristics a bit. https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:45: // exceeds its soft quota is chosen. On 2015/07/23 02:22:32, michaeln wrote: > An input missing from this heuristic is whether the site is actually accessing > its stored data. That's something that maybe could be put into the usage_map. Good point. I think we'll look at this in a future CL. We don't want a case where some large local storage never gets used but is kept around only due to site engagement. Added a TODO. https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:65: content::BrowserContext* browser_context, On 2015/07/23 02:22:32, michaeln wrote: > I don't think this is necessarily a problem but worth knowing, there can be > mutliple storagepartitions and quotamanangers for each browsercontext. Acknowledged. https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:90: void SiteEngagementEvictionPolicy::GetEvictionOrigin( On 2015/07/23 02:22:32, michaeln wrote: > I think this needs to be informed by the SpecialStoragePolicy too, > IsStorageUnlimited() or IsStorageDurable() origins should probably be excluded. Good point. https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... File storage/browser/quota/quota_callbacks.h (right): https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... storage/browser/quota/quota_callbacks.h:34: typedef base::Callback<void(const GURL&)> GetEvictionOriginCallback; On 2015/07/23 02:22:32, michaeln wrote: > maybe make it completely generic GetOriginCallback since it's in the common.h > file now Done. https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... storage/browser/quota/quota_manager.h:75: class STORAGE_EXPORT TemporaryStorageEvictionPolicy { On 2015/07/23 02:26:18, michaeln wrote: > I'd vote for a simpler name, QuotaEvictionPolicy. Done. https://codereview.chromium.org/1221523003/diff/180001/storage/browser/quota/... storage/browser/quota/quota_manager.h:79: virtual void GetEvictionOrigin(const std::map<GURL, int64>& usage_map, On 2015/07/23 02:22:32, michaeln wrote: > I think we need to add SpecialStoragePolicy* to the inputs here. And consider > adding lastaccess times too? I think the last access time is an improvement to the heuristic that can be added later. I added the SpecialStoragePolicy stuff since that would affect the correctness of this patch. Test added too.
michaeln@chromium.org changed reviewers: + jsbell@chromium.org
@jsbell, ptal at the quota computations https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:42: // The heuristic for deciding the next eviction origin calculates a soft On 2015/07/28 01:07:59, calamity wrote: > On 2015/07/23 02:22:32, michaeln wrote: > > It's very hard to evaluate the specifics of this heuristic? Where can i learn > > about how the 'score' is computed? > > Here's the doc I've been working from. > > https://docs.google.com/document/d/1GQE9gguqVMgXvR68jZyIsBhHVoAJb9m4fTn6omfqj... > > It's worth noting that this is still highly experimental so we'll be iterating > on the heuristics a bit. Thnx for the ptr to the doc. https://codereview.chromium.org/1221523003/diff/180001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:45: // exceeds its soft quota is chosen. On 2015/07/28 01:07:59, calamity wrote: > On 2015/07/23 02:22:32, michaeln wrote: > > An input missing from this heuristic is whether the site is actually accessing > > its stored data. That's something that maybe could be put into the usage_map. > > Good point. I think we'll look at this in a future CL. We don't want a case > where some large local storage never gets used but is kept around only due to > site engagement. Added a TODO. ok, how about we also file a bug about this too, i'd like to not forget about it prior to enabling the flag by default https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:27: return score * quota_per_point; Upon a first visit to a site, score is expected to be 1, is that right, is that the lowest 'score' possible? I see max is 100. 1G quota --> 50k quota-per-point --> 5M max softquota for the most engaged site 100G quota --> 5M quota-per-point --> 500M max softquota Those 5M and 500M numbers look very low to my eye for a nominal amount of storage that the least and most important sites can use. I'd like to get some other opinions about this strategy of computing this soft quota number prior to committing it. https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:31: scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy, ditto https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:72: scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy, ditto const ref https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:100: scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy, how about using const scoped_refptr<T>& here instead of passing by value https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:114: scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy, ditto
https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_eviction_policy.cc (right): https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:27: return score * quota_per_point; On 2015/07/29 00:46:45, michaeln wrote: > Upon a first visit to a site, score is expected to be 1, is that right, is that > the lowest 'score' possible? I see max is 100. Yes for now, but this may change. Even if it ends up as a minimum score of 0, it won't prevent sites from using temporary storage outright. > > 1G quota --> 50k quota-per-point --> 5M max softquota for the most engaged site > 100G quota --> 5M quota-per-point --> 500M max softquota > > Those 5M and 500M numbers look very low to my eye for a nominal amount of > storage that the least and most important sites can use. I'd like to get some > other opinions about this strategy of computing this soft quota number prior to > committing it. > Acknowledged. FYI UMA suggests that <10% of users have >100M total temporary storage usage and 99% have <100 temporary storage origins. https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:31: scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy, On 2015/07/29 00:46:46, michaeln wrote: > ditto Done. https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:72: scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy, On 2015/07/29 00:46:46, michaeln wrote: > ditto const ref Done. https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:100: scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy, On 2015/07/29 00:46:46, michaeln wrote: > how about using const scoped_refptr<T>& here instead of passing by value Done. https://codereview.chromium.org/1221523003/diff/200001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_eviction_policy.cc:114: scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy, On 2015/07/29 00:46:46, michaeln wrote: > ditto Done.
lgtm
What's the plan for landing this? It'd be nice to get it in and iterate, if michaeln@ is okay with the changes and general direction.
On 2015/08/24 18:04:53, jsbell wrote: > What's the plan for landing this? It'd be nice to get it in and iterate, if > michaeln@ is okay with the changes and general direction. Agreed. michaeln@: Was there anything else?
lgtm i'm pretty confident that delegating to an evictionhandler is a good thing, i'm less confident about all the details... but yet lets iterate https://codereview.chromium.org/1221523003/diff/220001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1221523003/diff/220001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:90: virtual int GetScore(const GURL& url) = 0; aside: i wonder if these values should be dbls to allow finer granularity? https://codereview.chromium.org/1221523003/diff/220001/storage/browser/quota/... File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1221523003/diff/220001/storage/browser/quota/... storage/browser/quota/quota_manager.h:86: ~QuotaEvictionPolicy() {} Should this dtor be virtual and/or public? What's the intent of the protected non-virtual destructor? The derived class has a public virtual dtor. The mix of those is a little odd. This CL doesn't yet establish ownership of eviction policy objects or how the quota system gets a ptr to the derived instance. It might make sense for the QM to get an own ptr to an evictionhandler in its constructor? Did you have anything in particular in mind.
calamity@chromium.org changed reviewers: + jam@chromium.org
TBRing jam for content/ test affected by name changes. https://codereview.chromium.org/1221523003/diff/220001/chrome/browser/engagem... File chrome/browser/engagement/site_engagement_service.h (right): https://codereview.chromium.org/1221523003/diff/220001/chrome/browser/engagem... chrome/browser/engagement/site_engagement_service.h:90: virtual int GetScore(const GURL& url) = 0; On 2015/08/26 23:32:37, michaeln wrote: > aside: i wonder if these values should be dbls to allow finer granularity? Hmm. I'm not sure how granular we want the exposed score to be since we want to keep the public API 'simple'-ish to work with. It's definitely worth thinking about. https://codereview.chromium.org/1221523003/diff/220001/storage/browser/quota/... File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1221523003/diff/220001/storage/browser/quota/... storage/browser/quota/quota_manager.h:86: ~QuotaEvictionPolicy() {} On 2015/08/26 23:32:37, michaeln wrote: > Should this dtor be virtual and/or public? > > What's the intent of the protected non-virtual destructor? The derived class has > a public virtual dtor. The mix of those is a little odd. > > This CL doesn't yet establish ownership of eviction policy objects or how the > quota system gets a ptr to the derived instance. It might make sense for the QM > to get an own ptr to an evictionhandler in its constructor? Did you have > anything in particular in mind. Oops. Yes, this destructor doesn't need to be defined just yet. I think I needed it for refptr stuff which isn't in this patch? Removing. I had the full hook-up sorted in another patch which I'm updating and will upload soon.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, michaeln@chromium.org, jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/1221523003/#ps240001 (title: "remove unnecessary destructor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1221523003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1221523003/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/508c6fa442424455b9746ba81bc4b9b59a059187 Cr-Commit-Position: refs/heads/master@{#345813} |
