Expire resources in ResourcePool after non-use

Evict resources from resource pool after timeout

This change adds a one second expiration to all resources in a
ResourcePool. Resource that have not been used within the
expiration period are deleted.

This eviction compliments (but doesn't replace) the hard-limit
based eviction that already occurs. We rely on freeing resources
to our hard-limits in TileManager, so we don't want to replace
that logic with something less deterministic.

BUG=520734, 224475
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/289d1f6ba870b30cbf4a19d114b2ee4fc23f893f
Cr-Commit-Position: refs/heads/master@{#345396}

[ericrk, 2015-08-20 16:25:36]

ericrk@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2015-08-20 21:43:37]

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:19: namespace {
nit: move this blank line below l.19 instead

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:22: }  // namespace
nit: blank line above this

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:71: evict_expired_resources_callback_ =
base::Bind(
Unlike one-copy where worker threads are in play, you can just inline  the bind
with the post task instead of doing it here.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:158:
pool_resource->set_last_usage(base::TimeTicks::Now());
Can we avoid calling Now() two times in this case. Once here and once in
scheduleevict..

Maybe take advantage of the fact that you know that we have evictable resources
here and if no eviction task is currently scheduled then this new unused task is
the one we should schedule an eviction task for.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:251:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
Should this task runner be passed to the ctor instead to be more consistent with
the rest of tile management code?

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:302: if (!busy_resources_.empty()) {
DCHECK(!busy_resources_.empty()) instead as we shouldn't call this unless we
have evictable resources.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.h:104: size_t max_unused_memory_usage_bytes_;
Let's remove all traces of unused usage based eviction in this cl as it's no
longer needed when adding this time based eviction.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.h:107: size_t unused_memory_usage_bytes_;
I think this member can also be removed.

[ericrk, 2015-08-21 12:48:21]

ericrk@chromium.org changed reviewers:
+ piman@chromium.org

[ericrk, 2015-08-21 12:48:23]

+piman for the (pretty straightforward) stuff under content.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:19: namespace {
On 2015/08/20 at 21:43:37, reveman wrote:
> nit: move this blank line below l.19 instead

I *think* I did what you meant? Tried to match other files.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:22: }  // namespace
On 2015/08/20 21:43:37, reveman wrote:
> nit: blank line above this

Done.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:71: evict_expired_resources_callback_ =
base::Bind(
On 2015/08/20 at 21:43:37, reveman wrote:
> Unlike one-copy where worker threads are in play, you can just inline  the
bind with the post task instead of doing it here.

ok, makes sense.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:158:
pool_resource->set_last_usage(base::TimeTicks::Now());
On 2015/08/20 at 21:43:37, reveman wrote:
> Can we avoid calling Now() two times in this case. Once here and once in
scheduleevict..
> 
> Maybe take advantage of the fact that you know that we have evictable
resources here and if no eviction task is currently scheduled then this new
unused task is the one we should schedule an eviction task for.

Ok, refactored so we now only call Now() once in this case and in the
re-scheduling case below.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:251:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2015/08/20 at 21:43:37, reveman wrote:
> Should this task runner be passed to the ctor instead to be more consistent
with the rest of tile management code?

Sure, we can do that.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.cc:302: if (!busy_resources_.empty()) {
On 2015/08/20 at 21:43:37, reveman wrote:
> DCHECK(!busy_resources_.empty()) instead as we shouldn't call this unless we
have evictable resources.

sounds good

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.h:104: size_t max_unused_memory_usage_bytes_;
On 2015/08/20 21:43:37, reveman wrote:
> Let's remove all traces of unused usage based eviction in this cl as it's no
> longer needed when adding this time based eviction.

Done.

https://codereview.chromium.org/1293873005/diff/60001/cc/resources/resource_p...
cc/resources/resource_pool.h:107: size_t unused_memory_usage_bytes_;
On 2015/08/20 21:43:37, reveman wrote:
> I think this member can also be removed.

This is still sort of nice to track - we trace this value in tile_manager for
instance, and I could see this being an UMA stat. What do you think?

[piman, 2015-08-21 16:43:04]

lgtm

[piman, 2015-08-21 16:46:52]

https://codereview.chromium.org/1293873005/diff/120001/cc/resources/resource_...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/1293873005/diff/120001/cc/resources/resource_...
cc/resources/resource_pool.cc:21: const int kResourceExpirationDelayMs = 1000;
Mmh, time-based processes are not great - they contribute to the
unpredictability of the browser, and bugs in the area become difficult to debug
(e.g. a breakpoint causes the delay to trigger).

What made us come up with 1s specifically?

[ericrk, 2015-08-24 17:06:35]

On 2015/08/21 at 16:46:52, piman wrote:
>
https://codereview.chromium.org/1293873005/diff/120001/cc/resources/resource_...
> File cc/resources/resource_pool.cc (right):
> 
>
https://codereview.chromium.org/1293873005/diff/120001/cc/resources/resource_...
> cc/resources/resource_pool.cc:21: const int kResourceExpirationDelayMs = 1000;
> Mmh, time-based processes are not great - they contribute to the
unpredictability of the browser, and bugs in the area become difficult to debug
(e.g. a breakpoint causes the delay to trigger).
> 
> What made us come up with 1s specifically?

No particular reason other than that this timeout matched the other scratch
memory pool in one copy and seemed long enough that it wouldn't accidentally
remove resources that were in frequent use. Can likely be shorter, but this
seemed like a safe starting point.

The other options are to trigger based on some signal (frame rendered? resource
acquired?). As we sometimes stop rendering frames (and want to clean up from
that rendering), that didn't seem ideal, so I went with time.

WDYT?

[reveman, 2015-08-24 17:28:01]

On 2015/08/24 at 17:06:35, ericrk wrote:
> On 2015/08/21 at 16:46:52, piman wrote:
> >
https://codereview.chromium.org/1293873005/diff/120001/cc/resources/resource_...
> > File cc/resources/resource_pool.cc (right):
> > 
> >
https://codereview.chromium.org/1293873005/diff/120001/cc/resources/resource_...
> > cc/resources/resource_pool.cc:21: const int kResourceExpirationDelayMs =
1000;
> > Mmh, time-based processes are not great - they contribute to the
unpredictability of the browser, and bugs in the area become difficult to debug
(e.g. a breakpoint causes the delay to trigger).
> > 
> > What made us come up with 1s specifically?
> 
> No particular reason other than that this timeout matched the other scratch
memory pool in one copy and seemed long enough that it wouldn't accidentally
remove resources that were in frequent use. Can likely be shorter, but this
seemed like a safe starting point.
> 
> The other options are to trigger based on some signal (frame rendered?
resource acquired?). As we sometimes stop rendering frames (and want to clean up
from that rendering), that didn't seem ideal, so I went with time.
> 
> WDYT?

One alternative that I considered for one-copy is to just release free memory at
"idle" time in the same way we release other types of free memory. See
RenderThreadImpl::IdleHandler. The problem with that was that it will sometimes
take a very long time before that is called and sometimes it would be called
when we actually needed the memory. When we have a better idle signal in place
then that might be a better approach but the 1 seconds lifetime mechanism turned
out to work much better in practice for now.

[ericrk, 2015-08-24 17:33:33]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-24 17:33:44]

The CQ bit was unchecked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-08-24 17:33:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293873005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293873005/120001

[ericrk, 2015-08-25 17:24:27]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-25 17:24:28]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1293873005/#ps160001
(title: "Fix unit test")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1293873005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1293873005/160001

[commit-bot: I haz the power, 2015-08-25 18:52:39]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-08-25 18:53:16]

Patchset 9 (id:??) landed as
https://crrev.com/289d1f6ba870b30cbf4a19d114b2ee4fc23f893f
Cr-Commit-Position: refs/heads/master@{#345396}