Revert CL 117933003. Re-add resource speculative prefetching code.

Revert CL 117933003. Re-add speculative resource prefetching code.
https://codereview.chromium.org/117933003/

The speculative resource prefetching code was experimental code developed by
shishir@. He found that it has little improvement on desktop Chrome (win). We
though this should be beneficial to mobile browsers. After discussing with
tburkard@ and kenjibaheux@, we decided to bring the code back and do more
analysis on mobile devices. Reverting the patchset to re-add the code is the
first step.

The following design doc has discussed all related approaches and action items.
https://docs.google.com/a/google.com/document/d/1ie3hu-zNNXvmTXm3aJAtKUGOh6nZfbNjA0aZE1bzzIg/edit?usp=sharing

BUG=408399, 405690
Committed: https://crrev.com/825722d5939dc0832d124b70958501611e6a4628
Cr-Commit-Position: refs/heads/master@{#294899}

[Zhen Wang, 2014-08-17 22:14:46]

ptal

[tburkard, 2014-08-18 12:33:33]

lgtm

[shishir1, 2014-08-25 16:21:16]

On 2014/08/18 12:33:33, tburkard wrote:
> lgtm

I dont see the code (IIRC it was in Prerender dir) where this service is
enabled/disabled. Please make sure it is disabled by default.

lgtm.

[Zhen Wang, 2014-08-25 16:34:51]

> I dont see the code (IIRC it was in Prerender dir) where this service is
> enabled/disabled. Please make sure it is disabled by default.

I think it was enabled in prerender_field_trial.cc. It's not added yet.
See https://chromiumcodereview.appspot.com/10834037/

[Zhen Wang, 2014-08-25 16:35:29]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-08-25 16:36:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhenw@chromium.org/462423004/1

[commit-bot: I haz the power, 2014-08-25 16:41:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-25 16:42:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 16:42:48]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Zhen Wang, 2014-08-26 01:46:54]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-08-26 01:48:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhenw@chromium.org/462423004/20001

[commit-bot: I haz the power, 2014-08-26 01:52:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-26 01:54:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 01:54:49]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Zhen Wang, 2014-08-26 05:51:31]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-08-26 05:52:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhenw@chromium.org/462423004/40001

[commit-bot: I haz the power, 2014-08-26 06:44:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-26 06:49:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 06:49:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Zhen Wang, 2014-08-26 16:44:20]

zhenw@chromium.org changed reviewers:
+ jam@chromium.org

[Zhen Wang, 2014-08-26 16:44:20]

Hi Jam,

Can you help to review this CL? The idea is to bring the resource speculative
prefetching code back and do more analysis on mobile devices.

This CL only reverts your CL 117933003 and fixes the conflicts.

Best
-Zhen

[jam, 2014-08-27 03:43:09]

On 2014/08/26 16:44:20, Zhen Wang wrote:
> Hi Jam,
> 
> Can you help to review this CL? The idea is to bring the resource speculative
> prefetching code back and do more analysis on mobile devices.
> 
> This CL only reverts your CL 117933003 and fixes the conflicts.
> 
> Best
> -Zhen

The reason that it was removed was that maintaining this code wasn't fee. If I
remember correctly, one problem was that this was getting in the way of site
isolation refactoring by keeping data on the IO thread related to which requests
are from the same tab. In a RenderView-based world, this is easy to do since the
requests will come from the same render_process_id/render_view_id. In the world
where we're moving to, RenderView is gone and is replaced by RenderFrame. A page
can have many RenderFrames in different processes, and the IO thread doesn't
have the mapping of which frame identifiers are in the same tab.

If you want to try this out again, this would have to be rewritten to use
RenderFrames instead of RenderViews.

[Zhen Wang, 2014-08-27 04:28:31]

