Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903573002:
[Thumbnails DB] Add functionality to clear unused on-demand favicons. (Closed)
Description[Thumbnails DB] Add functionality to clear unused on-demand favicons.
The preceding CL 2856873002 adds a concept of on-demand favicons into
ThumbnailsDatabase that should get evicted based on last_requested
timestamp.
This CL adds the functionality to clear on-demand favicons that have not
been used for long.
BUG=709471
Review-Url: https://codereview.chromium.org/2903573002
Cr-Commit-Position: refs/heads/master@{#489307}
Committed: https://chromium.googlesource.com/chromium/src/+/9946c3ce2b116a954f25a63a19be1ba414b3be53
Patch Set 1 #
Total comments: 10
Patch Set 2 : Rebase #Patch Set 3 : Peter's comments #
Total comments: 11
Patch Set 4 : Peter's comments #2 #Patch Set 5 : Implementation variants + unit-tests #
Total comments: 2
Patch Set 6 : Peter's comments #3 #
Total comments: 35
Patch Set 7 : Peter's comments #3 #Patch Set 8 : One forgotten nit #Patch Set 9 : Fix a failing test and minor cleanup #Patch Set 10 : Another build fix #
Total comments: 20
Patch Set 11 : Peter's comments #4 #
Total comments: 11
Patch Set 12 : Peter's comments #5 #Patch Set 13 : Update naming in expire_history_backend #
Total comments: 6
Patch Set 14 : Brett's comments #
Total comments: 8
Patch Set 15 : Brett's comments (done by mastiz@) #Patch Set 16 : Compile error fixes #Patch Set 17 : Compile error fixes #2 #
Total comments: 1
Messages
Total messages: 59 (35 generated)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jkrcal@chromium.org changed reviewers: + pkotwicz@chromium.org, sky@chromium.org
Could you PTAL? (detailed comments do not make sense before the previous CL https://codereview.chromium.org/2856873002/ stabilizes somewhat)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The code looks mostly good https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:462: "favicon_bitmaps.last_requested<?;")); If: 1) SetOnDemandFavicons() is called 2) SetFavicons() with the same page URL (as a result of a user visiting the page) Won't this function incorrectly delete the favicon? Don't you also need to check that favicon_bitmaps.last_updated is expired? https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:470: FaviconBitmapID bitmap_id = delete_candidates.ColumnInt64(0); The "id" column in the favicon_bitmaps table is the favicon_base::FaviconBitmapID. The "icon_id" column in the favicon_bitmaps table is the favicon_base::FaviconID https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:477: ids_to_delete.erase(bitmap_id); Is it possible for |bitmap_id| to have been added to |ids_to_delete|?
Thanks! Peter, could you please take another look? https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:462: "favicon_bitmaps.last_requested<?;")); On 2017/05/24 17:05:03, pkotwicz wrote: > If: > 1) SetOnDemandFavicons() is called > 2) SetFavicons() with the same page URL (as a result of a user visiting the > page) > > Won't this function incorrectly delete the favicon? > > Don't you also need to check that favicon_bitmaps.last_updated is expired? This has been fixed in the preceding CL: it holds that last_requested > 0 iff last_updated == 0. On the other hand, I am wondering about the transition to this system: - Before this work is complete, all on demand favicons are stored with both timestamps equal to 0 and I'd like to tidy up. - Thus, I'd like to delete here both old on demand favicons as well as expired on-visit favicons. - This could be only for a transitional phase and after one milestone, get back to deleting only old on-demand bitmaps. Do you see a problem in deleting expired icons? https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:470: FaviconBitmapID bitmap_id = delete_candidates.ColumnInt64(0); On 2017/05/24 17:05:03, pkotwicz wrote: > The "id" column in the favicon_bitmaps table is the > favicon_base::FaviconBitmapID. The "icon_id" column in the favicon_bitmaps table > is the favicon_base::FaviconID Ah, thanks! Done (incl. variable names). https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:477: ids_to_delete.erase(bitmap_id); On 2017/05/24 17:05:03, pkotwicz wrote: > Is it possible for |bitmap_id| to have been added to |ids_to_delete|? Yes, if first some non-bookmarked page with this icon appeared in the while loop. I reworded the variables, I hope it is clearer now.
https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:477: ids_to_delete.erase(bitmap_id); Ahhh. I see now. Thank you for the explanation https://codereview.chromium.org/2903573002/diff/40001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:431: void ThumbnailDatabase::ClearOldOnDemandFavicons( Can you please add the code to call this function in this CL too? It is hard to look at the CL without the calling code. https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:432: base::Time expiration_threshold) { Maybe rename |expiration_threshold| to |deletion_threshold| https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:440: "favicon_bitmaps.last_requested<?;")); - How long does this query take? I am wondering whether the query would be faster if it was done in multiple parts. Part #1: Get the icon ids Part #2: For each icon id get the page URLs - Will this query return multiple identical (icon_id, page_url) pairs (If a favicon is mapped to multiple favicon bitmaps)? https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:453: if (backend_client_ && backend_client_->IsBookmarked(page_url)) { In my opinion, ThumbnailDatabase should not know about bookmarks. HistoryBackend and ExpireHistoryBackend know about bookmarks though. https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:466: statement.Run(); Is deleting any "favicon bitmaps" and "icons" for |icon_id| faster than DeleteOrphanagedFaviconBitmaps() and DeleteOrphanagedMappings() ?
Thanks! PTAL, again. https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:462: "favicon_bitmaps.last_requested<?;")); On 2017/06/21 17:19:20, jkrcal wrote: > On 2017/05/24 17:05:03, pkotwicz wrote: > > If: > > 1) SetOnDemandFavicons() is called > > 2) SetFavicons() with the same page URL (as a result of a user visiting the > > page) > > > > Won't this function incorrectly delete the favicon? > > > > Don't you also need to check that favicon_bitmaps.last_updated is expired? > > This has been fixed in the preceding CL: it holds that > last_requested > 0 iff last_updated == 0. > > On the other hand, I am wondering about the transition to this system: > - Before this work is complete, all on demand favicons are stored with both > timestamps equal to 0 and I'd like to tidy up. > - Thus, I'd like to delete here both old on demand favicons as well as expired > on-visit favicons. > - This could be only for a transitional phase and after one milestone, get back > to deleting only old on-demand bitmaps. > > Do you see a problem in deleting expired icons? Pinging this question. https://codereview.chromium.org/2903573002/diff/40001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:431: void ThumbnailDatabase::ClearOldOnDemandFavicons( On 2017/06/21 19:50:18, pkotwicz wrote: > Can you please add the code to call this function in this CL too? It is hard to > look at the CL without the calling code. Done. I've added a feature that controls the calls (deleting icons can cause troubles if done wrongly so I believe we should roll out it experimentally). Later, I add metrics that helps us evaluate the effects. https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:432: base::Time expiration_threshold) { On 2017/06/21 19:50:17, pkotwicz wrote: > Maybe rename |expiration_threshold| to |deletion_threshold| Done. https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:440: "favicon_bitmaps.last_requested<?;")); On 2017/06/21 19:50:17, pkotwicz wrote: > - How long does this query take? I am wondering whether the query would be > faster if it was done in multiple parts. > Part #1: Get the icon ids > Part #2: For each icon id get the page URLs > > - Will this query return multiple identical (icon_id, page_url) pairs (If a > favicon is mapped to multiple favicon bitmaps)? Replying to both you performance commments here. I've added two variants you propose as variations of the current code and added unit-tests to measure time it takes (uploaded here, will remove again in the next patch set): Problem size Current DeleteOneByOne SelectNoJoin (on-demans icons x page URLs) 20 icons x 20 urls 0.12s 0.12s 0.12s 20 icons x 100 urls 1.78s 1.87s 1.91s 20 icons x 200 urls 6.55s 6.56s 6.60s 500 icons x 10 urls 10.14s 10.19s 10.44s 1000 icons x 2 urls 1.81s 1.86s 1.81s 1000 icons x 10 urls 40.84s 42.60s >50s 2000 icons x 2 urls 6.91s 6.95s 7.11s 3000 icons x 2 urls 15.28s 15.19s 15.25s All times are measured only once. By observation, there are fluctuations by ca. +-2%. As another experiment, I did a realistic small scale example 50 times and give the average time here: 20 icons x 10 urls (50 times) 0.030s 0.026s 0.034s It seems all options are roughly comparable. SelectNoJoin is rarely better and performs worse for many pairs in the join. I would rule it out. I am open to both Current and DeleteOneByOne. WDYT? https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:453: if (backend_client_ && backend_client_->IsBookmarked(page_url)) { On 2017/06/21 19:50:18, pkotwicz wrote: > In my opinion, ThumbnailDatabase should not know about bookmarks. HistoryBackend > and ExpireHistoryBackend know about bookmarks though. How would you like to solve that? What about passing here an interface FaviconDeletionFilter with a single function bool CanDeleteFaviconForPageUrl(const GURL& page url); The ExpirerHistoryBackend would then create this filter using the BookmarkModel? https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:466: statement.Run(); On 2017/06/21 19:50:18, pkotwicz wrote: > Is deleting any "favicon bitmaps" and "icons" for |icon_id| faster than > DeleteOrphanagedFaviconBitmaps() and DeleteOrphanagedMappings() ? See above.
https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:462: "favicon_bitmaps.last_requested<?;")); I don't see a problem in expiring the icons https://codereview.chromium.org/2903573002/diff/40001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:440: "favicon_bitmaps.last_requested<?;")); Let's go with DeleteOneByOne because it is the fastest https://codereview.chromium.org/2903573002/diff/80001/components/history/core... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/80001/components/history/core... components/history/core/browser/expire_history_backend.cc:491: GetCurrentExpirationTime(), reader, kNumExpirePerIteration); Can we call ClearOldOnDemandFavicons() here if |more_to_expire| is false You can make ClearOldOnDemandFavicons() return a std::vector of pairs containing (page_url, icon_id) and check whether any of the pages are bookmarked here. This would be consistent with how history expiry works. You can use: - ThumbnailDatabase::DeleteFavicon() (existing function) and - ThumbnailDatabase::DeleteIconMappingForFaviconId() (new function) to do the deletion
Thanks for your comments! Could you PTAL? (I am sorry for the delay, I've been travelling.) https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/1/components/history/core/bro... components/history/core/browser/thumbnail_database.cc:462: "favicon_bitmaps.last_requested<?;")); On 2017/06/22 20:32:22, pkotwicz wrote: > I don't see a problem in expiring the icons Done. https://codereview.chromium.org/2903573002/diff/80001/components/history/core... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/80001/components/history/core... components/history/core/browser/expire_history_backend.cc:491: GetCurrentExpirationTime(), reader, kNumExpirePerIteration); On 2017/06/22 20:32:22, pkotwicz wrote: > Can we call ClearOldOnDemandFavicons() here if |more_to_expire| is false > > You can make ClearOldOnDemandFavicons() return a std::vector of pairs containing > (page_url, icon_id) and check whether any of the pages are bookmarked here. This > would be consistent with how history expiry works. > > You can use: > - ThumbnailDatabase::DeleteFavicon() (existing function) > and > - ThumbnailDatabase::DeleteIconMappingForFaviconId() (new function) > to do the deletion Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/thumbnail_database_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:16: #include "build/build_config.h" "build/build_config.h" has been enforced by a presubmit check
Mostly nits https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend.cc:523: !thumb_db_->DeleteIconMappingsForFaviconId(icon_id)) Don't you need to update DeleteEffects::deleted_favicons ? https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend.cc:524: return; For the sake of consistency with DeleteFaviconsIfPossible() don't return if a given deletion fails https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/expire_history_backend.h (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend.h:193: void ClearOldOnDemandFavicons(base::Time expiration_threshold); Nit: Match the function order in the .cc and the .h file https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/expire_history_backend_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:894: Can you please add comments for these test cases https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:911: base::Time::Now() - base::TimeDelta::FromDays(100), gfx::Size()); Can you please use the 6 argument version of ThumbnailDatabase::AddFavicon() ? https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database.h:70: // (i.e. not after |threshold|). How about: "that have been requested prior to |threshold|." https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/thumbnail_database_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:396: Please add a comment for each test case. For this test case, how about: "Test that ThumbnailDatabase::GetOldOnDemandFavicons() returns on-demand icons which were requested prior to the passed in timestamp." https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:399: ThumbnailDatabase db(&mock_client); Does the test work if you pass in nullptr? https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:413: icon, favicon, FaviconBitmapType::ON_DEMAND, start, gfx::Size()); Nit: Can you use the 6 argument ThumbnailDatabase::AddFavicon() call? https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:417: base::Time delete_older_than = start + base::TimeDelta::FromDays(7); Might as well do: "+ base::TimeDelta::FromSeconds(1)" https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:420: // The icon is returned for deletion. Nit: Don't mention deletion. The fact that the id is used for deletion is an implementation detail of ExpireHistoryBackend https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:425: For this test's comment, how about: "Test that ThumbnailDatabase::GetOldOnDemandFavicons() returns on-visit icons if the on-visit icons have expired. We need this behavior in order to delete icons stored via HistoryService::SetOnDemandFavicons() prior to on-demand icons setting the "last_requested" time." https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:448: auto list = db.GetOldOnDemandFavicons(/*threshold=*/base::Time()); Maybe use base::Time::Now() instead? https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:456: TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsDoesNotReturnFresh) { For this test's comment how about: "Test that ThumbnailDatabase::GetOldOnDemandFavicons() does not return on-demand icons which were requested after to the passed in timestamp." https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:484: EXPECT_EQ(0u, list.size()); Nit: EXPECT_TRUE(list.empty()); https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:486: For this test's comment how about: "Test that ThumbnailDatabase::GetOldOnDemandFavicons() does not return non-expired on-visit icons." https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:511: EXPECT_EQ(0u, list.size()); Nit: EXPECT_TRUE(list.empty());
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the detailed comments, Peter! Could you PTAL? https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend.cc:523: !thumb_db_->DeleteIconMappingsForFaviconId(icon_id)) On 2017/07/07 00:42:38, pkotwicz wrote: > Don't you need to update DeleteEffects::deleted_favicons ? Done. This meant pulling also the icon_url. This was strong enough reason to create a struct for the result and move some of the logic back to thumbnail_database.cc. I like it much better now. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend.cc:524: return; On 2017/07/07 00:42:38, pkotwicz wrote: > For the sake of consistency with DeleteFaviconsIfPossible() don't return if a > given deletion fails Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/expire_history_backend.h (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend.h:193: void ClearOldOnDemandFavicons(base::Time expiration_threshold); On 2017/07/07 00:42:38, pkotwicz wrote: > Nit: Match the function order in the .cc and the .h file Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/expire_history_backend_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:894: On 2017/07/07 00:42:38, pkotwicz wrote: > Can you please add comments for these test cases Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:911: base::Time::Now() - base::TimeDelta::FromDays(100), gfx::Size()); On 2017/07/07 00:42:38, pkotwicz wrote: > Can you please use the 6 argument version of ThumbnailDatabase::AddFavicon() ? Ah, good point. I also expanded the second test a bit to check that "at least one starred" suffices for not deleting. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database.h:70: // (i.e. not after |threshold|). On 2017/07/07 00:42:38, pkotwicz wrote: > How about: "that have been requested prior to |threshold|." Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... File components/history/core/browser/thumbnail_database_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:396: On 2017/07/07 00:42:38, pkotwicz wrote: > Please add a comment for each test case. For this test case, how about: > > "Test that ThumbnailDatabase::GetOldOnDemandFavicons() returns on-demand icons > which were requested prior to the passed in timestamp." Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:399: ThumbnailDatabase db(&mock_client); On 2017/07/07 00:42:39, pkotwicz wrote: > Does the test work if you pass in nullptr? Ah, sure. It was there to mock being bookmarked. Not needed anymore. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:413: icon, favicon, FaviconBitmapType::ON_DEMAND, start, gfx::Size()); On 2017/07/07 00:42:39, pkotwicz wrote: > Nit: Can you use the 6 argument ThumbnailDatabase::AddFavicon() call? Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:417: base::Time delete_older_than = start + base::TimeDelta::FromDays(7); On 2017/07/07 00:42:39, pkotwicz wrote: > Might as well do: > > "+ base::TimeDelta::FromSeconds(1)" Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:420: // The icon is returned for deletion. On 2017/07/07 00:42:39, pkotwicz wrote: > Nit: Don't mention deletion. The fact that the id is used for deletion is an > implementation detail of ExpireHistoryBackend Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:425: On 2017/07/07 00:42:39, pkotwicz wrote: > For this test's comment, how about: > > "Test that ThumbnailDatabase::GetOldOnDemandFavicons() returns on-visit icons if > the on-visit icons have expired. We need this behavior in order to delete icons > stored via HistoryService::SetOnDemandFavicons() prior to on-demand icons > setting the "last_requested" time." Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:448: auto list = db.GetOldOnDemandFavicons(/*threshold=*/base::Time()); On 2017/07/07 00:42:39, pkotwicz wrote: > Maybe use base::Time::Now() instead? Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:456: TEST_F(ThumbnailDatabaseTest, GetOldOnDemandFaviconsDoesNotReturnFresh) { On 2017/07/07 00:42:39, pkotwicz wrote: > For this test's comment how about: > > "Test that ThumbnailDatabase::GetOldOnDemandFavicons() does not return on-demand > icons > which were requested after to the passed in timestamp." Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:484: EXPECT_EQ(0u, list.size()); On 2017/07/07 00:42:39, pkotwicz wrote: > Nit: > EXPECT_TRUE(list.empty()); Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:486: On 2017/07/07 00:42:39, pkotwicz wrote: > For this test's comment how about: > > "Test that ThumbnailDatabase::GetOldOnDemandFavicons() does not return > non-expired on-visit icons." Done. https://codereview.chromium.org/2903573002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:511: EXPECT_EQ(0u, list.size()); On 2017/07/07 00:42:39, pkotwicz wrote: > Nit: > EXPECT_TRUE(list.empty()); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 jkrcal@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Just nits! https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend.cc:119: std::vector<GURL> urls) { Nit: const std::vector<GURL>& urls https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend.cc:508: std::map<favicon_base::FaviconID, FaviconURLs> favicons = I would rather that you do one of: 1) Put the FaviconID and the favicon URL in a struct instead of the icon URL and the page URL. It will likely be harder to return a std::map if you go down this route. 2) Make use of the already existing IconMapping struct. sky@ will likely have an opinion as to whether you should fill all of the fields in the IconMapping struct 3) Rename the FaviconURLs struct to something more intuitive. I don't have any suggestions but it should likely have the word "mapping" in the name https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/expire_history_backend_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:902: "12346102356120394751634516591348710478123649165419234519234512349134"; Nit: Can blob be super short? (Like 1 character long) https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:937: favicon_base::FaviconID icon = thumb_db_->AddFavicon( Can you rename |icon| to |icon_id| to make it clearer that the variable refers to an ID and not an SkBitmap https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:944: GURL page_url2("http://google.com/1"); Nit: Shouldn't the URLs be different? https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/thumbnail_database_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:17: #include "components/history/core/browser/history_backend_client.h" Do you need this include? https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:32: using testing::StrictMock; Do you need this line? https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:413: // Icon: old unused case. You don't need the comment here because you have a comment above the test case https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:470: // on-demand icons which were requested after to the passed in timestamp. Nit: "after to the" -> "after the"
jkrcal@chromium.org changed reviewers: + brettw@chromium.org - sky@chromium.org
Thanks, Peter! PTAL, again. Brett, could you PTAL? (Adding Brett instead of Scott because Scott is still OOO) https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend.cc:119: std::vector<GURL> urls) { On 2017/07/10 05:23:28, pkotwicz wrote: > Nit: const std::vector<GURL>& urls Ah, sure. Thanks! https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend.cc:508: std::map<favicon_base::FaviconID, FaviconURLs> favicons = On 2017/07/10 05:23:28, pkotwicz wrote: > I would rather that you do one of: > 1) Put the FaviconID and the favicon URL in a struct instead of the icon URL and > the page URL. It will likely be harder to return a std::map if you go down this > route. > 2) Make use of the already existing IconMapping struct. sky@ will likely have an > opinion as to whether you should fill all of the fields in the IconMapping > struct > 3) Rename the FaviconURLs struct to something more intuitive. I don't have any > suggestions but it should likely have the word "mapping" in the name I do understand the benefits of 2) or 3) but miss the benefits of 1). If this was your preferred choice, could you clarify, please? I tried another shot on 3): IconMappingsForExpiry. I am also fine with the more generic approach of reusing IconMappings; I still prefer a custom struct for this task. https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/expire_history_backend_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:902: "12346102356120394751634516591348710478123649165419234519234512349134"; On 2017/07/10 05:23:28, pkotwicz wrote: > Nit: Can blob be super short? (Like 1 character long) Done. https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:937: favicon_base::FaviconID icon = thumb_db_->AddFavicon( On 2017/07/10 05:23:28, pkotwicz wrote: > Can you rename |icon| to |icon_id| to make it clearer that the variable refers > to an ID and not an SkBitmap Done. https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:944: GURL page_url2("http://google.com/1"); On 2017/07/10 05:23:28, pkotwicz wrote: > Nit: Shouldn't the URLs be different? Done. https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/thumbnail_database_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:17: #include "components/history/core/browser/history_backend_client.h" On 2017/07/10 05:23:28, pkotwicz wrote: > Do you need this include? Ah, correct. Thanks! https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:32: using testing::StrictMock; On 2017/07/10 05:23:28, pkotwicz wrote: > Do you need this line? Done. https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:413: // Icon: old unused case. On 2017/07/10 05:23:28, pkotwicz wrote: > You don't need the comment here because you have a comment above the test case Done. https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:470: // on-demand icons which were requested after to the passed in timestamp. On 2017/07/10 05:23:28, pkotwicz wrote: > Nit: "after to the" -> "after the" Done. (sorry for not proof-reading your previous proposal properly ;)
Looks good from my point of view https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend.cc:508: std::map<favicon_base::FaviconID, FaviconURLs> favicons = I don't have a strong preference between the 3 options. The idea with (1) was to group variables which are related to each other in the same struct. Currently you have: std::map<FaviconID, std::map<GURL, std::vector<GURL>>> where IconMappingsForExpiry is std::map<GURL, std::vector<GURL>> I was suggesting changing the structure to std::map<std::pair<FaviconID, GURL>, std::vector<GURL>> where std::pair<FaviconID, GURL> would be a new struct (Though I think you would need to implement a custom 'less than' operator for the struct) Returning a std::map of std::maps is kind of weird in my opinion https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/expire_history_backend_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:904: scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data)); Aside: There is a RefCountedBytes() constructor with a char* pointer and size https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:418: // while. Restrict to on-demand bitmaps (i.e. with last_requested != 0). Nit: Remove the sentence: "Select all bitmaps (and their page URLs) that have not been accessed for a while." This is clear from the comment in the .h file https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:428: "(last_requested>0 AND last_requested<?));")); Can (last_requested=0 AND last_updated=0) OR (last_requested>0 AND last_requested<?) be simplified to last_requested<? AND last_updated=0 https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:431: std::map<favicon_base::FaviconID, IconMappingsForExpiry> favicons; Maybe rename this variable to |icon_mappings| https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.h:208: // Deletes the icon mapping entries for the given favicon. Nit: "given favicon" -> "given favicon ID"
Looks good from my point of view https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend.cc:508: std::map<favicon_base::FaviconID, FaviconURLs> favicons = I don't have a strong preference between the 3 options. The idea with (1) was to group variables which are related to each other in the same struct. Currently you have: std::map<FaviconID, std::map<GURL, std::vector<GURL>>> where IconMappingsForExpiry is std::map<GURL, std::vector<GURL>> I was suggesting changing the structure to std::map<std::pair<FaviconID, GURL>, std::vector<GURL>> where std::pair<FaviconID, GURL> would be a new struct (Though I think you would need to implement a custom 'less than' operator for the struct) Returning a std::map of std::maps is kind of weird in my opinion https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/expire_history_backend_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:904: scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data)); Aside: There is a RefCountedBytes() constructor with a char* pointer and size https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:418: // while. Restrict to on-demand bitmaps (i.e. with last_requested != 0). Nit: Remove the sentence: "Select all bitmaps (and their page URLs) that have not been accessed for a while." This is clear from the comment in the .h file https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:428: "(last_requested>0 AND last_requested<?));")); Can (last_requested=0 AND last_updated=0) OR (last_requested>0 AND last_requested<?) be simplified to last_requested<? AND last_updated=0 https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:431: std::map<favicon_base::FaviconID, IconMappingsForExpiry> favicons; Maybe rename this variable to |icon_mappings| https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.h:208: // Deletes the icon mapping entries for the given favicon. Nit: "given favicon" -> "given favicon ID"
Thanks, Peter! Brett, friendly ping! https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/180001/components/history/cor... components/history/core/browser/expire_history_backend.cc:508: std::map<favicon_base::FaviconID, FaviconURLs> favicons = On 2017/07/10 19:50:19, pkotwicz wrote: > I don't have a strong preference between the 3 options. The idea with (1) was to > group variables which are related to each other in the same struct. > > Currently you have: > std::map<FaviconID, std::map<GURL, std::vector<GURL>>> > > where > IconMappingsForExpiry is > std::map<GURL, std::vector<GURL>> Not really. Currently, I have conceptually std::map<FaviconID, std::pair<GURL, std::vector<GURL>>> where IconMappingsForExpiry corresponds to std::pair<GURL, std::vector<GURL>. > I was suggesting changing the structure to > std::map<std::pair<FaviconID, GURL>, std::vector<GURL>> > where std::pair<FaviconID, GURL> would be a new struct (Though I think you would > need to implement a custom 'less than' operator for the struct) Thanks for explaining! I still lean towards option 3 (as implemented now). > Returning a std::map of std::maps is kind of weird in my opinion As argued above, I do not return a map of maps. https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/expire_history_backend_unittest.cc (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/expire_history_backend_unittest.cc:904: scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data)); On 2017/07/10 19:50:19, pkotwicz wrote: > Aside: There is a RefCountedBytes() constructor with a char* pointer and size Done. https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:418: // while. Restrict to on-demand bitmaps (i.e. with last_requested != 0). On 2017/07/10 19:50:19, pkotwicz wrote: > Nit: Remove the sentence: "Select all bitmaps (and their page URLs) that have > not been accessed for a while." This is clear from the comment in the .h file Done. https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:428: "(last_requested>0 AND last_requested<?));")); On 2017/07/10 19:50:19, pkotwicz wrote: > Can > > (last_requested=0 AND last_updated=0) OR (last_requested>0 AND last_requested<?) > > be simplified to > > last_requested<? AND last_updated=0 Yes, you are right. Still, I would stick to the current version. I believe that the current verbose version makes the proposed clean-up (in M63) easier to understand and thus less error-prone. https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:431: std::map<favicon_base::FaviconID, IconMappingsForExpiry> favicons; On 2017/07/10 19:50:19, pkotwicz wrote: > Maybe rename this variable to |icon_mappings| Done. https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.h:208: // Deletes the icon mapping entries for the given favicon. On 2017/07/10 19:50:19, pkotwicz wrote: > Nit: "given favicon" -> "given favicon ID" Done.
LGTM. Thank you for bearing with me. https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:428: "(last_requested>0 AND last_requested<?));")); Fair enough
Am I correct that on-demand favicons come when you have a NTP item you haven't visited yet? The current code looks like it will run a check for expired on-demand favicons every 5 minutes or so (this is the history expiration time delay). To do this, it does a brute-force search through all favicons. This seems like a lot of work to do at this timescale for the likelihood of deleting anything. It seems like a timescale of days might be more appropriate, or at least that we had some confidence that something likely changed. Do you have any thoughs on how to reduce the overhead here? We probably want to be sure this expiration is run when you explicitly clear any history. https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... components/history/core/browser/expire_history_backend.cc:496: // Otherwise do a final clean-up - remove old favicons not bound to visits. Style nit: I find this comment before the else with no {} weird to look at since it breaks up the if statement. Maybe add braces to the whole thing and do it like this? } else { // Otherwise... Clear()... } https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... components/history/core/browser/thumbnail_database.cc:424: "favicons, favicon_bitmaps, icon_mapping WHERE favicon_bitmaps.icon_id = " Can you express this as an explicit JOIN across the tables rather than implicitly with WHERE? Then you can reserve the WHERE clause for the thing you're actually looking for. https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... components/history/core/browser/thumbnail_database.cc:426: "((last_requested=0 AND last_updated=0) OR " I like to be explicit about the table names for the columns being selected when doing a query across multiple tables.
On 2017/07/11 19:26:43, brettw wrote: > Am I correct that on-demand favicons come when you have a NTP item you haven't > visited yet? Correct. There might be other use cases later (iOS is interested in this mechanism for high-res icons for synced bookmarks / history). > The current code looks like it will run a check for expired on-demand favicons > every 5 minutes or so (this is the history expiration time delay). To do this, > it does a brute-force search through all favicons. > > This seems like a lot of work to do at this timescale for the likelihood of > deleting anything. It seems like a timescale of days might be more appropriate, > or at least that we had some confidence that something likely changed. Do you > have any thoughs on how to reduce the overhead here? Ah, good point. I still think that putting the code into expire_history_backend makes sense. From my point of view, the best (simple) solution is to reduce the frequency by keeping a member variable (last_cleared_time_) that blocks this to at most once per day if Chrome keeps running / at most once per startup of Chrome, otherwise. It is not clear to me, how to persist this information / extract it from the DB without too much overhead (such as wiring in PrefService). Another (minor) optimization might be to first check in the favicon_bitmaps table whether there is anything to delete by a separate SELECT before doing the full JOIN query (not sure if this saves anything, depends on how SQLite implements the full query). WDYT? > We probably want to be sure this expiration is run when you explicitly clear any > history. Good point, will do. > https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... > File components/history/core/browser/expire_history_backend.cc (right): > > https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... > components/history/core/browser/expire_history_backend.cc:496: // Otherwise do a > final clean-up - remove old favicons not bound to visits. > Style nit: I find this comment before the else with no {} weird to look at since > it breaks up the if statement. Maybe add braces to the whole thing and do it > like this? > } else { > // Otherwise... > Clear()... > } > > https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... > File components/history/core/browser/thumbnail_database.cc (right): > > https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... > components/history/core/browser/thumbnail_database.cc:424: "favicons, > favicon_bitmaps, icon_mapping WHERE favicon_bitmaps.icon_id = " > Can you express this as an explicit JOIN across the tables rather than > implicitly with WHERE? Then you can reserve the WHERE clause for the thing > you're actually looking for. > > https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... > components/history/core/browser/thumbnail_database.cc:426: "((last_requested=0 > AND last_updated=0) OR " > I like to be explicit about the table names for the columns being selected when > doing a query across multiple tables.
On 2017/07/11 19:42:59, jkrcal wrote: > Ah, good point. I still think that putting the code into expire_history_backend > makes > sense. From my point of view, the best (simple) solution is to reduce the > frequency by > keeping a member variable (last_cleared_time_) that blocks this to at most once > per day if Chrome keeps running / at most once per startup of Chrome, otherwise. Sounds good. > It is not clear to me, how to persist this information / extract it from the DB > without too much > overhead (such as wiring in PrefService). The official way to persist it would be the meta table, but I don't think that's necessary, I'm OK running once every time you run the browser, as long as it's not too close to startup. > Another (minor) optimization might be to first check in the favicon_bitmaps > table > whether there is anything to delete by a separate SELECT before doing the full > JOIN query > (not sure if this saves anything, depends on how SQLite implements the full > query). I think it should be possible to express this in your SQL query depending on how you express the joins (you may want to consider a LEFT OUTER JOIN). One way to get an idea of what's happening is to run the sqlite command line tool: sqlite3 History Then type: explain query plan select * from urls; (replacing "select..." with your query). You can see the order of operations and what it's searching over
Thanks, PTAL, again! https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... components/history/core/browser/expire_history_backend.cc:496: // Otherwise do a final clean-up - remove old favicons not bound to visits. On 2017/07/11 19:26:43, brettw wrote: > Style nit: I find this comment before the else with no {} weird to look at since > it breaks up the if statement. Maybe add braces to the whole thing and do it > like this? > } else { > // Otherwise... > Clear()... > } Done. https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... components/history/core/browser/thumbnail_database.cc:424: "favicons, favicon_bitmaps, icon_mapping WHERE favicon_bitmaps.icon_id = " On 2017/07/11 19:26:43, brettw wrote: > Can you express this as an explicit JOIN across the tables rather than > implicitly with WHERE? Then you can reserve the WHERE clause for the thing > you're actually looking for. Done. https://codereview.chromium.org/2903573002/diff/240001/components/history/cor... components/history/core/browser/thumbnail_database.cc:426: "((last_requested=0 AND last_updated=0) OR " On 2017/07/11 19:26:43, brettw wrote: > I like to be explicit about the table names for the columns being selected when > doing a query across multiple tables. Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... components/history/core/browser/expire_history_backend.cc:513: if (expiration_threshold < Can you put a comment on this block about how this is avoiding running expiration very often? It kind of looks like it might be a functional part of the expiration logic. https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:421: sql::Statement old_icons(db_.GetCachedStatement( Can you make this be a non-cached ("unique") statement? Since we execute it so rarely, the caching will just waste memory. https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:423: "SELECT favicons.id, favicons.url, icon_mapping.page_url FROM " Can you wrap this SQL string like code so it's readable? "SELECT favicons.id, favicons.url, icon_mapping.page_url " "FROM favicons " "JOIN favicon_bitmaps ON (favicon_bitmaps.icon_id = favicons.id) " "JOIN icon_mapping ON (icon_mapping.icon_id = favicon_bitmaps.icon_id) " "WHERE ((favicon_bitmaps.last_requested = 0 AND " "favicon_bitmaps.last_updated = 0) OR " "(favicon_bitmaps.last_requested > 0 AND " "favicon_bitmaps.last_requested < ?))" https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:810: sql::Statement statement(db_.GetCachedStatement( Maybe do a unique statement here too. Might be worth adding a comment about how this is called rarely so it's not cached.
mastiz@chromium.org changed reviewers: + mastiz@chromium.org
jkrcal@ is OOO and has asked me to follow up on the remaining nits. I've done this in https://chromium-review.googlesource.com/574910, please take a look (patchset 1 is the last snapshot by jkrcal@ and patchset 2 contains the addressed comments). Would you mind LGTM-ing this other CL? Thx! https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... File components/history/core/browser/expire_history_backend.cc (right): https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... components/history/core/browser/expire_history_backend.cc:513: if (expiration_threshold < On 2017/07/13 23:50:16, brettw wrote: > Can you put a comment on this block about how this is avoiding running > expiration very often? It kind of looks like it might be a functional part of > the expiration logic. Done, but please double check because I didn't fully understand what you want to reflect there: the *how* seems quite obvious from the code, did you mean *why*? https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:421: sql::Statement old_icons(db_.GetCachedStatement( On 2017/07/13 23:50:17, brettw wrote: > Can you make this be a non-cached ("unique") statement? Since we execute it so > rarely, the caching will just waste memory. Done. https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:423: "SELECT favicons.id, favicons.url, icon_mapping.page_url FROM " On 2017/07/13 23:50:16, brettw wrote: > Can you wrap this SQL string like code so it's readable? > > "SELECT favicons.id, favicons.url, icon_mapping.page_url " > "FROM favicons " > "JOIN favicon_bitmaps ON (favicon_bitmaps.icon_id = favicons.id) " > "JOIN icon_mapping ON (icon_mapping.icon_id = favicon_bitmaps.icon_id) " > "WHERE ((favicon_bitmaps.last_requested = 0 AND " > "favicon_bitmaps.last_updated = 0) OR " > "(favicon_bitmaps.last_requested > 0 AND " > "favicon_bitmaps.last_requested < ?))" Done, with slight modifications because indentation needs to be inside the string literal. https://codereview.chromium.org/2903573002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:810: sql::Statement statement(db_.GetCachedStatement( On 2017/07/13 23:50:17, brettw wrote: > Maybe do a unique statement here too. Might be worth adding a comment about how > this is called rarely so it's not cached. Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry for the confusion, I am back from vacation and have uploaded the version of mastiz@ where the last nits are addressed. https://codereview.chromium.org/2903573002/diff/320001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2903573002/diff/320001/components/history/cor... components/history/core/browser/thumbnail_database.cc:432: old_icons.BindInt64(0, threshold.ToInternalValue()); I've ignored a presubmit warning to avoid deprecated Time::ToInternalValue(). To me, it makes sense to replace all instances in this file in one CL so that it stays consistent.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, mastiz@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2903573002/#ps320001 (title: "Compile error fixes #2")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1500995827808390,
"parent_rev": "a4b3c363967cb68a7d68a06545c6131c4d4a7171", "commit_rev":
"9946c3ce2b116a954f25a63a19be1ba414b3be53"}
Message was sent while issue was closed.
Description was changed from ========== [Thumbnails DB] Add functionality to clear unused on-demand favicons. The preceding CL 2856873002 adds a concept of on-demand favicons into ThumbnailsDatabase that should get evicted based on last_requested timestamp. This CL adds the functionality to clear on-demand favicons that have not been used for long. BUG=709471 ========== to ========== [Thumbnails DB] Add functionality to clear unused on-demand favicons. The preceding CL 2856873002 adds a concept of on-demand favicons into ThumbnailsDatabase that should get evicted based on last_requested timestamp. This CL adds the functionality to clear on-demand favicons that have not been used for long. BUG=709471 Review-Url: https://codereview.chromium.org/2903573002 Cr-Commit-Position: refs/heads/master@{#489307} Committed: https://chromium.googlesource.com/chromium/src/+/9946c3ce2b116a954f25a63a19be... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/9946c3ce2b116a954f25a63a19be... |
