predictors: Add resource type to manifest.

predictors: Add resource type to manifest.

This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.

Also we prioritize stylesheets above scripts since this CL. Stylesheets are 
almost always critical, scripts are not. It's clear win in the case if we're 
fetching resources from manifests because we don't have information about 
request priority in manifests. Thus we can't separate critical scripts from 
non-critical ones.

BUG=699115
Review-Url: https://codereview.chromium.org/2831233004
Cr-Commit-Position: refs/heads/master@{#467742}
Committed: https://chromium.googlesource.com/chromium/src/+/62ffefbb0a2dcd9d0ff0a1a12288577c766af408

[alexilin, 2017-04-21 12:07:27]

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

[alexilin, 2017-04-21 12:07:28]

Hi Benoit! PTAL.

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (left):

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:318: for (auto&
kv : *data_map) {
Resources are written to the database already sorted. There is no sense to
resort them when loading.

[alexilin, 2017-04-25 08:39:53]

ping lizeb.com

PING lizeb.com (184.168.221.51) 56(84) bytes of data.
64 bytes from ip-184-168-221-51.ip.secureserver.net (184.168.221.51): icmp_seq=1
ttl=50 time=164 ms
64 bytes from ip-184-168-221-51.ip.secureserver.net (184.168.221.51): icmp_seq=3
ttl=50 time=164 ms
^C
--- lizeb.com ping statistics ---
7 packets transmitted, 2 received, 71% packet loss, time 6031ms

[Benoit L, 2017-04-25 08:43:16]

On 2017/04/25 08:39:53, alexilin wrote:
> ping http://lizeb.com
> 
> PING http://lizeb.com (184.168.221.51) 56(84) bytes of data.
> 64 bytes from http://ip-184-168-221-51.ip.secureserver.net (184.168.221.51):
icmp_seq=1
> ttl=50 time=164 ms
> 64 bytes from http://ip-184-168-221-51.ip.secureserver.net (184.168.221.51):
icmp_seq=3
> ttl=50 time=164 ms
> ^C
> --- http://lizeb.com ping statistics ---
> 7 packets transmitted, 2 received, 71% packet loss, time 6031ms

That's a fairly representative packet loss!

[Benoit L, 2017-04-25 08:59:21]

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

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:1001: return
ResourcePrefetchPredictorTables::ComputePrecacheResourceScore(
nit: use a local "using" to make this a bit easier to read?

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (left):

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:318: for (auto&
kv : *data_map) {
On 2017/04/21 12:07:28, alexilin wrote:
> Resources are written to the database already sorted. There is no sense to
> resort them when loading.

Unless the sorting function changes, right?

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

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:449: switch
(resource.type()) {
Can we make that consistent with the other scoring function without duplication?
Since these are separate enums, it may cost more than what it brings (for
instance, converting types to:
(is_blink_resource_type << 16 + resource_type), and matching several cases)...
Not sure about that.

However, it would be best to have the same resource type ordering.

Also, can you add a comment explaining that this means a strict ordering, since
the weight ratio is in [0, 1)?

[alexilin, 2017-04-25 13:04:26]

A couple of questions before I send the next patch.

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

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:1001: return
ResourcePrefetchPredictorTables::ComputePrecacheResourceScore(
On 2017/04/25 08:59:21, Benoit L wrote:
> nit: use a local "using" to make this a bit easier to read?

C++ doesn't have using declaration for static class methods. I could use
function pointer alias, do you think it's better?

float (&compute_score)(const precache::PrecacheResource&) = 
    ResourcePrefetchPredictorTables::ComputePrecacheResourceScore;
return compute_score(*x) > compute_score(*y);

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (left):

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:318: for (auto&
kv : *data_map) {
On 2017/04/25 08:59:21, Benoit L wrote:
> On 2017/04/21 12:07:28, alexilin wrote:
> > Resources are written to the database already sorted. There is no sense to
> > resort them when loading.
> 
> Unless the sorting function changes, right?

We could return this statement back if sorting function changes. There was no
such a change since the last version bumping.
If you want to keep it, then we need to add the similar statement for the
origins table because it also keeps entries sorted.

[Benoit L, 2017-04-25 15:12:39]

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

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:1001: return
ResourcePrefetchPredictorTables::ComputePrecacheResourceScore(
On 2017/04/25 13:04:25, alexilin wrote:
> On 2017/04/25 08:59:21, Benoit L wrote:
> > nit: use a local "using" to make this a bit easier to read?
> 
> C++ doesn't have using declaration for static class methods. I could use
> function pointer alias, do you think it's better?
> 
> float (&compute_score)(const precache::PrecacheResource&) = 
>     ResourcePrefetchPredictorTables::ComputePrecacheResourceScore;
> return compute_score(*x) > compute_score(*y);

Ah, too bad. Let's keep it simple then.

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (left):

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:318: for (auto&
kv : *data_map) {
On 2017/04/25 13:04:25, alexilin wrote:
> On 2017/04/25 08:59:21, Benoit L wrote:
> > On 2017/04/21 12:07:28, alexilin wrote:
> > > Resources are written to the database already sorted. There is no sense to
> > > resort them when loading.
> > 
> > Unless the sorting function changes, right?
> 
> We could return this statement back if sorting function changes. There was no
> such a change since the last version bumping.
> If you want to keep it, then we need to add the similar statement for the
> origins table because it also keeps entries sorted.

Acknowledged.

[alexilin, 2017-04-25 15:47:58]

Description was changed from

==========
predictors: Add resource type to manifest.

This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.

BUG=699115
==========

to

==========
predictors: Add resource type to manifest.

This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.
Also we prioritize stylesheets above scripts since this CL.

BUG=699115
==========

[alexilin, 2017-04-25 17:21:04]

Description was changed from

==========
predictors: Add resource type to manifest.

This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.
Also we prioritize stylesheets above scripts since this CL.

BUG=699115
==========

to

==========
This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.

Also we prioritize stylesheets above scripts since this CL. Stylesheets are 
almost always critical, scripts are not. It's clear win in the case if we're 
fetching resources from manifests because we don't have information about 
request priority in manifests. Thus we can't separate critical scripts from 
non-critical ones.

BUG=699115
==========

[alexilin, 2017-04-25 17:21:34]

Description was changed from

==========
This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.

Also we prioritize stylesheets above scripts since this CL. Stylesheets are 
almost always critical, scripts are not. It's clear win in the case if we're 
fetching resources from manifests because we don't have information about 
request priority in manifests. Thus we can't separate critical scripts from 
non-critical ones.

BUG=699115
==========

to

==========
predictors: Add resource type to manifest.

This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.

Also we prioritize stylesheets above scripts since this CL. Stylesheets are 
almost always critical, scripts are not. It's clear win in the case if we're 
fetching resources from manifests because we don't have information about 
request priority in manifests. Thus we can't separate critical scripts from 
non-critical ones.

BUG=699115
==========

[alexilin, 2017-04-25 17:37:05]

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

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:1001: return
ResourcePrefetchPredictorTables::ComputePrecacheResourceScore(
On 2017/04/25 15:12:39, Benoit L wrote:
> On 2017/04/25 13:04:25, alexilin wrote:
> > On 2017/04/25 08:59:21, Benoit L wrote:
> > > nit: use a local "using" to make this a bit easier to read?
> > 
> > C++ doesn't have using declaration for static class methods. I could use
> > function pointer alias, do you think it's better?
> > 
> > float (&compute_score)(const precache::PrecacheResource&) = 
> >     ResourcePrefetchPredictorTables::ComputePrecacheResourceScore;
> > return compute_score(*x) > compute_score(*y);
> 
> Ah, too bad. Let's keep it simple then.

Acknowledged.

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

https://codereview.chromium.org/2831233004/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:449: switch
(resource.type()) {
On 2017/04/25 08:59:21, Benoit L wrote:
> Can we make that consistent with the other scoring function without
duplication?
> Since these are separate enums, it may cost more than what it brings (for
> instance, converting types to:
> (is_blink_resource_type << 16 + resource_type), and matching several cases)...
> Not sure about that.
> 
> However, it would be best to have the same resource type ordering.
> 
> Also, can you add a comment explaining that this means a strict ordering,
since
> the weight ratio is in [0, 1)?

Done.

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

https://codereview.chromium.org/2831233004/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:99: case
predictors::ResourceData::RESOURCE_TYPE_FONT_RESOURCE:
Actually, I'm also thinking about prioritizing fonts above scripts. Rationale:
Resource Fetch Prioritization in Chromium
https://docs.google.com/document/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuM....
Not preloaded fonts have higher priority than any script.
Again, there is almost no changes for non-manifest db but it'd make difference
for manifests.

[alexilin, 2017-04-26 16:29:36]

Friendly ping.

[Benoit L, 2017-04-27 08:54:15]

lgtm

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

https://codereview.chromium.org/2831233004/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:99: case
predictors::ResourceData::RESOURCE_TYPE_FONT_RESOURCE:
On 2017/04/25 17:37:05, alexilin wrote:
> Actually, I'm also thinking about prioritizing fonts above scripts. Rationale:
> Resource Fetch Prioritization in Chromium
>
https://docs.google.com/document/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuM....
> Not preloaded fonts have higher priority than any script.
> Again, there is almost no changes for non-manifest db but it'd make difference
> for manifests.

Well, this is because we start fetching font at the point where we needed them
yesterday.
Here we are fetching stuff ahead of time, and we will surely need CSS and
blocking scripts before the font, since we cannot reach layout before executing
these scripts.

https://codereview.chromium.org/2831233004/diff/20001/components/precache/cor...
File components/precache/core/proto/precache.proto (right):

https://codereview.chromium.org/2831233004/diff/20001/components/precache/cor...
components/precache/core/proto/precache.proto:42: optional Type type = 5;
Does the precache code need to be updated in any way?

[alexilin, 2017-04-27 15:32:24]

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

https://codereview.chromium.org/2831233004/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:99: case
predictors::ResourceData::RESOURCE_TYPE_FONT_RESOURCE:
On 2017/04/27 08:54:15, Benoit L wrote:
> On 2017/04/25 17:37:05, alexilin wrote:
> > Actually, I'm also thinking about prioritizing fonts above scripts.
Rationale:
> > Resource Fetch Prioritization in Chromium
> >
>
https://docs.google.com/document/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuM....
> > Not preloaded fonts have higher priority than any script.
> > Again, there is almost no changes for non-manifest db but it'd make
difference
> > for manifests.
> 
> Well, this is because we start fetching font at the point where we needed them
> yesterday.
> Here we are fetching stuff ahead of time, and we will surely need CSS and
> blocking scripts before the font, since we cannot reach layout before
executing
> these scripts.

Agree. Thanks for the explanation!

https://codereview.chromium.org/2831233004/diff/20001/components/precache/cor...
File components/precache/core/proto/precache.proto (right):

https://codereview.chromium.org/2831233004/diff/20001/components/precache/cor...
components/precache/core/proto/precache.proto:42: optional Type type = 5;
On 2017/04/27 08:54:15, Benoit L wrote:
> Does the precache code need to be updated in any way?

I don't think so. This field was in real manifests for a while as unknown_field
and declaring it explicitly in .proto shouldn't change anything.

[alexilin, 2017-04-27 15:32:45]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2017-04-27 15:33:21]

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-27 17:09:13]

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

[commit-bot: I haz the power, 2017-04-27 17:09:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alexilin, 2017-04-27 17:12:09]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2017-04-27 17:12:51]

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-27 19:08:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493313129335040,
"parent_rev": "4a9e4e47495a1c039b1948309bb45bdb8a88f6b5", "commit_rev":
"62ffefbb0a2dcd9d0ff0a1a12288577c766af408"}

[commit-bot: I haz the power, 2017-04-27 19:08:57]

Description was changed from

==========
predictors: Add resource type to manifest.

This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.

Also we prioritize stylesheets above scripts since this CL. Stylesheets are 
almost always critical, scripts are not. It's clear win in the case if we're 
fetching resources from manifests because we don't have information about 
request priority in manifests. Thus we can't separate critical scripts from 
non-critical ones.

BUG=699115
==========

to

==========
predictors: Add resource type to manifest.

This CL adds a new resource type field to the manifest proto.
ResourcePrefetchPredictor begins to use this field to prefetch all resources of
one resource type before another.

Also we prioritize stylesheets above scripts since this CL. Stylesheets are 
almost always critical, scripts are not. It's clear win in the case if we're 
fetching resources from manifests because we don't have information about 
request priority in manifests. Thus we can't separate critical scripts from 
non-critical ones.

BUG=699115

Review-Url: https://codereview.chromium.org/2831233004
Cr-Commit-Position: refs/heads/master@{#467742}
Committed:
https://chromium.googlesource.com/chromium/src/+/62ffefbb0a2dcd9d0ff0a1a12288...
==========

[commit-bot: I haz the power, 2017-04-27 19:08:58]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/62ffefbb0a2dcd9d0ff0a1a12288...