> The reason that it was removed was that maintaining this code wasn't fee. If I
> remember correctly, one problem was that this was getting in the way of site
> isolation refactoring by keeping data on the IO thread related to which
requests
> are from the same tab. In a RenderView-based world, this is easy to do since
the
> requests will come from the same render_process_id/render_view_id. In the
world
> where we're moving to, RenderView is gone and is replaced by RenderFrame. A
page
> can have many RenderFrames in different processes, and the IO thread doesn't
> have the mapping of which frame identifiers are in the same tab.
> 
> If you want to try this out again, this would have to be rewritten to use
> RenderFrames instead of RenderViews.

Are you talking about out-of-process iframes?
(http://www.chromium.org/developers/design-documents/oop-iframes)

Will rewrite to use RenderFrames and ask you for review once it's done. Thanks!

[jam, 2014-08-27 16:46:36]

On 2014/08/27 04:28:31, Zhen Wang wrote:
> > The reason that it was removed was that maintaining this code wasn't fee. If
I
> > remember correctly, one problem was that this was getting in the way of site
> > isolation refactoring by keeping data on the IO thread related to which
> requests
> > are from the same tab. In a RenderView-based world, this is easy to do since
> the
> > requests will come from the same render_process_id/render_view_id. In the
> world
> > where we're moving to, RenderView is gone and is replaced by RenderFrame. A
> page
> > can have many RenderFrames in different processes, and the IO thread doesn't
> > have the mapping of which frame identifiers are in the same tab.
> > 
> > If you want to try this out again, this would have to be rewritten to use
> > RenderFrames instead of RenderViews.
> 
> Are you talking about out-of-process iframes?
> (http://www.chromium.org/developers/design-documents/oop-iframes)

Correct

> 
> Will rewrite to use RenderFrames and ask you for review once it's done.
Thanks!

ok. note that we explicitely avoid having a map on the IO thread, so this will
have to do the bookeeping on the UI thread

[Zhen Wang, 2014-08-31 00:03:59]

I have made it work with RenderFrame instead of RenderView. Notice that current
version only record main frame requests/redirects/responses and ignore all
sub-frame activities. See
ResourcePrefetchPredictor::ShouldRecordRequest/ShouldRecordRedirect/ShouldRecordResponse.
Here are the reasons:
1. The original code use main frame activity as the only indicator and does not
handle sub-frames specifically. See
ResourcePrefetchPredictorTabHelper::DocumentOnLoadCompletedInMainFrame
2. It is debatable that whether we should record sub-frame activities
independently or together with the main frame.
3. This CL is to bring back the old code and make it work with current system.
It may be better to defer the debate on sub-frame activity record to a follow-up
CL.

By the way, I think the bookkeeping is already done on the UI thread. See
ResourcePrefetchPredictor.

ptal

[Zhen Wang, 2014-09-04 14:45:50]

Hi Jam,

Can you take a look? Thanks!

Best
-Zhen

On 2014/08/31 00:03:59, Zhen Wang wrote:
> I have made it work with RenderFrame instead of RenderView. Notice that
current
> version only record main frame requests/redirects/responses and ignore all
> sub-frame activities. See
>
ResourcePrefetchPredictor::ShouldRecordRequest/ShouldRecordRedirect/ShouldRecordResponse.
> Here are the reasons:
> 1. The original code use main frame activity as the only indicator and does
not
> handle sub-frames specifically. See
> ResourcePrefetchPredictorTabHelper::DocumentOnLoadCompletedInMainFrame
> 2. It is debatable that whether we should record sub-frame activities
> independently or together with the main frame.
> 3. This CL is to bring back the old code and make it work with current system.
> It may be better to defer the debate on sub-frame activity record to a
follow-up
> CL.
> 
> By the way, I think the bookkeeping is already done on the UI thread. See
> ResourcePrefetchPredictor.
> 
> ptal

[Zhen Wang, 2014-09-08 17:21:52]

Hi Jam,

Any thoughts on the CL?

Best
-Zhen

[jam, 2014-09-09 00:37:04]

jam@chromium.org changed reviewers:
+ thestig@chromium.org

[jam, 2014-09-09 00:37:05]

sorry for the delay, I didn't see your earlier messages.

adding thestig to do a thorough review of bringing-back-this-code-with-changes.
I'm overloaded with the code yellow. as long as render view map on IO thread
isn't brought back, i'm happy.

can you point to data structures that are kept on IO thread now (i.e. so I can
look at that without going through the whole cl)?

[Lei Zhang, 2014-09-09 03:32:20]

General comments:

- If we don't need this on the desktop, consider making the newly re-added code
Android-only.
- I have many comments below. I've added the "nit:" prefix to the less important
ones.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
File chrome/browser/net/resource_prefetch_predictor_observer.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.cc:43:
UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.RequestStats",
You need to update the histograms xml file and have a metrics OWNER review this.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.cc:109:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
nit: Please try to use DCHECK_CURRENTLY_ON() in new code.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:506: if
(inflight_navigations_.find(response.navigation_id) ==
nit: if you are already doing find(), save the iterator so you don't have to use
map[] below.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:541: if
(GetPrefetchData(navigation_id, requests.get(), &key_type)){
nit: ') {'

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:553:
inflight_navigations_[navigation_id].release();
nit: Likewise, you can just use |nav_it| from above here and for erase().

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:851: delete
results_map_[navigation_id];
Can you add DCHECK(ContainsKey(results_map_, navigation_id)) ?

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:874: if
(static_cast<int>(data_map->size()) >= max_data_map_size) {
nit: |max_data_map_size| probably should be a size_t, same with
|new_resources_size| below, unless you need to allow negative values for some
reason.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:971: if
(static_cast<int>(resources.size()) > config_.max_resources_per_entry)
nit: Ditto with max_resources_per_entry, it probably should be a size_t.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:976: if
(resources.size() == 0) {
nit: if (resources.empty())

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1037: 
nit: no blank lines

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1152: int
num_assumed_prefetched = std::min(static_cast<int>(predicted.size()),
nit: Ditto, use size_t if you can.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:8: #include
<utility>
nit: blank line after

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:114: score =
kMaxResourcesPerType - average_position;
nit: combine with default? i.e.

case content::RESOURCE_TYPE_IMAGE:
default:
  ...

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:167:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
Generally we DCHECK we are on the correct thread, unless you have some serious
need to CHECK(). If you switch over to DCHECK_CURRENTLY_ON(), you'll fix these
along the way.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:389: const char*
resource_table_creator =
nit: const char resource_table_creator[]

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc
(right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:244:
EXPECT_TRUE(lhs_it != lhs.end()) << rhs_it->first;
If you fail this, you are going to access invalid memory below.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:260:
EXPECT_TRUE(resources_seen.find(resource) == resources_seen.end());
nit: this is more readable as EXPECT_FALSE(ContainsKey(resources_seen,
resource)); ditto for EXPECT_TRUE() below.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:282:
EXPECT_TRUE(it != test_host_data_.end());
If you fail this, the next line dereferences an invalid iterator

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.h:33: class ResourcePrefetcher :
public base::NonThreadSafe,
nit: We generally prefer using ThreadChecker over NonThreadSafe.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:60:
PrefetcherMap::iterator prefetcher_it = prefetcher_map_.find(key);
nit: Use ContainsKey()

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:97:
scoped_ptr<ResourcePrefetcher::RequestVector> requests_ptr(requests);
nit: |requests_ptr| is poorly named. It should be "scoped_requests". Or make the
parameter |requests_ptr| and change the scoper to "requests".

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:123: if (predictor_)
nit braces around multi-line if block contnets.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_unittest.cc:254:
EXPECT_EQ((*requests_ptr)[0]->prefetch_status,
nit: EXPECT_EQ(expected, actual);

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_unittest.cc:279: delete
requests_ptr;
This doesn't look right. |requests_ptr| comes from |requests|, which is a
scoped_ptr. Whoever you Passed() the scoped_ptr to should take ownership... so
why do you need to do this delete?

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:328:
resource_prefetch_predictor_observer_;
nit: indent 4 spaces

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:40: #include
"chrome/browser/profiles/profile_android.h"
nit: not used at all?

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:118: Profile* profile =
Profile::FromBrowserContext(
nit: You don't need this, ResourcePrefetchPredictorFactory::GetForProfile()
takes a BrowserContext. Remove the chrome/browser/profiles/profile.h #include as
well.

[Zhen Wang, 2014-09-09 04:24:05]

Thanks for the comments!

jam:
The data structures I refer to are the database tables used by
ResourcePrefetchPredictor. They are at the end of resource_prefetch_predictor.h:

  NavigationMap inflight_navigations_;
  scoped_ptr<PrefetchDataMap> url_table_cache_;
  scoped_ptr<PrefetchDataMap> host_table_cache_;
  ResultsMap results_map_;
  STLValueDeleter<ResultsMap> results_map_deleter_;

thestig:
I quickly went over the comments, here is a quick question on the result of
failing EXPECT_TRUE in unittests
(resource_prefetch_predictor_tables_unittest.cc). In general, how should I treat
the case that EXPECT_TRUE may fail and result in bad memory access? Should I use
ASSERT to make sure it does not proceed? Or I should still use if clause after
the EXPECT_TRUE statement, so that even if EXPECT_TRUE fails, it still works
fine?

I will let you know once I address all the comments.

[Lei Zhang, 2014-09-09 04:39:18]

On 2014/09/09 04:24:05, Zhen Wang wrote:
> thestig:
> I quickly went over the comments, here is a quick question on the result of
> failing EXPECT_TRUE in unittests
> (resource_prefetch_predictor_tables_unittest.cc). In general, how should I
treat
> the case that EXPECT_TRUE may fail and result in bad memory access? Should I
use
> ASSERT to make sure it does not proceed? Or I should still use if clause after
> the EXPECT_TRUE statement, so that even if EXPECT_TRUE fails, it still works
> fine?

We generally ASSERT_TRUE(). The gtest primer says "However, you should use
ASSERT_* if it doesn't make sense to continue when the assertion in question
fails."

[Zhen Wang, 2014-09-13 00:36:33]

Hi Lei,

I have addressed your comments. ptal.

By the way, the re-added code is not specific to one platform. The decision to
try the feature on mobile but not on desktop is based on the potential
performance benefit. Is it suffice to control on which platform the feature is
enabled in the experiment?

Best
-Zhen

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
File chrome/browser/net/resource_prefetch_predictor_observer.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.cc:43:
UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.RequestStats",
> You need to update the histograms xml file and have a metrics OWNER review
this.

The main purpose of this CL is to bring back the old code, resolve all the
conflicts and make it work with current browser architecture. The feature is
disabled for now and will not affect current browser usage and experiment. We
have plans to examine all the metrics in the original code, write up the formal
finch experiment description and enable the experiment in later CLs.

See also:
https://code.google.com/p/chromium/issues/detail?id=405690#c12
https://code.google.com/p/chromium/issues/detail?id=406200
https://code.google.com/p/chromium/issues/detail?id=406203

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.cc:109:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: Please try to use DCHECK_CURRENTLY_ON() in new code.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
> nit: no (c)

I see other files using "Copyright (c)".

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:506: if
(inflight_navigations_.find(response.navigation_id) ==
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: if you are already doing find(), save the iterator so you don't have to
use
> map[] below.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:541: if
(GetPrefetchData(navigation_id, requests.get(), &key_type)){
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: ') {'

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:553:
inflight_navigations_[navigation_id].release();
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: Likewise, you can just use |nav_it| from above here and for erase().

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:851: delete
results_map_[navigation_id];
> Can you add DCHECK(ContainsKey(results_map_, navigation_id)) ?

results_map_ is empty in resource_prefetch_predictor_unittests. results_map_ is
used when the prefetcher is invloded. But testing the predictor does not involve
the prefetcher.

I think this is fine because deleting non-existing entry does not incur any
problem.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:874: if
(static_cast<int>(data_map->size()) >= max_data_map_size) {
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: |max_data_map_size| probably should be a size_t, same with
> |new_resources_size| below, unless you need to allow negative values for some
> reason.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:971: if
(static_cast<int>(resources.size()) > config_.max_resources_per_entry)
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: Ditto with max_resources_per_entry, it probably should be a size_t.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:976: if
(resources.size() == 0) {
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: if (resources.empty())

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1037: 
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: no blank lines

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1152: int
num_assumed_prefetched = std::min(static_cast<int>(predicted.size()),
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: Ditto, use size_t if you can.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:8: #include
<utility>
On 2014/09/09 03:32:19, Lei Zhang wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:114: score =
kMaxResourcesPerType - average_position;
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: combine with default? i.e.
> 
> case content::RESOURCE_TYPE_IMAGE:
> default:
>   ...

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:167:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
On 2014/09/09 03:32:19, Lei Zhang wrote:
> Generally we DCHECK we are on the correct thread, unless you have some serious
> need to CHECK(). If you switch over to DCHECK_CURRENTLY_ON(), you'll fix these
> along the way.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:389: const char*
resource_table_creator =
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: const char resource_table_creator[]

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc
(right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:244:
EXPECT_TRUE(lhs_it != lhs.end()) << rhs_it->first;
On 2014/09/09 03:32:20, Lei Zhang wrote:
> If you fail this, you are going to access invalid memory below.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:260:
EXPECT_TRUE(resources_seen.find(resource) == resources_seen.end());
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: this is more readable as EXPECT_FALSE(ContainsKey(resources_seen,
> resource)); ditto for EXPECT_TRUE() below.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:282:
EXPECT_TRUE(it != test_host_data_.end());
On 2014/09/09 03:32:20, Lei Zhang wrote:
> If you fail this, the next line dereferences an invalid iterator

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.h:33: class ResourcePrefetcher :
public base::NonThreadSafe,
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: We generally prefer using ThreadChecker over NonThreadSafe.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_manager.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:60:
PrefetcherMap::iterator prefetcher_it = prefetcher_map_.find(key);
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: Use ContainsKey()

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:97:
scoped_ptr<ResourcePrefetcher::RequestVector> requests_ptr(requests);
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: |requests_ptr| is poorly named. It should be "scoped_requests". Or make
the
> parameter |requests_ptr| and change the scoper to "requests".

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.cc:123: if (predictor_)
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit braces around multi-line if block contnets.

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_unittest.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_unittest.cc:254:
EXPECT_EQ((*requests_ptr)[0]->prefetch_status,
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: EXPECT_EQ(expected, actual);

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_unittest.cc:279: delete
requests_ptr;
You are right. Should not need to delete requests_ptr.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:328:
resource_prefetch_predictor_observer_;
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: indent 4 spaces

Done.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:40: #include
"chrome/browser/profiles/profile_android.h"
removed

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:118: Profile* profile =
Profile::FromBrowserContext(
On 2014/09/09 03:32:20, Lei Zhang wrote:
> nit: You don't need this, ResourcePrefetchPredictorFactory::GetForProfile()
> takes a BrowserContext. Remove the chrome/browser/profiles/profile.h #include
as
> well.

Done.

[Lei Zhang, 2014-09-13 01:20:25]

So if this code won't be used on desktop, what's the binary size impact of this
CL on a desktop build? Can you pick a platform, do a "component=static_library"
Release build with and without this CL, and compare the numbers for the main
chrome binary/dll?

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
On 2014/09/13 00:36:31, Zhen Wang wrote:
> > nit: no (c)
> 
> I see other files using "Copyright (c)".

Yes, but going forward...

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:851: delete
results_map_[navigation_id];
On 2014/09/13 00:36:31, Zhen Wang wrote:
> > Can you add DCHECK(ContainsKey(results_map_, navigation_id)) ?
> 
> results_map_ is empty in resource_prefetch_predictor_unittests. results_map_
is
> used when the prefetcher is invloded. But testing the predictor does not
involve
> the prefetcher.
> 
> I think this is fine because deleting non-existing entry does not incur any
> problem.

It's just inefficient because if |navigation_id| is not in |results_map_|, you
are actually adding it to |results_map_| as a result of operator[] and then
immediately erasing it.

Can we just write this as follows?

ResultsMap::iterator results_it = results_map_.find(navigation_id);
if (results_it != results_map_.end() {
  delete results_it->second;
  results_map_.erase(results_it);
}

https://codereview.chromium.org/462423004/diff/140001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/462423004/diff/140001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.cc:152: CHECK_GT(host_it->second,
(size_t)0);
You shouldn't need to cast. Use '0U' instead.

https://codereview.chromium.org/462423004/diff/140001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://codereview.chromium.org/462423004/diff/140001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.h:104: base::ThreadChecker
thread_checker_;
nit: this should be below with the other variables

[Zhen Wang, 2014-09-15 19:47:18]

On Ubuntu 12.04, I did release build of chrome with 
export GYP_DEFINES="clang=1 component=static_library goma=1 use_goma=1"

Master: 136 MB - 141867200 Bytes
With patch: 136 MB - 142014256 Bytes
Diff: 147056 Bytes

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
OK. I have removed "(c)" in all files.

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/462423004/diff/100001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:851: delete
results_map_[navigation_id];
OK

https://codereview.chromium.org/462423004/diff/140001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/462423004/diff/140001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.cc:152: CHECK_GT(host_it->second,
(size_t)0);
On 2014/09/13 01:20:25, Lei Zhang wrote:
> You shouldn't need to cast. Use '0U' instead.

Done.

https://codereview.chromium.org/462423004/diff/140001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.h (right):

https://codereview.chromium.org/462423004/diff/140001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.h:104: base::ThreadChecker
thread_checker_;
On 2014/09/13 01:20:25, Lei Zhang wrote:
> nit: this should be below with the other variables

Done.

[Lei Zhang, 2014-09-15 20:12:58]

lgtm, thanks.

On 2014/09/15 19:47:18, Zhen Wang wrote:
> On Ubuntu 12.04, I did release build of chrome with 
> export GYP_DEFINES="clang=1 component=static_library goma=1 use_goma=1"
> 
> Master: 136 MB - 141867200 Bytes
> With patch: 136 MB - 142014256 Bytes
> Diff: 147056 Bytes

0.1% may not seem much, but every little bit helps. (For comparison, it took me
50 CLs to remove 1.5 MB) If the predictor code is going to stay in the code base
for the long run, but will only be used on mobile, please consider doing a
little cleanup at a later time to make this mobile only.

[Zhen Wang, 2014-09-15 20:51:35]

> 0.1% may not seem much, but every little bit helps. (For comparison, it took
me
> 50 CLs to remove 1.5 MB) If the predictor code is going to stay in the code
base
> for the long run, but will only be used on mobile, please consider doing a
> little cleanup at a later time to make this mobile only.

Thanks! I will keep that in mind when we really launch the feature, if getting
positive results from the experiments.

[Zhen Wang, 2014-09-15 21:00:24]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-09-15 21:02:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/462423004/180001

[commit-bot: I haz the power, 2014-09-15 22:08:33]

Committed patchset #10 (id:180001) as 5a46cd3921a3c16163d91aeec9982b41784de748

[commit-bot: I haz the power, 2014-09-15 22:18:19]

Patchset 10 (id:??) landed as
https://crrev.com/825722d5939dc0832d124b70958501611e6a4628
Cr-Commit-Position: refs/heads/master@{#294899}

[nhiroki, 2014-09-16 04:56:16]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/577543002/ by nhiroki@chromium.org.

The reason for reverting is: This caused a bunch of memory leaks in following
tests:

- ResourcePrefetcherTest.TestPrefetcherFinishes
- ResourcePrefetcherTest.TestPrefetcherStopped

Linux Chromium OS ASan LSan Tests (3)
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...

Linux ASan LSan Tests (2)
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....