Avoid MemoryCache eviction of preloaded resources with devtools open

Avoid MemoryCache eviction of preloaded resources with devtools open

Currently, InspectorNetworkAgent::didCommitLoad is evicting all
resources from MemoryCache, including ones that were preloaded
using headers just before that, which results in double downloads
when devtools are open.

This CL fixes that, by making sure that this eviction spares unused
preloads.

BUG=652187
Committed: https://crrev.com/f78da02239eef5039ce480e8620d30a136918a74
Cr-Commit-Position: refs/heads/master@{#424907}

[Yoav Weiss, 2016-10-03 21:04:00]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 21:04:59]

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

[Yoav Weiss, 2016-10-03 21:07:48]

Description was changed from

==========
Avoid MemoryCache eviction of preloaded resources with devtools open

BUG=652187
==========

to

==========
Avoid MemoryCache eviction of preloaded resources with devtools open

Currently, InspectorNetworkAgent::didCommitLoad is evicting all 
resources from MemoryCache, including ones that were preloaded
using headers just before that, which results in double downloads
when devtools are open.

This CL fixes that, by making sure that this eviction spares unused
preloads.

BUG=652187
==========

[Yoav Weiss, 2016-10-03 21:07:48]

yoav@yoav.ws changed reviewers:
+ csharrison@chromium.org

[Yoav Weiss, 2016-10-03 21:15:16]

Hey Charlie,

I finally decided to take the route you suggested and avoid evicting unused
preloads, as separating the 2 didCommitLoad() calls in
FrameLoader::receivedFirstData() seems too magical...

Can you take a first look?

I still need to add tests...

[Charlie Harrison, 2016-10-03 21:48:21]

https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:639: if (policy ==
EvictAllResources || !resourceIter->value->resource() ||
Optional nit: I think !(resourceIter->value->resource() &&
resourceIter->value->resource()->isUnusedPreload()) is clearer as an explicit
null check.

https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:642: resourceIter =
resources->begin();
Hm... If we have N unused preloads, and then M regular resources, wouldn't this
loop have N * M iterations? I wonder if we can do something smarter about
incrementing the iterator so it doesn't just go back to the beginning. 

 Can we increment the iterator before we evict the resource? I don't think
iterators to other elements in a map are invalidated on erasure for STL maps,
but I'm not sure about WTF.

[commit-bot: I haz the power, 2016-10-03 22:23:13]

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

[commit-bot: I haz the power, 2016-10-03 22:23:14]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-10-04 08:15:20]

On 2016/10/03 21:48:21, Charlie Harrison wrote:
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):
> 
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/fetch/MemoryCache.cpp:639: if (policy ==
> EvictAllResources || !resourceIter->value->resource() ||
> Optional nit: I think !(resourceIter->value->resource() &&
> resourceIter->value->resource()->isUnusedPreload()) is clearer as an explicit
> null check.
> 
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/fetch/MemoryCache.cpp:642: resourceIter =
> resources->begin();
> Hm... If we have N unused preloads, and then M regular resources, wouldn't
this
> loop have N * M iterations? I wonder if we can do something smarter about
> incrementing the iterator so it doesn't just go back to the beginning. 
> 
>  Can we increment the iterator before we evict the resource? I don't think
> iterators to other elements in a map are invalidated on erasure for STL maps,
> but I'm not sure about WTF.

Unfortunately, that's not true for WTF (at least not for the maps used in
MemoryCache).
I agree regarding the current approach's inefficiency. I thought about
optimizing that by accumulating these entries in a vector, evicting them and
adding them later on.
However, since this code is only run one time per navigation, and only in
devtools, that seemed like a slight overkill. But since it's bugging you and we
don't know who would use that code in the future, I'm adding such a mechanism.

OTOH, I'm not adding a similar mechanism for the resource maps iteration, as
header based preloads would only be in the first one.

[Yoav Weiss, 2016-10-04 08:33:10]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 08:33:17]

