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

Unified Diff: chrome/browser/safe_browsing/safe_browsing_database.cc

Issue 815863002: Suffixed histograms for SB2.DatabaseKilobytes and SB2.PrefixSetKilobytes (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@aV_threadSafeStoreManager
Patch Set: Created 6 years ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/safe_browsing/safe_browsing_database.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index 0f11795edb63e2dafcbbedf451ae1771a38d25a5..6359b7037d41b6b3d8a1acc6ad7aa9b09115bb74 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -1329,7 +1329,7 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
DownloadDBFilename(filename_base_),
download_store_.get(),
FAILURE_DOWNLOAD_DATABASE_UPDATE_FINISH);
- UMA_HISTOGRAM_COUNTS("SB2.DownloadDatabaseKilobytes",
+ UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes.Download",
static_cast<int>(size_bytes / 1024));
}
@@ -1352,7 +1352,7 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
ExtensionBlacklistDBFilename(filename_base_),
extension_blacklist_store_.get(),
FAILURE_EXTENSION_BLACKLIST_UPDATE_FINISH);
- UMA_HISTOGRAM_COUNTS("SB2.ExtensionBlacklistKilobytes",
+ UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes.ExtensionBlacklist",
static_cast<int>(size_bytes / 1024));
}
@@ -1397,6 +1397,10 @@ void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
return;
}
+ const int64 file_size = GetFileSizeOrZero(store_filename);
+ RecordSBWhitelistCountsHistogram("SB2.DatabaseKilobytes", whitelist_id,
+ static_cast<int>(file_size / 1024));
+
#if defined(OS_MACOSX)
base::mac::SetFileBackupExclusion(store_filename);
#endif
@@ -1484,15 +1488,7 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
UMA_HISTOGRAM_LONG_TIMES("SB2.BuildFilter", base::TimeTicks::Now() - before);
- {
- // Persist the prefix set to disk. Do not grab the lock to avoid contention
- // while writing to disk. This is safe as only this thread can ever modify
- // |state_manager_|'s prefix sets anyways.
- scoped_ptr<ReadTransaction> txn =
- state_manager_.BeginReadTransactionNoLockOnMainThread();
- WritePrefixSet(db_filename, txn->GetPrefixSet(prefix_set_id),
- write_failure_type);
- }
+ WritePrefixSet(db_filename, prefix_set_id, write_failure_type);
// Gather statistics.
if (got_counters && metric->GetIOCounters(&io_after)) {
@@ -1511,8 +1507,8 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
}
const int64 file_size = GetFileSizeOrZero(db_filename);
- UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes",
- static_cast<int>(file_size / 1024));
+ RecordPrefixSetCountsHistogram("SB2.DatabaseKilobytes", prefix_set_id,
+ static_cast<int>(file_size / 1024));
Scott Hess - ex-Googler 2014/12/18 23:57:55 I find the prefix-set references misleading, here.
gab 2014/12/19 15:25:54 Right but I was trying to avoid introducing yet an
#if defined(OS_MACOSX)
base::mac::SetFileBackupExclusion(db_filename);
@@ -1532,8 +1528,15 @@ void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
return;
}
+ const base::FilePath ip_blacklist_filename =
+ IpBlacklistDBFilename(filename_base_);
+
+ const int64 file_size = GetFileSizeOrZero(ip_blacklist_filename);
+ UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes.IPBlacklist",
+ static_cast<int>(file_size / 1024));
+
#if defined(OS_MACOSX)
- base::mac::SetFileBackupExclusion(IpBlacklistDBFilename(filename_base_));
+ base::mac::SetFileBackupExclusion(ip_blacklist_filename);
#endif
LoadIpBlacklist(full_hashes);
@@ -1676,10 +1679,17 @@ bool SafeBrowsingDatabaseNew::Delete() {
}
void SafeBrowsingDatabaseNew::WritePrefixSet(const base::FilePath& db_filename,
- const PrefixSet* prefix_set,
+ PrefixSetId prefix_set_id,
FailureType write_failure_type) {
DCHECK(thread_checker_.CalledOnValidThread());
+ // Do not grab the lock to avoid contention while writing to disk. This is
+ // safe as only this thread can ever modify |state_manager_|'s prefix sets
+ // anyways.
+ scoped_ptr<ReadTransaction> txn =
+ state_manager_.BeginReadTransactionNoLockOnMainThread();
+ const PrefixSet* prefix_set = txn->GetPrefixSet(prefix_set_id);
+
if (!prefix_set)
return;
@@ -1690,8 +1700,8 @@ void SafeBrowsingDatabaseNew::WritePrefixSet(const base::FilePath& db_filename,
UMA_HISTOGRAM_TIMES("SB2.PrefixSetWrite", base::TimeTicks::Now() - before);
const int64 file_size = GetFileSizeOrZero(prefix_set_filename);
- UMA_HISTOGRAM_COUNTS("SB2.PrefixSetKilobytes",
- static_cast<int>(file_size / 1024));
+ RecordPrefixSetCountsHistogram("SB2.PrefixSetKilobytes", prefix_set_id,
+ static_cast<int>(file_size / 1024));
if (!write_ok)
RecordFailure(write_failure_type);
@@ -1798,3 +1808,50 @@ SafeBrowsingDatabaseNew::GetUnsynchronizedPrefixGetHashCacheForTesting() {
scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
return txn->prefix_gethash_cache();
}
+
+void SafeBrowsingDatabaseNew::RecordPrefixSetCountsHistogram(
+ const std::string& histogram_name_base,
+ PrefixSetId prefix_set_id,
+ int sample) {
+ std::string histogram_name_full(histogram_name_base);
+ switch (prefix_set_id) {
+ case PrefixSetId::BROWSE:
+ histogram_name_full.append(".Browse");
+ break;
+ case PrefixSetId::SIDE_EFFECT_FREE_WHITELIST:
+ histogram_name_full.append(".SideEffectFreeWhitelist");
+ break;
+ case PrefixSetId::UNWANTED_SOFTWARE:
+ histogram_name_full.append(".UnwantedSoftware");
+ break;
+ }
+
+ // Histogram properties as in UMA_HISTOGRAM_COUNTS macro.
+ base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet(
+ histogram_name_full, 1, 1000000, 50,
+ base::HistogramBase::kUmaTargetedHistogramFlag);
+
+ histogram_pointer->Add(sample);
+}
+
+void SafeBrowsingDatabaseNew::RecordSBWhitelistCountsHistogram(
+ const std::string& histogram_name_base,
+ SBWhitelistId whitelist_id,
+ int sample) {
+ std::string histogram_name_full(histogram_name_base);
+ switch (whitelist_id) {
+ case SBWhitelistId::CSD:
+ histogram_name_full.append(".CSD");
+ break;
+ case SBWhitelistId::DOWNLOAD:
+ histogram_name_full.append(".DownloadWhitelist");
+ break;
+ }
+
+ // Histogram properties as in UMA_HISTOGRAM_COUNTS macro.
+ base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet(
+ histogram_name_full, 1, 1000000, 50,
+ base::HistogramBase::kUmaTargetedHistogramFlag);
+
+ histogram_pointer->Add(sample);
+}
Scott Hess - ex-Googler 2014/12/18 23:57:55 I think these might usefully use a helper on the o
gab 2014/12/19 15:25:53 It felt to me like that avoiding the readability o

Powered by Google App Engine
This is Rietveld 408576698