Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12047042:
Add history-view-related UMA metrics and histograms for locally managed users. (Closed)
|
Created:
7 years, 11 months ago by Pam (message me for reviews) Modified:
7 years, 11 months ago CC:
chromium-reviews, browser-components-watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd history-view-related UMA metrics and histograms for locally managed users.
BUG=171382
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178971
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
Total comments: 2
Messages
Total messages: 14 (0 generated)
Please review, thanks! These data will be helpful in designing some changes to the history page. - Pam
https://codereview.chromium.org/12047042/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_database.cc (right): https://codereview.chromium.org/12047042/diff/1/chrome/browser/history/histor... chrome/browser/history/history_database.cc:76: base::TimeTicks::Now() - start_time); Overall looks good. Just a couple questions. First, the file actually will have 30 days worth of data? I never remember the relationships between History, Archived History, and History Index. Second, is this scan expected to be large for some users? You could either revise the scan to calculate both values in one pass, something like: SELECT sum(visit_time > ?), count(*) FROM visits WHERE visit_time > ? where the first parameters is one_week_ago and the second is one_month_ago, or (perhaps more easily) you could just have the second query only scan one_month_ago -> one_week_ago, then add the results. CPU-wise I expect they'll be equivalent. Hmm, poking around in my profiles, I'd say that the visit_time index is probably small enough that the index would fit in the (rather large) cache. In which case just do what's easiest. If you decide to stay with the current queries, then re-use the statement by calling Reset() then binding the new value, rather than having distinct-but-identical statements.
On 2013/01/23 22:15:37, shess wrote: > https://codereview.chromium.org/12047042/diff/1/chrome/browser/history/histor... > File chrome/browser/history/history_database.cc (right): > > https://codereview.chromium.org/12047042/diff/1/chrome/browser/history/histor... > chrome/browser/history/history_database.cc:76: base::TimeTicks::Now() - > start_time); > Overall looks good. Just a couple questions. > > First, the file actually will have 30 days worth of data? I never remember the > relationships between History, Archived History, and History Index. 90 days' worth, I believe. > Second, is this scan expected to be large for some users? You could either > revise the scan to calculate both values in one pass, something like: > SELECT sum(visit_time > ?), count(*) FROM visits WHERE visit_time > ? > where the first parameters is one_week_ago and the second is one_month_ago, or > (perhaps more easily) you could just have the second query only scan > one_month_ago -> one_week_ago, then add the results. CPU-wise I expect they'll > be equivalent. > > Hmm, poking around in my profiles, I'd say that the visit_time index is probably > small enough that the index would fit in the (rather large) cache. In which > case just do what's easiest. If you decide to stay with the current queries, > then re-use the statement by calling Reset() then binding the new value, rather > than having distinct-but-identical statements. There are always outliers. My database only has about 20k entries, but I'm sure there are people who spend a lot less time per visit than I do. But I've changed it to scan once for a week and once for the other three weeks; thanks for the idea. I've also now added several more detailed statistics, pulled over from another CL. Please take another look. Thanks, - Pam
https://codereview.chromium.org/12047042/diff/1004/chrome/browser/history/his... File chrome/browser/history/history_database.cc (right): https://codereview.chromium.org/12047042/diff/1004/chrome/browser/history/his... chrome/browser/history/history_database.cc:85: "ORDER BY last_visit_time DESC")); I don't see an index on last_visit_time, so it seems likely that this is going to request SQLite to scan everything into buffers then do a sort. Since you only want the sort to be able to detect the weekly threshold, and you're going to scan everything anyhow, I think modifying the code below to track a local weekly_url_count and weekly_hosts would probably be more efficient in CPU and memory. https://codereview.chromium.org/12047042/diff/1004/chrome/browser/history/his... chrome/browser/history/history_database.cc:95: if (!saved_weekly_counts && visit_time < one_week_ago) { Pedantic Man wants visit_time <= one_week_ago to match the sense of the earlier partitioning.
Pllease take another look. https://codereview.chromium.org/12047042/diff/1004/chrome/browser/history/his... File chrome/browser/history/history_database.cc (right): https://codereview.chromium.org/12047042/diff/1004/chrome/browser/history/his... chrome/browser/history/history_database.cc:85: "ORDER BY last_visit_time DESC")); On 2013/01/24 17:30:52, shess wrote: > I don't see an index on last_visit_time, so it seems likely that this is going > to request SQLite to scan everything into buffers then do a sort. Since you > only want the sort to be able to detect the weekly threshold, and you're going > to scan everything anyhow, I think modifying the code below to track a local > weekly_url_count and weekly_hosts would probably be more efficient in CPU and > memory. Makes sense. Thanks for knowing way more about the SQLite internals than I do! https://codereview.chromium.org/12047042/diff/1004/chrome/browser/history/his... chrome/browser/history/history_database.cc:95: if (!saved_weekly_counts && visit_time < one_week_ago) { On 2013/01/24 17:30:52, shess wrote: > Pedantic Man wants visit_time <= one_week_ago to match the sense of the earlier > partitioning. Now moot. The new > here matches the > for visits.
lgtm! https://codereview.chromium.org/12047042/diff/1004/chrome/browser/history/his... File chrome/browser/history/history_database.cc (right): https://codereview.chromium.org/12047042/diff/1004/chrome/browser/history/his... chrome/browser/history/history_database.cc:85: "ORDER BY last_visit_time DESC")); On 2013/01/25 08:07:48, Pam wrote: > On 2013/01/24 17:30:52, shess wrote: > > I don't see an index on last_visit_time, so it seems likely that this is going > > to request SQLite to scan everything into buffers then do a sort. Since you > > only want the sort to be able to detect the weekly threshold, and you're going > > to scan everything anyhow, I think modifying the code below to track a local > > weekly_url_count and weekly_hosts would probably be more efficient in CPU and > > memory. > > Makes sense. Thanks for knowing way more about the SQLite internals than I do! Unlike a server db like MYSQL, asking SQLite to do prep work on your behalf just shifts it from here to there in-process - unless there's an index or something which allows it to efficiently manage things.
Scott, owners approval please. Thanks, - Pam
https://codereview.chromium.org/12047042/diff/12003/chrome/browser/history/hi... File chrome/browser/history/history_database.cc (right): https://codereview.chromium.org/12047042/diff/12003/chrome/browser/history/hi... chrome/browser/history/history_database.cc:81: if (base::RandInt(1, 3) == 3) { This seems rather expensive. Are we sure we need to do this? If we do, it would be better to change visits to have a host so that we can do a select statement for this rather than iterating through all visits.
https://codereview.chromium.org/12047042/diff/12003/chrome/browser/history/hi... File chrome/browser/history/history_database.cc (right): https://codereview.chromium.org/12047042/diff/12003/chrome/browser/history/hi... chrome/browser/history/history_database.cc:81: if (base::RandInt(1, 3) == 3) { On 2013/01/25 16:33:45, sky wrote: > This seems rather expensive. Are we sure we need to do this? If we do, it would > be better to change visits to have a host so that we can do a select statement > for this rather than iterating through all visits. It's not quite so bad as that, since we're iterating URLs rather than visits; my own URL table is half the size of my visit table, at about 10k entries total. It's not *absolutely* necessary, of course, but it will be valuable in designing some aspects of the new history page. But I don't have a good feel for the processes here. Just how slow do you expect it to be?
I guess land it and watch the times on canary. If it looks long, remove it. LGTM
On 2013/01/25 16:50:05, sky wrote: > I guess land it and watch the times on canary. If it looks long, remove it. LGTM Thanks, will do. What's blocked by this? History availability, I assume, but not startup itself?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pam@chromium.org/12047042/12003
That's right, it shouldn't block startup, other than more load happening when everything is trying to come up. It likely effects loading of the InMemoryDB, but I don't think that's used by the omnibox anymore.
Message was sent while issue was closed.
Change committed as 178971 |
