Clear Image Decode Cache on Navigation

Clear Image Decode Cache on Navigation

We can't reach cache entries after a page navigation, so we should
clear them (even if unlocked). In the unlocked case this will help
ensure the right things get evicted.

R=vmpstr@chromium.org
BUG=707847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2795803002
Cr-Commit-Position: refs/heads/master@{#461847}
Committed: https://chromium.googlesource.com/chromium/src/+/9e73df9aebdf6e7374791bc2d076b93ef7961eaa

[ericrk, 2017-04-03 18:10:13]

Description was changed from

==========
Clear Image Decode Cache on Navigation

We can't reach cache entries after a page navigation, so we should
clear them (even if unlocked). In the unlocked case this will help
ensure the right things get evicted.

R=vmpstr@chromium.org
BUG=707847
==========

to

==========
Clear Image Decode Cache on Navigation

We can't reach cache entries after a page navigation, so we should
clear them (even if unlocked). In the unlocked case this will help
ensure the right things get evicted.

R=vmpstr@chromium.org
BUG=707847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ericrk, 2017-04-03 18:10:37]

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

[commit-bot: I haz the power, 2017-04-03 18:11:24]

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

[vmpstr, 2017-04-03 18:46:11]

https://codereview.chromium.org/2795803002/diff/1/cc/tiles/gpu_image_decode_c...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2795803002/diff/1/cc/tiles/gpu_image_decode_c...
cc/tiles/gpu_image_decode_cache.cc:589: for (auto it =
persistent_cache_.rbegin(); it != persistent_cache_.rend();) {
Not that it matters, but can we make this loop forward instead of reverse, so
that it's clear that iteration order doesn't matter?

https://codereview.chromium.org/2795803002/diff/1/cc/tiles/software_image_dec...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2795803002/diff/1/cc/tiles/software_image_dec...
cc/tiles/software_image_decode_cache.cc:823: LOG(ERROR) << "Clearing " <<
decoded_images_.size() << " images.";
Debugging? Can this be a trace instead?

https://codereview.chromium.org/2795803002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2795803002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2347: void
LayerTreeHostImpl::ClearCachesOnNavigation() {
Maybe ClearImageCacheOnNavigation?

[commit-bot: I haz the power, 2017-04-03 19:40:18]

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

[commit-bot: I haz the power, 2017-04-03 19:40:18]

Dry run: This issue passed the CQ dry run.

[ericrk, 2017-04-03 22:58:54]

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

[commit-bot: I haz the power, 2017-04-03 22:59:47]

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, 2017-04-03 23:17:28]

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

[commit-bot: I haz the power, 2017-04-03 23:17:29]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[ericrk, 2017-04-04 00:26:03]

https://codereview.chromium.org/2795803002/diff/1/cc/tiles/gpu_image_decode_c...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2795803002/diff/1/cc/tiles/gpu_image_decode_c...
cc/tiles/gpu_image_decode_cache.cc:589: for (auto it =
persistent_cache_.rbegin(); it != persistent_cache_.rend();) {
On 2017/04/03 18:46:10, vmpstr wrote:
> Not that it matters, but can we make this loop forward instead of reverse, so
> that it's clear that iteration order doesn't matter?

yup - done.

https://codereview.chromium.org/2795803002/diff/1/cc/tiles/software_image_dec...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2795803002/diff/1/cc/tiles/software_image_dec...
cc/tiles/software_image_decode_cache.cc:823: LOG(ERROR) << "Clearing " <<
decoded_images_.size() << " images.";
On 2017/04/03 18:46:10, vmpstr wrote:
> Debugging? Can this be a trace instead?

my bad - left this in from debugging. removed.

https://codereview.chromium.org/2795803002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2795803002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2347: void
LayerTreeHostImpl::ClearCachesOnNavigation() {
On 2017/04/03 18:46:11, vmpstr wrote:
> Maybe ClearImageCacheOnNavigation?

I was thinking maybe we'd want to use it for other things in the future - but I
guess we can re-name it if/when that happens :D - changed.

[ericrk, 2017-04-04 00:26:10]

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

[commit-bot: I haz the power, 2017-04-04 00:26:29]

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

[vmpstr, 2017-04-04 00:43:18]

lgtm with a couple of nits

https://codereview.chromium.org/2795803002/diff/20001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache_unittest.cc (right):

https://codereview.chromium.org/2795803002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache_unittest.cc:1469: cache.ClearCache();
Is there anything we can query to check if the cache is indeed empty? Maybe a
ForTesting function that just returns the number of items or something?

https://codereview.chromium.org/2795803002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2795803002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2348: if (image_decode_cache_) {
nit: no braces

[commit-bot: I haz the power, 2017-04-04 00:56:22]

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

[commit-bot: I haz the power, 2017-04-04 00:56:23]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[ericrk, 2017-04-04 17:17:18]

https://codereview.chromium.org/2795803002/diff/20001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache_unittest.cc (right):

https://codereview.chromium.org/2795803002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache_unittest.cc:1469: cache.ClearCache();
On 2017/04/04 00:43:17, vmpstr wrote:
> Is there anything we can query to check if the cache is indeed empty? Maybe a
> ForTesting function that just returns the number of items or something?

yup - meant to ask you if you were OK with adding this :P - sounds like you are.

https://codereview.chromium.org/2795803002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2795803002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2348: if (image_decode_cache_) {
On 2017/04/04 00:43:18, vmpstr wrote:
> nit: no braces

Done.

[ericrk, 2017-04-04 17:17:23]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2017-04-04 17:17:24]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2795803002/#ps40001
(title: "nits")

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

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-04-04 17:40:39]

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

[commit-bot: I haz the power, 2017-04-04 17:40:40]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ericrk, 2017-04-04 18:03:50]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:04:27]

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-04-04 19:14:12]

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

[commit-bot: I haz the power, 2017-04-04 19:14:13]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ericrk, 2017-04-04 20:14:00]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2017-04-04 20:14:00]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2795803002/#ps60001
(title: "fix conversion")

[commit-bot: I haz the power, 2017-04-04 20:14:57]

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-04-04 21:38:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491336840606750,
"parent_rev": "ac0f205c4e0e4924b7bbcab69dea918a5d6f267e", "commit_rev":
"9e73df9aebdf6e7374791bc2d076b93ef7961eaa"}

[commit-bot: I haz the power, 2017-04-04 21:39:47]

Description was changed from

==========
Clear Image Decode Cache on Navigation

We can't reach cache entries after a page navigation, so we should
clear them (even if unlocked). In the unlocked case this will help
ensure the right things get evicted.

R=vmpstr@chromium.org
BUG=707847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Clear Image Decode Cache on Navigation

We can't reach cache entries after a page navigation, so we should
clear them (even if unlocked). In the unlocked case this will help
ensure the right things get evicted.

R=vmpstr@chromium.org
BUG=707847
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2795803002
Cr-Commit-Position: refs/heads/master@{#461847}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e73df9aebdf6e7374791bc2d076...
==========

[commit-bot: I haz the power, 2017-04-04 21:39:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9e73df9aebdf6e7374791bc2d076...