Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1897143002:
[CompositorWorker] Do not create WorkerBackingThread in shutdown sequence (Closed)
Description[CompositorWorker] Do not create WorkerBackingThread in shutdown sequence
Previously CompositorWorkerThread::clearSharedBackingThread() instantiated
a WebThreadSupportingGC just before destructing it if no one had used
a CompositorWorkerThread. This change removes the function, because the
thread is shutdown when the last compositor worker thread is gone and
leaving CompositorWorkerThread::m_thread is harmless.
BUG=575532
Committed: https://crrev.com/eb7ab820fa77af56e63697a296d2ea54698d39c3
Cr-Commit-Position: refs/heads/master@{#390050}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 28 (12 generated)
Description was changed from ========== [CompositorWorker] Do not create BackingThreadHolder during shutdown sequence. BUG=575532 ========== to ========== [CompositorWorker] Do not create WebThreadSupportingGC during shutdown sequence BUG=575532 ==========
Description was changed from ========== [CompositorWorker] Do not create WebThreadSupportingGC during shutdown sequence BUG=575532 ========== to ========== [CompositorWorker] Do not create WebThreadSupportingGC in shutdown sequence BUG=575532 ==========
Description was changed from ========== [CompositorWorker] Do not create WebThreadSupportingGC in shutdown sequence BUG=575532 ========== to ========== [CompositorWorker] Do not create WebThreadSupportingGC in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change stops creating a WebThreadSupportingGC in such a case. BUG=575532 ==========
yhirano@chromium.org changed reviewers: + vollick@chromium.org
This looks a bit nasty. As I commented in the WorkerBackingThread CL, would it be possible to stop using a singleton for BackingThreadHolder? Instead we can create a BackingThreadHolder when # of CompositorThreads goes up to 1 and destructs the BackingThreadHolder when # of CompositorThreads goes down to 0.
On 2016/04/20 04:14:00, haraken wrote: > This looks a bit nasty. > > As I commented in the WorkerBackingThread CL, would it be possible to stop using > a singleton for BackingThreadHolder? Instead we can create a BackingThreadHolder > when # of CompositorThreads goes up to 1 and destructs the BackingThreadHolder > when # of CompositorThreads goes down to 0. When the second CompositorThreads is created, we need to find the existing BackingThreadHolder. How can we do that without a singleton?
Description was changed from ========== [CompositorWorker] Do not create WebThreadSupportingGC in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change stops creating a WebThreadSupportingGC in such a case. BUG=575532 ========== to ========== [CompositorWorker] Do not create WorkerBackingThread in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change stops creating a WorkerBackingThread in such a case. BUG=575532 ==========
On 2016/04/20 07:41:54, yhirano wrote: > On 2016/04/20 04:14:00, haraken wrote: > > This looks a bit nasty. > > > > As I commented in the WorkerBackingThread CL, would it be possible to stop > using > > a singleton for BackingThreadHolder? Instead we can create a > BackingThreadHolder > > when # of CompositorThreads goes up to 1 and destructs the BackingThreadHolder > > when # of CompositorThreads goes down to 0. > > When the second CompositorThreads is created, we need to find the existing > BackingThreadHolder. How can we do that without a singleton? Why not add a conditional here to see if an instance has already been created? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Something like this, maybe? if (BackingThreadHolder::hasInstance()) BackingThreadHolder::instance().clear();
On 2016/04/20 15:43:47, vollick wrote: > On 2016/04/20 07:41:54, yhirano wrote: > > On 2016/04/20 04:14:00, haraken wrote: > > > This looks a bit nasty. > > > > > > As I commented in the WorkerBackingThread CL, would it be possible to stop > > using > > > a singleton for BackingThreadHolder? Instead we can create a > > BackingThreadHolder > > > when # of CompositorThreads goes up to 1 and destructs the > BackingThreadHolder > > > when # of CompositorThreads goes down to 0. > > > > When the second CompositorThreads is created, we need to find the existing > > BackingThreadHolder. How can we do that without a singleton? > > Why not add a conditional here to see if an instance has already been created? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Something like this, maybe? > > if (BackingThreadHolder::hasInstance()) > BackingThreadHolder::instance().clear(); It's possible, but since CompositorWorkerThread::workerBackingThread() is accessed from both the main thread and the compositor worker thread, we need a mutex (See PS2). I wanted to avoid that, but I'm fine with that if you like it.
On 2016/04/21 02:19:50, yhirano wrote: > On 2016/04/20 15:43:47, vollick wrote: > > On 2016/04/20 07:41:54, yhirano wrote: > > > On 2016/04/20 04:14:00, haraken wrote: > > > > This looks a bit nasty. > > > > > > > > As I commented in the WorkerBackingThread CL, would it be possible to stop > > > using > > > > a singleton for BackingThreadHolder? Instead we can create a > > > BackingThreadHolder > > > > when # of CompositorThreads goes up to 1 and destructs the > > BackingThreadHolder > > > > when # of CompositorThreads goes down to 0. > > > > > > When the second CompositorThreads is created, we need to find the existing > > > BackingThreadHolder. How can we do that without a singleton? haraken@, please correct me if I'm wrong, but I interpreted your comment to be more about how we managed the lifetime of the backing thread than about it being a global. Before, https://codereview.chromium.org/1733353004/, we'd had a singleton in CompositorWorkerThread that managed a count and we'd tear down or create the backing thread based on this count. I think he may have been suggesting something similar so that we could avoid needing to clear the holder via ModulesInitializer::shutdown? > > > > Why not add a conditional here to see if an instance has already been created? > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Something like this, maybe? > > > > if (BackingThreadHolder::hasInstance()) > > BackingThreadHolder::instance().clear(); > > It's possible, but since CompositorWorkerThread::workerBackingThread() is > accessed from both the main thread and the compositor worker thread, we need a > mutex (See PS2). I wanted to avoid that, but I'm fine with that if you like it. Ah right, thank you. I agree that this is worse than your original patch set.
Uploaded PS3 which is equivalent to PS1.
vollick@chromium.org changed reviewers: + haraken@chromium.org
On 2016/04/22 09:01:17, yhirano wrote: > Uploaded PS3 which is equivalent to PS1. lgtm % haraken
(Sorry about the late reply...) The complexity comes from the fact that BackingThreadHolder::instance() implicitly creates a thread. Would it be possible to introduce: - BackingThreadHolder::instance().getThread() (which creates a thread if it isn't yet created.) - BackingThreadHolder::instance().clearThread() (which clears a thread if it is created.) ?
On 2016/04/26 08:16:12, haraken wrote: > (Sorry about the late reply...) > > The complexity comes from the fact that BackingThreadHolder::instance() > implicitly creates a thread. > > Would it be possible to introduce: > > - BackingThreadHolder::instance().getThread() (which creates a thread if it > isn't yet created.) > - BackingThreadHolder::instance().clearThread() (which clears a thread if it is > created.) > > ? I think it will be similar to PS2. We need one more mutex to protect the thread instance. Do you like that? Is it better than PS2? One another option is stop clearing BackingThreadHolder. Assuming the thread is appropriately shutdown, actually we don't need to call BackingThreadHolder::clear. What do you think about the idea?
On 2016/04/26 11:21:50, yhirano wrote: > On 2016/04/26 08:16:12, haraken wrote: > > (Sorry about the late reply...) > > > > The complexity comes from the fact that BackingThreadHolder::instance() > > implicitly creates a thread. > > > > Would it be possible to introduce: > > > > - BackingThreadHolder::instance().getThread() (which creates a thread if it > > isn't yet created.) > > - BackingThreadHolder::instance().clearThread() (which clears a thread if it > is > > created.) > > > > ? > > I think it will be similar to PS2. We need one more mutex to protect the thread > instance. Do you like that? Is it better than PS2? > One another option is stop clearing BackingThreadHolder. Assuming the thread is > appropriately shutdown, actually we don't need to call > BackingThreadHolder::clear. What do you think about the idea? Yeah, this sounds nicer. Adding an ASSERT would be enough to check the assumption.
On 2016/04/26 11:23:48, haraken wrote: > On 2016/04/26 11:21:50, yhirano wrote: > > On 2016/04/26 08:16:12, haraken wrote: > > > (Sorry about the late reply...) > > > > > > The complexity comes from the fact that BackingThreadHolder::instance() > > > implicitly creates a thread. > > > > > > Would it be possible to introduce: > > > > > > - BackingThreadHolder::instance().getThread() (which creates a thread if it > > > isn't yet created.) > > > - BackingThreadHolder::instance().clearThread() (which clears a thread if it > > is > > > created.) > > > > > > ? > > > > I think it will be similar to PS2. We need one more mutex to protect the > thread > > instance. Do you like that? Is it better than PS2? > > One another option is stop clearing BackingThreadHolder. Assuming the thread > is > > appropriately shutdown, actually we don't need to call > > BackingThreadHolder::clear. What do you think about the idea? > > Yeah, this sounds nicer. Adding an ASSERT would be enough to check the > assumption. Uploaded PS5. I'm not sure what you mean by adding ASSERT. Doesn't such assertions have the same problem?
On 2016/04/27 10:43:37, yhirano wrote: > On 2016/04/26 11:23:48, haraken wrote: > > On 2016/04/26 11:21:50, yhirano wrote: > > > On 2016/04/26 08:16:12, haraken wrote: > > > > (Sorry about the late reply...) > > > > > > > > The complexity comes from the fact that BackingThreadHolder::instance() > > > > implicitly creates a thread. > > > > > > > > Would it be possible to introduce: > > > > > > > > - BackingThreadHolder::instance().getThread() (which creates a thread if > it > > > > isn't yet created.) > > > > - BackingThreadHolder::instance().clearThread() (which clears a thread if > it > > > is > > > > created.) > > > > > > > > ? > > > > > > I think it will be similar to PS2. We need one more mutex to protect the > > thread > > > instance. Do you like that? Is it better than PS2? > > > One another option is stop clearing BackingThreadHolder. Assuming the thread > > is > > > appropriately shutdown, actually we don't need to call > > > BackingThreadHolder::clear. What do you think about the idea? > > > > Yeah, this sounds nicer. Adding an ASSERT would be enough to check the > > assumption. > > Uploaded PS5. I'm not sure what you mean by adding ASSERT. Doesn't such > assertions have the same problem? Yeah, right. LGTM.
Description was changed from ========== [CompositorWorker] Do not create WorkerBackingThread in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change stops creating a WorkerBackingThread in such a case. BUG=575532 ========== to ========== [CompositorWorker] Do not create WorkerBackingThread in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change removes the function, because the thread is shutdown when the last compositor worker thread is gone and leaving CompositorWorkerThread::m_thread is harmless. BUG=575532 ==========
The CQ bit was checked by yhirano@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org Link to the patchset: https://codereview.chromium.org/1897143002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1897143002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897143002/80001
yhirano@chromium.org changed reviewers: - yhirano@google.com
Message was sent while issue was closed.
Description was changed from ========== [CompositorWorker] Do not create WorkerBackingThread in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change removes the function, because the thread is shutdown when the last compositor worker thread is gone and leaving CompositorWorkerThread::m_thread is harmless. BUG=575532 ========== to ========== [CompositorWorker] Do not create WorkerBackingThread in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change removes the function, because the thread is shutdown when the last compositor worker thread is gone and leaving CompositorWorkerThread::m_thread is harmless. BUG=575532 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [CompositorWorker] Do not create WorkerBackingThread in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change removes the function, because the thread is shutdown when the last compositor worker thread is gone and leaving CompositorWorkerThread::m_thread is harmless. BUG=575532 ========== to ========== [CompositorWorker] Do not create WorkerBackingThread in shutdown sequence Previously CompositorWorkerThread::clearSharedBackingThread() instantiated a WebThreadSupportingGC just before destructing it if no one had used a CompositorWorkerThread. This change removes the function, because the thread is shutdown when the last compositor worker thread is gone and leaving CompositorWorkerThread::m_thread is harmless. BUG=575532 Committed: https://crrev.com/eb7ab820fa77af56e63697a296d2ea54698d39c3 Cr-Commit-Position: refs/heads/master@{#390050} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/eb7ab820fa77af56e63697a296d2ea54698d39c3 Cr-Commit-Position: refs/heads/master@{#390050} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
