Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1733353004:
Introduce WorkerBackingThread (Closed)
|
Created:
4 years, 9 months ago by yhirano Modified:
4 years, 8 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, falken, kinuko+worker_chromium.org, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce WorkerBackingThread
This change decouples thread-related part from WorkerThread. Now
multiple WorkerThread can be attached to one WorkerBackingThread
and one WorkerBackingThread is associated to one platform thread.
BUG=575532
Committed: https://crrev.com/378375cdaef0338e971e8c997ef6323a043499b6
Cr-Commit-Position: refs/heads/master@{#386948}
Patch Set 1 : #
Total comments: 13
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : rebase #Patch Set 10 : rebase #Patch Set 11 : rebase #
Total comments: 1
Patch Set 12 : rebase #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : rebase #
Total comments: 13
Patch Set 17 : #
Total comments: 2
Patch Set 18 : #Patch Set 19 : #Patch Set 20 : #Patch Set 21 : #Patch Set 22 : #
Total comments: 10
Patch Set 23 : #Patch Set 24 : #
Total comments: 20
Patch Set 25 : #Patch Set 26 : #Patch Set 27 : #Patch Set 28 : rebase #
Total comments: 8
Patch Set 29 : rebase #Patch Set 30 : #Messages
Total messages: 78 (21 generated)
Patchset #4 (id:60001) has been deleted
Description was changed from ========== [WIP] WorkerBackingThread BUG=575532 ========== to ========== Introduce WorkerBackingThread This change decouples thread-related part from WorkerThread. Now multiple WorkerThread can be attached to one WorkerBackingThread and one WorkerBackingThread is associated to one platform thread. BUG=575532 ==========
yhirano@chromium.org changed reviewers: + kinuko@chromium.org, sadrul@chromium.org, vollick@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:140001) has been deleted
PTAL There is one TODO in CompositorWorkerThreadTest (blocked by https://groups.google.com/forum/#!topic/v8-users/bhfAVJySjIA), but it will not affect the overall design.
On 2016/02/26 22:13:37, yhirano wrote: > PTAL > > There is one TODO in CompositorWorkerThreadTest (blocked by > https://groups.google.com/forum/#!topic/v8-users/bhfAVJySjIA), but it will not > affect the overall design. Leaks are reported, I will address the issue in the next week. https://codereview.chromium.org/1316233004
On 2016/02/26 22:13:37, yhirano wrote: > PTAL > > There is one TODO in CompositorWorkerThreadTest (blocked by > https://groups.google.com/forum/#!topic/v8-users/bhfAVJySjIA), but it will not > affect the overall design. Leaks are reported, I will address the issue in the next week. https://codereview.chromium.org/1316233004
Let me clarify one thing... is the main intention of this CL to cleanup the code (indirectly to make your thread termination changes smoother), or do we also have other intention? https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:44: Platform::current()->mainThread()->taskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&noop, m_backingThread.release())); So we make this class thread-safe-refcounted and post task here basically to make the lifetime management self-contained within this class (and for making things work for compositor worker)? Posting tasks in dtor everywhere makes me feel a bit nervous (as we're also introducing one in WebThreadSupportingGC), it tends to make things harder to track... I feel making these explicit and have each worker container own WorkerBackingThread solely on the main thread could be better? https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:47: WaitableEvent* initializationEventForTest() { return m_initializationEventForTest.get(); } Hmm could we not have these? https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:401: } The comment just leaves readers unsure if this code is right or not... initialize() sequence looks ok to me as attach won't be called once we pass this part? While it looks it's possible that we don't take this branch but shutdown() is called on worker thread, but it should be fine too as we don't need to worry about terminating v8 execution. Are there other cases we need to take care? Also: it doesn't feel WorkerBackingThread should expose this detail here, would we be able to move this part hidden in WorkerBackingThread? https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:69: WebThreadSupportingGC& backingThread(); It feels unfortunate that we need to expose two backingThread's... https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:30: void clear() { m_thread = nullptr; } Who calls this? https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:163: waitForWaitableEventAfterIteratingCurrentLoop(compositorWorker->workerBackingThread().initializationEventForTest()); Would it be possible to just replace this with posting a task & waiting for the task to finish? I don't really remember if we needed to specifically wait for the initialize() in these tests...
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1733353004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1733353004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> Let me clarify one thing... is the main intention of this CL to cleanup the code > (indirectly to make your thread termination changes smoother) Yes. https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:44: Platform::current()->mainThread()->taskRunner()->postTask(BLINK_FROM_HERE, threadSafeBind(&noop, m_backingThread.release())); On 2016/02/29 09:32:53, kinuko wrote: > So we make this class thread-safe-refcounted and post task here basically to > make the lifetime management self-contained within this class (and for making > things work for compositor worker)? Posting tasks in dtor everywhere makes me > feel a bit nervous (as we're also introducing one in WebThreadSupportingGC), it > tends to make things harder to track... I feel making these explicit and have > each worker container own WorkerBackingThread solely on the main thread could be > better? When the WebThreadSuppotingGC changes are landed we will not need this call. https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:47: WaitableEvent* initializationEventForTest() { return m_initializationEventForTest.get(); } On 2016/02/29 09:32:53, kinuko wrote: > Hmm could we not have these? Done. https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:401: } On 2016/02/29 09:32:53, kinuko wrote: > The comment just leaves readers unsure if this code is right or not... > initialize() sequence looks ok to me as attach won't be called once we pass this > part? While it looks it's possible that we don't take this branch but > shutdown() is called on worker thread, but it should be fine too as we don't > need to worry about terminating v8 execution. Are there other cases we need to > take care? > > Also: it doesn't feel WorkerBackingThread should expose this detail here, would > we be able to move this part hidden in WorkerBackingThread? Imagine there are two WorkerThread x and y attached to a WorkerBackingThread and we want to shutdown both. ----- Call x->terminate() on the main thread. Call y->terminate() on the main thread. ----- It is possible that y->terminate() runs on the main thread before x->shutdown() on the worker thread. In this case, TerminateExecution will never be called. It's problematic if the worker backing loop runs an infinite JS loop: we cannot terminate the worker backing thread. Moreover, if we call terminateAndWait on the main thread, it blocks the main thread too. https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:69: WebThreadSupportingGC& backingThread(); On 2016/02/29 09:32:53, kinuko wrote: > It feels unfortunate that we need to expose two backingThread's... Done. https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:30: void clear() { m_thread = nullptr; } On 2016/02/29 09:32:54, kinuko wrote: > Who calls this? I wanted it to be called in the global shutdown sequence. Fixed. https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:163: waitForWaitableEventAfterIteratingCurrentLoop(compositorWorker->workerBackingThread().initializationEventForTest()); On 2016/02/29 09:32:54, kinuko wrote: > Would it be possible to just replace this with posting a task & waiting for the > task to finish? I don't really remember if we needed to specifically wait for > the initialize() in these tests... Done.
https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:401: } On 2016/02/29 23:47:32, yhirano wrote: > On 2016/02/29 09:32:53, kinuko wrote: > > The comment just leaves readers unsure if this code is right or not... > > initialize() sequence looks ok to me as attach won't be called once we pass > this > > part? While it looks it's possible that we don't take this branch but > > shutdown() is called on worker thread, but it should be fine too as we don't > > need to worry about terminating v8 execution. Are there other cases we need > to > > take care? > > > > Also: it doesn't feel WorkerBackingThread should expose this detail here, > would > > we be able to move this part hidden in WorkerBackingThread? > > Imagine there are two WorkerThread x and y attached to a WorkerBackingThread and > we want to shutdown both. > > ----- > Call x->terminate() on the main thread. > Call y->terminate() on the main thread. > ----- > > It is possible that y->terminate() runs on the main thread before x->shutdown() > on the worker thread. In this case, TerminateExecution will never be called. > It's problematic if the worker backing loop runs an infinite JS loop: we cannot > terminate the worker backing thread. Moreover, if we call terminateAndWait on > the main thread, it blocks the main thread too. Correction: before x->shutdown() on the worker thread. => before x->shutdown() runs on the worker thread. if the worker backing loop runs => if the worker backing thread runs
On 2016/03/01 00:04:53, yhirano wrote: > https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): > > https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/workers/WorkerThread.cpp:401: } > On 2016/02/29 23:47:32, yhirano wrote: > > On 2016/02/29 09:32:53, kinuko wrote: > > > The comment just leaves readers unsure if this code is right or not... > > > initialize() sequence looks ok to me as attach won't be called once we pass > > this > > > part? While it looks it's possible that we don't take this branch but > > > shutdown() is called on worker thread, but it should be fine too as we don't > > > need to worry about terminating v8 execution. Are there other cases we need > > to > > > take care? > > > > > > Also: it doesn't feel WorkerBackingThread should expose this detail here, > > would > > > we be able to move this part hidden in WorkerBackingThread? > > > > Imagine there are two WorkerThread x and y attached to a WorkerBackingThread > and > > we want to shutdown both. > > > > ----- > > Call x->terminate() on the main thread. > > Call y->terminate() on the main thread. > > ----- > > > > It is possible that y->terminate() runs on the main thread before > x->shutdown() > > on the worker thread. In this case, TerminateExecution will never be called. > > It's problematic if the worker backing loop runs an infinite JS loop: we > cannot > > terminate the worker backing thread. Moreover, if we call terminateAndWait on > > the main thread, it blocks the main thread too. > > Correction: > before x->shutdown() on the worker thread. => before x->shutdown() runs on the > worker thread. > if the worker backing loop runs => if the worker backing thread runs I see thanks, but then we need to fix the code or at least have TODO?
Patchset #4 (id:220001) has been deleted
On 2016/03/01 12:50:21, kinuko wrote: > On 2016/03/01 00:04:53, yhirano wrote: > > > https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): > > > > > https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/workers/WorkerThread.cpp:401: } > > On 2016/02/29 23:47:32, yhirano wrote: > > > On 2016/02/29 09:32:53, kinuko wrote: > > > > The comment just leaves readers unsure if this code is right or not... > > > > initialize() sequence looks ok to me as attach won't be called once we > pass > > > this > > > > part? While it looks it's possible that we don't take this branch but > > > > shutdown() is called on worker thread, but it should be fine too as we > don't > > > > need to worry about terminating v8 execution. Are there other cases we > need > > > to > > > > take care? > > > > > > > > Also: it doesn't feel WorkerBackingThread should expose this detail here, > > > would > > > > we be able to move this part hidden in WorkerBackingThread? > > > > > > Imagine there are two WorkerThread x and y attached to a WorkerBackingThread > > and > > > we want to shutdown both. > > > > > > ----- > > > Call x->terminate() on the main thread. > > > Call y->terminate() on the main thread. > > > ----- > > > > > > It is possible that y->terminate() runs on the main thread before > > x->shutdown() > > > on the worker thread. In this case, TerminateExecution will never be called. > > > It's problematic if the worker backing loop runs an infinite JS loop: we > > cannot > > > terminate the worker backing thread. Moreover, if we call terminateAndWait > on > > > the main thread, it blocks the main thread too. > > > > Correction: > > before x->shutdown() on the worker thread. => before x->shutdown() runs on > the > > worker thread. > > if the worker backing loop runs => if the worker backing thread runs > > I see thanks, but then we need to fix the code or at least have TODO? This CL doesn't change the behavior on this point, and I think this is more related to "shutdown a web worker more gracefully" bug. Added a TODO comment.
On 2016/03/01 17:59:07, yhirano wrote: > On 2016/03/01 12:50:21, kinuko wrote: > > On 2016/03/01 00:04:53, yhirano wrote: > > > > > > https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): > > > > > > > > > https://codereview.chromium.org/1733353004/diff/160001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/workers/WorkerThread.cpp:401: } > > > On 2016/02/29 23:47:32, yhirano wrote: > > > > On 2016/02/29 09:32:53, kinuko wrote: > > > > > The comment just leaves readers unsure if this code is right or not... > > > > > initialize() sequence looks ok to me as attach won't be called once we > > pass > > > > this > > > > > part? While it looks it's possible that we don't take this branch but > > > > > shutdown() is called on worker thread, but it should be fine too as we > > don't > > > > > need to worry about terminating v8 execution. Are there other cases we > > need > > > > to > > > > > take care? > > > > > > > > > > Also: it doesn't feel WorkerBackingThread should expose this detail > here, > > > > would > > > > > we be able to move this part hidden in WorkerBackingThread? > > > > > > > > Imagine there are two WorkerThread x and y attached to a > WorkerBackingThread > > > and > > > > we want to shutdown both. > > > > > > > > ----- > > > > Call x->terminate() on the main thread. > > > > Call y->terminate() on the main thread. > > > > ----- > > > > > > > > It is possible that y->terminate() runs on the main thread before > > > x->shutdown() > > > > on the worker thread. In this case, TerminateExecution will never be > called. > > > > It's problematic if the worker backing loop runs an infinite JS loop: we > > > cannot > > > > terminate the worker backing thread. Moreover, if we call terminateAndWait > > on > > > > the main thread, it blocks the main thread too. > > > > > > Correction: > > > before x->shutdown() on the worker thread. => before x->shutdown() runs on > > the > > > worker thread. > > > if the worker backing loop runs => if the worker backing thread runs > > > > I see thanks, but then we need to fix the code or at least have TODO? > > This CL doesn't change the behavior on this point, and I think this is more > related to "shutdown a web worker more gracefully" bug. Added a TODO comment. Thanks. Yep I understand this is not a new issue, what concerned me was that the comment felt a bit unhelpful / unclear. (I'll take another look soon later)
This works for me, I initially wanted to avoid having logic that is only necessary for compositor worker in core/, but this looks easier to follow to me. https://codereview.chromium.org/1733353004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:22: class CORE_EXPORT WorkerBackingThread final : public ThreadSafeRefCounted<WorkerBackingThread> { Is there a chance that we could make this non-threadsafe-ref-counted?
https://codereview.chromium.org/1733353004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:22: class CORE_EXPORT WorkerBackingThread final : public ThreadSafeRefCounted<WorkerBackingThread> { On 2016/03/02 07:50:23, kinuko wrote: > Is there a chance that we could make this non-threadsafe-ref-counted? I made this RefCounted in PS8, but I'm not sure which is better. In PS8 a WorkerThread keeps having a detached WorkerBackingThread.
On 2016/03/02 22:35:07, yhirano wrote: > https://codereview.chromium.org/1733353004/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): > > https://codereview.chromium.org/1733353004/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/workers/WorkerBackingThread.h:22: class > CORE_EXPORT WorkerBackingThread final : public > ThreadSafeRefCounted<WorkerBackingThread> { > On 2016/03/02 07:50:23, kinuko wrote: > > Is there a chance that we could make this non-threadsafe-ref-counted? > > I made this RefCounted in PS8, but I'm not sure which is better. In PS8 a > WorkerThread keeps having a detached WorkerBackingThread. Hm, I hoped if we could get rid of ref-counting part but it looks it's not possible?
On 2016/03/03 08:39:38, kinuko wrote: > On 2016/03/02 22:35:07, yhirano wrote: > > > https://codereview.chromium.org/1733353004/diff/260001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): > > > > > https://codereview.chromium.org/1733353004/diff/260001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/workers/WorkerBackingThread.h:22: class > > CORE_EXPORT WorkerBackingThread final : public > > ThreadSafeRefCounted<WorkerBackingThread> { > > On 2016/03/02 07:50:23, kinuko wrote: > > > Is there a chance that we could make this non-threadsafe-ref-counted? > > > > I made this RefCounted in PS8, but I'm not sure which is better. In PS8 a > > WorkerThread keeps having a detached WorkerBackingThread. > > Hm, I hoped if we could get rid of ref-counting part but it looks it's not > possible? It's impossible as multiple WorkerThreads may share the ownership of one WorkerBackingThread.
vollick@: do you have any comments on modules/compositeworker/?
ping
I'm very sorry for the slow review. I'm a bit concerned about the lifetime changes in this CL. IIUC, the worker backing thread (and, consequently, the isolate) for CompositorWorker now sticks around until modules shutdown. This may be a perf win when we spin up a second CompositorWorker, but I don't have a sense of the cost. Could you estimate the memory impact? Is it necessary to have the backing thread survive the last CompositorWorker? I'd like to better understand what the tradeoff is here and if it's really needed. https://codereview.chromium.org/1733353004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp (right): https://codereview.chromium.org/1733353004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp:176: // Tests that a new WebThread is created if all existing workers are terminated before a new worker is created. This comment no longer reflects what the test does. Moreover, the old behavior did seem important as it ensured that when we stopped using CompositorWorkers that we'd stop incurring cost on the page.
On 2016/03/26 14:13:27, vollick wrote: > I'm very sorry for the slow review. I'm a bit concerned about the lifetime > changes in this CL. IIUC, the worker backing thread (and, consequently, the > isolate) for CompositorWorker now sticks around until modules shutdown. > The isolate is destructed when all compositor workers are gone. This CL doesn't change the behavior on this point. > This may be a perf win when we spin up a second CompositorWorker, but I don't > have a sense of the cost. Could you estimate the memory impact? Is it necessary > to have the backing thread survive the last CompositorWorker? > > I'd like to better understand what the tradeoff is here and if it's really > needed. > It's true that WorkerBackingThread and WebThreadSupportingGC instances remain until the global shutdown, but they are fairly light after WorkerBackingThread::shutdown is called, so I don't think it's problematic in terms of memory consumption.
On 2016/03/26 14:13:27, vollick wrote: > I'm very sorry for the slow review. I'm a bit concerned about the lifetime > changes in this CL. IIUC, the worker backing thread (and, consequently, the > isolate) for CompositorWorker now sticks around until modules shutdown. > The isolate is destructed when all compositor workers are gone. This CL doesn't change the behavior on this point. > This may be a perf win when we spin up a second CompositorWorker, but I don't > have a sense of the cost. Could you estimate the memory impact? Is it necessary > to have the backing thread survive the last CompositorWorker? > > I'd like to better understand what the tradeoff is here and if it's really > needed. > It's true that WorkerBackingThread and WebThreadSupportingGC instances remain until the global shutdown, but they are fairly light after WorkerBackingThread::shutdown is called, so I don't think it's problematic in terms of memory consumption.
On 2016/03/28 05:40:57, yhirano wrote: > On 2016/03/26 14:13:27, vollick wrote: > > I'm very sorry for the slow review. I'm a bit concerned about the lifetime > > changes in this CL. IIUC, the worker backing thread (and, consequently, the > > isolate) for CompositorWorker now sticks around until modules shutdown. > > > > The isolate is destructed when all compositor workers are gone. This CL doesn't > change the behavior on this point. > > > This may be a perf win when we spin up a second CompositorWorker, but I don't > > have a sense of the cost. Could you estimate the memory impact? Is it > necessary > > to have the backing thread survive the last CompositorWorker? > > > > I'd like to better understand what the tradeoff is here and if it's really > > needed. > > > It's true that WorkerBackingThread and WebThreadSupportingGC instances remain > until the global shutdown, but they are fairly light after > WorkerBackingThread::shutdown is called, so I don't think it's problematic in > terms of memory consumption. Thank you very much for the explanation. compositorworker/ lgtm.
Patchset #15 (id:460001) has been deleted
https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:36: void detach(); nit: please have comments for attach/detach https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:49: v8::Isolate* isolate() const { return m_isolate; } This pattern seems to have exist before your change, but returning non-const pointer from const method is not recommended https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:53: explicit WorkerBackingThread(WebThread*, bool shouldCallGCOnSHutdown); nit: no need of explicit https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:31: void reset() Is this only called in ctor? https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:49: RefPtr<WorkerBackingThread> m_thread; If we make WorkerThread not hold BackingThread but let each subclass of WorkerThread and this holder own it we can make it non-ref-counted?
https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:36: void detach(); On 2016/03/30 16:03:05, kinuko wrote: > nit: please have comments for attach/detach Done. https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:49: v8::Isolate* isolate() const { return m_isolate; } On 2016/03/30 16:03:05, kinuko wrote: > This pattern seems to have exist before your change, but returning non-const > pointer from const method is not recommended That's funny, some reviewers prefer it and I was once told to put const for such an interface :( https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:53: explicit WorkerBackingThread(WebThread*, bool shouldCallGCOnSHutdown); On 2016/03/30 16:03:05, kinuko wrote: > nit: no need of explicit Done. https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:31: void reset() On 2016/03/30 16:03:05, kinuko wrote: > Is this only called in ctor? Done. https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:49: RefPtr<WorkerBackingThread> m_thread; On 2016/03/30 16:03:05, kinuko wrote: > If we make WorkerThread not hold BackingThread but let each subclass of > WorkerThread and this holder own it we can make it non-ref-counted? Technically yes, but then we will have another ref-counted object. As I said before, it comes from the fact that the backing thread is shared by multiple scripts. The only solution I come up with is to stop sharing code between compositor workers and other workers.
https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:49: v8::Isolate* isolate() const { return m_isolate; } On 2016/03/31 08:29:11, yhirano wrote: > On 2016/03/30 16:03:05, kinuko wrote: > > This pattern seems to have exist before your change, but returning non-const > > pointer from const method is not recommended > > That's funny, some reviewers prefer it and I was once told to put const for such > an interface :( https://google.github.io/styleguide/cppguide.html#Use_of_const "Declare methods to be const whenever possible. Accessors should almost always be const. Other methods should be const if they do not modify any data members, do not call any non-const methods, and do not return a non-const pointer or non-const reference to a data member." Putting const is definitely preferred but if all these conditions apply.
https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:49: RefPtr<WorkerBackingThread> m_thread; On 2016/03/31 08:29:11, yhirano wrote: > On 2016/03/30 16:03:05, kinuko wrote: > > If we make WorkerThread not hold BackingThread but let each subclass of > > WorkerThread and this holder own it we can make it non-ref-counted? > > Technically yes, but then we will have another ref-counted object. As I said > before, it comes from the fact that the backing thread is shared by multiple > scripts. The only solution I come up with is to stop sharing code between > compositor workers and other workers. Ah.. I'm probably missing something here. Is it because even after we call modulesInitializer::shutdown() the thread might be still running? https://codereview.chromium.org/1733353004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:40: ASSERT(!m_thread || (m_thread->workerScriptCount() == 0)); When we're in ctor do we need these assertions?
https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:49: RefPtr<WorkerBackingThread> m_thread; On 2016/03/31 09:19:28, kinuko wrote: > On 2016/03/31 08:29:11, yhirano wrote: > > On 2016/03/30 16:03:05, kinuko wrote: > > > If we make WorkerThread not hold BackingThread but let each subclass of > > > WorkerThread and this holder own it we can make it non-ref-counted? > > > > Technically yes, but then we will have another ref-counted object. As I said > > before, it comes from the fact that the backing thread is shared by multiple > > scripts. The only solution I come up with is to stop sharing code between > > compositor workers and other workers. > > Ah.. I'm probably missing something here. Is it because even after we call > modulesInitializer::shutdown() the thread might be still running? Done. https://codereview.chromium.org/1733353004/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:40: ASSERT(!m_thread || (m_thread->workerScriptCount() == 0)); On 2016/03/31 09:19:28, kinuko wrote: > When we're in ctor do we need these assertions? Done.
kinuko@chromium.org changed reviewers: + blink-worker-reviews@chromium.org
Thanks, I feel the new ownership model's a bit simpler. A few more nits Also +blink-worker-reviews so that someone in the team can also take a look https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8IdleTaskRunner.h (right): https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8IdleTaskRunner.h:35: #include "public/platform/WebTraceLocation.h" Do we need this change here? https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:20: class CORE_EXPORT WorkerBackingThread final { Could we have a nice class-level comment for this? https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:32: // attach() and detach() atacches and detatches Oilpan and V8 to / from nit: atacches -> attaches detatches -> detaches https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:40: unsigned workerScriptCount() nit: put one empty line after detach() before workerScriptCount()
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org - blink-worker-reviews@chromium.org
On 2016/04/04 16:18:16, kinuko wrote: > Thanks, I feel the new ownership model's a bit simpler. A few more nits > > Also +blink-worker-reviews so that someone in the team can also take a look -blink-worker-reviews@ I'll review this on behalf of the worker team.
lgtm https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:63: unsigned m_workerScriptCount = 0; It'd be nice to have a comment about what |m_mutex| guards.
https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8IdleTaskRunner.h (right): https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8IdleTaskRunner.h:35: #include "public/platform/WebTraceLocation.h" On 2016/04/04 16:18:16, kinuko wrote: > Do we need this change here? BLINK_FROM_HERE is used in this file. As this file lacks this include directive, removing an unnecessary including directive in another file leads to a build failure. https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:20: class CORE_EXPORT WorkerBackingThread final { On 2016/04/04 16:18:16, kinuko wrote: > Could we have a nice class-level comment for this? I wrote something, though I'm not sure if it's nice. https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:32: // attach() and detach() atacches and detatches Oilpan and V8 to / from On 2016/04/04 16:18:16, kinuko wrote: > nit: > atacches -> attaches > detatches -> detaches Done. https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:40: unsigned workerScriptCount() On 2016/04/04 16:18:16, kinuko wrote: > nit: put one empty line after detach() before workerScriptCount() Done. https://codereview.chromium.org/1733353004/diff/620001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:63: unsigned m_workerScriptCount = 0; On 2016/04/05 10:04:33, nhiroki wrote: > It'd be nice to have a comment about what |m_mutex| guards. Done.
lgtm
yhirano@chromium.org changed reviewers: + dgozman@chromium.org, haraken@chromium.org
+dgozman@ for devtools related part (mostly in WorkerThread.cpp and WorkerBackingThread.cpp) - can you take a look? +haraken@ as a modules/ OWNER.
https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:35: { Shall we add: ASSERT(m_workerScriptCount == 0); ? https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:40: ASSERT(m_backingThread->isCurrentThread()); I think this ASSERT needs to be protected by the MutexLocker. I guess the access to m_backingThread will race. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:92: V8PerIsolateData::destroy(m_isolate); We initialize m_backingThread and then V8PerIsolateData. So we should destroy V8PerIsolateData and then m_backingThread. (Things should get destroyed in a reverse order in which they get initialized.) https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:69: unsigned m_workerScriptCount = 0; m_workerScriptCount => m_attachedWorkerThreadCount? https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:210: V8PerIsolateData::from(isolate())->setThreadDebugger(adoptPtr(new WorkerThreadDebugger(this, isolate()))); It would be better to call this in WorkerBackingThread::initialize. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:338: if (workerBackingThread().workerScriptCount() == 1) { Hmm, it looks a bit weird to expose the concept of workerScriptCount to outside of WorkerBackingThread... For example, what happens if workerBackingThread().workerScriptCount() is 2 at this point? Then the termination signal is never sent to the backing thread? https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:339: // This condition is not entirely correct because other scripts scripts => worker threads ? https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:407: if (isolate()) Nit: I don't think we need this check. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:23: static BackingThreadHolder& instance() It's a bit nasty to make this a singleton because BackingThreadHolder should have a clear lifetime -- it should be initialized when the first CompositorWorkerThread is initialized and be destroyed when the last CompositorWorkerThread is destroyed. You can fix it in a follow-up CL.
devtools lgtm modulo shouldAttachThreadDebugger https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:210: V8PerIsolateData::from(isolate())->setThreadDebugger(adoptPtr(new WorkerThreadDebugger(this, isolate()))); On 2016/04/06 09:23:17, haraken wrote: > > It would be better to call this in WorkerBackingThread::initialize. I agree. We'd better pass shouldAttachDebugger to WorkerThreadDebugger constructor, and remove shouldAttachThreadDebugger() entirely.
https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:35: { On 2016/04/06 09:23:16, haraken wrote: > > Shall we add: > > ASSERT(m_workerScriptCount == 0); > > ? Done. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:40: ASSERT(m_backingThread->isCurrentThread()); On 2016/04/06 09:23:17, haraken wrote: > > I think this ASSERT needs to be protected by the MutexLocker. I guess the access > to m_backingThread will race. > > After WebThread::shutdown is called, you mean? Then we need to track m_backingThread's status in a thread-safe manner, and I think it's too much. I Removed assertions and added a comment. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:92: V8PerIsolateData::destroy(m_isolate); On 2016/04/06 09:23:16, haraken wrote: > > We initialize m_backingThread and then V8PerIsolateData. So we should destroy > V8PerIsolateData and then m_backingThread. (Things should get destroyed in a > reverse order in which they get initialized.) Done. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.h:69: unsigned m_workerScriptCount = 0; On 2016/04/06 09:23:17, haraken wrote: > > m_workerScriptCount => m_attachedWorkerThreadCount? As in https://codereview.chromium.org/1728803002/, I think WorkerThread is a bad name and it should be called as something like WorkerScript. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:210: V8PerIsolateData::from(isolate())->setThreadDebugger(adoptPtr(new WorkerThreadDebugger(this, isolate()))); On 2016/04/06 16:50:17, dgozman wrote: > On 2016/04/06 09:23:17, haraken wrote: > > > > It would be better to call this in WorkerBackingThread::initialize. > > I agree. We'd better pass shouldAttachDebugger to WorkerThreadDebugger > constructor, and remove shouldAttachThreadDebugger() entirely. I don't think so. WorkerThreadDebugger interacts with the WorkerGlobalScope in WorkerThread, so WorkerBackingThread should not interact with it. This CL is separating thread-specific parts from WorkerThread, and WorkerThreadDebugger is not part of the thread-specific parts. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:338: if (workerBackingThread().workerScriptCount() == 1) { On 2016/04/06 09:23:17, haraken wrote: > > Hmm, it looks a bit weird to expose the concept of workerScriptCount to outside > of WorkerBackingThread... > > For example, what happens if workerBackingThread().workerScriptCount() is 2 at > this point? Then the termination signal is never sent to the backing thread? > I agree. I expect that the "graceful shutdown" will resolve the issue. See also: https://codereview.chromium.org/1733353004/#msg20. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:339: // This condition is not entirely correct because other scripts On 2016/04/06 09:23:17, haraken wrote: > > scripts => worker threads ? Ditto: I think it should not be called as "worker thread". https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:407: if (isolate()) On 2016/04/06 09:23:17, haraken wrote: > > Nit: I don't think we need this check. It's needed. Though I don't understand the mechanism, removing this condition leads to test failures. https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:23: static BackingThreadHolder& instance() On 2016/04/06 09:23:17, haraken wrote: > > It's a bit nasty to make this a singleton because BackingThreadHolder should > have a clear lifetime -- it should be initialized when the first > CompositorWorkerThread is initialized and be destroyed when the last > CompositorWorkerThread is destroyed. > > You can fix it in a follow-up CL. After all compositor workers are terminated, it's possible that a new compositor worker is initialized, right? The new compositor worker uses the same backing thread and hence the backing thread lives as long as the renderer lives, so having a singleton here looks natural to me.
https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:23: static BackingThreadHolder& instance() On 2016/04/07 08:12:40, yhirano wrote: > On 2016/04/06 09:23:17, haraken wrote: > > > > It's a bit nasty to make this a singleton because BackingThreadHolder should > > have a clear lifetime -- it should be initialized when the first > > CompositorWorkerThread is initialized and be destroyed when the last > > CompositorWorkerThread is destroyed. > > > > You can fix it in a follow-up CL. > > After all compositor workers are terminated, it's possible that a new compositor > worker is initialized, right? The new compositor worker uses the same backing > thread and hence the backing thread lives as long as the renderer lives, so > having a singleton here looks natural to me. I'd prefer destroying the BackingThreadHolder when the last compositor thread is gone to save memory. Then we can create another BackingThreadHolder when a new compositor thread starts. Anyways, this is just a drive-by comment. You don't need to address it in this CL. https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:87: V8GCController::collectAllGarbageForTesting(m_isolate); Can we move the GC logic to the testing code? (and remove m_shouldCallGCOnShutdown) https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:332: if (workerBackingThread().workerScriptCount() == 1) { I don't yet understand why we want to have this check. Before this CL, TerminateExecution was unconditionally sent when WorkerThread::terminateInternal() is called. Why do we want to make the change about the condition?
https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:87: V8GCController::collectAllGarbageForTesting(m_isolate); On 2016/04/07 11:54:45, haraken wrote: > > Can we move the GC logic to the testing code? (and remove > m_shouldCallGCOnShutdown) Having test-only code in production code is not really welcomed in general, but if this code is short-lived I'm fine either way. https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:332: if (workerBackingThread().workerScriptCount() == 1) { On 2016/04/07 11:54:45, haraken wrote: > > I don't yet understand why we want to have this check. Before this CL, > TerminateExecution was unconditionally sent when > WorkerThread::terminateInternal() is called. Why do we want to make the change > about the condition? WorkerThread::terminateV8Execution() was virtual method and overridden in CompositorWorker code to call TerminateExecution conditionally. My understanding is that WorkerBackingThread is an attempt to make the script:thread relationship more explicit in the core code, thus we have this here, I think it kinda makes sense. (Back then it was hidden in CompositorWorkerThread, which had its own benefit as we didn't need to expose this in core code, but instead we needed really a lot of virtual methods that get overridden in subclasses, which was making the overall code structure less clear-- that's my feeling)
Mostly looks good. https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:332: if (workerBackingThread().workerScriptCount() == 1) { On 2016/04/07 12:34:17, kinuko wrote: > On 2016/04/07 11:54:45, haraken wrote: > > > > I don't yet understand why we want to have this check. Before this CL, > > TerminateExecution was unconditionally sent when > > WorkerThread::terminateInternal() is called. Why do we want to make the change > > about the condition? > > WorkerThread::terminateV8Execution() was virtual method and overridden in > CompositorWorker code to call TerminateExecution conditionally. My > understanding is that WorkerBackingThread is an attempt to make the > script:thread relationship more explicit in the core code, thus we have this > here, I think it kinda makes sense. (Back then it was hidden in > CompositorWorkerThread, which had its own benefit as we didn't need to expose > this in core code, but instead we needed really a lot of virtual methods that > get overridden in subclasses, which was making the overall code structure less > clear-- that's my feeling) Ah, thanks, got it. I understand this CL doesn't change the existing behavior. BTW, why do you not send the termination signal when workerScriptCount > 1? I think the intention of the termination signal is to kill the target thread immediately in any event. https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:332: if (workerBackingThread().workerScriptCount() == 1) { I still think it's not a good idea to expose the concept of workerScriptCount() to outside WorkerBackingThread. Would it be an option to move the termination logic into WorkerBackingThread?
https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:332: if (workerBackingThread().workerScriptCount() == 1) { On 2016/04/07 13:55:08, haraken wrote: > > I still think it's not a good idea to expose the concept of workerScriptCount() > to outside WorkerBackingThread. Would it be an option to move the termination > logic into WorkerBackingThread? I basically agree with this (I had the same request/question), while given that we have clear TODO comment and this logic is to be revisited / cleaned up soon again having this code as is in this patch works for me too, this still looks better than the existing code. (Leaving the option to make further changes in this patch up to hirano-san and you) https://codereview.chromium.org/1733353004/diff/740001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.cpp:332: if (workerBackingThread().workerScriptCount() == 1) { On 2016/04/07 13:55:08, haraken wrote: > On 2016/04/07 12:34:17, kinuko wrote: > > On 2016/04/07 11:54:45, haraken wrote: > > > > > > I don't yet understand why we want to have this check. Before this CL, > > > TerminateExecution was unconditionally sent when > > > WorkerThread::terminateInternal() is called. Why do we want to make the > change > > > about the condition? > > > > WorkerThread::terminateV8Execution() was virtual method and overridden in > > CompositorWorker code to call TerminateExecution conditionally. My > > understanding is that WorkerBackingThread is an attempt to make the > > script:thread relationship more explicit in the core code, thus we have this > > here, I think it kinda makes sense. (Back then it was hidden in > > CompositorWorkerThread, which had its own benefit as we didn't need to expose > > this in core code, but instead we needed really a lot of virtual methods that > > get overridden in subclasses, which was making the overall code structure less > > clear-- that's my feeling) > > Ah, thanks, got it. I understand this CL doesn't change the existing behavior. > > BTW, why do you not send the termination signal when workerScriptCount > 1? I > think the intention of the termination signal is to kill the target thread > immediately in any event. As far as the thread is shared we can't really forcibly kill each script execution without stopping other scripts in the current model, and sending immediate kill signal is not the only purpose of terminate(), as we should still be able to stop the script execution unless the script has infinite loop etc (and that's what we're considering applying to other workers too in graceful shutdown mode). We'll still want to (immediately) kill the thread's script execution in blink shutdown sequence, and in the case we call terminateAndWaitForAllworkers so we should be eventually reaching workerScriptCount == 1.
> https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/workers/WorkerThread.cpp:210: > V8PerIsolateData::from(isolate())->setThreadDebugger(adoptPtr(new > WorkerThreadDebugger(this, isolate()))); > On 2016/04/06 16:50:17, dgozman wrote: > > On 2016/04/06 09:23:17, haraken wrote: > > > > > > It would be better to call this in WorkerBackingThread::initialize. > > > > I agree. We'd better pass shouldAttachDebugger to WorkerThreadDebugger > > constructor, and remove shouldAttachThreadDebugger() entirely. > > I don't think so. WorkerThreadDebugger interacts with the WorkerGlobalScope in > WorkerThread, so WorkerBackingThread should not interact with it. This CL is > separating thread-specific parts from WorkerThread, and WorkerThreadDebugger is > not part of the thread-specific parts. I cannot agree. WorkerThreadDebugger is about thread (running message loop on pause) and isolate (owning v8 debugger, interrupting it), which are both thread-specific. WDYT? If you are concerned about single usage of workerGlobalScope - we can just pass the url it retrieves as a parameter to contextCreated. Maybe I don't understand something, but now I'm surprised that all task-related methods are left in WorkerThread instead of WorkerBackingThread. Tasks are about threading after all. Same goes for running nested debugger message loop on pause.
On 2016/04/07 14:51:18, dgozman wrote: > > > https://codereview.chromium.org/1733353004/diff/660001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/workers/WorkerThread.cpp:210: > > V8PerIsolateData::from(isolate())->setThreadDebugger(adoptPtr(new > > WorkerThreadDebugger(this, isolate()))); > > On 2016/04/06 16:50:17, dgozman wrote: > > > On 2016/04/06 09:23:17, haraken wrote: > > > > > > > > It would be better to call this in WorkerBackingThread::initialize. > > > > > > I agree. We'd better pass shouldAttachDebugger to WorkerThreadDebugger > > > constructor, and remove shouldAttachThreadDebugger() entirely. > > > > I don't think so. WorkerThreadDebugger interacts with the WorkerGlobalScope in > > WorkerThread, so WorkerBackingThread should not interact with it. This CL is > > separating thread-specific parts from WorkerThread, and WorkerThreadDebugger > is > > not part of the thread-specific parts. > > I cannot agree. WorkerThreadDebugger is about thread (running message loop on > pause) and isolate (owning v8 debugger, interrupting it), which are both > thread-specific. WDYT? If you are concerned about single usage of > workerGlobalScope - we can just pass the url it retrieves as a parameter to > contextCreated. > > Maybe I don't understand something, but now I'm surprised that all task-related > methods are left in WorkerThread instead of WorkerBackingThread. Tasks are about > threading after all. Same goes for running nested debugger message loop on > pause. A thread can be shared by multiple multiple worker scripts. In that case the relationship between isolate and WorkerGlobalScope is not 1:1. That is the reason why I don't want WorkerBackingThread to interact with WorkerThreadDebugger.
> A thread can be shared by multiple multiple worker scripts. In that case the > relationship between isolate and WorkerGlobalScope is not 1:1. That is the > reason why I don't want WorkerBackingThread to interact with > WorkerThreadDebugger. But WorkerThreadDebugger does actually debug the isolate shared between scripts and pauses the thread, it has nothing to do with WorkerGlobalScope.
On 2016/04/07 16:58:14, dgozman wrote: > > A thread can be shared by multiple multiple worker scripts. In that case the > > relationship between isolate and WorkerGlobalScope is not 1:1. That is the > > reason why I don't want WorkerBackingThread to interact with > > WorkerThreadDebugger. > > But WorkerThreadDebugger does actually debug the isolate shared between scripts > and pauses the thread, it has nothing to do with WorkerGlobalScope. For example, WorkerThreadDebugger calls[1] WorkerThread::startRunningDebuggerTasksOnPause which uses the WorkerThread's WorkerGalobalScope[2]. 1: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... 2: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I don't fully understand what it does, so it could be possible that it actually doesn't depend on WorkerGlobalScope / ScriptState / etc, but I don't want to do the refactoring in this CL, and I don't want WorkerBackingThread to interact with WotkerThreadDebugger at least for now. I'd appreciate if you could do the refactoring after this CL is landed.
On 2016/04/07 17:40:20, yhirano wrote: > On 2016/04/07 16:58:14, dgozman wrote: > > > A thread can be shared by multiple multiple worker scripts. In that case the > > > relationship between isolate and WorkerGlobalScope is not 1:1. That is the > > > reason why I don't want WorkerBackingThread to interact with > > > WorkerThreadDebugger. > > > > But WorkerThreadDebugger does actually debug the isolate shared between > scripts > > and pauses the thread, it has nothing to do with WorkerGlobalScope. > > For example, WorkerThreadDebugger calls[1] > WorkerThread::startRunningDebuggerTasksOnPause which uses the WorkerThread's > WorkerGalobalScope[2]. > > 1: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > 2: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > I don't fully understand what it does, so it could be possible that it actually > doesn't depend on WorkerGlobalScope / ScriptState / etc, but I don't want to do > the refactoring in this CL, and I don't want WorkerBackingThread to interact > with WotkerThreadDebugger at least for now. > I'd appreciate if you could do the refactoring after this CL is landed. Sure, I can follow up. Could you please outline somewhere (or is it already in some doc?) what's the desired responsibility of WorkerThread vs WorkerBackingThread. I'm especially interested in where postTasking and debugger tasks should go.
On 2016/04/07 18:00:13, dgozman wrote: > On 2016/04/07 17:40:20, yhirano wrote: > > On 2016/04/07 16:58:14, dgozman wrote: > > > > A thread can be shared by multiple multiple worker scripts. In that case > the > > > > relationship between isolate and WorkerGlobalScope is not 1:1. That is the > > > > reason why I don't want WorkerBackingThread to interact with > > > > WorkerThreadDebugger. > > > > > > But WorkerThreadDebugger does actually debug the isolate shared between > > scripts > > > and pauses the thread, it has nothing to do with WorkerGlobalScope. > > > > For example, WorkerThreadDebugger calls[1] > > WorkerThread::startRunningDebuggerTasksOnPause which uses the WorkerThread's > > WorkerGalobalScope[2]. > > > > 1: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > 2: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > I don't fully understand what it does, so it could be possible that it > actually > > doesn't depend on WorkerGlobalScope / ScriptState / etc, but I don't want to > do > > the refactoring in this CL, and I don't want WorkerBackingThread to interact > > with WotkerThreadDebugger at least for now. > > I'd appreciate if you could do the refactoring after this CL is landed. > > Sure, I can follow up. Could you please outline somewhere (or is it already in > some doc?) what's the desired responsibility of WorkerThread vs > WorkerBackingThread. I'm especially interested in where postTasking and debugger > tasks should go. (I think the underlying issue is that we're introducing too many classes around worker. I'm confused with the responsibility of each class as well. WorkerThread, WorkerBackingThread, WorkerScriptController, WorkerGlobalScope etc.)
On 2016/04/07 18:00:13, dgozman wrote: > On 2016/04/07 17:40:20, yhirano wrote: > > On 2016/04/07 16:58:14, dgozman wrote: > > > > A thread can be shared by multiple multiple worker scripts. In that case > the > > > > relationship between isolate and WorkerGlobalScope is not 1:1. That is the > > > > reason why I don't want WorkerBackingThread to interact with > > > > WorkerThreadDebugger. > > > > > > But WorkerThreadDebugger does actually debug the isolate shared between > > scripts > > > and pauses the thread, it has nothing to do with WorkerGlobalScope. > > > > For example, WorkerThreadDebugger calls[1] > > WorkerThread::startRunningDebuggerTasksOnPause which uses the WorkerThread's > > WorkerGalobalScope[2]. > > > > 1: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > 2: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > I don't fully understand what it does, so it could be possible that it > actually > > doesn't depend on WorkerGlobalScope / ScriptState / etc, but I don't want to > do > > the refactoring in this CL, and I don't want WorkerBackingThread to interact > > with WotkerThreadDebugger at least for now. > > I'd appreciate if you could do the refactoring after this CL is landed. > > Sure, I can follow up. Could you please outline somewhere (or is it already in > some doc?) what's the desired responsibility of WorkerThread vs > WorkerBackingThread. I'm especially interested in where postTasking and debugger > tasks should go. Thanks, I added comments in WorkerBackingThread.h. Kinuko wrote a document[1], so I'll update the description. 1: https://docs.google.com/document/d/1i3IA3TG00rpQ7MKlpNFYUF6EfLcV01_Cv3IYG_DjF...
Chatted offline. LGTM assuming that you fix the following issues in follow-up CLs: 1) The logic to send a termination signal is broken. 2) The relationship between WorkerThread, WorkerBackingThread, WebThreadSupportingGC and WorkerScriptController is unclear. 2) is the main reason I'm not fully happy about introducing another new class (i.e., WorkerBackingThread), but I'm okay with landing this CL and addressing the issue later. I want to see a clear plan before renaming WorkerThread to WorkerScript etc.
> Thanks, I added comments in WorkerBackingThread.h. Kinuko wrote a document[1], > so I'll update the description. > > 1: > https://docs.google.com/document/d/1i3IA3TG00rpQ7MKlpNFYUF6EfLcV01_Cv3IYG_DjF... Sorry, but I didn't find anything related to WorkerBackingThread in this document. Could you please follow up with more detailed description? I also find it unfortunate that we now have two cross-thread classes (WorkerThread and WorkerBackingThread) instead of one.
Looks like some of the confusions coming from the fact that WorkerThreadDebugger has not been used / enabled at all for CompositorWorker where an irregular thread setting is being applied and also where this patch makes a little more sense. Yutaka-- maybe look into how MainThreadDebugger is being hooked up might help us clarify how things should be laid out? Also: I wonder if renaming WorkerBackingThread to WorkerThreadAdapter or something that implies thinner layer just for glueing things together might help. On 2016/04/11 17:41:13, dgozman wrote: > > Thanks, I added comments in WorkerBackingThread.h. Kinuko wrote a document[1], > > so I'll update the description. > > > > 1: > > > https://docs.google.com/document/d/1i3IA3TG00rpQ7MKlpNFYUF6EfLcV01_Cv3IYG_DjF... > > Sorry, but I didn't find anything related to WorkerBackingThread in this > document. Could you please follow up with more detailed description? The doc was written before-WorkerBackingThread-era and it doesn't have any notion of it (yet). Yutaka seems to have meant the comment added in WorkerBackingThread.h. > I also find it unfortunate that we now have two cross-thread classes > (WorkerThread and WorkerBackingThread) instead of one. If we face it these are not the only two. We also have WebThreadSupportingGC, WebThread and base::Thread. When WorkerThread:isolate:backing-thread was always 1:1:1 the existing code structure was simple and good enough, but that no longer holds true and I don't feel happy with the current situation either. Introducing one extra layer needs to be always considered very carefully but I can see some benefits for introducing this patch, we just seem to need a bit more research about how the inspector could be hooked up if we take this path (or not).
On 2016/04/11 23:42:34, kinuko wrote: > Looks like some of the confusions coming from the fact that WorkerThreadDebugger > has not been used / enabled at all for CompositorWorker where an irregular > thread setting is being applied and also where this patch makes a little more > sense. > > Yutaka-- maybe look into how MainThreadDebugger is being hooked up might help us > clarify how things should be laid out? > > Also: I wonder if renaming WorkerBackingThread to WorkerThreadAdapter or > something that implies thinner layer just for glueing things together might > help. > > On 2016/04/11 17:41:13, dgozman wrote: > > > Thanks, I added comments in WorkerBackingThread.h. Kinuko wrote a > document[1], > > > so I'll update the description. > > > > > > 1: > > > > > > https://docs.google.com/document/d/1i3IA3TG00rpQ7MKlpNFYUF6EfLcV01_Cv3IYG_DjF... > > > > Sorry, but I didn't find anything related to WorkerBackingThread in this > > document. Could you please follow up with more detailed description? > > The doc was written before-WorkerBackingThread-era and it doesn't have any > notion of it (yet). Yutaka seems to have meant the comment added in > WorkerBackingThread.h. > > > I also find it unfortunate that we now have two cross-thread classes > > (WorkerThread and WorkerBackingThread) instead of one. > > If we face it these are not the only two. We also have WebThreadSupportingGC, > WebThread and base::Thread. When WorkerThread:isolate:backing-thread was always > 1:1:1 the existing code structure was simple and good enough, but that no longer > holds true and I don't feel happy with the current situation either. > Introducing one extra layer needs to be always considered very carefully but I > can see some benefits for introducing this patch, we just seem to need a bit > more research about how the inspector could be hooked up if we take this path > (or not). Thanks for clarifications! I'm not against this patch by any means, just want clear understanding of where do we move. It's not only about inspector, but a bit more general. For example, where does post-tasking go? Who manages the lifetime of worker implementation? Should we only do single WorkerMicrotaskRunner per isolate when it's shared? How do worklets fit here? Thank you for cleaning up what we have now!
On 2016/04/12 00:48:51, dgozman wrote: > On 2016/04/11 23:42:34, kinuko wrote: > > Looks like some of the confusions coming from the fact that > WorkerThreadDebugger > > has not been used / enabled at all for CompositorWorker where an irregular > > thread setting is being applied and also where this patch makes a little more > > sense. > > > > Yutaka-- maybe look into how MainThreadDebugger is being hooked up might help > us > > clarify how things should be laid out? > > > > Also: I wonder if renaming WorkerBackingThread to WorkerThreadAdapter or > > something that implies thinner layer just for glueing things together might > > help. > > > > On 2016/04/11 17:41:13, dgozman wrote: > > > > Thanks, I added comments in WorkerBackingThread.h. Kinuko wrote a > > document[1], > > > > so I'll update the description. > > > > > > > > 1: > > > > > > > > > > https://docs.google.com/document/d/1i3IA3TG00rpQ7MKlpNFYUF6EfLcV01_Cv3IYG_DjF... > > > > > > Sorry, but I didn't find anything related to WorkerBackingThread in this > > > document. Could you please follow up with more detailed description? > > > > The doc was written before-WorkerBackingThread-era and it doesn't have any > > notion of it (yet). Yutaka seems to have meant the comment added in > > WorkerBackingThread.h. > > > > > I also find it unfortunate that we now have two cross-thread classes > > > (WorkerThread and WorkerBackingThread) instead of one. > > > > If we face it these are not the only two. We also have WebThreadSupportingGC, > > WebThread and base::Thread. When WorkerThread:isolate:backing-thread was > always > > 1:1:1 the existing code structure was simple and good enough, but that no > longer > > holds true and I don't feel happy with the current situation either. > > Introducing one extra layer needs to be always considered very carefully but I > > can see some benefits for introducing this patch, we just seem to need a bit > > more research about how the inspector could be hooked up if we take this path > > (or not). > > Thanks for clarifications! I'm not against this patch by any means, just want > clear understanding of where do we move. > It's not only about inspector, but a bit more general. For example, where does > post-tasking go? Who manages the lifetime of worker implementation? Should we > only do single WorkerMicrotaskRunner per isolate when it's shared? How do > worklets fit here? Yep these are all very good questions. Where post-tasking and WorkerMicrotaskRunner should go needs to be clarified as you say. Worker lifetime is defined differently per worker types and is managed outside the common worker code. Worklet doesn't have all these worker threads stuff so we could possibly think about it separately for now. > Thank you for cleaning up what we have now! Thanks for all these feedback!
Thanks for feedback. I'm landing this CL, and will update documents and design with the worker team.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org, nhiroki@chromium.org, kinuko@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1733353004/#ps780001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1733353004/780001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1733353004/780001
Message was sent while issue was closed.
Description was changed from ========== Introduce WorkerBackingThread This change decouples thread-related part from WorkerThread. Now multiple WorkerThread can be attached to one WorkerBackingThread and one WorkerBackingThread is associated to one platform thread. BUG=575532 ========== to ========== Introduce WorkerBackingThread This change decouples thread-related part from WorkerThread. Now multiple WorkerThread can be attached to one WorkerBackingThread and one WorkerBackingThread is associated to one platform thread. BUG=575532 ==========
Message was sent while issue was closed.
Committed patchset #30 (id:780001)
Message was sent while issue was closed.
Description was changed from ========== Introduce WorkerBackingThread This change decouples thread-related part from WorkerThread. Now multiple WorkerThread can be attached to one WorkerBackingThread and one WorkerBackingThread is associated to one platform thread. BUG=575532 ========== to ========== Introduce WorkerBackingThread This change decouples thread-related part from WorkerThread. Now multiple WorkerThread can be attached to one WorkerBackingThread and one WorkerBackingThread is associated to one platform thread. BUG=575532 Committed: https://crrev.com/378375cdaef0338e971e8c997ef6323a043499b6 Cr-Commit-Position: refs/heads/master@{#386948} ==========
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/378375cdaef0338e971e8c997ef6323a043499b6 Cr-Commit-Position: refs/heads/master@{#386948}
Message was sent while issue was closed.
On 2016/04/13 09:34:32, commit-bot: I haz the power wrote: > Patchset 30 (id:??) landed as > https://crrev.com/378375cdaef0338e971e8c997ef6323a043499b6 > Cr-Commit-Position: refs/heads/master@{#386948} Hey, it looks like this patch may cause a WebThreadSupportingGC to be created during shutdown (it tries to create something due to an ::instance() call). Here's a stack trace: #0 blink::WebThreadSupportingGC::WebThreadSupportingGC (this=0x2af35e42d890, name=0x0, thread=0x0) at ../../third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp:26 #1 in blink::WebThreadSupportingGC::createForThread (thread=0x0) at ../../third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp:20 #2 in blink::WorkerBackingThread::WorkerBackingThread (this=0x1ddd409156a0, thread=0x0, shouldCallGCOnShutdown=false) at ../../third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:28 #3 in blink::WorkerBackingThread::create (thread=0x0) at ../../third_party/WebKit/Source/core/workers/WorkerBackingThread.h:31 #4 in blink::(anonymous namespace)::BackingThreadHolder::BackingThreadHolder (this=0x1ddd40c80460) at ../../third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:38 #5 in blink::(anonymous namespace)::BackingThreadHolder::instance () at ../../third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:25 #6 in blink::CompositorWorkerThread::clearSharedBackingThread () at ../../third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:76 #7 in blink::ModulesInitializer::shutdown (this=0x2af35e440190) at ../../third_party/WebKit/Source/modules/ModulesInitializer.cpp:64 #8 in blink::shutdown () at ../../third_party/WebKit/Source/web/WebKit.cpp:111 #9 in content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport (this=0x1ddd4083ede0) at ../../content/test/test_blink_web_unit_test_support.cc:157
Message was sent while issue was closed.
On 2016/04/18 21:28:13, vollick wrote: > On 2016/04/13 09:34:32, commit-bot: I haz the power wrote: > > Patchset 30 (id:??) landed as > > https://crrev.com/378375cdaef0338e971e8c997ef6323a043499b6 > > Cr-Commit-Position: refs/heads/master@{#386948} > > Hey, it looks like this patch may cause a WebThreadSupportingGC to be created > during shutdown (it tries to create something due to an ::instance() call). > Here's a stack trace: > > #0 blink::WebThreadSupportingGC::WebThreadSupportingGC (this=0x2af35e42d890, > name=0x0, thread=0x0) at > ../../third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp:26 > #1 in blink::WebThreadSupportingGC::createForThread (thread=0x0) at > ../../third_party/WebKit/Source/platform/WebThreadSupportingGC.cpp:20 > #2 in blink::WorkerBackingThread::WorkerBackingThread (this=0x1ddd409156a0, > thread=0x0, shouldCallGCOnShutdown=false) at > ../../third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:28 > #3 in blink::WorkerBackingThread::create (thread=0x0) at > ../../third_party/WebKit/Source/core/workers/WorkerBackingThread.h:31 > #4 in blink::(anonymous namespace)::BackingThreadHolder::BackingThreadHolder > (this=0x1ddd40c80460) at > ../../third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:38 > #5 in blink::(anonymous namespace)::BackingThreadHolder::instance () at > ../../third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:25 > #6 in blink::CompositorWorkerThread::clearSharedBackingThread () at > ../../third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:76 > #7 in blink::ModulesInitializer::shutdown (this=0x2af35e440190) at > ../../third_party/WebKit/Source/modules/ModulesInitializer.cpp:64 > #8 in blink::shutdown () at ../../third_party/WebKit/Source/web/WebKit.cpp:111 > #9 in content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport > (this=0x1ddd4083ede0) at > ../../content/test/test_blink_web_unit_test_support.cc:157 Thanks, I will fix it. |
