predictors: Several improvements for redirects database.

predictors: Several improvements for redirects database.

This CL aims to improve redirects handling in the ResourcePrefetchPredictor by
considering the URL/host itself along with redirect endpoints (1) and handling
special case when there is no permanent redirect for a host (2).

(1) helps to avoid incorrect predictions in scenario when the URL/host doesn't
have a permanent redirect and the predictor mistakenly uses the fallback to
fetching based on the first main frame url in the redirect chain.

(2) allows to ignore "megamorphic" URLs/hosts. For example, url shorteners.

BUG=631966
Review-Url: https://codereview.chromium.org/2800783002
Cr-Commit-Position: refs/heads/master@{#464103}
Committed: https://chromium.googlesource.com/chromium/src/+/93cb7df1f935461d1b3c5544f0c7efe38097d8d7

[alexilin, 2017-04-05 16:23:11]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org

[alexilin, 2017-04-05 16:23:12]

Hi lizeb, please review my change!

Tests are broken, I want to get your approval for this approach before modify
them.

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_common.cc (right):

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_common.cc:136:
max_redirect_consecutive_misses(20),
Effectively, this is maximum number of redirect endpoints per host.
We keep more entries than we used to (max 3) because that way we keep
remembering that the host behaved as "megamorphic". In the same time, we allow
to reestablish "permanent" status after 20 loads.

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:394: const std::string&
entry_point,
Renamed to match the names in .h file.

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:420: const float
kMinRedirectConfidenceToTriggerPrefetch = 0.9f;
The cost of error too high, we want to predict only "permanent" redirects.

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:427:
best_redirect->url() != entry_point)) {
Corresponds to no-redirect case. We don't need min threshold in this case.

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:1315:
LearnRedirect(key_before_redirects, key_type, key, max_data_map_size,
Put a host/URL itself to redirects database to count hits/misses correctly.
Before we didn't update misses in redirects database if there was not redirect.

[Benoit L, 2017-04-06 12:10:55]

Looks good, an alternative as discussed offline is to track consecutive
hits/misses at the top level struct, and keep only a few entries in the
database.

Up to you.

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:420: const float
kMinRedirectConfidenceToTriggerPrefetch = 0.9f;
On 2017/04/05 16:23:12, alexilin wrote:
> The cost of error too high, we want to predict only "permanent" redirects.

Maybe make that a comment in the code?

[alexilin, 2017-04-06 13:05:21]

I propose the solution similar to min number of consecutive hits. 
I could add a field explicitly if you think that we need to use different
constants for min_consecutive_hits and max_redirect_consecutive_misses.

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2800783002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:420: const float
kMinRedirectConfidenceToTriggerPrefetch = 0.9f;
On 2017/04/06 12:10:55, Benoit L wrote:
> On 2017/04/05 16:23:12, alexilin wrote:
> > The cost of error too high, we want to predict only "permanent" redirects.
> 
> Maybe make that a comment in the code?

Done.

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

https://codereview.chromium.org/2800783002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:408: if
(redirect_data.redirect_endpoints_size() > 1) {
This is the same thing as to require minimum number of consecutive hits for a
redirect endpoint.
But with that we also allow to make a prediction for a new host earlier. (the
same this if we initialize consecutive_hits=5 when create a RedirectData).

[alexilin, 2017-04-06 17:52:11]

Benoit, are you OK with this implementation?

[Benoit L, 2017-04-07 09:04:36]

On 2017/04/06 17:52:11, alexilin wrote:
> Benoit, are you OK with this implementation?

Yes, looks good :-)

[alexilin, 2017-04-07 12:57:01]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 12:57:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexilin, 2017-04-07 13:01:19]

Tests are fixed now, your review is welcome.

[commit-bot: I haz the power, 2017-04-07 13:45:22]

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

[commit-bot: I haz the power, 2017-04-07 13:45:23]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-04-10 09:50:35]

https://codereview.chromium.org/2800783002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_common.h (right):

https://codereview.chromium.org/2800783002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_common.h:115: // The number of
consecutive misses after we stop tracking a redirect
nit: after which
here and above.

https://codereview.chromium.org/2800783002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2800783002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:324: bool
GetRedirectEndpoint(const std::string& entry_point,
Can you add (in the implementation, not the .h) a comment explaining why the
heuristics are set this way?
It's not entirely clear without context, so let's be nice with future-us, or
with the poor soul who will look at this code in the future.

[alexilin, 2017-04-10 17:43:25]

https://codereview.chromium.org/2800783002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_common.h (right):

https://codereview.chromium.org/2800783002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_common.h:115: // The number of
consecutive misses after we stop tracking a redirect
On 2017/04/10 09:50:35, Benoit L wrote:
> nit: after which
> here and above.

Done.

https://codereview.chromium.org/2800783002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2800783002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:324: bool
GetRedirectEndpoint(const std::string& entry_point,
On 2017/04/10 09:50:35, Benoit L wrote:
> Can you add (in the implementation, not the .h) a comment explaining why the
> heuristics are set this way?
> It's not entirely clear without context, so let's be nice with future-us, or
> with the poor soul who will look at this code in the future.

Done.

[Benoit L, 2017-04-11 09:14:15]

lgtm, thanks.

[alexilin, 2017-04-12 16:57:47]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2017-04-12 16:57:47]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/2800783002/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-12 16:58:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 19:16:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492016267542770,
"parent_rev": "96bf4bd8c5c82dcedec738e785d5a5d8fc37acab", "commit_rev":
"93cb7df1f935461d1b3c5544f0c7efe38097d8d7"}

[commit-bot: I haz the power, 2017-04-12 19:21:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492016267542770,
"parent_rev": "96bf4bd8c5c82dcedec738e785d5a5d8fc37acab", "commit_rev":
"93cb7df1f935461d1b3c5544f0c7efe38097d8d7"}

[commit-bot: I haz the power, 2017-04-12 19:21:43]

Description was changed from

==========
predictors: Several improvements for redirects database.

This CL aims to improve redirects handling in the ResourcePrefetchPredictor by
considering the URL/host itself along with redirect endpoints (1) and handling
special case when there is no permanent redirect for a host (2).

(1) helps to avoid incorrect predictions in scenario when the URL/host doesn't
have a permanent redirect and the predictor mistakenly uses the fallback to
fetching based on the first main frame url in the redirect chain.

(2) allows to ignore "megamorphic" URLs/hosts. For example, url shorteners.

BUG=631966
==========

to

==========
predictors: Several improvements for redirects database.

This CL aims to improve redirects handling in the ResourcePrefetchPredictor by
considering the URL/host itself along with redirect endpoints (1) and handling
special case when there is no permanent redirect for a host (2).

(1) helps to avoid incorrect predictions in scenario when the URL/host doesn't
have a permanent redirect and the predictor mistakenly uses the fallback to
fetching based on the first main frame url in the redirect chain.

(2) allows to ignore "megamorphic" URLs/hosts. For example, url shorteners.

BUG=631966

Review-Url: https://codereview.chromium.org/2800783002
Cr-Commit-Position: refs/heads/master@{#464103}
Committed:
https://chromium.googlesource.com/chromium/src/+/93cb7df1f935461d1b3c5544f0c7...
==========

[commit-bot: I haz the power, 2017-04-12 19:21:46]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/93cb7df1f935461d1b3c5544f0c7...