From a96dcd5baf3ff6cd28dbf002cc22b6c794afa389 Mon Sep 17 00:00:00 2001 From: yiyaoliu Date: Fri, 17 Apr 2015 10:07:58 -0700 Subject: [PATCH] Only record the execution time during startup. Also record the delta size between cached and up-to-date top sites to a histogram. BUG=223430 Review URL: https://codereview.chromium.org/1005873011 Cr-Commit-Position: refs/heads/master@{#325656} --- chrome/browser/history/top_sites_impl.cc | 37 ++++++++++++++++++---- chrome/browser/history/top_sites_impl.h | 21 +++++++++++- chrome/browser/history/top_sites_impl_unittest.cc | 3 +- .../history/core/browser/top_sites_backend.cc | 17 ++++++---- .../history/core/browser/top_sites_backend.h | 15 +++++++-- tools/metrics/histograms/histograms.xml | 23 ++++++++++++++ 6 files changed, 98 insertions(+), 18 deletions(-) diff --git a/chrome/browser/history/top_sites_impl.cc b/chrome/browser/history/top_sites_impl.cc index 2cad9e33fbef..92032e9c7df2 100644 --- a/chrome/browser/history/top_sites_impl.cc +++ b/chrome/browser/history/top_sites_impl.cc @@ -96,6 +96,9 @@ static const int64 kMaxUpdateIntervalMinutes = 60; // artifacts for these small sized, highly detailed images. static const int kTopSitesImageQuality = 100; +// Initially, histogram is not recorded. +bool TopSitesImpl::histogram_recorded_ = false; + TopSitesImpl::TopSitesImpl(Profile* profile, const PrepopulatedPageList& prepopulated_pages) : backend_(NULL), @@ -625,7 +628,7 @@ bool TopSitesImpl::AddForcedURL(const GURL& url, const base::Time& time) { new_list.insert(mid, new_url); mid = new_list.begin() + num_forced; // Mid was invalidated. std::inplace_merge(new_list.begin(), mid, mid + 1, ForcedURLComparator); - SetTopSites(new_list); + SetTopSites(new_list, CALL_LOCATION_FROM_OTHER_PLACES); return true; } @@ -752,7 +755,8 @@ void TopSitesImpl::Observe(int type, } } -void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites) { +void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites, + const CallLocation location) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); MostVisitedURLList top_sites(new_top_sites); @@ -761,8 +765,26 @@ void TopSitesImpl::SetTopSites(const MostVisitedURLList& new_top_sites) { TopSitesDelta delta; DiffMostVisited(cache_->top_sites(), top_sites, &delta); + + TopSitesBackend::RecordHistogram record_or_not = + TopSitesBackend::RECORD_HISTOGRAM_NO; + + // Record the delta size into a histogram if this function is called from + // function OnGotMostVisitedThumbnails and no histogram value has been + // recorded before. + if (location == CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS && + !histogram_recorded_) { + size_t delta_size = + delta.deleted.size() + delta.added.size() + delta.moved.size(); + UMA_HISTOGRAM_COUNTS_100("History.FirstSetTopSitesDeltaSize", delta_size); + // Will be passed to TopSitesBackend to let it record the histogram too. + record_or_not = TopSitesBackend::RECORD_HISTOGRAM_YES; + // Change it to true so that the histogram will not be recorded any more. + histogram_recorded_ = true; + } + if (!delta.deleted.empty() || !delta.added.empty() || !delta.moved.empty()) { - backend_->UpdateTopSites(delta); + backend_->UpdateTopSites(delta, record_or_not); } last_num_urls_changed_ = delta.added.size() + delta.moved.size(); @@ -879,7 +901,8 @@ void TopSitesImpl::OnGotMostVisitedThumbnails( // Set the top sites directly in the cache so that SetTopSites diffs // correctly. cache_->SetTopSites(thumbnails->most_visited); - SetTopSites(thumbnails->most_visited); + SetTopSites(thumbnails->most_visited, + CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS); cache_->SetThumbnails(thumbnails->url_to_images_map); ResetThreadSafeImageCache(); @@ -894,7 +917,7 @@ void TopSitesImpl::OnGotMostVisitedThumbnails( void TopSitesImpl::OnTopSitesAvailableFromHistory( const MostVisitedURLList* pages) { DCHECK(pages); - SetTopSites(*pages); + SetTopSites(*pages, CALL_LOCATION_FROM_OTHER_PLACES); } void TopSitesImpl::OnURLsDeleted(HistoryService* history_service, @@ -906,7 +929,7 @@ void TopSitesImpl::OnURLsDeleted(HistoryService* history_service, return; if (all_history) { - SetTopSites(MostVisitedURLList()); + SetTopSites(MostVisitedURLList(), CALL_LOCATION_FROM_OTHER_PLACES); backend_->ResetDatabase(); } else { std::set indices_to_delete; // Indices into top_sites_. @@ -923,7 +946,7 @@ void TopSitesImpl::OnURLsDeleted(HistoryService* history_service, i != indices_to_delete.rend(); i++) { new_top_sites.erase(new_top_sites.begin() + *i); } - SetTopSites(new_top_sites); + SetTopSites(new_top_sites, CALL_LOCATION_FROM_OTHER_PLACES); } StartQueryForMostVisited(); } diff --git a/chrome/browser/history/top_sites_impl.h b/chrome/browser/history/top_sites_impl.h index 91518ec742b6..cef762a65ff8 100644 --- a/chrome/browser/history/top_sites_impl.h +++ b/chrome/browser/history/top_sites_impl.h @@ -98,6 +98,20 @@ class TopSitesImpl : public TopSites, ~TopSitesImpl() override; private: + // TODO(yiyaoliu): Remove the enums and related code when crbug/223430 is + // fixed. + // An enum representing different situations under which function + // SetTopSites can be initiated. + // This is needed because a histogram is used to record speed related metrics + // when SetTopSites are initiated from OnGotMostVisitedThumbnails, which + // usually happens early and might affect Chrome startup speed. + enum CallLocation { + // SetTopSites is called from function OnGotMostVisitedThumbnails. + CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS, + // All other situations. + CALL_LOCATION_FROM_OTHER_PLACES + }; + friend class TopSitesImplTest; FRIEND_TEST_ALL_PREFIXES(TopSitesImplTest, DiffMostVisited); FRIEND_TEST_ALL_PREFIXES(TopSitesImplTest, DiffMostVisitedWithForced); @@ -195,7 +209,8 @@ class TopSitesImpl : public TopSites, // if the list of forced URLs overflows, the oldest ones are dropped. // All mutations to cache_ *must* go through this. Should // be called from the UI thread. - void SetTopSites(const MostVisitedURLList& new_top_sites); + void SetTopSites(const MostVisitedURLList& new_top_sites, + const CallLocation location); // Returns the number of most visited results to request from history. This // changes depending upon how many urls have been blacklisted. Should be @@ -276,6 +291,10 @@ class TopSitesImpl : public TopSites, // Are we loaded? bool loaded_; + // Have the SetTopSites execution time related histograms been recorded? + // The histogram should only be recorded once for each Chrome execution. + static bool histogram_recorded_; + ScopedObserver history_service_observer_; diff --git a/chrome/browser/history/top_sites_impl_unittest.cc b/chrome/browser/history/top_sites_impl_unittest.cc index 4974e9915976..5de85ce65134 100644 --- a/chrome/browser/history/top_sites_impl_unittest.cc +++ b/chrome/browser/history/top_sites_impl_unittest.cc @@ -307,7 +307,8 @@ class TopSitesImplTest : public HistoryUnitTestBase { } void SetTopSites(const MostVisitedURLList& new_top_sites) { - top_sites()->SetTopSites(new_top_sites); + top_sites()->SetTopSites(new_top_sites, + TopSitesImpl::CALL_LOCATION_FROM_OTHER_PLACES); } bool AddForcedURL(const GURL& url, base::Time time) { diff --git a/components/history/core/browser/top_sites_backend.cc b/components/history/core/browser/top_sites_backend.cc index 2383c1c9d213..c119212c20aa 100644 --- a/components/history/core/browser/top_sites_backend.cc +++ b/components/history/core/browser/top_sites_backend.cc @@ -47,10 +47,12 @@ void TopSitesBackend::GetMostVisitedThumbnails( base::Bind(callback, thumbnails)); } -void TopSitesBackend::UpdateTopSites(const TopSitesDelta& delta) { +void TopSitesBackend::UpdateTopSites(const TopSitesDelta& delta, + const RecordHistogram record_or_not) { db_task_runner_->PostTask( FROM_HERE, - base::Bind(&TopSitesBackend::UpdateTopSitesOnDBThread, this, delta)); + base::Bind(&TopSitesBackend::UpdateTopSitesOnDBThread, this, delta, + record_or_not)); } void TopSitesBackend::SetPageThumbnail(const MostVisitedURL& url, @@ -101,14 +103,13 @@ void TopSitesBackend::GetMostVisitedThumbnailsOnDBThread( } } -void TopSitesBackend::UpdateTopSitesOnDBThread(const TopSitesDelta& delta) { +void TopSitesBackend::UpdateTopSitesOnDBThread( + const TopSitesDelta& delta, const RecordHistogram record_or_not) { TRACE_EVENT0("startup", "history::TopSitesBackend::UpdateTopSitesOnDBThread"); if (!db_) return; - // TODO(yiyaoliu): Remove the histogram and related code when crbug/223430 is - // fixed. base::TimeTicks begin_time = base::TimeTicks::Now(); for (size_t i = 0; i < delta.deleted.size(); ++i) @@ -120,8 +121,10 @@ void TopSitesBackend::UpdateTopSitesOnDBThread(const TopSitesDelta& delta) { for (size_t i = 0; i < delta.moved.size(); ++i) db_->UpdatePageRank(delta.moved[i].url, delta.moved[i].rank); - UMA_HISTOGRAM_TIMES("History.UpdateTopSitesOnDBThreadTime", - base::TimeTicks::Now() - begin_time); + if (record_or_not == RECORD_HISTOGRAM_YES) { + UMA_HISTOGRAM_TIMES("History.FirstUpdateTime", + base::TimeTicks::Now() - begin_time); + } } void TopSitesBackend::SetPageThumbnailOnDBThread(const MostVisitedURL& url, diff --git a/components/history/core/browser/top_sites_backend.h b/components/history/core/browser/top_sites_backend.h index d3c054b81032..ad0d9a98f0d0 100644 --- a/components/history/core/browser/top_sites_backend.h +++ b/components/history/core/browser/top_sites_backend.h @@ -26,6 +26,15 @@ class TopSitesDatabase; // thread. class TopSitesBackend : public base::RefCountedThreadSafe { public: + // TODO(yiyaoliu): Remove the enums and related code when crbug/223430 is + // fixed. + // An enum representing whether the UpdateTopSites execution time related + // histogram should be recorded. + enum RecordHistogram { + RECORD_HISTOGRAM_YES, + RECORD_HISTOGRAM_NO + }; + // The boolean parameter indicates if the DB existed on disk or needs to be // migrated. typedef base::Callback&)> @@ -45,7 +54,8 @@ class TopSitesBackend : public base::RefCountedThreadSafe { base::CancelableTaskTracker* tracker); // Updates top sites database from the specified delta. - void UpdateTopSites(const TopSitesDelta& delta); + void UpdateTopSites(const TopSitesDelta& delta, + const RecordHistogram record_or_not); // Sets the thumbnail. void SetPageThumbnail(const MostVisitedURL& url, @@ -77,7 +87,8 @@ class TopSitesBackend : public base::RefCountedThreadSafe { scoped_refptr thumbnails); // Updates top sites. - void UpdateTopSitesOnDBThread(const TopSitesDelta& delta); + void UpdateTopSitesOnDBThread(const TopSitesDelta& delta, + const RecordHistogram record_or_not); // Sets the thumbnail. void SetPageThumbnailOnDBThread(const MostVisitedURL& url, diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 4976ad2dd870..5968cb7f0122 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -11721,6 +11721,25 @@ Therefore, the affected-histogram name has to have at least one dot in it. + + yiyaoliu@chromium.org + + The count of differences between cached top sites and up-to-date top sites + when function TopSitesImpl::SetTopSites is firstly called at startup. + + + + + yiyaoliu@chromium.org + + The amount of time for function + history::TopSitesBackend::UpdateTopSitesOnDBThread to execute when this + function is called during startup. Excludes the case where local + TopSitesDatabase db_ is unavailable, i.e. where the update doesn't really + happen. + + + rkaplow@chromium.org @@ -11818,6 +11837,10 @@ Therefore, the affected-histogram name has to have at least one dot in it. + + Deprecated because only the execution time at startup is of interest. See + histogram History.UpdateTopSitesOnDBThread_Startup_Time. + yiyaoliu@chromium.org The amount of time for function -- 2.11.4.GIT