Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2945813002:
cc: Interface to evict images from cc image decode cache. (Closed)
Descriptioncc: Interface to evict images from cc image decode cache.
This adds interface to enable eviction of images from
image decode cache in cc. This CL sets up the interface
and eviction logic in gpu image decode cache. A follow
up would handle the sw cache eviction.
BUG=730784
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2945813002
Cr-Commit-Position: refs/heads/master@{#480961}
Committed: https://chromium.googlesource.com/chromium/src/+/676e8ccff70ea1c30674e8f97e38296bdd1af8fc
Patch Set 1 #Patch Set 2 : update #
Total comments: 10
Patch Set 3 : update #
Total comments: 2
Patch Set 4 : update #Patch Set 5 : rebase #
Messages
Total messages: 37 (20 generated)
Description was changed from ========== cc: [WIP] Interface to evict from cc image decode cache This adds interface to enable eviction of images from image decode cache in cc. BUG= ========== to ========== cc: [WIP] Interface to evict from cc image decode cache This adds interface to enable eviction of images from image decode cache in cc. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Patchset #2 (id:20001) has been deleted
sohan.jyoti@huawei.com changed reviewers: + vmpstr@chromium.org
PTAL, if this is what we wanted ? Thanks.
Description was changed from ========== cc: [WIP] Interface to evict from cc image decode cache This adds interface to enable eviction of images from image decode cache in cc. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: [WIP] Interface to evict from cc image decode cache This adds interface to enable eviction of images from image decode cache in cc. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
sohan.jyoti@huawei.com changed reviewers: + ericrk@chromium.org
Yes, this looks exactly like the change we need. Thanks! https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_cache.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_cache.cc:628: if (it != persistent_cache_.end() && !it->second->decode.is_locked()) { I _believe_ you can still mark things as orphaned (although you can't erase them) if they are locked. +ericrk to confirm. So, I think this looks fine but I think we can do slightly better by just reworking the if-conditions here. https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/image_controll... File cc/tiles/image_controller_unittest.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/image_controll... cc/tiles/image_controller_unittest.cc:132: void NotifyImageUnused(uint32_t skimage_id) override{}; space between override and {}. (or just run git cl format on the change) https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/software_image... File cc/tiles/software_image_decode_cache.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:857: // TODO(sohanjg) :Implement it. Can you reference a bug here?
https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_cache.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_cache.cc:628: if (it != persistent_cache_.end() && !it->second->decode.is_locked()) { On 2017/06/19 16:34:41, vmpstr wrote: > I _believe_ you can still mark things as orphaned (although you can't erase > them) if they are locked. +ericrk to confirm. So, I think this looks fine but I > think we can do slightly better by just reworking the if-conditions here. Agreed - you shouldn't need to check is_locked here: In the case where we have any refs, and you set is_orphaned, we can be locked, this won't cause problems. In the case where we have no refs, we must never be locked (you can DCHECK(!is_locked)). https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_cache_unittest.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_cache_unittest.cc:1717: TEST(GpuImageDecodeCacheTest, RemoveUnusedImage) { Not needed right now, but I wonder if we should move some number of tests to a "Common Image Decode Cache Test" file, where we can just parameterize the test by GPU/SW. Certainly for some tests we want to test per-cache behavior, but for many (this one), it would be nice to keep the test uniform.
PTAL. Thanks. https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_cache.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_cache.cc:628: if (it != persistent_cache_.end() && !it->second->decode.is_locked()) { On 2017/06/19 17:09:30, ericrk wrote: > On 2017/06/19 16:34:41, vmpstr wrote: > > I _believe_ you can still mark things as orphaned (although you can't erase > > them) if they are locked. +ericrk to confirm. So, I think this looks fine but > I > > think we can do slightly better by just reworking the if-conditions here. > > Agreed - you shouldn't need to check is_locked here: > > In the case where we have any refs, and you set is_orphaned, we can be locked, > this won't cause problems. > > In the case where we have no refs, we must never be locked (you can > DCHECK(!is_locked)). Done. https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_cache_unittest.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_cache_unittest.cc:1717: TEST(GpuImageDecodeCacheTest, RemoveUnusedImage) { On 2017/06/19 17:09:30, ericrk wrote: > Not needed right now, but I wonder if we should move some number of tests to a > "Common Image Decode Cache Test" file, where we can just parameterize the test > by GPU/SW. Certainly for some tests we want to test per-cache behavior, but for > many (this one), it would be nice to keep the test uniform. If we remove this test for future, we are essentially pushing in an un-tested code, will it be a good practice ? Maybe once we implement the evict logic in both the caches, we can start moving the common tests as you pointed. wdyt ? https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/image_controll... File cc/tiles/image_controller_unittest.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/image_controll... cc/tiles/image_controller_unittest.cc:132: void NotifyImageUnused(uint32_t skimage_id) override{}; On 2017/06/19 16:34:41, vmpstr wrote: > space between override and {}. (or just run git cl format on the change) Done. Oddly git cl format, removes the space instead of adding it :/ Did manually :) https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/software_image... File cc/tiles/software_image_decode_cache.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:857: // TODO(sohanjg) :Implement it. On 2017/06/19 16:34:42, vmpstr wrote: > Can you reference a bug here? Done.
Patchset #3 (id:60001) has been deleted
Description was changed from ========== cc: [WIP] Interface to evict from cc image decode cache This adds interface to enable eviction of images from image decode cache in cc. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Interface to evict images from cc image decode cache. This adds interface to enable eviction of images from image decode cache in cc. This CL sets up the interface and eviction logic in gpu image decode cache. A follow up would handle the sw cache eviction. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc: Interface to evict images from cc image decode cache. This adds interface to enable eviction of images from image decode cache in cc. This CL sets up the interface and eviction logic in gpu image decode cache. A follow up would handle the sw cache eviction. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Interface to evict images from cc image decode cache. This adds interface to enable eviction of images from image decode cache in cc. This CL sets up the interface and eviction logic in gpu image decode cache. A follow up would handle the sw cache eviction. BUG=730784 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
LGTM, thanks! https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_cache_unittest.cc (right): https://codereview.chromium.org/2945813002/diff/40001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_cache_unittest.cc:1717: TEST(GpuImageDecodeCacheTest, RemoveUnusedImage) { On 2017/06/20 13:31:39, sohan wrote: > On 2017/06/19 17:09:30, ericrk wrote: > > Not needed right now, but I wonder if we should move some number of tests to a > > "Common Image Decode Cache Test" file, where we can just parameterize the test > > by GPU/SW. Certainly for some tests we want to test per-cache behavior, but > for > > many (this one), it would be nice to keep the test uniform. > > If we remove this test for future, we are essentially pushing in an un-tested > code, will it be a good practice ? > > Maybe once we implement the evict logic in both the caches, we can start moving > the common tests as you pointed. wdyt ? SGTM, thanks
lgtm https://codereview.chromium.org/2945813002/diff/80001/cc/tiles/image_controll... File cc/tiles/image_controller_unittest.cc (right): https://codereview.chromium.org/2945813002/diff/80001/cc/tiles/image_controll... cc/tiles/image_controller_unittest.cc:132: void NotifyImageUnused(uint32_t skimage_id) override {}; I think format is confused because you have a trailing semicolon there. If you remove it it should do the right thing
PTAL. Thanks. https://codereview.chromium.org/2945813002/diff/80001/cc/tiles/image_controll... File cc/tiles/image_controller_unittest.cc (right): https://codereview.chromium.org/2945813002/diff/80001/cc/tiles/image_controll... cc/tiles/image_controller_unittest.cc:132: void NotifyImageUnused(uint32_t skimage_id) override {}; On 2017/06/20 17:10:23, vmpstr wrote: > I think format is confused because you have a trailing semicolon there. If you > remove it it should do the right thing Done. Ah! good catch :)
The CQ bit was checked by sohan.jyoti@huawei.com
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/2945813002/#ps100001 (title: "update")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sohan.jyoti@huawei.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/20 18:42:05, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) sorry, I landed a conflicting patch :/... rebase should be ~straightforward, but if you have any questions feel free to ping me.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/06/20 18:53:11, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Ahh..no problem. I'll try and rebase with sleepy eyes :)
On 2017/06/20 19:09:19, sohan wrote: > On 2017/06/20 18:53:11, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > Ahh..no problem. I'll try and rebase with sleepy eyes :) PTAL. Thanks.
The CQ bit was checked by sohan.jyoti@huawei.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still LGTM, thanks!
The CQ bit was unchecked by sohan.jyoti@huawei.com
The CQ bit was checked by sohan.jyoti@huawei.com
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/2945813002/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497988680011560,
"parent_rev": "d253eface79145f5ca57c1ed973d1970258ba798", "commit_rev":
"676e8ccff70ea1c30674e8f97e38296bdd1af8fc"}
Message was sent while issue was closed.
Description was changed from ========== cc: Interface to evict images from cc image decode cache. This adds interface to enable eviction of images from image decode cache in cc. This CL sets up the interface and eviction logic in gpu image decode cache. A follow up would handle the sw cache eviction. BUG=730784 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Interface to evict images from cc image decode cache. This adds interface to enable eviction of images from image decode cache in cc. This CL sets up the interface and eviction logic in gpu image decode cache. A follow up would handle the sw cache eviction. BUG=730784 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2945813002 Cr-Commit-Position: refs/heads/master@{#480961} Committed: https://chromium.googlesource.com/chromium/src/+/676e8ccff70ea1c30674e8f97e38... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/676e8ccff70ea1c30674e8f97e38... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