Dry run: 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, 2016-10-04 10:09:16]

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

[commit-bot: I haz the power, 2016-10-04 10:09:17]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-10-04 12:48:30]

On 2016/10/04 08:15:20, Yoav Weiss wrote:
> On 2016/10/03 21:48:21, Charlie Harrison wrote:
> >
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/fetch/MemoryCache.cpp:639: if (policy ==
> > EvictAllResources || !resourceIter->value->resource() ||
> > Optional nit: I think !(resourceIter->value->resource() &&
> > resourceIter->value->resource()->isUnusedPreload()) is clearer as an
explicit
> > null check.
> > 
> >
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/fetch/MemoryCache.cpp:642: resourceIter =
> > resources->begin();
> > Hm... If we have N unused preloads, and then M regular resources, wouldn't
> this
> > loop have N * M iterations? I wonder if we can do something smarter about
> > incrementing the iterator so it doesn't just go back to the beginning. 
> > 
> >  Can we increment the iterator before we evict the resource? I don't think
> > iterators to other elements in a map are invalidated on erasure for STL
maps,
> > but I'm not sure about WTF.
> 
> Unfortunately, that's not true for WTF (at least not for the maps used in
> MemoryCache).
> I agree regarding the current approach's inefficiency. I thought about
> optimizing that by accumulating these entries in a vector, evicting them and
> adding them later on.
> However, since this code is only run one time per navigation, and only in
> devtools, that seemed like a slight overkill. But since it's bugging you and
we
> don't know who would use that code in the future, I'm adding such a mechanism.
> 
> OTOH, I'm not adding a similar mechanism for the resource maps iteration, as
> header based preloads would only be in the first one.

Sorry, I didn't mean to say you must change this code. Honestly I'm not sure
this way is faster than the previous iteration approach in real world scenarios.
In PS 1 we just iterate a lot more through a HashMap while this one suffers from
needlessly deleting and adding entries to the hash maps and vector.

I'm happy with this code, but I'm also happy with PS 1 with a comment about how
iterators will be invalidated if we try anything trickier.

[Yoav Weiss, 2016-10-07 09:47:37]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 09:47:52]

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

[Yoav Weiss, 2016-10-07 09:58:04]

On 2016/10/04 12:48:30, Charlie Harrison wrote:
> On 2016/10/04 08:15:20, Yoav Weiss wrote:
> > On 2016/10/03 21:48:21, Charlie Harrison wrote:
> > >
> >
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/fetch/MemoryCache.cpp:639: if (policy ==
> > > EvictAllResources || !resourceIter->value->resource() ||
> > > Optional nit: I think !(resourceIter->value->resource() &&
> > > resourceIter->value->resource()->isUnusedPreload()) is clearer as an
> explicit
> > > null check.
> > > 
> > >
> >
>
https://codereview.chromium.org/2385313002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/fetch/MemoryCache.cpp:642: resourceIter =
> > > resources->begin();
> > > Hm... If we have N unused preloads, and then M regular resources, wouldn't
> > this
> > > loop have N * M iterations? I wonder if we can do something smarter about
> > > incrementing the iterator so it doesn't just go back to the beginning. 
> > > 
> > >  Can we increment the iterator before we evict the resource? I don't think
> > > iterators to other elements in a map are invalidated on erasure for STL
> maps,
> > > but I'm not sure about WTF.
> > 
> > Unfortunately, that's not true for WTF (at least not for the maps used in
> > MemoryCache).
> > I agree regarding the current approach's inefficiency. I thought about
> > optimizing that by accumulating these entries in a vector, evicting them and
> > adding them later on.
> > However, since this code is only run one time per navigation, and only in
> > devtools, that seemed like a slight overkill. But since it's bugging you and
> we
> > don't know who would use that code in the future, I'm adding such a
mechanism.
> > 
> > OTOH, I'm not adding a similar mechanism for the resource maps iteration, as
> > header based preloads would only be in the first one.
> 
> Sorry, I didn't mean to say you must change this code. Honestly I'm not sure
> this way is faster than the previous iteration approach in real world
scenarios.
> In PS 1 we just iterate a lot more through a HashMap while this one suffers
from
> needlessly deleting and adding entries to the hash maps and vector.

