Separate preload matching from MemoryCache

Separate preload matching from MemoryCache

This is a reland of https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf2441e7f0686d53
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this change,
preload matching is done for ResourceFetcher::preloads_.

This is the first step and part of the second step in the design document[1].

1: https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_dWM/edit#


BUG=652228
Review-Url: https://codereview.chromium.org/2871233002
Cr-Commit-Position: refs/heads/master@{#472421}
Committed: https://chromium.googlesource.com/chromium/src/+/37589655a8b0ab6436390749e63cfb18a770f314

[yhirano, 2017-05-10 14:46:44]

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

[commit-bot: I haz the power, 2017-05-10 14:47:33]

Dry run: CQ is trying da patch.

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

[yhirano, 2017-05-10 14:48:23]

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

[commit-bot: I haz the power, 2017-05-10 14:48:37]

Dry run: CQ is trying da patch.

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

[yhirano, 2017-05-10 14:50:20]

Description was changed from

==========
reland

BUG=
==========

to

==========
Separate preaload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::prealods_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=695939
==========

[yhirano, 2017-05-10 14:50:20]

yhirano@chromium.org changed reviewers:
+ japhet@chromium.org

[yhirano, 2017-05-10 14:55:08]

Hi Nate, can you take a look again?

PS1 is the rebased original CL, and PS2 is imported from [1]: it keeps matched
preloads until ClearPreloads is called, in order to avoid MemoryCache eviction.
It looks [1] recovers TTFCP and TTFMP regressed by the original CL (see [2],
[3]).

1: https://codereview.chromium.org/2873073003/
2:
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bise...
3:
https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h...

[commit-bot: I haz the power, 2017-05-10 16:49:41]

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

[commit-bot: I haz the power, 2017-05-10 16:49:43]

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

[yhirano, 2017-05-11 08:14:19]

Description was changed from

==========
Separate preaload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::prealods_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=695939
==========

to

==========
Separate preaload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::prealods_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=652228
==========

[yhirano, 2017-05-12 15:09:02]

On 2017/05/10 14:55:08, yhirano wrote:
> Hi Nate, can you take a look again?
> 
> PS1 is the rebased original CL, and PS2 is imported from [1]: it keeps matched
> preloads until ClearPreloads is called, in order to avoid MemoryCache
eviction.
> It looks [1] recovers TTFCP and TTFMP regressed by the original CL (see [2],
> [3]).
> 
> 1: https://codereview.chromium.org/2873073003/
> 2:
>
https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bise...
> 3:
>
https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h...

ping

[Yoav Weiss, 2017-05-12 16:33:57]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2017-05-12 16:33:57]

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1027: if
(!IsReusableForPreloading(fetch_params, existing_resource, is_static_data))
Can we maybe change the name of `isReusableForPreloading`, as it's applied to to
all resources?

[Yoav Weiss, 2017-05-12 16:53:46]

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:804:
GetMemoryCache()->Add(resource);
Won't this add preloaded resources also to the memoryCache?

