Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2410153008:
Add remaining implementations to PreviewsOptOutStoreSQL (Closed)
DescriptionAdd remaining implementations to PreviewsOptOutStoreSQL
This adds an implementation for AddPreviewNavigation and ClearBlackList
to PreviewsOptOutStoreSQL.
AddPreviewNavigation adds a new row to the database with the passed in
fields. It then prunes the DB to at most 32 rows per host and 3200
rows total in the DB.
ClearBlackList removes all entries from the DB between two passed in
times.
BUG=653730
Committed: https://crrev.com/fd52f6bb42533bc43ea7095cba6aacf78c159572
Cr-Commit-Position: refs/heads/master@{#425821}
Patch Set 1 #Patch Set 2 : changing comments #
Total comments: 14
Patch Set 3 : tbansal comments #
Total comments: 9
Patch Set 4 : shess comments #
Total comments: 2
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by ryansturm@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.
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + shess@chromium.org, tbansal@chromium.org
shess, tbansal: PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:33: const int kMaxRowsPerHost = 32; FWIW, 32 entries per host seem like a lot. Is there a reason to set it to 32? Also, is it going to be controlled by field trial? https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:37: const int kMaxEntriesInDB = 3200; May be call it kMaxRowsInDB to match the name above? https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:127: // Removes entires for |host_name| the per-host row limit is exceeded. s/entires/entries/ s/|host_name| the/|host_name| if the/ https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:133: const char kSqlDeleteByHost[] = "DELETE FROM " PREVIEWS_TABLE_NAME Are there plans to add scoped trackers or some other UMA to measure how long these calls take? Not sure if that's worth it though. https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:138: " LIMIT -1 OFFSET ?)"; What does LIMIT -1 do? https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:209: // Deletes every row in the table that has entry time between |begin_time| and nit: Move the comment above the method?
The CQ bit was checked by ryansturm@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...
https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:138: " LIMIT -1 OFFSET ?)"; On 2016/10/12 22:35:25, tbansal1 wrote: > What does LIMIT -1 do? Yeah, I think -1 here is non-obvious enough to warrant a specific mention. https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:151: " LIMIT -1 OFFSET ?)"; I didn't understand that this was going to be called on every update. This has to be a full table scan, so that's perhaps not appropriate. The insert and the per-host delete are well-matched in terms of what they need to hit. Would it be reasonable to move this to load time? Like the load-time code could count the inputs as they go by, and call this if there are "too many"? https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:210: scoped_refptr<base::SingleThreadTaskRunner> runner) { What is |runner| for? https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:212: "DELETE FROM " PREVIEWS_TABLE_NAME " WHERE time > ? and time < ?"; Just wanted to double-check whether these should be exclusive or inclusive of begin_time and end_time. Like c-style would be inclusive of begin_time. It's not obvious where these times are coming from, in the end, so I Wasn't able to immediately answer for myself. https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:226: MaybeEvictEntryFromDataBase(db, host_name); SQLite automatically tucks standalone updates into a transaction, so this causes three serial transactions. Wrap the entire thing in an explicit transaction for performance reasons. In the ClearBlackList case, you have a single statement in a single transaction, so that's going to be fine. Pedantically, MaybeEvictEntry should have a nested transaction just in case someone decides to start using it from elsewhere, but IMHO that's a style issue I'll leave to you.
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 ryansturm@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...
shess: PTAL https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:33: const int kMaxRowsPerHost = 32; On 2016/10/12 22:35:25, tbansal1 wrote: > FWIW, 32 entries per host seem like a lot. Is there a reason to set it to 32? > > Also, is it going to be controlled by field trial? 32 is an upper bound for previews::params::MaxStoredHistoryLengthForBlackList(). This isn't controlled by field trial as the DB outlives the params. Bengr and I discussed 32 as teh number, but I'll talk with him again about it. https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:37: const int kMaxEntriesInDB = 3200; On 2016/10/12 22:35:25, tbansal1 wrote: > May be call it kMaxRowsInDB to match the name above? Done. https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:127: // Removes entires for |host_name| the per-host row limit is exceeded. On 2016/10/12 22:35:25, tbansal1 wrote: > s/entires/entries/ > s/|host_name| the/|host_name| if the/ Done. https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:133: const char kSqlDeleteByHost[] = "DELETE FROM " PREVIEWS_TABLE_NAME On 2016/10/12 22:35:25, tbansal1 wrote: > Are there plans to add scoped trackers or some other UMA to measure how long > these calls take? Not sure if that's worth it though. I wasn't planning on it, and it seems like it will give pretty inconsistent results based on the current size of the DB for the user. It might be worth it for looking at worst case, but I don't think this is a major concern for us. https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:138: " LIMIT -1 OFFSET ?)"; On 2016/10/12 22:35:25, tbansal1 wrote: > What does LIMIT -1 do? Limit x offset y, means to look at the next x rows after the first y rows for the query. Limit -1 means Offset y means look at every row that is not in the first y rows. For this query, it means delete all but the first 32 rows for the matching host_name when ordering by time descending. https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:209: // Deletes every row in the table that has entry time between |begin_time| and On 2016/10/12 22:35:25, tbansal1 wrote: > nit: Move the comment above the method? Done. https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:151: " LIMIT -1 OFFSET ?)"; On 2016/10/12 23:44:35, Scott Hess wrote: > I didn't understand that this was going to be called on every update. This has > to be a full table scan, so that's perhaps not appropriate. The insert and the > per-host delete are well-matched in terms of what they need to hit. > > Would it be reasonable to move this to load time? Like the load-time code could > count the inputs as they go by, and call this if there are "too many"? I think load time should be fine. I couldn't find much information on profiles that never get restarted (so the DB keeps growing), but that was my only concern with putting it here vs loading. I think due to the organic restriction of number of hosts visited, 32 entries per host, and chrome upgrades forcing a restart, there shouldn't be much to worry about, but I will add UMA to see how big that count gets. https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:210: scoped_refptr<base::SingleThreadTaskRunner> runner) { On 2016/10/12 23:44:35, Scott Hess wrote: > What is |runner| for? Good catch. Initial implementation built the loading portion of clearing the blacklist firectly into ClearBlackListSync. https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:212: "DELETE FROM " PREVIEWS_TABLE_NAME " WHERE time > ? and time < ?"; On 2016/10/12 23:44:35, Scott Hess wrote: > Just wanted to double-check whether these should be exclusive or inclusive of > begin_time and end_time. Like c-style would be inclusive of begin_time. It's > not obvious where these times are coming from, in the end, so I Wasn't able to > immediately answer for myself. Done. Ultimately called from: https://cs.chromium.org/chromium/src/chrome/browser/browsing_data/browsing_da... https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:226: MaybeEvictEntryFromDataBase(db, host_name); On 2016/10/12 23:44:35, Scott Hess wrote: > SQLite automatically tucks standalone updates into a transaction, so this causes > three serial transactions. Wrap the entire thing in an explicit transaction for > performance reasons. > > In the ClearBlackList case, you have a single statement in a single transaction, > so that's going to be fine. > > Pedantically, MaybeEvictEntry should have a nested transaction just in case > someone decides to start using it from elsewhere, but IMHO that's a style issue > I'll leave to you. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Your call on whether to retain the if() around the DELETE at load time. Both sides have arguments. https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:212: "DELETE FROM " PREVIEWS_TABLE_NAME " WHERE time > ? and time < ?"; On 2016/10/17 19:19:00, Ryan Sturm wrote: > On 2016/10/12 23:44:35, Scott Hess wrote: > > Just wanted to double-check whether these should be exclusive or inclusive of > > begin_time and end_time. Like c-style would be inclusive of begin_time. It's > > not obvious where these times are coming from, in the end, so I Wasn't able to > > immediately answer for myself. > > Done. Ultimately called from: > > https://cs.chromium.org/chromium/src/chrome/browser/browsing_data/browsing_da... :-). But it just passes the times from someone else. My guess is that it's turtles all the way down. Probably this is for the "past hour", "past day", etc cases, all using base::Time::Max() for end_time. In that case, inclusive seems prudent. https://codereview.chromium.org/2410153008/diff/60001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/60001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:180: // TODO(ryansturm): Add UMA to log |count|. crbug.com/656739 Also please be sure to test this. In thinking about this, I'm finding myself mixed. On the one hand, I'd prefer not to have multiple full-table scans like this, so counting and only running when necessary seems like a good idea. On the other hand, code which almost never runs sometimes is found to be broken. So I'd support either run-as-needed like this, or just running the query every time. If I understand things right, even when someone gets to this situation their data still most likely fit in the cache, so there shouldn't be a cache-thrashing issue.
Thanks! tbansal: PTAL https://codereview.chromium.org/2410153008/diff/60001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/60001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:180: // TODO(ryansturm): Add UMA to log |count|. crbug.com/656739 On 2016/10/17 23:18:09, Scott Hess wrote: > Also please be sure to test this. > > In thinking about this, I'm finding myself mixed. On the one hand, I'd prefer > not to have multiple full-table scans like this, so counting and only running > when necessary seems like a good idea. On the other hand, code which almost > never runs sometimes is found to be broken. > > So I'd support either run-as-needed like this, or just running the query every > time. If I understand things right, even when someone gets to this situation > their data still most likely fit in the cache, so there shouldn't be a > cache-thrashing issue. Acknowledged.
lgtm https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2410153008/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:138: " LIMIT -1 OFFSET ?)"; On 2016/10/17 19:19:00, Ryan Sturm wrote: > On 2016/10/12 22:35:25, tbansal1 wrote: > > What does LIMIT -1 do? > > Limit x offset y, means to look at the next x rows after the first y rows for > the query. Limit -1 means Offset y means look at every row that is not in the > first y rows. > > For this query, it means delete all but the first 32 rows for the matching > host_name when ordering by time descending. Talked offline. I did not know that LIMIT is compulsory when using OFFSET.
The CQ bit was checked by ryansturm@chromium.org
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add remaining implementations to PreviewsOptOutStoreSQL This adds an implementation for AddPreviewNavigation and ClearBlackList to PreviewsOptOutStoreSQL. AddPreviewNavigation adds a new row to the database with the passed in fields. It then prunes the DB to at most 32 rows per host and 3200 rows total in the DB. ClearBlackList removes all entries from the DB between two passed in times. BUG=653730 ========== to ========== Add remaining implementations to PreviewsOptOutStoreSQL This adds an implementation for AddPreviewNavigation and ClearBlackList to PreviewsOptOutStoreSQL. AddPreviewNavigation adds a new row to the database with the passed in fields. It then prunes the DB to at most 32 rows per host and 3200 rows total in the DB. ClearBlackList removes all entries from the DB between two passed in times. BUG=653730 Committed: https://crrev.com/fd52f6bb42533bc43ea7095cba6aacf78c159572 Cr-Commit-Position: refs/heads/master@{#425821} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fd52f6bb42533bc43ea7095cba6aacf78c159572 Cr-Commit-Position: refs/heads/master@{#425821} |
