Revalidate using the same Resource, attempt #2

Revalidate using the same Resource, attempt #2

Currently, when we perform a revalidation, we create a new resource and put it in the MemoryCache in place of the resource being revalidated. If the revalidation results in a 304, we silently swap all of the clients of the revalidation to the now-revalidated resource and swap which Resource is in the MemoryCache.

This changes the behavior to use the existing resource to revalidate itself, which simplifies the logic quite a bit. It also gives us the option of removing ResourcePtr (which is used to implement the silent client swap in the 304 case) and make Resources simply RefCounted in a future change.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199145

[Nate Chapin, 2015-07-16 23:35:26]

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

[Nate Chapin, 2015-07-16 23:35:27]

https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource.cpp
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource....
Source/core/fetch/Resource.cpp:291: m_revalidatingRequest = ResourceRequest();
The only difference between this and the original CL is that this was an early
return. I misread the logic of what we did for failed revalidations before this
change and incorrectly replicated it. Currently, we replace the old, valid
resource with the revalidating resource, and when the revalidating resource
fails, we evict it from the cache and there is no resource for the url in the
cache when we are done.

This patch now replicates that behavior, except the part where we remove the
valid resource from the cache and replace it with a new one. We just keep the
same resource, use it for the revalidation, and evict it on failure.

[Mike West, 2015-07-17 07:27:51]

LGTM if the Windows failures are flakes. It's tough to tell.

[Yoav Weiss, 2015-07-17 08:57:03]

https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource.cpp
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource....
Source/core/fetch/Resource.cpp:291: m_revalidatingRequest = ResourceRequest();
On 2015/07/16 23:35:27, Nate Chapin wrote:
> The only difference between this and the original CL is that this was an early
> return. I misread the logic of what we did for failed revalidations before
this
> change and incorrectly replicated it. Currently, we replace the old, valid
> resource with the revalidating resource, and when the revalidating resource
> fails, we evict it from the cache and there is no resource for the url in the
> cache when we are done.
> 
> This patch now replicates that behavior, except the part where we remove the
> valid resource from the cache and replace it with a new one. We just keep the
> same resource, use it for the revalidation, and evict it on failure.

Do we have tests that caught this logic change?

[Nate Chapin, 2015-07-17 17:00:10]

On 2015/07/17 08:57:03, Yoav Weiss wrote:
>
https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource.cpp
> File Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource....
> Source/core/fetch/Resource.cpp:291: m_revalidatingRequest = ResourceRequest();
> On 2015/07/16 23:35:27, Nate Chapin wrote:
> > The only difference between this and the original CL is that this was an
early
> > return. I misread the logic of what we did for failed revalidations before
> this
> > change and incorrectly replicated it. Currently, we replace the old, valid
> > resource with the revalidating resource, and when the revalidating resource
> > fails, we evict it from the cache and there is no resource for the url in
the
> > cache when we are done.
> > 
> > This patch now replicates that behavior, except the part where we remove the
> > valid resource from the cache and replace it with a new one. We just keep
the
> > same resource, use it for the revalidation, and evict it on failure.
> 
> Do we have tests that caught this logic change?

We caught it via unexpected memory leaks, I didn't see any other tests fail.

[Nate Chapin, 2015-07-17 17:10:28]

On 2015/07/17 17:00:10, Nate Chapin wrote:
> On 2015/07/17 08:57:03, Yoav Weiss wrote:
> >
>
https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource.cpp
> > File Source/core/fetch/Resource.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource....
> > Source/core/fetch/Resource.cpp:291: m_revalidatingRequest =
ResourceRequest();
> > On 2015/07/16 23:35:27, Nate Chapin wrote:
> > > The only difference between this and the original CL is that this was an
> early
> > > return. I misread the logic of what we did for failed revalidations before
> > this
> > > change and incorrectly replicated it. Currently, we replace the old, valid
> > > resource with the revalidating resource, and when the revalidating
resource
> > > fails, we evict it from the cache and there is no resource for the url in
> the
> > > cache when we are done.
> > > 
> > > This patch now replicates that behavior, except the part where we remove
the
> > > valid resource from the cache and replace it with a new one. We just keep
> the
> > > same resource, use it for the revalidation, and evict it on failure.
> > 
> > Do we have tests that caught this logic change?
> 
> We caught it via unexpected memory leaks, I didn't see any other tests fail.

I'm going to go ahead and land this. I'm trying to figure out how I would test
this without creating something too brittle, let me know if you have ideas.

[Nate Chapin, 2015-07-17 17:10:35]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-07-17 17:11:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1237983003/1

[Yoav Weiss, 2015-07-17 21:41:26]

On 2015/07/17 17:10:28, Nate Chapin wrote:
> On 2015/07/17 17:00:10, Nate Chapin wrote:
> > On 2015/07/17 08:57:03, Yoav Weiss wrote:
> > >
> >
>
https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource.cpp
> > > File Source/core/fetch/Resource.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1237983003/diff/1/Source/core/fetch/Resource....
> > > Source/core/fetch/Resource.cpp:291: m_revalidatingRequest =
> ResourceRequest();
> > > On 2015/07/16 23:35:27, Nate Chapin wrote:
> > > > The only difference between this and the original CL is that this was an
> > early
> > > > return. I misread the logic of what we did for failed revalidations
before
> > > this
> > > > change and incorrectly replicated it. Currently, we replace the old,
valid
> > > > resource with the revalidating resource, and when the revalidating
> resource
> > > > fails, we evict it from the cache and there is no resource for the url
in
> > the
> > > > cache when we are done.
> > > > 
> > > > This patch now replicates that behavior, except the part where we remove
> the
> > > > valid resource from the cache and replace it with a new one. We just
keep
> > the
> > > > same resource, use it for the revalidation, and evict it on failure.
> > > 
> > > Do we have tests that caught this logic change?
> > 
> > We caught it via unexpected memory leaks, I didn't see any other tests fail.
> 
> I'm going to go ahead and land this. I'm trying to figure out how I would test
> this without creating something too brittle, let me know if you have ideas.

Regarding the clearing from cache parts, I guess that's what the mem bots are
for :)

I'm thinking if and how the rest can be tested, but not sure. I'll let you know
if I'd think of something

[commit-bot: I haz the power, 2015-07-18 00:57:35]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199145

[Nate Chapin, 2015-07-29 18:01:52]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1269563003/ by japhet@chromium.org.

The reason for reverting is: This caused several different regressions that are
not quick and easy to fix.

https://code.google.com/p/chromium/issues/detail?id=512459
https://code.google.com/p/chromium/issues/detail?id=512405
https://code.google.com/p/chromium/issues/detail?id=511939
https://code.google.com/p/chromium/issues/detail?id=513572.