From 02bf16eb1cd50ea179cbfb4e63430773fb18d93f Mon Sep 17 00:00:00 2001 From: zhenw Date: Fri, 8 May 2015 12:23:39 -0700 Subject: [PATCH] Get correct PLT measurement for Resource Prefetching for Mobile Web The starting point of the PLT measurement for Resource Prefetching for Mobile Web is when the main resource request is created and sent out. Current implementation actually did not store the start time in the right data structure, leading to incorrect PLT measurement. This CL fixes this. inflight_navigations_ stores the NavigationID. When measuring PLT, we need to use the navigation ID stored in inflight_navigations_, instead of using the newly created ones. BUG=405690 Review URL: https://codereview.chromium.org/1133493002 Cr-Commit-Position: refs/heads/master@{#329009} --- .../net/resource_prefetch_predictor_observer.cc | 1 + .../predictors/resource_prefetch_predictor.cc | 14 +++- .../predictors/resource_prefetch_predictor.h | 9 ++- .../resource_prefetch_predictor_unittest.cc | 93 ++++++++++++++++------ 4 files changed, 86 insertions(+), 31 deletions(-) diff --git a/chrome/browser/net/resource_prefetch_predictor_observer.cc b/chrome/browser/net/resource_prefetch_predictor_observer.cc index 5a3f4a1979b0..81d9b07ff98f 100644 --- a/chrome/browser/net/resource_prefetch_predictor_observer.cc +++ b/chrome/browser/net/resource_prefetch_predictor_observer.cc @@ -118,6 +118,7 @@ void ResourcePrefetchPredictorObserver::OnRequestStarted( summary.navigation_id.render_process_id = child_id; summary.navigation_id.render_frame_id = frame_id; summary.navigation_id.main_frame_url = request->first_party_for_cookies(); + summary.navigation_id.creation_time = request->creation_time(); summary.resource_url = request->original_url(); summary.resource_type = resource_type; diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc index 3e1055e33bc9..616028b3555f 100644 --- a/chrome/browser/predictors/resource_prefetch_predictor.cc +++ b/chrome/browser/predictors/resource_prefetch_predictor.cc @@ -513,18 +513,22 @@ void ResourcePrefetchPredictor::OnSubresourceResponse( nav_it->second->push_back(response); } -void ResourcePrefetchPredictor::OnNavigationComplete( - const NavigationID& navigation_id) { +base::TimeDelta ResourcePrefetchPredictor::OnNavigationComplete( + const NavigationID& nav_id_without_timing_info) { DCHECK_CURRENTLY_ON(BrowserThread::UI); NavigationMap::iterator nav_it = - inflight_navigations_.find(navigation_id); + inflight_navigations_.find(nav_id_without_timing_info); if (nav_it == inflight_navigations_.end()) { RecordNavigationEvent(NAVIGATION_EVENT_ONLOAD_UNTRACKED_URL); - return; + return base::TimeDelta(); } RecordNavigationEvent(NAVIGATION_EVENT_ONLOAD_TRACKED_URL); + // Get and use the navigation ID stored in |inflight_navigations_| because it + // has the timing infomation. + const NavigationID navigation_id(nav_it->first); + // Report any stats. base::TimeDelta plt = base::TimeTicks::Now() - navigation_id.creation_time; ReportPageLoadTimeStats(plt); @@ -580,6 +584,8 @@ void ResourcePrefetchPredictor::OnNavigationComplete( base::Bind(&ResourcePrefetchPredictor::OnVisitCountLookup, AsWeakPtr()))), &history_lookup_consumer_); + + return plt; } bool ResourcePrefetchPredictor::GetPrefetchData( diff --git a/chrome/browser/predictors/resource_prefetch_predictor.h b/chrome/browser/predictors/resource_prefetch_predictor.h index f446a40974c3..d6a0053cec96 100644 --- a/chrome/browser/predictors/resource_prefetch_predictor.h +++ b/chrome/browser/predictors/resource_prefetch_predictor.h @@ -141,6 +141,7 @@ class ResourcePrefetchPredictor FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, OnMainFrameRedirect); FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, OnSubresourceResponse); + FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetCorrectPLT); enum InitializationState { NOT_INITIALIZED = 0, @@ -190,9 +191,11 @@ class ResourcePrefetchPredictor void OnSubresourceResponse(const URLRequestSummary& response); // Called when onload completes for a navigation. We treat this point as the - // "completion" of the navigation. The resources requested by the page upto - // this point are the only ones considered for prefetching. - void OnNavigationComplete(const NavigationID& navigation_id); + // "completion" of the navigation. The resources requested by the page up to + // this point are the only ones considered for prefetching. Return the page + // load time for testing. + base::TimeDelta OnNavigationComplete( + const NavigationID& nav_id_without_timing_info); // Returns true if there is PrefetchData that can be used for the // navigation and fills in the |prefetch_data| to resources that need to be diff --git a/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc b/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc index a4e8165ce86d..ae8f29ebf1c0 100644 --- a/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc +++ b/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc @@ -382,7 +382,7 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationNotRecorded) { std::string(), false); predictor_->RecordURLRequest(main_frame); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); // Now add a few subresources. URLRequestSummary resource1 = CreateURLRequestSummary( @@ -440,7 +440,7 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDB) { std::string(), false); predictor_->RecordURLRequest(main_frame); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); URLRequestSummary resource1 = CreateURLRequestSummary( 1, 1, "http://www.google.com", "http://google.com/style1.css", @@ -522,8 +522,8 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlInDB) { SetArgPointee<1>(test_host_data_))); ResetPredictor(); InitializePredictor(); - EXPECT_EQ(3, static_cast(predictor_->url_table_cache_->size())); - EXPECT_EQ(2, static_cast(predictor_->host_table_cache_->size())); + EXPECT_EQ(3U, predictor_->url_table_cache_->size()); + EXPECT_EQ(2U, predictor_->host_table_cache_->size()); URLRequestSummary main_frame = CreateURLRequestSummary(1, @@ -534,7 +534,7 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlInDB) { std::string(), false); predictor_->RecordURLRequest(main_frame); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); URLRequestSummary resource1 = CreateURLRequestSummary( 1, 1, "http://www.google.com", "http://google.com/style1.css", @@ -646,8 +646,8 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDBAndDBFull) { SetArgPointee<1>(test_host_data_))); ResetPredictor(); InitializePredictor(); - EXPECT_EQ(3, static_cast(predictor_->url_table_cache_->size())); - EXPECT_EQ(2, static_cast(predictor_->host_table_cache_->size())); + EXPECT_EQ(3U, predictor_->url_table_cache_->size()); + EXPECT_EQ(2U, predictor_->host_table_cache_->size()); URLRequestSummary main_frame = CreateURLRequestSummary(1, @@ -658,7 +658,7 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDBAndDBFull) { std::string(), false); predictor_->RecordURLRequest(main_frame); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); URLRequestSummary resource1 = CreateURLRequestSummary( 1, 1, "http://www.nike.com", "http://nike.com/style1.css", @@ -746,8 +746,8 @@ TEST_F(ResourcePrefetchPredictorTest, DeleteUrls) { DeleteData(ContainerEq(urls_to_delete), ContainerEq(hosts_to_delete))); predictor_->DeleteUrls(rows); - EXPECT_EQ(2, static_cast(predictor_->url_table_cache_->size())); - EXPECT_EQ(1, static_cast(predictor_->host_table_cache_->size())); + EXPECT_EQ(2U, predictor_->url_table_cache_->size()); + EXPECT_EQ(1U, predictor_->host_table_cache_->size()); EXPECT_CALL(*mock_tables_.get(), DeleteAllData()); @@ -783,11 +783,11 @@ TEST_F(ResourcePrefetchPredictorTest, OnMainFrameRequest) { false); predictor_->OnMainFrameRequest(summary1); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); predictor_->OnMainFrameRequest(summary2); - EXPECT_EQ(2, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(2U, predictor_->inflight_navigations_.size()); predictor_->OnMainFrameRequest(summary3); - EXPECT_EQ(3, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(3U, predictor_->inflight_navigations_.size()); // Insert anther with same navigation id. It should replace. URLRequestSummary summary4 = @@ -808,13 +808,13 @@ TEST_F(ResourcePrefetchPredictorTest, OnMainFrameRequest) { false); predictor_->OnMainFrameRequest(summary4); - EXPECT_EQ(3, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(3U, predictor_->inflight_navigations_.size()); // Change this creation time so that it will go away on the next insert. summary5.navigation_id.creation_time = base::TimeTicks::Now() - base::TimeDelta::FromDays(1); predictor_->OnMainFrameRequest(summary5); - EXPECT_EQ(3, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(3U, predictor_->inflight_navigations_.size()); URLRequestSummary summary6 = CreateURLRequestSummary(3, @@ -825,7 +825,7 @@ TEST_F(ResourcePrefetchPredictorTest, OnMainFrameRequest) { std::string(), false); predictor_->OnMainFrameRequest(summary6); - EXPECT_EQ(3, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(3U, predictor_->inflight_navigations_.size()); EXPECT_TRUE(predictor_->inflight_navigations_.find(summary3.navigation_id) != predictor_->inflight_navigations_.end()); @@ -865,14 +865,14 @@ TEST_F(ResourcePrefetchPredictorTest, OnMainFrameRedirect) { EXPECT_TRUE(predictor_->inflight_navigations_.empty()); predictor_->OnMainFrameRequest(summary1); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); predictor_->OnMainFrameRequest(summary2); - EXPECT_EQ(2, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(2U, predictor_->inflight_navigations_.size()); predictor_->OnMainFrameRedirect(summary3); - EXPECT_EQ(2, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(2U, predictor_->inflight_navigations_.size()); predictor_->OnMainFrameRedirect(summary1); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); predictor_->OnMainFrameRedirect(summary2); EXPECT_TRUE(predictor_->inflight_navigations_.empty()); } @@ -895,7 +895,7 @@ TEST_F(ResourcePrefetchPredictorTest, OnSubresourceResponse) { std::string(), false); predictor_->OnMainFrameRequest(main_frame1); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); // Now add a few subresources. URLRequestSummary resource2 = CreateURLRequestSummary( @@ -908,9 +908,9 @@ TEST_F(ResourcePrefetchPredictorTest, OnSubresourceResponse) { predictor_->OnSubresourceResponse(resource2); predictor_->OnSubresourceResponse(resource3); - EXPECT_EQ(1, static_cast(predictor_->inflight_navigations_.size())); - EXPECT_EQ(3, static_cast( - predictor_->inflight_navigations_[main_frame1.navigation_id]->size())); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); + EXPECT_EQ(3U, + predictor_->inflight_navigations_[main_frame1.navigation_id]->size()); EXPECT_TRUE(URLRequestSummaryAreEqual( resource1, predictor_->inflight_navigations_[main_frame1.navigation_id]->at(0))); @@ -922,4 +922,49 @@ TEST_F(ResourcePrefetchPredictorTest, OnSubresourceResponse) { predictor_->inflight_navigations_[main_frame1.navigation_id]->at(2))); } +TEST_F(ResourcePrefetchPredictorTest, GetCorrectPLT) { + // Single navigation but history count is low, so should not record. + AddUrlToHistory("http://www.google.com", 1); + + URLRequestSummary main_frame = + CreateURLRequestSummary(1, + 1, + "http://www.google.com", + "http://www.google.com", + content::RESOURCE_TYPE_MAIN_FRAME, + std::string(), + false); + predictor_->RecordURLRequest(main_frame); + EXPECT_EQ(1U, predictor_->inflight_navigations_.size()); + + // Reset the creation time in |main_frame.navigation_id|. The correct creation + // time is stored in |inflight_navigations_| and should be used later. + main_frame.navigation_id.creation_time = base::TimeTicks(); + EXPECT_TRUE(main_frame.navigation_id.creation_time.is_null()); + + // Now add a subresource. + URLRequestSummary resource1 = CreateURLRequestSummary( + 1, 1, "http://www.google.com", "http://google.com/style1.css", + content::RESOURCE_TYPE_STYLESHEET, "text/css", false); + predictor_->RecordURLResponse(resource1); + + PrefetchData host_data(PREFETCH_KEY_TYPE_HOST, "www.google.com"); + host_data.resources.push_back(ResourceRow(std::string(), + "http://google.com/style1.css", + content::RESOURCE_TYPE_STYLESHEET, + 1, + 0, + 0, + 1.0)); + EXPECT_CALL(*mock_tables_.get(), UpdateData(empty_url_data_, host_data)); + + // The page load time will be collected by RPP_HISTOGRAM_MEDIUM_TIMES, which + // has a upper bound of 3 minutes. + base::TimeDelta plt = + predictor_->OnNavigationComplete(main_frame.navigation_id); + EXPECT_LT(plt, base::TimeDelta::FromSeconds(180)); + + profile_->BlockUntilHistoryProcessesPendingRequests(); +} + } // namespace predictors -- 2.11.4.GIT