Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(86)

Issue 2353033003: Idle cleanup for worker context (Closed)

Created:
4 years, 3 months ago by ericrk
Modified:
4 years, 2 months ago
Reviewers:
danakj, jbauman
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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+256 lines, -44 lines) Patch
M cc/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M cc/output/context_cache_controller.h View 1 2 3 4 4 chunks +42 lines, -14 lines 0 comments Download
M cc/output/context_cache_controller.cc View 1 2 3 4 4 chunks +113 lines, -10 lines 0 comments Download
M cc/output/context_cache_controller_unittest.cc View 1 2 3 chunks +65 lines, -6 lines 0 comments Download
M cc/output/context_provider.h View 4 chunks +5 lines, -12 lines 0 comments Download
A cc/output/context_provider.cc View 1 chunk +26 lines, -0 lines 0 comments Download
M content/common/gpu/client/context_provider_command_buffer.cc View 1 chunk +4 lines, -2 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 66 (45 generated)
ericrk
4 years, 3 months ago (2016-09-20 20:36:26 UTC) #9
danakj
LGTM https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cache_controller.h File cc/output/context_cache_controller.h (right): https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cache_controller.h#newcode92 cc/output/context_cache_controller.h:92: base::CancelableCallback<void(uint32_t idle_generation)> idle_callback_; I think you just want ...
4 years, 3 months ago (2016-09-22 00:01:26 UTC) #17
ericrk
+jbauman for content/common/gpu owners. Thanks! https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cache_controller.h File cc/output/context_cache_controller.h (right): https://codereview.chromium.org/2353033003/diff/120001/cc/output/context_cache_controller.h#newcode92 cc/output/context_cache_controller.h:92: base::CancelableCallback<void(uint32_t idle_generation)> idle_callback_; On ...
4 years, 3 months ago (2016-09-22 00:28:59 UTC) #19
jbauman
https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cache_controller.cc File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cache_controller.cc#newcode36 cc/output/context_cache_controller.cc:36: weak_factory_(this) {} You need to do weak_factory_.GetWeakPtr() in the ...
4 years, 3 months ago (2016-09-22 02:13:43 UTC) #24
ericrk
Great points! thanks for the feedback. Let me know how this looks. https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cache_controller.cc File cc/output/context_cache_controller.cc ...
4 years, 2 months ago (2016-09-22 18:35:39 UTC) #26
ericrk
https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cache_controller.cc File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/140001/cc/output/context_cache_controller.cc#newcode36 cc/output/context_cache_controller.cc:36: weak_factory_(this) {} On 2016/09/22 18:35:39, ericrk wrote: > On ...
4 years, 2 months ago (2016-09-22 18:40:51 UTC) #27
danakj
Thanks for the good comments john, LGTM more https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cache_controller.cc File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cache_controller.cc#newcode37 cc/output/context_cache_controller.cc:37: weak_ptr_ ...
4 years, 2 months ago (2016-09-22 18:56:35 UTC) #30
ericrk
https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cache_controller.cc File cc/output/context_cache_controller.cc (right): https://codereview.chromium.org/2353033003/diff/180001/cc/output/context_cache_controller.cc#newcode37 cc/output/context_cache_controller.cc:37: weak_ptr_ = weak_factory_.GetWeakPtr(); On 2016/09/22 18:56:34, danakj wrote: > ...
4 years, 2 months ago (2016-09-22 19:08:18 UTC) #31
jbauman
lgtm
4 years, 2 months ago (2016-09-22 22:36:46 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2353033003/200001
4 years, 2 months ago (2016-09-22 22:38:46 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2353033003/200001
4 years, 2 months ago (2016-09-22 23:14:48 UTC) #45
commit-bot: I haz the power
Committed patchset #5 (id:200001)
4 years, 2 months ago (2016-09-22 23:20:46 UTC) #46
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/051ae83bf29b52cefd82235ebfb90f203912afbb Cr-Commit-Position: refs/heads/master@{#420496}
4 years, 2 months ago (2016-09-22 23:22:23 UTC) #48
ericrk
A revert of this CL (patchset #5 id:200001) has been created in https://codereview.chromium.org/2366033002/ by ericrk@chromium.org. ...
4 years, 2 months ago (2016-09-23 21:36:42 UTC) #49
ericrk
Re-landing now that layout test issues have been addressed.
4 years, 2 months ago (2016-09-26 20:52:25 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2353033003/200001
4 years, 2 months ago (2016-09-26 22:45:05 UTC) #57
commit-bot: I haz the power
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_chromeos_rel_ng/builds/284518)
4 years, 2 months ago (2016-09-26 23:37:15 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2353033003/200001
4 years, 2 months ago (2016-09-26 23:46:43 UTC) #61
commit-bot: I haz the power
Committed patchset #5 (id:200001)
4 years, 2 months ago (2016-09-27 03:35:59 UTC) #63
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/6714c23b526197220d8b9abdbd38b6d9c206d912 Cr-Commit-Position: refs/heads/master@{#421092}
4 years, 2 months ago (2016-09-27 03:38:17 UTC) #65
brianderson
4 years, 2 months ago (2016-09-30 22:47:52 UTC) #66
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 ..

Powered by Google App Engine
This is Rietveld 408576698