Evict resources from memory cache after running rendering test.

Evict resources from memory cache after running rendering test.

BUG=640344
TEST=Manual steps in bug.

Committed: https://crrev.com/c90a6ca1d464e103448621a244ebb93e1f7d440c
Cr-Commit-Position: refs/heads/master@{#414691}

[flackr, 2016-08-25 17:14:01]

flackr@chromium.org changed reviewers:
+ haraken@chromium.org

[flackr, 2016-08-25 17:14:01]

Can you take a look at this? Please also see my comments on the bug. It seems
dangerous that these RefPtr's are not necessarily cleared because they are still
in the memory caches memory (and were technically not destructed). Should we be
clearing all of the RefPtr's explicitly?

[flackr, 2016-08-25 18:00:27]

Description was changed from

==========
Explicitly remove reference to RefPtr on ResourceRequest.

BUG=640344
TEST=Manual steps in bug.
==========

to

==========
Evict resources from memory cache after running rendering test.

BUG=640344
TEST=Manual steps in bug.
==========

[flackr, 2016-08-25 20:03:46]

On 2016/08/25 at 17:14:01, flackr wrote:
> Can you take a look at this? Please also see my comments on the bug. It seems
dangerous that these RefPtr's are not necessarily cleared because they are still
in the memory caches memory (and were technically not destructed). Should we be
clearing all of the RefPtr's explicitly?

As I mentioned in the bug, it seems we just need to manually call
evictResources() after running a rendering test to ensure these resources are
cleaned up.

[haraken, 2016-08-26 00:58:50]

https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right):

https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:48:
memoryCache()->evictResources();

Just help me understand: What's a difference between prune() and
evictResource()? Is calling prune() not enough to release the underlying
Resources?

[flackr, 2016-08-26 01:17:03]

On 2016/08/26 at 00:58:50, haraken wrote:
>
https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right):
> 
>
https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:48:
memoryCache()->evictResources();
> 
> Just help me understand: What's a difference between prune() and
evictResource()? Is calling prune() not enough to release the underlying
Resources?

Correct, prune is what is called when the resource is no longer needed, and it
is exiting on the "fast path" in the test because the capacity has not been
reached:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Mem...

[haraken, 2016-08-26 01:43:33]

On 2016/08/26 01:17:03, flackr wrote:
> On 2016/08/26 at 00:58:50, haraken wrote:
> >
>
https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:48:
> memoryCache()->evictResources();
> > 
> > Just help me understand: What's a difference between prune() and
> evictResource()? Is calling prune() not enough to release the underlying
> Resources?
> 
> Correct, prune is what is called when the resource is no longer needed, and it
> is exiting on the "fast path" in the test because the capacity has not been
> reached:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Mem...

Then what's a difference between pruneAll() and evictResources()?

[hiroshige, 2016-08-26 04:58:55]

On 2016/08/26 01:43:33, haraken wrote:
> On 2016/08/26 01:17:03, flackr wrote:
> > On 2016/08/26 at 00:58:50, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:48:
> > memoryCache()->evictResources();
> > > 
> > > Just help me understand: What's a difference between prune() and
> > evictResource()? Is calling prune() not enough to release the underlying
> > Resources?
> > 
> > Correct, prune is what is called when the resource is no longer needed, and
it
> > is exiting on the "fast path" in the test because the capacity has not been
> > reached:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Mem...
> 
> Then what's a difference between pruneAll() and evictResources()?

pruneAll() doesn't remove references to "live" resources.
Also, prune-related things contain many heuristics and are not well-tested, so
I'd like to avoid calling pruneAll() in tests, unless we are testing pruneAll()
behavior itself.
evictResources() has clearer semantics ("clear all MemoryCache entries") and
looks more appropriate for this kind of teardown.

[haraken, 2016-08-26 04:59:54]

On 2016/08/26 04:58:55, hiroshige wrote:
> On 2016/08/26 01:43:33, haraken wrote:
> > On 2016/08/26 01:17:03, flackr wrote:
> > > On 2016/08/26 at 00:58:50, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2278723002/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/layout/LayoutTestHelper.cpp:48:
> > > memoryCache()->evictResources();
> > > > 
> > > > Just help me understand: What's a difference between prune() and
> > > evictResource()? Is calling prune() not enough to release the underlying
> > > Resources?
> > > 
> > > Correct, prune is what is called when the resource is no longer needed,
and
> it
> > > is exiting on the "fast path" in the test because the capacity has not
been
> > > reached:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Mem...
> > 
> > Then what's a difference between pruneAll() and evictResources()?
> 
> pruneAll() doesn't remove references to "live" resources.
> Also, prune-related things contain many heuristics and are not well-tested, so
> I'd like to avoid calling pruneAll() in tests, unless we are testing
pruneAll()
> behavior itself.
> evictResources() has clearer semantics ("clear all MemoryCache entries") and
> looks more appropriate for this kind of teardown.

Ah, thanks. That makes sense. LGTM.

[flackr, 2016-08-26 11:11:47]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-08-26 11:12:03]

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-08-26 12:34:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-26 12:35:27]

Description was changed from

==========
Evict resources from memory cache after running rendering test.

BUG=640344
TEST=Manual steps in bug.
==========

to

==========
Evict resources from memory cache after running rendering test.

BUG=640344
TEST=Manual steps in bug.

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

[commit-bot: I haz the power, 2016-08-26 12:35:28]

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