[kinuko, 2017-05-15 02:09:03]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-05-15 02:09:04]

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h (right):

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h:15: // PreloadKey
is a key type of the prealods map in a fetch group (a.k.a.
prealods -> preloads

Here, in the issue title and description.

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1027: if
(!IsReusableForPreloading(fetch_params, existing_resource, is_static_data))
On 2017/05/12 16:33:57, Yoav Weiss wrote:
> Can we maybe change the name of `isReusableForPreloading`, as it's applied to
to
> all resources?

+1, felt slightly confusing.

[yhirano, 2017-05-16 11:05:36]

Description was changed from

==========
Separate preaload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::prealods_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=652228
==========

to

==========
Separate preload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::prelods_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=652228
==========

[yhirano, 2017-05-16 11:05:51]

Description was changed from

==========
Separate preload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::prelods_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=652228
==========

to

==========
Separate preload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::preloads_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=652228
==========

[yhirano, 2017-05-16 11:08:40]

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

[commit-bot: I haz the power, 2017-05-16 11:08:53]

Dry run: CQ is trying da patch.

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

[yhirano, 2017-05-16 11:19:24]

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h (right):

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h:15: // PreloadKey
is a key type of the prealods map in a fetch group (a.k.a.
On 2017/05/15 02:09:04, kinuko wrote:
> prealods -> preloads
> 
> Here, in the issue title and description.

Done.

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:804:
GetMemoryCache()->Add(resource);
On 2017/05/12 16:53:46, Yoav Weiss wrote:
> Won't this add preloaded resources also to the memoryCache?

Yes, this CL changes how to match preloads, and doesn't change how to store
resources to MemoryCache.

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1027: if
(!IsReusableForPreloading(fetch_params, existing_resource, is_static_data))
On 2017/05/15 02:09:04, kinuko wrote:
> On 2017/05/12 16:33:57, Yoav Weiss wrote:
> > Can we maybe change the name of `isReusableForPreloading`, as it's applied
to
> to
> > all resources?
> 
> +1, felt slightly confusing.

I couldn't come up with a better name. Do you have any recommendation?

[commit-bot: I haz the power, 2017-05-16 11:58:42]

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

[commit-bot: I haz the power, 2017-05-16 11:58:43]

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

[kinuko, 2017-05-16 22:58:53]

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1027: if
(!IsReusableForPreloading(fetch_params, existing_resource, is_static_data))
IsReusableAlsoForPreloading ...?  (I'm not good at naming either)

[yhirano, 2017-05-17 04:23:47]

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

[yhirano, 2017-05-17 04:24:44]

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
(right):

https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1027: if
(!IsReusableForPreloading(fetch_params, existing_resource, is_static_data))
On 2017/05/16 22:58:53, kinuko wrote:
> IsReusableAlsoForPreloading ...?  (I'm not good at naming either)

Done.

[commit-bot: I haz the power, 2017-05-17 04:24:54]

Dry run: CQ is trying da patch.

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

[Yoav Weiss, 2017-05-17 05:11:04]

On 2017/05/16 11:19:24, yhirano wrote:
>
https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h (right):
> 
>
https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/loader/fetch/PreloadKey.h:15: // PreloadKey
> is a key type of the prealods map in a fetch group (a.k.a.
> On 2017/05/15 02:09:04, kinuko wrote:
> > prealods -> preloads
> > 
> > Here, in the issue title and description.
> 
> Done.
> 
>
https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
> (right):
> 
>
https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:804:
> GetMemoryCache()->Add(resource);
> On 2017/05/12 16:53:46, Yoav Weiss wrote:
> > Won't this add preloaded resources also to the memoryCache?
> 
> Yes, this CL changes how to match preloads, and doesn't change how to store
> resources to MemoryCache.

OK

> 
>
https://codereview.chromium.org/2871233002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1027: if
> (!IsReusableForPreloading(fetch_params, existing_resource, is_static_data))
> On 2017/05/15 02:09:04, kinuko wrote:
> > On 2017/05/12 16:33:57, Yoav Weiss wrote:
> > > Can we maybe change the name of `isReusableForPreloading`, as it's applied
> to
> > to
> > > all resources?
> > 
> > +1, felt slightly confusing.
> 
> I couldn't come up with a better name. Do you have any recommendation?

LGTM

[kinuko, 2017-05-17 06:20:55]

lgtm

[yhirano, 2017-05-17 06:24:04]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2017-05-17 06:24:04]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2017-05-17 06:29:14]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2017-05-17 06:30:01]

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-05-17 11:12:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495002554744250,
"parent_rev": "4a998cb6d11cbf87bd22cf32b99819fd61416be4", "commit_rev":
"37589655a8b0ab6436390749e63cfb18a770f314"}

[commit-bot: I haz the power, 2017-05-17 11:12:42]

Description was changed from

==========
Separate preload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::preloads_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=652228
==========

to

==========
Separate preload matching from MemoryCache

This is a reland of
https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf24...
which is reverted due to performance regressions.

This change decouples preload matching from MemoryCache lookup. With this
change,
preload matching is done for ResourceFetcher::preloads_.

This is the first step and part of the second step in the design document[1].

1:
https://docs.google.com/document/d/1oq8ixPnaDxuAlKUTRQ3WoYHlJenVkzNU9xnkuZX_d...


BUG=652228

Review-Url: https://codereview.chromium.org/2871233002
Cr-Commit-Position: refs/heads/master@{#472421}
Committed:
https://chromium.googlesource.com/chromium/src/+/37589655a8b0ab6436390749e63c...
==========

[commit-bot: I haz the power, 2017-05-17 11:12:44]

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