Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2199763002:
Add in expiring budget (Closed)
|
Created:
4 years, 4 months ago by harkness Modified:
4 years, 4 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@budget_database Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd in expiring budget
Before returning budget chunks to the user or writing chunks to the store, remove any chunks that have expired.
BUG=617971
Committed: https://crrev.com/5186e6d82af00c6c3381c552af31e2e7b132f067
Cr-Commit-Position: refs/heads/master@{#410708}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Converted to base::Time return and other code review changes #
Total comments: 9
Patch Set 3 : Added IsCached and tests, moved the Internal to time::Base conversion #
Total comments: 1
Patch Set 4 : const IsCache #Patch Set 5 : rebased #Patch Set 6 : full rebase #
Depends on Patchset: Messages
Total messages: 25 (12 generated)
Description was changed from ========== Add in expiring budget BUG=617971 ========== to ========== Add in expiring budget Before returning budget chunks to the user or writing chunks to the store, remove any chunks that have expired. BUG=617971 ==========
harkness@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database.cc (left): https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:86: void BudgetDatabase::WriteCachedValuesToDatabase( This just moved to the bottom of the file to mirror the order in the header.
https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:144: std::make_pair(total, clock_->Now().ToInternalValue())); Sorry that I missed this before- the GetBudgetDetailsCallback callback should of course receive base::Time instances. Use of the internal values should be limited to this class and its underlying storage. As such, let's convert the values to base::Time in AddToCache() and rely on base::Time everywhere else. They support comparison operators too, so your use on line 202 only becomes simpler. https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:193: return; style nit: no comment (redundant), no brackets for one-line ifs https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:204: } What if |chunks.empty()| is true now? https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database_unittest.cc:145: iter = expectation().begin(); Please ASSERT() something before this line, if begin() == end() then line 146 will end up accessing something undefined. (Dito on line 156.)
https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:144: std::make_pair(total, clock_->Now().ToInternalValue())); On 2016/08/01 17:35:28, Peter Beverloo wrote: > Sorry that I missed this before- the GetBudgetDetailsCallback callback should of > course receive base::Time instances. Use of the internal values should be > limited to this class and its underlying storage. > > As such, let's convert the values to base::Time in AddToCache() and rely on > base::Time everywhere else. They support comparison operators too, so your use > on line 202 only becomes simpler. Done. https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:193: return; On 2016/08/01 17:35:28, Peter Beverloo wrote: > style nit: no comment (redundant), no brackets for one-line ifs Done. https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database.cc:204: } On 2016/08/01 17:35:28, Peter Beverloo wrote: > What if |chunks.empty()| is true now? I don't expect budget_map_ to become so large that performance is an issue, but I can go ahead and clean it up here. Actually, I really need to do this, since otherwise it won't actually remove the entry from the physical storage (although it will 0 them out). https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... File chrome/browser/budget_service/budget_database_unittest.cc (right): https://codereview.chromium.org/2199763002/diff/1/chrome/browser/budget_servi... chrome/browser/budget_service/budget_database_unittest.cc:145: iter = expectation().begin(); On 2016/08/01 17:35:28, Peter Beverloo wrote: > Please ASSERT() something before this line, if begin() == end() then line 146 > will end up accessing something undefined. > > (Dito on line 156.) Done.
https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:192: double now = clock_->Now().ToInternalValue(); Supposedly we'd like to change the second double in BudgetChunks to base::Time as well? (I.e. remove ToInternalValue() from line 79; add FromInternalValue() to line 104.) That would allow removal of ToInternalValue() here. https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:198: while (cleanup_iter != chunks.end() && cleanup_iter->second <= now) { Not for this CL, but I'd hope you agree that all the |first|/|second| usage in this file is not at all readable :-). Maybe we could consider using small structures w/ named members instead? https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:198: while (cleanup_iter != chunks.end() && cleanup_iter->second <= now) { nit: remove {} for one-line statements https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:204: budget_map_.erase(origin.spec()); Maybe add a test to verify that we actually remove entries from the database when this happens?
https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:192: double now = clock_->Now().ToInternalValue(); On 2016/08/08 15:02:55, Peter Beverloo wrote: > Supposedly we'd like to change the second double in BudgetChunks to base::Time > as well? (I.e. remove ToInternalValue() from line 79; add FromInternalValue() to > line 104.) That would allow removal of ToInternalValue() here. I was thinking we would have the cache be a true mirror of the database, but there's really no reason it needs to be, since we're already switching from protos to stl structures. I'll move the translation boundary. https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:198: while (cleanup_iter != chunks.end() && cleanup_iter->second <= now) { On 2016/08/08 15:02:55, Peter Beverloo wrote: > nit: remove {} for one-line statements Done. https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:198: while (cleanup_iter != chunks.end() && cleanup_iter->second <= now) { On 2016/08/08 15:02:55, Peter Beverloo wrote: > Not for this CL, but I'd hope you agree that all the |first|/|second| usage in > this file is not at all readable :-). > > Maybe we could consider using small structures w/ named members instead? As we discussed in person yesterday, I'm planning to remove the debt, and then consider making a wrapper class/structure for BudgetChunks. https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:204: budget_map_.erase(origin.spec()); On 2016/08/08 15:02:55, Peter Beverloo wrote: > Maybe add a test to verify that we actually remove entries from the database > when this happens? Added a test and found a bug/incomplete implementation. Op success!
lgtm https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2199763002/diff/20001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:204: budget_map_.erase(origin.spec()); On 2016/08/09 12:57:40, harkness wrote: > On 2016/08/08 15:02:55, Peter Beverloo wrote: > > Maybe add a test to verify that we actually remove entries from the database > > when this happens? > > Added a test and found a bug/incomplete implementation. Op success! As the British would say: "woop!" https://codereview.chromium.org/2199763002/diff/40001/chrome/browser/budget_s... File chrome/browser/budget_service/budget_database.cc (right): https://codereview.chromium.org/2199763002/diff/40001/chrome/browser/budget_s... chrome/browser/budget_service/budget_database.cc:86: bool BudgetDatabase::IsCached(const GURL& origin) { nit: make const?
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2199763002/#ps60001 (title: "const IsCache")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2199763002/#ps80001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2199763002/#ps100001 (title: "full rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add in expiring budget Before returning budget chunks to the user or writing chunks to the store, remove any chunks that have expired. BUG=617971 ========== to ========== Add in expiring budget Before returning budget chunks to the user or writing chunks to the store, remove any chunks that have expired. BUG=617971 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add in expiring budget Before returning budget chunks to the user or writing chunks to the store, remove any chunks that have expired. BUG=617971 ========== to ========== Add in expiring budget Before returning budget chunks to the user or writing chunks to the store, remove any chunks that have expired. BUG=617971 Committed: https://crrev.com/5186e6d82af00c6c3381c552af31e2e7b132f067 Cr-Commit-Position: refs/heads/master@{#410708} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5186e6d82af00c6c3381c552af31e2e7b132f067 Cr-Commit-Position: refs/heads/master@{#410708} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
