Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(176)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2441923003: PVer4: Add UMA metrics for time taken to: load DB, do prefix check, (Closed)

Created:
4 years, 2 months ago by vakh (use Gerrit instead)
Modified:
4 years, 1 month ago
CC:
chromium-reviews, vakh+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

PVer4: Add UMA metrics for time taken to: load DB, do prefix check, and network time to do full hash check SB2.DatabaseOpen: The time it takes to read, parse, and load the database from disk into memory. SB2.FilterCheck: The time it takes to check the prefix hashes for a match. I tested a known phishing URL. SB2.Network: The time it takes to get the full hash response from the server and parse it. This is anecdotal, but I ran the two DB managers in parallel mode 5 times (5 different launches of Chrome) and got the following results: + SB2.DatabaseOpen is consistently faster for PVer4 by 59% (0.0919344s vs 0.0375474s). The PVer4 version is called: SafeBrowsing.V4DatabaseOpen.Time + SB2.FilterCheck is consistently faster for PVer4 by 30% (0.0093838s vs 0.00659247s). The PVer4 version is called: SafeBrowsing.V4GetPrefixMatches.Time + SB2.Network is consistently *slower* for PVer4 by 4.94% (0.2113358s vs 0.221783s). -- This slowness needs to be investigated but is somewhat acceptable since this is only used for 1/1000 hash checks. The PVer4 version is called: SafeBrowsing.V4GetHashNetwork.Time Adding these metrics would allow us to do a more reasonable comparison. BUG=543161 Committed: https://crrev.com/9ba50173ad377dcb9dadeb6dc1389929e23f65aa Cr-Commit-Position: refs/heads/master@{#426985}

Patch Set 1 #

Patch Set 2 : git rebase #

Total comments: 10

Patch Set 3 : nparker@'s review #

Patch Set 4 : Add new histograms to histograms.xml #

Total comments: 9

Patch Set 5 : nparker@'s review #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -14 lines) Patch
M components/safe_browsing_db/v4_database.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M components/safe_browsing_db/v4_database.cc View 1 2 4 chunks +10 lines, -4 lines 0 comments Download
M components/safe_browsing_db/v4_get_hash_protocol_manager.h View 1 2 3 4 2 chunks +6 lines, -1 line 0 comments Download
M components/safe_browsing_db/v4_get_hash_protocol_manager.cc View 1 2 3 4 3 chunks +6 lines, -2 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager.h View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager.cc View 1 2 3 4 3 chunks +17 lines, -6 lines 1 comment Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 2 chunks +24 lines, -0 lines 0 comments Download

Messages