I agree it's not clear which one is faster in real life. Personally, I doubt
there's a measurable difference for most pages. At least in this version, the
complexity is O(n).
If we'll see it being a bottleneck in the future, I can revise.

> 
> I'm happy with this code, but I'm also happy with PS 1 with a comment about
how
> iterators will be invalidated if we try anything trickier.

Added a test which makes sure only 1 resource is loaded. PTAL?

[commit-bot: I haz the power, 2016-10-07 11:00:22]

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

[commit-bot: I haz the power, 2016-10-07 11:00:23]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-10-10 20:56:25]

friendly ping :)

[Charlie Harrison, 2016-10-10 21:00:21]

Sorry, I thought I had reviewed this already. non-owner LGTM.

https://codereview.chromium.org/2385313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/2385313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/MemoryCache.cpp:651: resourceIter =
resources->begin();
Optional: Can this update step go into the for loop?

[Yoav Weiss, 2016-10-10 21:14:35]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 21:15:12]

Dry run: 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, 2016-10-10 22:42:25]

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

[commit-bot: I haz the power, 2016-10-10 22:42:25]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-10-11 07:58:42]

yoav@yoav.ws changed reviewers:
+ japhet@chromium.org

[Yoav Weiss, 2016-10-11 07:58:42]

Thanks Charlie! :)

Nate - care to take a look?

[Nate Chapin, 2016-10-12 22:26:42]

lgtm

[Yoav Weiss, 2016-10-12 22:28:07]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2016-10-12 22:28:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2385313002/#ps60001
(title: "review comment")

[commit-bot: I haz the power, 2016-10-12 22:29: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, 2016-10-12 23:44:08]

Description was changed from

==========
Avoid MemoryCache eviction of preloaded resources with devtools open

Currently, InspectorNetworkAgent::didCommitLoad is evicting all 
resources from MemoryCache, including ones that were preloaded
using headers just before that, which results in double downloads
when devtools are open.

This CL fixes that, by making sure that this eviction spares unused
preloads.

BUG=652187
==========

to

==========
Avoid MemoryCache eviction of preloaded resources with devtools open

Currently, InspectorNetworkAgent::didCommitLoad is evicting all 
resources from MemoryCache, including ones that were preloaded
using headers just before that, which results in double downloads
when devtools are open.

This CL fixes that, by making sure that this eviction spares unused
preloads.

BUG=652187
==========

[commit-bot: I haz the power, 2016-10-12 23:44:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-12 23:47:04]

Description was changed from

==========
Avoid MemoryCache eviction of preloaded resources with devtools open

Currently, InspectorNetworkAgent::didCommitLoad is evicting all 
resources from MemoryCache, including ones that were preloaded
using headers just before that, which results in double downloads
when devtools are open.

This CL fixes that, by making sure that this eviction spares unused
preloads.

BUG=652187
==========

to

==========
Avoid MemoryCache eviction of preloaded resources with devtools open

Currently, InspectorNetworkAgent::didCommitLoad is evicting all
resources from MemoryCache, including ones that were preloaded
using headers just before that, which results in double downloads
when devtools are open.

This CL fixes that, by making sure that this eviction spares unused
preloads.

BUG=652187

Committed: https://crrev.com/f78da02239eef5039ce480e8620d30a136918a74
Cr-Commit-Position: refs/heads/master@{#424907}
==========

[commit-bot: I haz the power, 2016-10-12 23:47:05]

Patchset 4 (id:??) landed as
https://crrev.com/f78da02239eef5039ce480e8620d30a136918a74
Cr-Commit-Position: refs/heads/master@{#424907}