Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1032763002:
Add favicon database histograms to UMA. (Closed)
|
Created:
5 years, 9 months ago by Roger McFarlane (Chromium) Modified:
5 years, 8 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, beaudoin, huangs, brettw Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd favicon database histograms to UMA.
This CL adds UMA histograms to track the follwing properties of the
Favicon database.
- database size
- total number of favicon files/urls tracked by the database
- number of large (touch) favicon files/urls tracked by the database
(this is a subset of the above)
- total number of favicon bitmap assets cached in the database
- number of large (touch) favicon bitmap assets cached in the database
(this is a subset of the above)
- the number of URL mappings (generic url to icon url) in the database.
The goal of this CL is to provide insight into the size of this database
and the volume of data cached within it, particularly on mobile, which
aggressively caches large/touch icons.
R=sky@chromium.org, jwd@chromium.org
BUG=467712
Committed: https://crrev.com/5520a4f099d29301612bc0e660309ceece929a8a
Cr-Commit-Position: refs/heads/master@{#323647}
Patch Set 1 #Patch Set 2 : rebased #
Total comments: 8
Patch Set 3 : simplify metric queries + add a historgram for number of url mappings being tracked #
Total comments: 2
Patch Set 4 : fixed a code layout nit #
Total comments: 16
Patch Set 5 : Apply review feedback. #
Total comments: 8
Patch Set 6 : improve code and histogram comments #
Total comments: 2
Patch Set 7 : Added description of when metrics are logged. #Patch Set 8 : Rebase + fix owner email #
Messages
Total messages: 31 (10 generated)
This CL adds some missing favicon database metrics. PTAL?
https://codereview.chromium.org/1032763002/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:628: UMA_HISTOGRAM_MEMORY_MB("History.FaviconDatabaseSizeMB", file_mb); KB? https://codereview.chromium.org/1032763002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1032763002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11280: +<histogram name="History.FaviconDatabaseAdvancedMetricsTime" units="ticks"> To my understanding, ticks doesn't seem like a particular portable unit across samples? This should really be ms, or similar? The same for History.DatabaseAdvancedMetricsTime (which this metric copies)? https://codereview.chromium.org/1032763002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11287: +<histogram name="History.FaviconDatabaseSizeMB" units="MB"> I did this in MB to be consistent with the History.DatabaseSizeMB metric; however, looking at the actual histograms, perhaps both should be in KB instead? https://codereview.chromium.org/1032763002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11334: +<histogram name="History.NumFaviconBitmapsInDB"> This is a pre-existing metric which wasn't declared in histograms.xml
beaudoin@chromium.org changed reviewers: + beaudoin@chromium.org
My worry here is doing that many SQL queries at init time to gather stats. I'm not sure it's a great pattern. Deferring to sky for comments on that. (Note that collecting these stats was strongly encouraged by brettw on the chrome-eng-review thread.) https://codereview.chromium.org/1032763002/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:628: UMA_HISTOGRAM_MEMORY_MB("History.FaviconDatabaseSizeMB", file_mb); On 2015/03/25 18:20:18, Roger McFarlane (Chromium) wrote: > KB? +1, MB has an overly large first bin. https://codereview.chromium.org/1032763002/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:655: "SELECT COUNT(*) FROM favicons WHERE icon_type > ?")); I think I'd prefer something like "width > 64" or "width > 128" (whichever is our current cutoff) since we may eventually decide to keep large favicons using favicon_base::FAVICON.
On 2015/03/25 18:31:49, beaudoin wrote: > My worry here is doing that many SQL queries at init time to gather stats. I'm > not sure it's a great pattern. Deferring to sky for comments on that. In deference to this, the metrics are only gathered with random probability of 1% for a given initialization. See: https://code.google.com/p/chromium/codesearch#chromium/src/components/history...
brettw@chromium.org changed reviewers: + brettw@chromium.org
I think the performance should be fine. Except for the nested select I mentioned below, the queries are all really simple and should operate on data that's already loaded anyway, and it only happens 1% of the time as previously mentioned. https://codereview.chromium.org/1032763002/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:655: "SELECT COUNT(*) FROM favicons WHERE icon_type > ?")); Also, I think this allows you to delete the large bitmap check below. The nested select will have much worse performance, so we should only do that if it's really necessary, and it doesn't seem so.
thanks. Another look? jwd@... any comment/guidance on the histogram choices (MB vs KB, for example)? https://codereview.chromium.org/1032763002/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:655: "SELECT COUNT(*) FROM favicons WHERE icon_type > ?")); On 2015/03/25 19:38:53, brettw wrote: > Also, I think this allows you to delete the large bitmap check below. The nested > select will have much worse performance, so we should only do that if it's > really necessary, and it doesn't seem so. The schema doesn't quite allow for this, but perhaps simply counting the large bitmaps is sufficient. I.e., we can't directly (without join) count the number of icons urls which map to stored large icon bitmaps. Icons (more precicely icon urls) are stored with a type, but only bitmaps are stored with size (since a given icon file can map to multiple bitmaps). This histogram counts the number of candidate urls at which we expect to find large icons.
LGTM with 1 nit. https://codereview.chromium.org/1032763002/diff/40001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:681: "SELECT COUNT(*) FROM icon_mapping")); Nit: don't wrap.
https://codereview.chromium.org/1032763002/diff/40001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/40001/components/history/core... components/history/core/browser/thumbnail_database.cc:681: "SELECT COUNT(*) FROM icon_mapping")); On 2015/03/26 19:36:28, beaudoin wrote: > Nit: don't wrap. Done.
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
Just nits https://codereview.chromium.org/1032763002/diff/60001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/60001/components/history/core... components/history/core/browser/thumbnail_database.cc:652: // which we usually store only small (16x16 or 32x32) bitmaps by default. Can you rename the UMA metric to be "History.NumTouchIconsInDB". This is representative of what the query does currently. If we end up adding a new icon type, it would be probably useful to a new histogram to count the number of icons of that type https://codereview.chromium.org/1032763002/diff/60001/components/history/core... components/history/core/browser/thumbnail_database.cc:681: UMA_HISTOGRAM_COUNTS_10000( I checked the "icon_mapping" table for the instance of Chrome that I run locally and I have 16501 icon mappings (You should probably use UMA_HISTOGRAM_CUSTOM_COUNTS) https://codereview.chromium.org/1032763002/diff/60001/components/history/core... components/history/core/browser/thumbnail_database.cc:682: "History.NumFaviconsMappingsInDB", Nit: History.NumFaviconsMappingsInDB -> History.NumFaviconMappingsInDB https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11291: + The wallclock time taken to gather favicon database metrics. Nit: "wallclock" -> "wall clock" https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11300: + next losest MB). So, it does not quite represent the amount of actual data Nit: I think you can remove "(floored to next losest MB)". I think that the rounding is an implementation detail. https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11347: + assets at different resolutions/sizes. How about: "The total number of favicon bitmaps (of all sizes) cached in a user's Favicon database. A given favicon URL may be associated with multiple bitmaps (of different sizes)" https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11357: + Hisory.NumFaviconBitmapsInDB for that. How about: "The total number of favicon URLs (e.g. http://www.google.com/favicon.ico) tracked in a user's Favicon database. This metric tracks knowledge of a favicon URL not whether there are cached bitmaps for that favicon URL. See History.NumFaviconBitmapsInDB for that." Aside: We currently should not have an entry in the "favicons" table without any associated entries in the "favicon_bitmaps" table https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11368: + count. Can you please update the description to match the SQL query? On Android, non-touch large favicons (of type FAVICON) are stored to the Favicon database
Patchset #5 (id:80001) has been deleted
Thanks. Another look? https://codereview.chromium.org/1032763002/diff/60001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/60001/components/history/core... components/history/core/browser/thumbnail_database.cc:652: // which we usually store only small (16x16 or 32x32) bitmaps by default. On 2015/03/27 15:45:25, pkotwicz wrote: > Can you rename the UMA metric to be "History.NumTouchIconsInDB". This is > representative of what the query does currently. If we end up adding a new icon > type, it would be probably useful to a new histogram to count the number of > icons of that type Done. https://codereview.chromium.org/1032763002/diff/60001/components/history/core... components/history/core/browser/thumbnail_database.cc:681: UMA_HISTOGRAM_COUNTS_10000( On 2015/03/27 15:45:25, pkotwicz wrote: > I checked the "icon_mapping" table for the instance of Chrome that I run locally > and I have 16501 icon mappings (You should probably use > UMA_HISTOGRAM_CUSTOM_COUNTS) Done. https://codereview.chromium.org/1032763002/diff/60001/components/history/core... components/history/core/browser/thumbnail_database.cc:682: "History.NumFaviconsMappingsInDB", On 2015/03/27 15:45:25, pkotwicz wrote: > Nit: History.NumFaviconsMappingsInDB -> History.NumFaviconMappingsInDB Done. https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11291: + The wallclock time taken to gather favicon database metrics. On 2015/03/27 15:45:25, pkotwicz wrote: > Nit: "wallclock" -> "wall clock" Done. More precisely, "wall-clock"... which is consistently used throughout this file. https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11300: + next losest MB). So, it does not quite represent the amount of actual data On 2015/03/27 15:45:25, pkotwicz wrote: > Nit: I think you can remove "(floored to next losest MB)". I think that the > rounding is an implementation detail. Done. https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11347: + assets at different resolutions/sizes. On 2015/03/27 15:45:25, pkotwicz wrote: > How about: > "The total number of favicon bitmaps (of all sizes) cached in a user's Favicon > database. A given favicon URL may be associated with multiple bitmaps (of > different sizes)" Done. https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11357: + Hisory.NumFaviconBitmapsInDB for that. On 2015/03/27 15:45:25, pkotwicz wrote: > How about: > "The total number of favicon URLs (e.g. http://www.google.com/favicon.ico) > tracked in a user's Favicon database. This metric tracks knowledge of a favicon > URL not whether there are cached bitmaps for that favicon URL. See > History.NumFaviconBitmapsInDB for that." > > Aside: We currently should not have an entry in the "favicons" table without any > associated entries in the "favicon_bitmaps" table Done. https://codereview.chromium.org/1032763002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11368: + count. On 2015/03/27 15:45:25, pkotwicz wrote: > Can you please update the description to match the SQL query? > On Android, non-touch large favicons (of type FAVICON) are stored to the Favicon > database Updated this to just track the number of large (by pixel size) icons in the dababase.
LGTM with nits https://codereview.chromium.org/1032763002/diff/100001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/1032763002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database.cc:650: // Count the subset of "touch" icon URLs referenced by the DB. How about: "Count "touch" icon URLs referenced in the DB." (You want to count the subset of icon URLs which are "touch" icon URLs) https://codereview.chromium.org/1032763002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database.cc:655: "SELECT COUNT(*) FROM favicons WHERE icon_type in (?, ?)")); Nit: 'in' -> 'IN' https://codereview.chromium.org/1032763002/diff/100001/components/history/cor... components/history/core/browser/thumbnail_database.cc:663: // Count the subset of "large" bitmap resources cached in the DB. How about: "Count "large" bitmap resources cached in the DB." https://codereview.chromium.org/1032763002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1032763002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:11352: + The number of URL to favicon mappings stored in a user's Favicon database. How about: "The number of page URL (e.g. http://www.google.com) to favicon URL (e.g. http://www.google.com/favicon.ico) mappings stored in a user's Favicon database.
Patchset #6 (id:120001) has been deleted
Thanks. jwd@: can I get an OWNERS stamp of approval for histograms.xml? sky@: can I get an OWNERS stamp of approval for thumbnail_database.cc? https://chromiumcodereview.appspot.com/1032763002/diff/100001/components/hist... File components/history/core/browser/thumbnail_database.cc (right): https://chromiumcodereview.appspot.com/1032763002/diff/100001/components/hist... components/history/core/browser/thumbnail_database.cc:650: // Count the subset of "touch" icon URLs referenced by the DB. On 2015/03/30 19:25:19, pkotwicz wrote: > How about: "Count "touch" icon URLs referenced in the DB." > > (You want to count the subset of icon URLs which are "touch" icon URLs) Done. https://chromiumcodereview.appspot.com/1032763002/diff/100001/components/hist... components/history/core/browser/thumbnail_database.cc:655: "SELECT COUNT(*) FROM favicons WHERE icon_type in (?, ?)")); On 2015/03/30 19:25:19, pkotwicz wrote: > Nit: 'in' -> 'IN' Done. https://chromiumcodereview.appspot.com/1032763002/diff/100001/components/hist... components/history/core/browser/thumbnail_database.cc:663: // Count the subset of "large" bitmap resources cached in the DB. On 2015/03/30 19:25:19, pkotwicz wrote: > How about: "Count "large" bitmap resources cached in the DB." Done. https://chromiumcodereview.appspot.com/1032763002/diff/100001/tools/metrics/h... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1032763002/diff/100001/tools/metrics/h... tools/metrics/histograms/histograms.xml:11352: + The number of URL to favicon mappings stored in a user's Favicon database. On 2015/03/30 19:25:19, pkotwicz wrote: > How about: "The number of page URL (e.g. http://www.google.com) to favicon URL > (e.g. http://www.google.com/favicon.ico) mappings stored in a user's Favicon > database. Done.
https://codereview.chromium.org/1032763002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1032763002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:11291: + The wall-clock time taken to gather favicon database metrics. Can you add a description of when these get logged.
Thanks. https://codereview.chromium.org/1032763002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1032763002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:11291: + The wall-clock time taken to gather favicon database metrics. On 2015/03/31 15:09:41, Jesse Doherty wrote: > Can you add a description of when these get logged. Done.
lgtm
LGTM
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1032763002/#ps160001 (title: "Added description of when metrics are logged.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1032763002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from beaudoin@chromium.org, pkotwicz@chromium.org, jwd@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1032763002/#ps180001 (title: "Rebase + fix owner email")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1032763002/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5520a4f099d29301612bc0e660309ceece929a8a Cr-Commit-Position: refs/heads/master@{#323647} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