Total messages: 40 (26 generated)
vakh (use Gerrit instead)
git rebase
4 years, 2 months ago (2016-10-21 01:50:12 UTC) #3
Nathan Parker
https://codereview.chromium.org/2441923003/diff/20001/components/safe_browsing_db/v4_database.cc File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2441923003/diff/20001/components/safe_browsing_db/v4_database.cc#newcode81 components/safe_browsing_db/v4_database.cc:81: UMA_HISTOGRAM_TIMES("SB2.DatabaseOpen", TimeTicks::Now() - before); note: This time includes the ...
4 years, 2 months ago (2016-10-21 20:21:19 UTC) #12
vakh (use Gerrit instead)
Thanks for taking a look. PTAL. https://codereview.chromium.org/2441923003/diff/20001/components/safe_browsing_db/v4_database.cc File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2441923003/diff/20001/components/safe_browsing_db/v4_database.cc#newcode81 components/safe_browsing_db/v4_database.cc:81: UMA_HISTOGRAM_TIMES("SB2.DatabaseOpen", TimeTicks::Now() - ...
4 years, 2 months ago (2016-10-21 22:54:05 UTC) #13
vakh (use Gerrit instead)
nparker@'s review
4 years, 2 months ago (2016-10-21 22:54:05 UTC) #14
vakh (use Gerrit instead)
Add new histograms to histograms.xml
4 years, 2 months ago (2016-10-21 22:57:49 UTC) #17
vakh (use Gerrit instead)
holte@ -- can you please review the changes in tools/metrics/histograms/histograms.xml Thanks!
4 years, 2 months ago (2016-10-21 23:00:19 UTC) #23
Steven Holte
histograms lgtm
4 years, 2 months ago (2016-10-21 23:04:28 UTC) #24
Nathan Parker
lgtm https://codereview.chromium.org/2441923003/diff/60001/components/safe_browsing_db/v4_database.h File components/safe_browsing_db/v4_database.h (right): https://codereview.chromium.org/2441923003/diff/60001/components/safe_browsing_db/v4_database.h#newcode168 components/safe_browsing_db/v4_database.h:168: const base::TimeTicks before); create_start_time, like the .cc https://codereview.chromium.org/2441923003/diff/60001/components/safe_browsing_db/v4_local_database_manager.cc ...
4 years, 2 months ago (2016-10-22 00:24:56 UTC) #25
vakh (use Gerrit instead)
nparker@'s review
4 years, 2 months ago (2016-10-22 01:33:11 UTC) #26
vakh (use Gerrit instead)
Thanks for the review nparker@ and holte@ shess@ -- I will submit this CL soon ...
4 years, 2 months ago (2016-10-22 01:34:34 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2441923003/80001
4 years, 2 months ago (2016-10-22 06:23:28 UTC) #35
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 2 months ago (2016-10-22 06:34:54 UTC) #37
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/9ba50173ad377dcb9dadeb6dc1389929e23f65aa Cr-Commit-Position: refs/heads/master@{#426985}
4 years, 2 months ago (2016-10-22 06:43:28 UTC) #39
Scott Hess - ex-Googler
4 years, 1 month ago (2016-10-24 19:24:22 UTC) #40
Message was sent while issue was closed. 
Treat these as peanut-gallery comments, I don't think they necessarily require
action.

https://codereview.chromium.org/2441923003/diff/60001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2441923003/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:368:
base::TimeDelta::FromMicroseconds(100),
On 2016/10/22 01:34:34, vakh (Varun Khaneja) wrote:
> On 2016/10/22 00:24:55, Nathan Parker wrote:
> > Does this generate a log or linear scale of buckets?  Linear would be bad
> since
> > it'd be (20.1/50) = 402ms buckets, which doesn't help much. I _think_ it'll
do
> > log-spaced buckets, which should be fine.
> > 
> > I'd put the max at 1 second, since anything above that for local memory
access
> > is really broken and therefore uninteresting. And then the minimum could
move
> > lower, say to 20us. If you want to tune it more carefully, you could try it
on
> a
> > release build, load up http://cnn.com, and verify the histogram has good
> resolution
> > around the common values.
> > 
> > We should note that this doesn't distinguish which DBs it's searching
through.
> 
> > The vast majority of the entries in this histogram will be from searching
the
> > three BrowseURL DBs.(I don't think it's really worth logging them
separately.)
> 
> Done. The comments for UMA_HISTOGRAM_CUSTOM_TIMES suggest log scale.
> UMA_HISTOGRAM_TIMES uses UMA_HISTOGRAM_CUSTOM_TIMES so it should be consistent
> with the other histograms that we have.

Log scale unless you're doing something special.

Given log scale, IMHO be conservative about how tight you make things.  The
difference between 1s and 20s is only going to add like 5 buckets.  So if 1s is
literally "I don't even", then fine.  I usually aim to have very few hits in the
overflow bucket, simply because the improved precision is often not all that
valuable, but having to respin the entire histogram entry is annoying :-).

https://codereview.chromium.org/2441923003/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2441923003/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:360: NOTREACHED() <<
"Unexpected client_callback_type encountered";
Is this case guaranteed to be nominal to nonexistent?  Otherwise you might end
up with a lot of not-interesting min-bucket values.

Powered by Google App Engine
This is Rietveld 408576698