|
|
Created:
4 years, 7 months ago by flackr Modified:
4 years, 6 months ago CC:
blink-reviews, blink-worker-reviews_chromium.org, chromium-reviews, falken, horo+watch_chromium.org, kinuko+worker_chromium.org, nhiroki Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncompositor-worker: Keep worker backing thread alive for the lifetime of the compositor thread.
This change is to support creating and holding onto oilpan objects on the compositor thread
in https://codereview.chromium.org/1895873006
BUG=430155
TEST=CompositorWorkerThreadTest.*
Committed: https://crrev.com/05f2063bff785b913d17727ab5a4c2af206b6ea5
Cr-Commit-Position: refs/heads/master@{#396732}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Expand on comments. #
Total comments: 11
Patch Set 3 : Refactor WorkerBackingThread to isolate Compositor Worker behavior into separate subclass. #Patch Set 4 : Call attach and detach from BackingThreadHolder. #Patch Set 5 : Remove unneeded scheduler changes. #
Total comments: 15
Patch Set 6 : Address comments. #
Total comments: 16
Patch Set 7 : Call TerminateExecution manually, check threads, etc. #
Total comments: 5
Patch Set 8 : Address comments. #Patch Set 9 : Merge with master and only call TerminateExecution if initialization has occurred. #Patch Set 10 : Use the constructed WorkerBackingThread to post tasks. #
Messages
Total messages: 61 (14 generated)
flackr@chromium.org changed reviewers: + haraken@chromium.org, yhirano@chromium.org
This patch contains the changes to the compositor worker backing thread from https://codereview.chromium.org/1895873006 Please review, thanks!
https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:33: thread->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerBackingThread::initialize, AllowCrossThreadAccess(this))); Would you elaborate on why you need to post a task? https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:39: signalShutdownAndWait(); Would you help me understand why you need to call this in WorkerBackingThread's destructor? (i.e., isn't there any more explicit place you can shutdown the backing thrad?) In general, it's not a good idea to do complicated stuff in destructors (like posting tasks). https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/ModulesInitializer.cpp:67: CompositorWorkerThread::clearSharedBackingThread(); Backing threads of other workers are shut down in WorkerThread::terminateAndWaitForAllWorkers(). Can we shut down the backing thread of the compositor worker there as well? https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp:25: CompositorWorkerThread::ensureSharedBackingThread(); Move this just before worker->initialize? At least, it doesn't need to be initialized until we call worker->initialize. https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:25: // renderrer process. BackingThreadHolst::m_thread is cleared by renderer https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:43: if (s_hasBackingThreadHolder) Just to confirm: The access to s_hasBackingThreadHolder doesn't cause threading race because it's guaranteed that the BackingThreadHolder is instantiated before any compositor thread starts running and the BackingThreadHolder is destructed after all compositor threads finished, right? If my understanding is correct, it would be better to make s_hasBackingThreadHolder a member variable of BackingThreadHolder. Then the call site can just call instance().clear(). If my understanding is wrong, you need to address the threading race.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
Would you be able to give a bit more explanation about what you're trying to achieve by this CL? We've / yhirano's tried to cleanup the initialize/shutdown sequence by introducing WorkerBackingThread but this CL seems to be adding a bit different assumptions from what we had imagined.
Description was changed from ========== compositor-worker: Keep worker backing thread alive for the lifetime of the compositor thread. This change is to support creating and holding onto oilpan objects on the compositor thread in https://codereview.chromium.org/1895873006 BUG=430155 TEST=CompositorWorkerThreadTest.* ========== to ========== compositor-worker: Keep worker backing thread alive for the lifetime of the compositor thread. This change is to support creating and holding onto oilpan objects on the compositor thread in https://codereview.chromium.org/1895873006 BUG=430155 TEST=CompositorWorkerThreadTest.* ==========
On 2016/05/09 at 12:35:10, kinuko wrote: > Would you be able to give a bit more explanation about what you're trying to achieve by this CL? We've / yhirano's tried to cleanup the initialize/shutdown sequence by introducing WorkerBackingThread but this CL seems to be adding a bit different assumptions from what we had imagined. We need to have called ThreadState::attach before we can construct the CompositorMutatorImpl on the compositor thread where it is owned. Once this object has been created we keep it until we shut down the thread so we can't call ThreadState::detach until we are shutting in down (in ModulesInitializer::shutdown). You can see the construction of CompositorMutatorImpl in https://codereview.chromium.org/1895873006. I've avoided the redundant work during shutdown which yhirano cleaned up by checking whether we have a backing thread holder instance before calling instance() to clear it.
https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:33: thread->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerBackingThread::initialize, AllowCrossThreadAccess(this))); On 2016/05/09 at 02:00:53, haraken wrote: > Would you elaborate on why you need to post a task? Added a comment - we create WorkerBackingThread on main and initialize needs to be run on the thread to attach that thread. https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:39: signalShutdownAndWait(); On 2016/05/09 at 02:00:53, haraken wrote: > Would you help me understand why you need to call this in WorkerBackingThread's destructor? (i.e., isn't there any more explicit place you can shutdown the backing thrad?) > > In general, it's not a good idea to do complicated stuff in destructors (like posting tasks). Similar to initialization, the shutdown must be done on the thread. The WorkerBackingThread object must still exist in order to run the shutdown task, otherwise we try to access this destructed object. We could call signalShutdownAndWait (indirectly) from CompositorWorkerThread::clearSharedBackingThread to be explicit about it and just assert that it has been shut down in the destructor. https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/ModulesInitializer.cpp:67: CompositorWorkerThread::clearSharedBackingThread(); On 2016/05/09 at 02:00:53, haraken wrote: > Backing threads of other workers are shut down in WorkerThread::terminateAndWaitForAllWorkers(). Can we shut down the backing thread of the compositor worker there as well? This does make sense, however core/ is not allowed to include modules so we aren't allowed to get the CompositorWorkerThread's BackingThreadHolder instance to clear the thread pointer. This does suggest though that maybe we should clear the shared backing thread after calling CoreInitializer::shutdown call. https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp:25: CompositorWorkerThread::ensureSharedBackingThread(); On 2016/05/09 at 02:00:53, haraken wrote: > Move this just before worker->initialize? At least, it doesn't need to be initialized until we call worker->initialize. Unless I'm mistaken, we have to ensure the shared backing thread is created and an initialize task posted before calling worker->initialize because when we create the worker client we need to create CompositorMutatorImpl (GC'd class) on the compositor thread. This all happens before the worker is created so the thread needs to have been attached before we can do the worker initialization. You can see this at https://codereview.chromium.org/1895873006 in createCompositorProxyClient (called during worker initialization). I've added to the comment to explain this. https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:25: // renderrer process. BackingThreadHolst::m_thread is cleared by On 2016/05/09 at 02:00:53, haraken wrote: > renderer Done. https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:43: if (s_hasBackingThreadHolder) On 2016/05/09 at 02:00:53, haraken wrote: > Just to confirm: The access to s_hasBackingThreadHolder doesn't cause threading race because it's guaranteed that the BackingThreadHolder is instantiated before any compositor thread starts running and the BackingThreadHolder is destructed after all compositor threads finished, right? Right. > > If my understanding is correct, it would be better to make s_hasBackingThreadHolder a member variable of BackingThreadHolder. Then the call site can just call instance().clear(). > > If my understanding is wrong, you need to address the threading race. The problem is that calling instance() creates the BackingThreadHolder (and WorkerBackingThread) if we haven't yet. This means that during shutdown we would unnecessarily create the WorkerBackingThread just to clear it. This is why yhirano removed the call to clearSharedBackingThread() in https://codereview.chromium.org/1897143002. The problem is that we can't call ThreadState::detach until we are no longer referencing oilpan objects from the compositor thread (CompositorMutatorImpl).
I want to have yhirano@ take a look at this. https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/ModulesInitializer.cpp:67: CompositorWorkerThread::clearSharedBackingThread(); On 2016/05/10 18:07:39, flackr wrote: > On 2016/05/09 at 02:00:53, haraken wrote: > > Backing threads of other workers are shut down in > WorkerThread::terminateAndWaitForAllWorkers(). Can we shut down the backing > thread of the compositor worker there as well? > > This does make sense, however core/ is not allowed to include modules so we > aren't allowed to get the CompositorWorkerThread's BackingThreadHolder instance > to clear the thread pointer. This does suggest though that maybe we should clear > the shared backing thread after calling CoreInitializer::shutdown call. Can we add some virtual method on WorkerThread and override it on CompositorWorkerThread? That way you can call CompositorWorkerThread's method from CoreInitializer::shutdown. https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:35: thread->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerBackingThread::initialize, AllowCrossThreadAccess(this))); I still don't understand why you have to call initialize() here. attach() also calls initialize(). Don't we end up with calling initialize() twice? https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:41: signalShutdownAndWait(); I'd move this to CompositorWorkerThread::clearSharedBackingThread (since it's not a good idea to do complicated stuff in a destructor). https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp (right): https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp:27: CompositorWorkerThread::ensureSharedBackingThread(); Sorry for not being clear: I'm suggesting to move this to *just before* the worker->initialize().
This may be a silly question, but isn't it possible to create a CompositorMutator in (the first) CompositorWorkerGlobalScope's constructor? ThreadState should be already attached. If you truly need initialize / shutdown threads before / after CompositorWorkerThread's construction / destruction, I'd suggest calling attach / detach, rather than modifying WorkerBackingThread's internal logic. https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:34: s_hasBackingThreadHolder = true; You need to protect the boolean with a mutex. https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:41: // Calling instance() if it has not already been called would create ditto https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:94: CompositorWorkerThread::clearSharedBackingThread(); I think the latter half of https://codereview.chromium.org/1895873006/diff/200001/third_party/WebKit/Sou... has not been addressed yet.
I think a lot of confusion came from the fact that I was trying to repurpose WorkerBackingThread to have different lifetime semantics depending on whether it owned the thread or not. I've refactored WorkerBackingThread so that CompositorWorkerBackingThreadImpl can subclass it and implement the lifetime semantics it wants without affecting regular workers. WDYT? https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/ModulesInitializer.cpp:67: CompositorWorkerThread::clearSharedBackingThread(); On 2016/05/11 at 00:18:31, haraken wrote: > On 2016/05/10 18:07:39, flackr wrote: > > On 2016/05/09 at 02:00:53, haraken wrote: > > > Backing threads of other workers are shut down in > > WorkerThread::terminateAndWaitForAllWorkers(). Can we shut down the backing > > thread of the compositor worker there as well? > > > > This does make sense, however core/ is not allowed to include modules so we > > aren't allowed to get the CompositorWorkerThread's BackingThreadHolder instance > > to clear the thread pointer. This does suggest though that maybe we should clear > > the shared backing thread after calling CoreInitializer::shutdown call. > > Can we add some virtual method on WorkerThread and override it on CompositorWorkerThread? That way you can call CompositorWorkerThread's method from CoreInitializer::shutdown. No, the problem is that the backing thread is a singleton shared by all CompositorWorkerThreads and required even after they all go away (because the LayerTreeHostImpl will still be holding onto an oilpan CompositorMutatorImpl). We may not have any CompositorWorkerThreads but still have a backing thread. https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:35: thread->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&WorkerBackingThread::initialize, AllowCrossThreadAccess(this))); On 2016/05/11 at 00:18:31, haraken wrote: > I still don't understand why you have to call initialize() here. attach() also calls initialize(). Don't we end up with calling initialize() twice? I've changed WorkerBackingThread so that when you wrap an existing thread it initializes once and does nothing when attach / detach are called and doesn't call shutdown until you destruct the WorkerBackingThread. This is because we can't call ThreadState::detach until we no longer have any oilpan objects owned by the compositor thread. https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:41: signalShutdownAndWait(); On 2016/05/11 at 00:18:31, haraken wrote: > I'd move this to CompositorWorkerThread::clearSharedBackingThread (since it's not a good idea to do complicated stuff in a destructor). Done. https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp (right): https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorker.cpp:27: CompositorWorkerThread::ensureSharedBackingThread(); On 2016/05/11 at 00:18:31, haraken wrote: > Sorry for not being clear: I'm suggesting to move this to *just before* the worker->initialize(). Ah, my apologies. Done. https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:34: s_hasBackingThreadHolder = true; On 2016/05/11 at 02:21:02, yhirano wrote: > You need to protect the boolean with a mutex. Why does this need to be protected, aren't instance() and clear() always accessed on the main thread? https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1955693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:94: CompositorWorkerThread::clearSharedBackingThread(); On 2016/05/11 at 02:21:02, yhirano wrote: > I think the latter half of https://codereview.chromium.org/1895873006/diff/200001/third_party/WebKit/Sou... has not been addressed yet. It is a bit fragile, though we normally don't re-use BackingThreadHolder after shutting it down and we would have to call resetForTest anyways to enable the GC shutdown behavior. Perhaps instead of bool s_hasBackingThreadHolder just a static pointer for the instance which can be referenced from clear?
I still think > If you truly need initialize / shutdown threads before / after > CompositorWorkerThread's construction / destruction, I'd suggest calling attach > / detach, rather than modifying WorkerBackingThread's internal logic. is a better way.
> This may be a silly question, but isn't it possible to create a CompositorMutator in (the first) CompositorWorkerGlobalScope's constructor? ThreadState should be already attached. I realized I didn't address this earlier, but no I don't believe it's possible. There's no way to access the LayerTreeHostImpl directly so we have to plumb the mutator through as part of the commit process. On 2016/05/18 at 05:32:56, yhirano wrote: > I still think > > > If you truly need initialize / shutdown threads before / after > > CompositorWorkerThread's construction / destruction, I'd suggest calling attach > > / detach, rather than modifying WorkerBackingThread's internal logic. > > is a better way. Done. I had preferred not to do this because it's lying about the worker script count and it seemed hacky to use that to undo the reference counting construction / destruction of the WorkerBackingThread.
https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:67: CompositorWorkerThread::clearSharedBackingThread(); I think this should be placed after CoreInitializer::shutdown(). https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:34: if (!s_holderInstance) You need a mutex. https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:62: void initializeOnThread() DCHECK(0, m_thread->workerScriptCount()); https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:74: void shutdownOnThread(WaitableEvent* doneEvent) DCHECK(1, m_thread->workerScriptCount()); https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.h (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.h:26: static void resetSharedBackingThreadForTest(); I think this function can be misleading now and should be removed. https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:85: CompositorWorkerThread::resetSharedBackingThreadForTest(); ASSERT_EQ(0, compositorWorker->workerBackingThread()->workerScriptCount());
https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/ModulesInitializer.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/ModulesInitializer.cpp:67: CompositorWorkerThread::clearSharedBackingThread(); On 2016/05/19 at 05:35:37, yhirano wrote: > I think this should be placed after CoreInitializer::shutdown(). Yes, correct. This should happen after the worker shutdown in CoreInitializer::shutdown. Done. https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:34: if (!s_holderInstance) On 2016/05/19 at 05:35:37, yhirano wrote: > You need a mutex. Done. But are you sure? It seems to me like instance() and clear() are always accessed from the main thread. https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:62: void initializeOnThread() On 2016/05/19 at 05:35:37, yhirano wrote: > DCHECK(0, m_thread->workerScriptCount()); Done. https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:74: void shutdownOnThread(WaitableEvent* doneEvent) On 2016/05/19 at 05:35:37, yhirano wrote: > DCHECK(1, m_thread->workerScriptCount()); Done. https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.h (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.h:26: static void resetSharedBackingThreadForTest(); On 2016/05/19 at 05:35:37, yhirano wrote: > I think this function can be misleading now and should be removed. This recreates the worker backing thread with the shouldCallGCOnShutdown flag set to true. Don't we still need this as described here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Mostly looks good. https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:34: if (!s_holderInstance) On 2016/05/19 22:24:16, flackr wrote: > On 2016/05/19 at 05:35:37, yhirano wrote: > > You need a mutex. > > Done. But are you sure? It seems to me like instance() and clear() are always > accessed from the main thread. Yeah, as I commented on https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m..., I don't think the mutex is needed. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:23: static BackingThreadHolder* s_holderInstance = nullptr; Can we make this a static member variable of BackingThreadHolder? https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:67: Platform::current()->compositorThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&BackingThreadHolder::initializeOnThread, AllowCrossThreadAccess(this))); Add DCHECK(isMainThread()) (for documentation purpose). https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:72: DCHECK_EQ(0u, m_thread->workerScriptCount()) << "BackingThreadHolder should be the first to attach to WorkerBackingThread"; Once we land this CL, it should be guaranteed that BackingThreadHolder is the first/last thread that gets attached/detached to/from the WorkerBackingThread. Then maybe we no longer need to reference-count the WorkerBackingThread? i.e., once we land this CL, can we remove the concept of workerScriptCount() entirely? https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:78: WaitableEvent doneEvent; Add DCHECK(isMainThread()). https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:126: BackingThreadHolder::instance(); Add DCHECK(isMainThread()). https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:131: BackingThreadHolder::clear(); Add DCHECK(isMainThread()).
Could you modify |workerBackingThread().workerScriptCount() == 1| in WorkerThread.cpp to |workerBackingThread().workerScriptCount() <= 2| (and add some comments)? https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:34: if (!s_holderInstance) On 2016/05/20 00:24:44, haraken wrote: > On 2016/05/19 22:24:16, flackr wrote: > > On 2016/05/19 at 05:35:37, yhirano wrote: > > > You need a mutex. > > > > Done. But are you sure? It seems to me like instance() and clear() are always > > accessed from the main thread. > > Yeah, as I commented on > https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m..., > I don't think the mutex is needed. instance() can be called from worker threads via WorkerThread::workerBackingThread(). I'm a bit nervous because the native thread exists before the first call of ensureSharedBackingThread. With enough comments and assertions it might be OK to remove mutex. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:41: if (!s_holderInstance) Can you create another function for ensureBackingThread and move this if-branch to that function? https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:72: DCHECK_EQ(0u, m_thread->workerScriptCount()) << "BackingThreadHolder should be the first to attach to WorkerBackingThread"; On 2016/05/20 00:24:44, haraken wrote: > > Once we land this CL, it should be guaranteed that BackingThreadHolder is the > first/last thread that gets attached/detached to/from the WorkerBackingThread. > Then maybe we no longer need to reference-count the WorkerBackingThread? i.e., > once we land this CL, can we remove the concept of workerScriptCount() entirely? > > It's now used to see if we need call TerminateExecution. Graceful shutdown will solve that problem.
https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:34: if (!s_holderInstance) On 2016/05/20 02:24:54, yhirano wrote: > On 2016/05/20 00:24:44, haraken wrote: > > On 2016/05/19 22:24:16, flackr wrote: > > > On 2016/05/19 at 05:35:37, yhirano wrote: > > > > You need a mutex. > > > > > > Done. But are you sure? It seems to me like instance() and clear() are > always > > > accessed from the main thread. > > > > Yeah, as I commented on > > > https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m..., > > I don't think the mutex is needed. > > instance() can be called from worker threads via > WorkerThread::workerBackingThread(). > I'm a bit nervous because the native thread exists before the first call of > ensureSharedBackingThread. If I'm understanding correctly, it *should not* happen because the main thread must call ensureBackingThread before any compositor worker thread starts running. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:72: DCHECK_EQ(0u, m_thread->workerScriptCount()) << "BackingThreadHolder should be the first to attach to WorkerBackingThread"; On 2016/05/20 02:24:54, yhirano wrote: > On 2016/05/20 00:24:44, haraken wrote: > > > > Once we land this CL, it should be guaranteed that BackingThreadHolder is the > > first/last thread that gets attached/detached to/from the WorkerBackingThread. > > Then maybe we no longer need to reference-count the WorkerBackingThread? i.e., > > once we land this CL, can we remove the concept of workerScriptCount() > entirely? > > > > > > It's now used to see if we need call TerminateExecution. Graceful shutdown will > solve that problem. But this CL makes the TerminateExecution useless, doesn't it? Given that the compositor thread keeps one reference count to the backing thread until blink::shutdown, it won't happen that workerScriptCount() returns 1 when processing the TerminateExecution (workerScriptCount() returns a number larger than 2).
https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:72: DCHECK_EQ(0u, m_thread->workerScriptCount()) << "BackingThreadHolder should be the first to attach to WorkerBackingThread"; On 2016/05/20 02:35:24, haraken wrote: > On 2016/05/20 02:24:54, yhirano wrote: > > On 2016/05/20 00:24:44, haraken wrote: > > > > > > Once we land this CL, it should be guaranteed that BackingThreadHolder is > the > > > first/last thread that gets attached/detached to/from the > WorkerBackingThread. > > > Then maybe we no longer need to reference-count the WorkerBackingThread? > i.e., > > > once we land this CL, can we remove the concept of workerScriptCount() > > entirely? > > > > > > > > > > It's now used to see if we need call TerminateExecution. Graceful shutdown > will > > solve that problem. > > But this CL makes the TerminateExecution useless, doesn't it? Given that the > compositor thread keeps one reference count to the backing thread until > blink::shutdown, it won't happen that workerScriptCount() returns 1 when > processing the TerminateExecution (workerScriptCount() returns a number larger > than 2). It depends on whether we want to keep the "current" behavior. I thought we wanted to (see the first comment in https://codereview.chromium.org/1955693003/#msg18). If we don't have to, it's great in terms of code simplicity.
yhirano@chromium.org changed reviewers: + nhiroki@chromium.org
+nhiroki@ Talked offline w/haraken@. With this CL compositor workers will not receive TerminateExecution as haraken@ commented. That's OK tentatively with the following fix. 1. TerminateExecution should be called when the process is going to shutdown. 2. TerminateExecution should be called when the script seems busy for some time after terminate() is called from the main thread. flackr@, can you implement 1 in this CL? It's basically calling isolate->TerminateExecution() in the process shutdown sequence. The only tricky part is that calling TerminateExecution should precede CoreInitializer::shutdown() but CoreInitializer::shutdown() should precede the last detach() for the compositor worker backing thread. 2 will be implemented as Graceful Shutdown. Hence I retract the following comment. > Could you modify |workerBackingThread().workerScriptCount() == 1| in > WorkerThread.cpp to |workerBackingThread().workerScriptCount() <= 2| (and add > some comments)? Thank you!
Thanks for all the help, please take another look. Thanks. https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:34: if (!s_holderInstance) On 2016/05/20 at 02:35:24, haraken wrote: > On 2016/05/20 02:24:54, yhirano wrote: > > On 2016/05/20 00:24:44, haraken wrote: > > > On 2016/05/19 22:24:16, flackr wrote: > > > > On 2016/05/19 at 05:35:37, yhirano wrote: > > > > > You need a mutex. > > > > > > > > Done. But are you sure? It seems to me like instance() and clear() are > > always > > > > accessed from the main thread. > > > > > > Yeah, as I commented on > > > > > https://codereview.chromium.org/1955693003/diff/1/third_party/WebKit/Source/m..., > > > I don't think the mutex is needed. > > > > instance() can be called from worker threads via > > WorkerThread::workerBackingThread(). > > I'm a bit nervous because the native thread exists before the first call of > > ensureSharedBackingThread. > > If I'm understanding correctly, it *should not* happen because the main thread must call ensureBackingThread before any compositor worker thread starts running. Correct, that is how it is supposed to work. I see your point on the Mutex, it shouldn't ever happen that there is contention calling instance() since we ensure the instance exists before the first worker is created and the workers should be shut down before we shut down the instance but can't hurt to be safe. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:23: static BackingThreadHolder* s_holderInstance = nullptr; On 2016/05/20 at 00:24:45, haraken wrote: > Can we make this a static member variable of BackingThreadHolder? Done. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:41: if (!s_holderInstance) On 2016/05/20 at 02:24:55, yhirano wrote: > Can you create another function for ensureBackingThread and move this if-branch to that function? Done. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:67: Platform::current()->compositorThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&BackingThreadHolder::initializeOnThread, AllowCrossThreadAccess(this))); On 2016/05/20 at 00:24:45, haraken wrote: > Add DCHECK(isMainThread()) (for documentation purpose). Done. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:78: WaitableEvent doneEvent; On 2016/05/20 at 00:24:45, haraken wrote: > Add DCHECK(isMainThread()). Done. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:126: BackingThreadHolder::instance(); On 2016/05/20 at 00:24:45, haraken wrote: > Add DCHECK(isMainThread()). Done. https://codereview.chromium.org/1955693003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:131: BackingThreadHolder::clear(); On 2016/05/20 at 00:24:45, haraken wrote: > Add DCHECK(isMainThread()). Done. https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:57: s_instance = new BackingThreadHolder(WorkerBackingThread::createForTest(Platform::current()->compositorThread())); I didn't like how resetForTest swapped out the m_thread (which could have then had the wrong workerBackingCount if the posted task had already run) and so I'd have to check that the task to attach was still pending, so I decided we should just pass the test WorkerBackingThread in to the constructor.
LGTM on my side. Thank for being persistent on this!
As written at #22, TerminteExecution should be called before CoreInitializer::shutdown (specifically WorkerThread::terminateAndWaitForAllWorkers()) because it waits for worker threads that can be blocked by scripts.
I'm basically deferring this to yhirano, one nit (in addition to TerminateExecution thing) https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:37: MutexLocker locker(holderInstanceMutex()); I'd like to remove this lock if it shouldn't be needed. The threading assertions we have seem to be enough, or are there still other possibilities that we could be stumbled on? https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:186: ASSERT_EQ(2u, workerBackingThread->workerScriptCount()); (yhirano: are we going to remove worker script count concept after this patch lands? Well I could probably just talk to you offline)
https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:186: ASSERT_EQ(2u, workerBackingThread->workerScriptCount()); On 2016/05/23 09:53:55, kinuko wrote: > (yhirano: are we going to remove worker script count concept after this patch > lands? Well I could probably just talk to you offline) Yes, unless threaded worklets require otherwise.
Sorry for missing the termination order part of the comment in #22. This is done now. I couldn't find a good way to verify that TerminateExecution had been called other than to add a boolean to track whether Terminate had been called before clearSharedBackingThread. Calling isolate()->IsExecutionTerminating() resulted in a crash I suppose accessing an invalid isolate? https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1955693003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:37: MutexLocker locker(holderInstanceMutex()); On 2016/05/23 at 09:53:55, kinuko wrote: > I'd like to remove this lock if it shouldn't be needed. The threading assertions we have seem to be enough, or are there still other possibilities that we could be stumbled on? Given s_instance is cleared on main after a synchronous shutdown, I think removing this is fine. Done.
lgtm, thank you.
> Calling isolate()->IsExecutionTerminating() resulted in a crash I suppose accessing an invalid isolate? Sorry, I overlooked this sentence. When do you see the crash? I ran webkit_unit_tests but it didn't crash on my environment. I found that it's possible to call TerminateExecution on a BackingThreadHolder whose backing thread has been created but has not been initialized yet. You can add one more flag (initialized) and turn it on in initializeOnThread (with mutex). I'm not sure if your crash is caused by this problem, though.
On 2016/05/26 at 03:57:50, yhirano wrote: > > Calling isolate()->IsExecutionTerminating() resulted in a crash I suppose accessing an invalid isolate? > > Sorry, I overlooked this sentence. When do you see the crash? I ran webkit_unit_tests but it didn't crash on my environment. > I found that it's possible to call TerminateExecution on a BackingThreadHolder whose backing thread has been created but has not been initialized yet. You can add one more flag (initialized) and turn it on in initializeOnThread (with mutex). I'm not sure if your crash is caused by this problem, though. Sorry, to be clear I meant instead of using the boolean to verify that I had called TerminateExecution I tried checking isolate()->IsExecutionTerminating() in the DCHECK in BackingThreadHolder::clear(). Running the CompositorWorkerThread.* test suite in debug crashed on this check. Is it wrong to check this after calling TerminateExecution or should I be able to remove the boolean I have to check if I've already called TerminateExecution?
On 2016/05/26 at 03:57:50, yhirano wrote: > > Calling isolate()->IsExecutionTerminating() resulted in a crash I suppose accessing an invalid isolate? > > Sorry, I overlooked this sentence. When do you see the crash? I ran webkit_unit_tests but it didn't crash on my environment. > I found that it's possible to call TerminateExecution on a BackingThreadHolder whose backing thread has been created but has not been initialized yet. You can add one more flag (initialized) and turn it on in initializeOnThread (with mutex). I'm not sure if your crash is caused by this problem, though. Done.
The CQ bit was checked by flackr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1955693003/#ps160001 (title: "Merge with master and only call TerminateExecution if initialization has occurred.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955693003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955693003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/05/26 at 15:27:28, commit-bot wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) In the webexposed/ tests we don't have a compositor thread. vollick suggested moving the test webexposed/global-interface-listing-compositor-worker.html into virtual/threaded which adds --enable-threaded-compositing and just failing if we require compositor worker when we don't have a compositor thread for now but I'm still trying to get this to work correctly. Alternately I suppose we could either use the main thread or spin up a thread to run compositor worker on but either may require adding a lot of switching to CompositorWorkerThread based on a switch we rarely use.
On 2016/05/27 02:10:03, flackr wrote: > On 2016/05/26 at 15:27:28, commit-bot wrote: > > Try jobs failed on following builders: > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > In the webexposed/ tests we don't have a compositor thread. vollick suggested > moving the test webexposed/global-interface-listing-compositor-worker.html into > virtual/threaded which adds --enable-threaded-compositing and just failing if we > require compositor worker when we don't have a compositor thread for now but I'm > still trying to get this to work correctly. Alternately I suppose we could > either use the main thread or spin up a thread to run compositor worker on but > either may require adding a lot of switching to CompositorWorkerThread based on > a switch we rarely use. It looks content::RenderThreadImpl::InitializeCompositorThread is called in production but not in layout tests, is that right?
On 2016/05/27 at 03:09:16, yhirano wrote: > On 2016/05/27 02:10:03, flackr wrote: > > On 2016/05/26 at 15:27:28, commit-bot wrote: > > > Try jobs failed on following builders: > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > In the webexposed/ tests we don't have a compositor thread. vollick suggested > > moving the test webexposed/global-interface-listing-compositor-worker.html into > > virtual/threaded which adds --enable-threaded-compositing and just failing if we > > require compositor worker when we don't have a compositor thread for now but I'm > > still trying to get this to work correctly. Alternately I suppose we could > > either use the main thread or spin up a thread to run compositor worker on but > > either may require adding a lot of switching to CompositorWorkerThread based on > > a switch we rarely use. > > It looks content::RenderThreadImpl::InitializeCompositorThread is called in production but not in layout tests, is that right? Correct. I was looking into why this works on tip of tree - it's because when we have a null thread in WorkerBackingThread, we create one. So, if I just use the constructed worker backing thread to post the tasks everything works. Sound good? I could alternately construct and own a thread when we have no compositor thread in BackingThreadHolder and change WorkerBackingThread so that if you pass it a thread, it must actually exist.
On 2016/05/27 15:10:46, flackr wrote: > On 2016/05/27 at 03:09:16, yhirano wrote: > > On 2016/05/27 02:10:03, flackr wrote: > > > On 2016/05/26 at 15:27:28, commit-bot wrote: > > > > Try jobs failed on following builders: > > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > > > In the webexposed/ tests we don't have a compositor thread. vollick > suggested > > > moving the test webexposed/global-interface-listing-compositor-worker.html > into > > > virtual/threaded which adds --enable-threaded-compositing and just failing > if we > > > require compositor worker when we don't have a compositor thread for now but > I'm > > > still trying to get this to work correctly. Alternately I suppose we could > > > either use the main thread or spin up a thread to run compositor worker on > but > > > either may require adding a lot of switching to CompositorWorkerThread based > on > > > a switch we rarely use. > > > > It looks content::RenderThreadImpl::InitializeCompositorThread is called in > production but not in layout tests, is that right? > > Correct. I was looking into why this works on tip of tree - it's because when we > have a null thread in WorkerBackingThread, we create one. So, if I just use the > constructed worker backing thread to post the tasks everything works. Sound > good? I could alternately construct and own a thread when we have no compositor > thread in BackingThreadHolder and change WorkerBackingThread so that if you pass > it a thread, it must actually exist. I guess the root cause is that sometimes "CompositorWorker" runtime enabled flag is enabled without --enable-threaded-compositing. Or is it intended?
On 2016/05/27 at 15:22:25, yhirano wrote: > On 2016/05/27 15:10:46, flackr wrote: > > On 2016/05/27 at 03:09:16, yhirano wrote: > > > On 2016/05/27 02:10:03, flackr wrote: > > > > On 2016/05/26 at 15:27:28, commit-bot wrote: > > > > > Try jobs failed on following builders: > > > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > > > > > In the webexposed/ tests we don't have a compositor thread. vollick > > suggested > > > > moving the test webexposed/global-interface-listing-compositor-worker.html > > into > > > > virtual/threaded which adds --enable-threaded-compositing and just failing > > if we > > > > require compositor worker when we don't have a compositor thread for now but > > I'm > > > > still trying to get this to work correctly. Alternately I suppose we could > > > > either use the main thread or spin up a thread to run compositor worker on > > but > > > > either may require adding a lot of switching to CompositorWorkerThread based > > on > > > > a switch we rarely use. > > > > > > It looks content::RenderThreadImpl::InitializeCompositorThread is called in > > production but not in layout tests, is that right? > > > > Correct. I was looking into why this works on tip of tree - it's because when we > > have a null thread in WorkerBackingThread, we create one. So, if I just use the > > constructed worker backing thread to post the tasks everything works. Sound > > good? I could alternately construct and own a thread when we have no compositor > > thread in BackingThreadHolder and change WorkerBackingThread so that if you pass > > it a thread, it must actually exist. > > I guess the root cause is that sometimes "CompositorWorker" runtime enabled flag is enabled without --enable-threaded-compositing. Or is it intended? I think CompositorWorker should still eventually work without threaded compositing - but maybe it should actually just run on main in that case.
On 2016/05/27 15:28:36, flackr wrote: > On 2016/05/27 at 15:22:25, yhirano wrote: > > On 2016/05/27 15:10:46, flackr wrote: > > > On 2016/05/27 at 03:09:16, yhirano wrote: > > > > On 2016/05/27 02:10:03, flackr wrote: > > > > > On 2016/05/26 at 15:27:28, commit-bot wrote: > > > > > > Try jobs failed on following builders: > > > > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > > > > > > > In the webexposed/ tests we don't have a compositor thread. vollick > > > suggested > > > > > moving the test > webexposed/global-interface-listing-compositor-worker.html > > > into > > > > > virtual/threaded which adds --enable-threaded-compositing and just > failing > > > if we > > > > > require compositor worker when we don't have a compositor thread for now > but > > > I'm > > > > > still trying to get this to work correctly. Alternately I suppose we > could > > > > > either use the main thread or spin up a thread to run compositor worker > on > > > but > > > > > either may require adding a lot of switching to CompositorWorkerThread > based > > > on > > > > > a switch we rarely use. > > > > > > > > It looks content::RenderThreadImpl::InitializeCompositorThread is called > in > > > production but not in layout tests, is that right? > > > > > > Correct. I was looking into why this works on tip of tree - it's because > when we > > > have a null thread in WorkerBackingThread, we create one. So, if I just use > the > > > constructed worker backing thread to post the tasks everything works. Sound > > > good? I could alternately construct and own a thread when we have no > compositor > > > thread in BackingThreadHolder and change WorkerBackingThread so that if you > pass > > > it a thread, it must actually exist. > > > > I guess the root cause is that sometimes "CompositorWorker" runtime enabled > flag is enabled without --enable-threaded-compositing. Or is it intended? > > I think CompositorWorker should still eventually work without threaded > compositing - but maybe it should actually just run on main in that case. Wow, compositor worker is much more complicated than I thought, then...
On 2016/05/27 at 15:37:24, yhirano wrote: > On 2016/05/27 15:28:36, flackr wrote: > > On 2016/05/27 at 15:22:25, yhirano wrote: > > > On 2016/05/27 15:10:46, flackr wrote: > > > > On 2016/05/27 at 03:09:16, yhirano wrote: > > > > > On 2016/05/27 02:10:03, flackr wrote: > > > > > > On 2016/05/26 at 15:27:28, commit-bot wrote: > > > > > > > Try jobs failed on following builders: > > > > > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > > > > > > > > > In the webexposed/ tests we don't have a compositor thread. vollick > > > > suggested > > > > > > moving the test > > webexposed/global-interface-listing-compositor-worker.html > > > > into > > > > > > virtual/threaded which adds --enable-threaded-compositing and just > > failing > > > > if we > > > > > > require compositor worker when we don't have a compositor thread for now > > but > > > > I'm > > > > > > still trying to get this to work correctly. Alternately I suppose we > > could > > > > > > either use the main thread or spin up a thread to run compositor worker > > on > > > > but > > > > > > either may require adding a lot of switching to CompositorWorkerThread > > based > > > > on > > > > > > a switch we rarely use. > > > > > > > > > > It looks content::RenderThreadImpl::InitializeCompositorThread is called > > in > > > > production but not in layout tests, is that right? > > > > > > > > Correct. I was looking into why this works on tip of tree - it's because > > when we > > > > have a null thread in WorkerBackingThread, we create one. So, if I just use > > the > > > > constructed worker backing thread to post the tasks everything works. Sound > > > > good? I could alternately construct and own a thread when we have no > > compositor > > > > thread in BackingThreadHolder and change WorkerBackingThread so that if you > > pass > > > > it a thread, it must actually exist. > > > > > > I guess the root cause is that sometimes "CompositorWorker" runtime enabled > > flag is enabled without --enable-threaded-compositing. Or is it intended? > > > > I think CompositorWorker should still eventually work without threaded > > compositing - but maybe it should actually just run on main in that case. > > Wow, compositor worker is much more complicated than I thought, then... Well, from a developer perspective, compositor worker is expressing / enforcing the intent to only make compositing related changes to elements. It's an implementation detail whether compositing happens on a separate thread. I don't think developers should have to worry about their effects not running if we didn't have threaded compositing. All of this being said, I think in practice we're always going to run with threaded compositing.
On 2016/05/27 15:43:48, flackr wrote: > On 2016/05/27 at 15:37:24, yhirano wrote: > > On 2016/05/27 15:28:36, flackr wrote: > > > On 2016/05/27 at 15:22:25, yhirano wrote: > > > > On 2016/05/27 15:10:46, flackr wrote: > > > > > On 2016/05/27 at 03:09:16, yhirano wrote: > > > > > > On 2016/05/27 02:10:03, flackr wrote: > > > > > > > On 2016/05/26 at 15:27:28, commit-bot wrote: > > > > > > > > Try jobs failed on following builders: > > > > > > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > > > > > > > > > > > In the webexposed/ tests we don't have a compositor thread. vollick > > > > > suggested > > > > > > > moving the test > > > webexposed/global-interface-listing-compositor-worker.html > > > > > into > > > > > > > virtual/threaded which adds --enable-threaded-compositing and just > > > failing > > > > > if we > > > > > > > require compositor worker when we don't have a compositor thread for > now > > > but > > > > > I'm > > > > > > > still trying to get this to work correctly. Alternately I suppose we > > > could > > > > > > > either use the main thread or spin up a thread to run compositor > worker > > > on > > > > > but > > > > > > > either may require adding a lot of switching to > CompositorWorkerThread > > > based > > > > > on > > > > > > > a switch we rarely use. > > > > > > > > > > > > It looks content::RenderThreadImpl::InitializeCompositorThread is > called > > > in > > > > > production but not in layout tests, is that right? > > > > > > > > > > Correct. I was looking into why this works on tip of tree - it's because > > > when we > > > > > have a null thread in WorkerBackingThread, we create one. So, if I just > use > > > the > > > > > constructed worker backing thread to post the tasks everything works. > Sound > > > > > good? I could alternately construct and own a thread when we have no > > > compositor > > > > > thread in BackingThreadHolder and change WorkerBackingThread so that if > you > > > pass > > > > > it a thread, it must actually exist. > > > > > > > > I guess the root cause is that sometimes "CompositorWorker" runtime > enabled > > > flag is enabled without --enable-threaded-compositing. Or is it intended? > > > > > > I think CompositorWorker should still eventually work without threaded > > > compositing - but maybe it should actually just run on main in that case. > > > > Wow, compositor worker is much more complicated than I thought, then... > > Well, from a developer perspective, compositor worker is expressing / enforcing > the intent to only make compositing related changes to elements. It's an > implementation detail whether compositing happens on a separate thread. I don't > think developers should have to worry about their effects not running if we > didn't have threaded compositing. All of this being said, I think in practice > we're always going to run with threaded compositing. But I thin blink's worker related code expect that web workers run on different threads from the main thread. At least WorkerThread::terminateAndWait[ForAllWorkers] lead to deadlock when some workers are actually running on the main thread.
On 2016/05/27 at 15:51:22, yhirano wrote: > On 2016/05/27 15:43:48, flackr wrote: > > On 2016/05/27 at 15:37:24, yhirano wrote: > > > On 2016/05/27 15:28:36, flackr wrote: > > > > On 2016/05/27 at 15:22:25, yhirano wrote: > > > > > On 2016/05/27 15:10:46, flackr wrote: > > > > > > On 2016/05/27 at 03:09:16, yhirano wrote: > > > > > > > On 2016/05/27 02:10:03, flackr wrote: > > > > > > > > On 2016/05/26 at 15:27:28, commit-bot wrote: > > > > > > > > > Try jobs failed on following builders: > > > > > > > > > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > > > > > > > > > > > > > > > In the webexposed/ tests we don't have a compositor thread. vollick > > > > > > suggested > > > > > > > > moving the test > > > > webexposed/global-interface-listing-compositor-worker.html > > > > > > into > > > > > > > > virtual/threaded which adds --enable-threaded-compositing and just > > > > failing > > > > > > if we > > > > > > > > require compositor worker when we don't have a compositor thread for > > now > > > > but > > > > > > I'm > > > > > > > > still trying to get this to work correctly. Alternately I suppose we > > > > could > > > > > > > > either use the main thread or spin up a thread to run compositor > > worker > > > > on > > > > > > but > > > > > > > > either may require adding a lot of switching to > > CompositorWorkerThread > > > > based > > > > > > on > > > > > > > > a switch we rarely use. > > > > > > > > > > > > > > It looks content::RenderThreadImpl::InitializeCompositorThread is > > called > > > > in > > > > > > production but not in layout tests, is that right? > > > > > > > > > > > > Correct. I was looking into why this works on tip of tree - it's because > > > > when we > > > > > > have a null thread in WorkerBackingThread, we create one. So, if I just > > use > > > > the > > > > > > constructed worker backing thread to post the tasks everything works. > > Sound > > > > > > good? I could alternately construct and own a thread when we have no > > > > compositor > > > > > > thread in BackingThreadHolder and change WorkerBackingThread so that if > > you > > > > pass > > > > > > it a thread, it must actually exist. > > > > > > > > > > I guess the root cause is that sometimes "CompositorWorker" runtime > > enabled > > > > flag is enabled without --enable-threaded-compositing. Or is it intended? > > > > > > > > I think CompositorWorker should still eventually work without threaded > > > > compositing - but maybe it should actually just run on main in that case. > > > > > > Wow, compositor worker is much more complicated than I thought, then... > > > > Well, from a developer perspective, compositor worker is expressing / enforcing > > the intent to only make compositing related changes to elements. It's an > > implementation detail whether compositing happens on a separate thread. I don't > > think developers should have to worry about their effects not running if we > > didn't have threaded compositing. All of this being said, I think in practice > > we're always going to run with threaded compositing. > > But I thin blink's worker related code expect that web workers run on different threads from the main thread. At least WorkerThread::terminateAndWait[ForAllWorkers] lead to deadlock when some workers are actually running on the main thread. Can we keep the current behavior of creating a thread when we don't have a platform compositor thread for now and work out these problems in later patches?
Sorry, I still don't understand. > I was looking into why this works on tip of tree - it's because when we > have a null thread in WorkerBackingThread, we create one. Are you referring to the case when we pass null WebThread to the WorkerBackingThread constructor? I think that will lead to crash on trybots because the WebThreadSupportinGC's constructor has an assertion for the case: #if ENABLE(ASSERT) ASSERT(!name || !thread); // We call this regardless of whether an existing thread is given or not, // as it means that blink is going to run with more than one thread. WTF::willCreateThread(); #endif
On 2016/05/27 16:18:40, yhirano wrote: > Sorry, I still don't understand. > > > I was looking into why this works on tip of tree - it's because when we > > have a null thread in WorkerBackingThread, we create one. > > Are you referring to the case when we pass null WebThread to the > WorkerBackingThread constructor? I think that will lead to crash on trybots > because the WebThreadSupportinGC's constructor has an assertion for the case: > > #if ENABLE(ASSERT) > ASSERT(!name || !thread); > // We call this regardless of whether an existing thread is given or not, > // as it means that blink is going to run with more than one thread. > WTF::willCreateThread(); > #endif Oh, I was completely wrong about the assertion. I'm sorry. >> Can we keep the current behavior of creating a thread when we don't have a >> platform compositor thread for now and work out these problems in later patches? Sounds reasonable.
The CQ bit was checked by flackr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/1955693003/#ps180001 (title: "Use the constructed WorkerBackingThread to post tasks.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955693003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955693003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by flackr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955693003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955693003/180001
Message was sent while issue was closed.
Description was changed from ========== compositor-worker: Keep worker backing thread alive for the lifetime of the compositor thread. This change is to support creating and holding onto oilpan objects on the compositor thread in https://codereview.chromium.org/1895873006 BUG=430155 TEST=CompositorWorkerThreadTest.* ========== to ========== compositor-worker: Keep worker backing thread alive for the lifetime of the compositor thread. This change is to support creating and holding onto oilpan objects on the compositor thread in https://codereview.chromium.org/1895873006 BUG=430155 TEST=CompositorWorkerThreadTest.* ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== compositor-worker: Keep worker backing thread alive for the lifetime of the compositor thread. This change is to support creating and holding onto oilpan objects on the compositor thread in https://codereview.chromium.org/1895873006 BUG=430155 TEST=CompositorWorkerThreadTest.* ========== to ========== compositor-worker: Keep worker backing thread alive for the lifetime of the compositor thread. This change is to support creating and holding onto oilpan objects on the compositor thread in https://codereview.chromium.org/1895873006 BUG=430155 TEST=CompositorWorkerThreadTest.* Committed: https://crrev.com/05f2063bff785b913d17727ab5a4c2af206b6ea5 Cr-Commit-Position: refs/heads/master@{#396732} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/05f2063bff785b913d17727ab5a4c2af206b6ea5 Cr-Commit-Position: refs/heads/master@{#396732}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
Seems like there are some lifetime issues with CompositorProxyClientImpl, https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg... |