|
|
Created:
4 years, 3 months ago by ericrk Modified:
4 years, 2 months ago CC:
chromium-reviews, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, cc-bugs_chromium.org, danakj+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIdle cleanup for worker context
Currently, Skia flushes unused items out of its caches after ~1 second
of non-use (50 flushes). This works fine while Skia is in-use, but when
a worker context goes idle we stop calling into Skia altogether. This
means that Skia will never flush out unused cache items from the last
piece of work it did. This can lead to some rather large temporaries
being kept around (~20mb).
This change adds an idle cleanup process which flushes Skia's caches
and cleans up worker context resources after a worker context is idle
for 1 second.
BUG=624630
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel;master.tryserver.blink:linux_precise_blink_dbg
Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb
Committed: https://crrev.com/6714c23b526197220d8b9abdbd38b6d9c206d912
Cr-Original-Commit-Position: refs/heads/master@{#420496}
Cr-Commit-Position: refs/heads/master@{#421092}
Patch Set 1 : ++ #Patch Set 2 : fix windows build #
Total comments: 6
Patch Set 3 : nits and use WeakPtrFactory #
Total comments: 7
Patch Set 4 : Feedback and threading cleanup #
Total comments: 6
Patch Set 5 : comments + feedback #
Dependent Patchsets: Messages
Total messages: 66 (45 generated)
Description was changed from ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 ========== to ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
ericrk@chromium.org changed reviewers: + danakj@chromium.org
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by ericrk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... File cc/output/context_cache_controller.h (right): https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... cc/output/context_cache_controller.h:92: base::CancelableCallback<void(uint32_t idle_generation)> idle_callback_; I think you just want a WeakPtrFactory in this class instead of a callback wrapped in another callback with a WeakPtrFactory (maybe we always want that but I can't see any argument for using this here) https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... File cc/output/context_cache_controller_unittest.cc (right): https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... cc/output/context_cache_controller_unittest.cc:29: auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner()); nit: no() https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... cc/output/context_cache_controller_unittest.cc:43: auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner()); nitto
ericrk@chromium.org changed reviewers: + jbauman@chromium.org
+jbauman for content/common/gpu owners. Thanks! https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... File cc/output/context_cache_controller.h (right): https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... cc/output/context_cache_controller.h:92: base::CancelableCallback<void(uint32_t idle_generation)> idle_callback_; On 2016/09/22 00:01:25, danakj wrote: > I think you just want a WeakPtrFactory in this class instead of a callback > wrapped in another callback with a WeakPtrFactory (maybe we always want that but > I can't see any argument for using this here) Yeah - that is nicer - updated. https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... File cc/output/context_cache_controller_unittest.cc (right): https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... cc/output/context_cache_controller_unittest.cc:29: auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner()); On 2016/09/22 00:01:25, danakj wrote: > nit: no() Done. https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cach... cc/output/context_cache_controller_unittest.cc:43: auto task_runner = make_scoped_refptr(new base::TestMockTimeTaskRunner()); On 2016/09/22 00:01:25, danakj wrote: > nitto Done.
The CQ bit was checked by ericrk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... cc/output/context_cache_controller.cc:36: weak_factory_(this) {} You need to do weak_factory_.GetWeakPtr() in the constructor to ensure it's bound to the right thread so you don't hit DCHECKs. https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... cc/output/context_cache_controller.cc:101: base::TimeDelta::FromSeconds(kIdleCleanupDelaySeconds)); I think that when the context has a lock, that will mean that num_clients_busy always oscillates between 0 and 1. That means a lot of unnecessary tasks will be posted. It would probably be better to either have some timer slack (and not post a new task if one was posted in the last 0.5 seconds or something) or do something like base::Timer and have the last posted task post a new task if current_idle_generation_ was changed since it was posted. https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... cc/output/context_cache_controller.cc:106: std::unique_ptr<base::AutoLock> hold; This will unnecessarily grab the lock (and block the main thread) if work's currently happening. Maybe there should be a different lock just for current_idle_generation_. Or we could try using base::subtle::Atomic, though that's probably overkill.
Patchset #4 (id:160001) has been deleted
Great points! thanks for the feedback. Let me know how this looks. https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... cc/output/context_cache_controller.cc:36: weak_factory_(this) {} On 2016/09/22 02:13:43, jbauman wrote: > You need to do weak_factory_.GetWeakPtr() in the constructor to ensure it's > bound to the right thread so you don't hit DCHECKs. Hmm... Good point - If we're sure we always use this on the same thread, it shouldn't matter, but it can't hurt as a sanity check. https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... cc/output/context_cache_controller.cc:101: base::TimeDelta::FromSeconds(kIdleCleanupDelaySeconds)); On 2016/09/22 02:13:42, jbauman wrote: > I think that when the context has a lock, that will mean that num_clients_busy > always oscillates between 0 and 1. That means a lot of unnecessary tasks will be > posted. > > It would probably be better to either have some timer slack (and not post a new > task if one was posted in the last 0.5 seconds or something) or do something > like base::Timer and have the last posted task post a new task if > current_idle_generation_ was changed since it was posted. I like your second option I think - we don't need this to run fast, and this results in the minimal amount of unnecessary tasks. https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... cc/output/context_cache_controller.cc:106: std::unique_ptr<base::AutoLock> hold; On 2016/09/22 02:13:43, jbauman wrote: > This will unnecessarily grab the lock (and block the main thread) if work's > currently happening. Maybe there should be a different lock just for > current_idle_generation_. Or we could try using base::subtle::Atomic, though > that's probably overkill. Good point - actually meant to do something different here but forgot to get to it. I've re-worked a bit.
https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cach... cc/output/context_cache_controller.cc:36: weak_factory_(this) {} On 2016/09/22 18:35:39, ericrk wrote: > On 2016/09/22 02:13:43, jbauman wrote: > > You need to do weak_factory_.GetWeakPtr() in the constructor to ensure it's > > bound to the right thread so you don't hit DCHECKs. > > Hmm... Good point - If we're sure we always use this on the same thread, it > shouldn't matter, but it can't hurt as a sanity check. Ah, but we don't... yeah... I see what you mean.
The CQ bit was checked by ericrk@chromium.org 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...
Thanks for the good comments john, LGTM more https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cach... File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cach... cc/output/context_cache_controller.cc:37: weak_ptr_ = weak_factory_.GetWeakPtr(); pls explain with comment https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cach... cc/output/context_cache_controller.cc:117: callback_pending_ = true; scoped outside the lock plz https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cach... cc/output/context_cache_controller.cc:122: void ContextCacheController::PostIdleCallbackWithIdleGenerationLockAcquired() { pass in the id and you dont need to claim lock is held
https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cach... File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cach... cc/output/context_cache_controller.cc:37: weak_ptr_ = weak_factory_.GetWeakPtr(); On 2016/09/22 18:56:34, danakj wrote: > pls explain with comment Done. https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cach... cc/output/context_cache_controller.cc:117: callback_pending_ = true; On 2016/09/22 18:56:34, danakj wrote: > scoped outside the lock plz Done. https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cach... cc/output/context_cache_controller.cc:122: void ContextCacheController::PostIdleCallbackWithIdleGenerationLockAcquired() { On 2016/09/22 18:56:34, danakj wrote: > pass in the id and you dont need to claim lock is held Done.
The CQ bit was checked by ericrk@chromium.org 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...
lgtm
The CQ bit was unchecked by ericrk@chromium.org
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2353033003/#ps200001 (title: "comments + feedback")
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 ericrk@chromium.org
The CQ bit was checked by ericrk@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ericrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:200001) has been created in https://codereview.chromium.org/2366033002/ by ericrk@chromium.org. The reason for reverting is: This appears to be breaking some number of unittests - need to investigate further..
Message was sent while issue was closed.
Description was changed from ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496} ========== to ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496} ==========
Re-landing now that layout test issues have been addressed.
Description was changed from ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496} ========== to ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel;master.tryserver.blink:linux_precise_blink_dbg Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496} ==========
The CQ bit was checked by ericrk@chromium.org 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...
The CQ bit was unchecked by ericrk@chromium.org
The CQ bit was checked by ericrk@chromium.org
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 ericrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel;master.tryserver.blink:linux_precise_blink_dbg Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496} ========== to ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel;master.tryserver.blink:linux_precise_blink_dbg Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel;master.tryserver.blink:linux_precise_blink_dbg Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496} ========== to ========== Idle cleanup for worker context Currently, Skia flushes unused items out of its caches after ~1 second of non-use (50 flushes). This works fine while Skia is in-use, but when a worker context goes idle we stop calling into Skia altogether. This means that Skia will never flush out unused cache items from the last piece of work it did. This can lead to some rather large temporaries being kept around (~20mb). This change adds an idle cleanup process which flushes Skia's caches and cleans up worker context resources after a worker context is idle for 1 second. BUG=624630 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel;master.tryserver.blink:linux_precise_blink_dbg Committed: https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Committed: https://crrev.com/6714c23b526197220d8b9abdbd38b6d9c206d912 Cr-Original-Commit-Position: refs/heads/master@{#420496} Cr-Commit-Position: refs/heads/master@{#421092} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6714c23b526197220d8b9abdbd38b6d9c206d912 Cr-Commit-Position: refs/heads/master@{#421092}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:200001) has been created in https://codereview.chromium.org/2382443012/ by brianderson@chromium.org. The reason for reverting is: Speculative revert to see if it helps with crbug.com/645736 .. |