Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2025783002:
Worker: Introduce an observation mechanism for WorkerThread termination (Closed)
|
Created:
4 years, 6 months ago by nhiroki Modified:
4 years, 3 months ago CC:
chromium-reviews, blink-reviews, kinuko+worker_chromium.org, falken, blink-worker-reviews_chromium.org, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@delayed_task Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorker: Introduce an observation mechanism for WorkerThread termination
This mechanism provides a way to observe WorkerThread termination from the main
thread. This is useful when an object living on the main thread needs to release
references to objects on the worker thread before it gets terminated.
As an example of using this mechanism, this CL applies it to WebSocket.
WorkerWebSocketChannel::Peer observes worker thread termination from the main
thread and asks DocumentWebSocketChannel to release references to objects on the
worker thread when the termination happens.
Design doc: https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5WQ0/edit
BUG=575532
Committed: https://crrev.com/ff24de0d44123ca7ae9b9511268fdd926356a36f
Cr-Commit-Position: refs/heads/master@{#398886}
Patch Set 1 : #Patch Set 2 : maybe fix tests #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : address review comments #Patch Set 5 : remake and add tests #
Total comments: 16
Patch Set 6 : rebase #Patch Set 7 : address review comments #Patch Set 8 : update implementation comments #
Total comments: 4
Patch Set 9 : Stop tracing CrossThread(Weak)Persistents #
Total comments: 6
Depends on Patchset: Messages
Total messages: 60 (26 generated)
Description was changed from ========== [WIP] Worker: Introduce an observation mechanism for WorkerThraed termination BUG=575532 ========== to ========== [WIP] Worker: Introduce an observation mechanism for WorkerThread termination BUG=575532 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [WIP] Worker: Introduce an observation mechanism for WorkerThread termination BUG=575532 ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful if an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it for WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects living on the worker thread when the termination happens. BUG=575532 ==========
Description was changed from ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful if an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it for WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects living on the worker thread when the termination happens. BUG=575532 ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful if an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it for WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects living on the worker thread when the termination happens. BUG=575532 ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Description was changed from ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful if an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it for WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects living on the worker thread when the termination happens. BUG=575532 ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it for WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects living on the worker thread when the termination happens. BUG=575532 ==========
Patchset #1 (id:140001) has been deleted
Description was changed from ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it for WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects living on the worker thread when the termination happens. BUG=575532 ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it for WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. BUG=575532 ==========
nhiroki@chromium.org changed reviewers: + yhirano@chromium.org
Hi yhirano@, can you give me your early feedback on this? If this looks good to you, I'll add tests and implementation comments. Thanks!
Description was changed from ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it for WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. BUG=575532 ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. BUG=575532 ==========
https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h (right): https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:16: class CORE_EXPORT WorkerThreadLifecycleObserver : public LifecycleObserver<WorkerThreadContext, WorkerThreadLifecycleObserver> { +comments https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:20: bool isWorkerThreadTerminated() const; +comments https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:20: bool isWorkerThreadTerminated() const; On the design discussion at https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W..., there were negative feedback for "polling" function. See comments started by Sami on the doc. https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:417: // for more details. We need to wait here for the synchronous mixed content check.
Thank you for reviewing! Can you take another look? As we chatted offline, the latest patchset5 creates WorkerWebSocketChannel::Peer not on the worker thread but on the main thread because WorkerThreadLifecycleObserver should live on the main thread. https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h (right): https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:16: class CORE_EXPORT WorkerThreadLifecycleObserver : public LifecycleObserver<WorkerThreadContext, WorkerThreadLifecycleObserver> { On 2016/06/03 07:40:36, yhirano wrote: > +comments Done. https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:20: bool isWorkerThreadTerminated() const; On 2016/06/03 07:40:37, yhirano wrote: > On the design discussion at > https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W..., > there were negative feedback for "polling" function. See comments started by > Sami on the doc. Oh, I missed that comment. Tweaked this function. https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:20: bool isWorkerThreadTerminated() const; On 2016/06/03 07:40:36, yhirano wrote: > +comments Done. https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2025783002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:417: // for more details. On 2016/06/03 07:40:37, yhirano wrote: > We need to wait here for the synchronous mixed content check. I see. Revised this comment.
https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h (right): https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:36: }; no semicolon https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h (right): https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h:76: MockWorkerThreadLifecycleObserver(WorkerThreadContext* context) +explicit https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:192: ASSERT(!isMainThread()); DCHECK(isMainThread()) https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:369: } m_bridge = nullptr; https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:396: DCHECK(isMainThread()); +DCHECK(!m_peer); https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:398: m_peer->initialize(std::move(location), context); [optional] Is it good to set |m_peer| only when necessary? Peer* peer = new Peer(...); if (peer->initialize(...)) m_peer = peer; m_syncHelper->signalWorkerThread(); https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h (right): https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:87: // Allocated and used in the main thread, but destructed in the worker |Peer| is an on-heap class so it will be destructed on the allocated thread. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:122: Member<Bridge> m_bridge; If you use CrossThreadPersistent in Member, this should be CrossThreadWeakPersistent.
Thank you for reviewing! Updated. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h (right): https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:36: }; On 2016/06/09 04:55:13, yhirano wrote: > no semicolon Done. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h (right): https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h:76: MockWorkerThreadLifecycleObserver(WorkerThreadContext* context) On 2016/06/09 04:55:13, yhirano wrote: > +explicit Done. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:192: ASSERT(!isMainThread()); On 2016/06/09 04:55:13, yhirano wrote: > DCHECK(isMainThread()) Done. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:369: } On 2016/06/09 04:55:13, yhirano wrote: > m_bridge = nullptr; Done. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:396: DCHECK(isMainThread()); On 2016/06/09 04:55:13, yhirano wrote: > +DCHECK(!m_peer); Done. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:398: m_peer->initialize(std::move(location), context); On 2016/06/09 04:55:13, yhirano wrote: > [optional] Is it good to set |m_peer| only when necessary? > > Peer* peer = new Peer(...); > if (peer->initialize(...)) > m_peer = peer; > m_syncHelper->signalWorkerThread(); Done. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h (right): https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:87: // Allocated and used in the main thread, but destructed in the worker On 2016/06/09 04:55:13, yhirano wrote: > |Peer| is an on-heap class so it will be destructed on the allocated thread. Done. https://codereview.chromium.org/2025783002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:122: Member<Bridge> m_bridge; On 2016/06/09 04:55:13, yhirano wrote: > If you use CrossThreadPersistent in Member, this should be > CrossThreadWeakPersistent. Done.
nhiroki@chromium.org changed reviewers: + hiroshige@chromium.org
+hiroshige, can you review CrossThreadCopier.h? Thanks!
lgtm, thank you!
nhiroki@chromium.org changed reviewers: + haraken@chromium.org
+haraken@, can you review core/ and platform/? Thanks!
Description was changed from ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. BUG=575532 ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. Design doc: https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W... BUG=575532 ==========
platform/ LGTM
Sorry for after-l-g-t-m comments, I've just noticed I don't know if we should trace CrossThread[Weak]Persistent. https://codereview.chromium.org/2025783002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2025783002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:375: visitor->trace(m_bridge); Peer::m_bridge is not Member. Should we keep this? https://codereview.chromium.org/2025783002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:505: visitor->trace(m_peer); Bridge::m_peer is not Member. Should we keep this?
On 2016/06/09 11:36:20, yhirano wrote: > Sorry for after-l-g-t-m comments, I've just noticed I don't know if we should > trace CrossThread[Weak]Persistent. Good point. I don't think we need to trace CrossThread(Weak)Persistent because they should be traced as GC roots like (Weak)Persistent. In addition, BlinkGCAPIReference.md does not say they must be traced.
Updated. https://codereview.chromium.org/2025783002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2025783002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:375: visitor->trace(m_bridge); On 2016/06/09 11:36:20, yhirano wrote: > Peer::m_bridge is not Member. Should we keep this? Removed. https://codereview.chromium.org/2025783002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:505: visitor->trace(m_peer); On 2016/06/09 11:36:20, yhirano wrote: > Bridge::m_peer is not Member. Should we keep this? Removed.
On 2016/06/09 13:23:21, nhiroki wrote: > On 2016/06/09 11:36:20, yhirano wrote: > > Sorry for after-l-g-t-m comments, I've just noticed I don't know if we should > > trace CrossThread[Weak]Persistent. > > Good point. I don't think we need to trace CrossThread(Weak)Persistent because > they should be traced as GC roots like (Weak)Persistent. In addition, > BlinkGCAPIReference.md does not say they must be traced. You don't need to trace them. Hopefully a GC plugin will catch the error (if not, please tell us).
On 2016/06/09 13:35:06, haraken wrote: > On 2016/06/09 13:23:21, nhiroki wrote: > > On 2016/06/09 11:36:20, yhirano wrote: > > > Sorry for after-l-g-t-m comments, I've just noticed I don't know if we > should > > > trace CrossThread[Weak]Persistent. > > > > Good point. I don't think we need to trace CrossThread(Weak)Persistent because > > they should be traced as GC roots like (Weak)Persistent. In addition, > > BlinkGCAPIReference.md does not say they must be traced. > > You don't need to trace them. > > Hopefully a GC plugin will catch the error (if not, please tell us). Thank you for the information. On the patchset 8, CrossThread(Weak)Persistents are traced but the plugin doesn't complain.
haraken@chromium.org changed reviewers: + sigbjornf@opera.com
> Thank you for the information. On the patchset 8, CrossThread(Weak)Persistents > are traced but the plugin doesn't complain. +sigbjorn
On 2016/06/09 14:15:07, haraken wrote: > > Thank you for the information. On the patchset 8, CrossThread(Weak)Persistents > > are traced but the plugin doesn't complain. > > +sigbjorn Thanks, the gc plugin hasn't caught up with these newer *Persistent variants.
On 2016/06/09 14:24:30, sof wrote: > On 2016/06/09 14:15:07, haraken wrote: > > > Thank you for the information. On the patchset 8, > CrossThread(Weak)Persistents > > > are traced but the plugin doesn't complain. > > > > +sigbjorn > > Thanks, the gc plugin hasn't caught up with these newer *Persistent variants. Acknowledged. Thanks!
The CQ bit was checked by nhiroki@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/2025783002/#ps320001 (title: "Stop tracing CrossThread(Weak)Persistents")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2025783002/320001
Message was sent while issue was closed.
Description was changed from ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. Design doc: https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W... BUG=575532 ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. Design doc: https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W... BUG=575532 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:320001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. Design doc: https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W... BUG=575532 ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. Design doc: https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W... BUG=575532 Committed: https://crrev.com/ff24de0d44123ca7ae9b9511268fdd926356a36f Cr-Commit-Position: refs/heads/master@{#398886} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ff24de0d44123ca7ae9b9511268fdd926356a36f Cr-Commit-Position: refs/heads/master@{#398886}
Message was sent while issue was closed.
On 2016/06/09 15:00:35, nhiroki wrote: > On 2016/06/09 14:24:30, sof wrote: > > On 2016/06/09 14:15:07, haraken wrote: > > > > Thank you for the information. On the patchset 8, > > CrossThread(Weak)Persistents > > > > are traced but the plugin doesn't complain. > > > > > > +sigbjorn > > > > Thanks, the gc plugin hasn't caught up with these newer *Persistent variants. > > Acknowledged. Thanks! I'll update the plugin, but why doesn't the class use WeakMember<> ?
Message was sent while issue was closed.
On 2016/06/09 15:19:58, sof wrote: > On 2016/06/09 15:00:35, nhiroki wrote: > > On 2016/06/09 14:24:30, sof wrote: > > > On 2016/06/09 14:15:07, haraken wrote: > > > > > Thank you for the information. On the patchset 8, > > > CrossThread(Weak)Persistents > > > > > are traced but the plugin doesn't complain. > > > > > > > > +sigbjorn > > > > > > Thanks, the gc plugin hasn't caught up with these newer *Persistent > variants. > > > > Acknowledged. Thanks! > > I'll update the plugin, but why doesn't the class use WeakMember<> ? iow, WorkerWebSocketChannel should fail to compile in its current form -- we don't allow persistent references from heap-allocated objects (unless we declare them as exemptions.)
Message was sent while issue was closed.
On 2016/06/09 15:31:32, sof wrote: > On 2016/06/09 15:19:58, sof wrote: > > On 2016/06/09 15:00:35, nhiroki wrote: > > > On 2016/06/09 14:24:30, sof wrote: > > > > On 2016/06/09 14:15:07, haraken wrote: > > > > > > Thank you for the information. On the patchset 8, > > > > CrossThread(Weak)Persistents > > > > > > are traced but the plugin doesn't complain. > > > > > > > > > > +sigbjorn > > > > > > > > Thanks, the gc plugin hasn't caught up with these newer *Persistent > > variants. > > > > > > Acknowledged. Thanks! > > > > I'll update the plugin, but why doesn't the class use WeakMember<> ? > > iow, WorkerWebSocketChannel should fail to compile in its current form -- we > don't allow persistent references from heap-allocated objects (unless we declare > them as exemptions.) I heard that cross thread members will be replaced with cross thread [weak] persistents.
Message was sent while issue was closed.
On 2016/06/09 16:01:34, yhirano wrote: > On 2016/06/09 15:31:32, sof wrote: > > On 2016/06/09 15:19:58, sof wrote: > > > On 2016/06/09 15:00:35, nhiroki wrote: > > > > On 2016/06/09 14:24:30, sof wrote: > > > > > On 2016/06/09 14:15:07, haraken wrote: > > > > > > > Thank you for the information. On the patchset 8, > > > > > CrossThread(Weak)Persistents > > > > > > > are traced but the plugin doesn't complain. > > > > > > > > > > > > +sigbjorn > > > > > > > > > > Thanks, the gc plugin hasn't caught up with these newer *Persistent > > > variants. > > > > > > > > Acknowledged. Thanks! > > > > > > I'll update the plugin, but why doesn't the class use WeakMember<> ? > > > > iow, WorkerWebSocketChannel should fail to compile in its current form -- we > > don't allow persistent references from heap-allocated objects (unless we > declare > > them as exemptions.) > > I heard that cross thread members will be replaced with cross thread [weak] > persistents. That doesn't make too much sense (to me) without context; is there a discussion somewhere about the use of CrossThreadWeakPersistent<> in this CL?
Message was sent while issue was closed.
Description was changed from ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. Design doc: https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W... BUG=575532 Committed: https://crrev.com/ff24de0d44123ca7ae9b9511268fdd926356a36f Cr-Commit-Position: refs/heads/master@{#398886} ========== to ========== Worker: Introduce an observation mechanism for WorkerThread termination This mechanism provides a way to observe WorkerThread termination from the main thread. This is useful when an object living on the main thread needs to release references to objects on the worker thread before it gets terminated. As an example of using this mechanism, this CL applies it to WebSocket. WorkerWebSocketChannel::Peer observes worker thread termination from the main thread and asks DocumentWebSocketChannel to release references to objects on the worker thread when the termination happens. Design doc: https://docs.google.com/document/d/1rYFKem6SwghdISe_to5b4apORIKqjOpsV5FkE3f5W... BUG=575532 Committed: https://crrev.com/ff24de0d44123ca7ae9b9511268fdd926356a36f Cr-Commit-Position: refs/heads/master@{#398886} ==========
Message was sent while issue was closed.
nhiroki@chromium.org changed reviewers: + keishi@chromium.org
Message was sent while issue was closed.
On 2016/06/09 18:00:10, sof wrote: > On 2016/06/09 16:01:34, yhirano wrote: > > On 2016/06/09 15:31:32, sof wrote: > > > On 2016/06/09 15:19:58, sof wrote: > > > > On 2016/06/09 15:00:35, nhiroki wrote: > > > > > On 2016/06/09 14:24:30, sof wrote: > > > > > > On 2016/06/09 14:15:07, haraken wrote: > > > > > > > > Thank you for the information. On the patchset 8, > > > > > > CrossThread(Weak)Persistents > > > > > > > > are traced but the plugin doesn't complain. > > > > > > > > > > > > > > +sigbjorn > > > > > > > > > > > > Thanks, the gc plugin hasn't caught up with these newer *Persistent > > > > variants. > > > > > > > > > > Acknowledged. Thanks! > > > > > > > > I'll update the plugin, but why doesn't the class use WeakMember<> ? > > > > > > iow, WorkerWebSocketChannel should fail to compile in its current form -- we > > > don't allow persistent references from heap-allocated objects (unless we > > declare > > > them as exemptions.) > > > > I heard that cross thread members will be replaced with cross thread [weak] > > persistents. > > That doesn't make too much sense (to me) without context; is there a discussion > somewhere about the use of CrossThreadWeakPersistent<> in this CL? Design doc about "Per-thread Heap Design Doc" [1] says, "When one thread wants to reference an object that is owned by another thread’s ThreadHeap, it must use the a special handle, CrossThreadPersistent, instead of a Member.". (+keishi@ who is an author of the doc) [1] https://docs.google.com/document/d/14MzxMuUV9e5ZgxbZL3-jCMKa3S-UJGsbYSmRodLh9... In this CL, Bridge is owned by the worker thread and Peer is owned by the main thread, so I think they need to reference each other via CrossThread(Weak)Persistent.
Message was sent while issue was closed.
On 2016/06/09 23:04:41, nhiroki wrote: > On 2016/06/09 18:00:10, sof wrote: > > On 2016/06/09 16:01:34, yhirano wrote: > > > On 2016/06/09 15:31:32, sof wrote: > > > > On 2016/06/09 15:19:58, sof wrote: > > > > > On 2016/06/09 15:00:35, nhiroki wrote: > > > > > > On 2016/06/09 14:24:30, sof wrote: > > > > > > > On 2016/06/09 14:15:07, haraken wrote: > > > > > > > > > Thank you for the information. On the patchset 8, > > > > > > > CrossThread(Weak)Persistents > > > > > > > > > are traced but the plugin doesn't complain. > > > > > > > > > > > > > > > > +sigbjorn > > > > > > > > > > > > > > Thanks, the gc plugin hasn't caught up with these newer *Persistent > > > > > variants. > > > > > > > > > > > > Acknowledged. Thanks! > > > > > > > > > > I'll update the plugin, but why doesn't the class use WeakMember<> ? > > > > > > > > iow, WorkerWebSocketChannel should fail to compile in its current form -- > we > > > > don't allow persistent references from heap-allocated objects (unless we > > > declare > > > > them as exemptions.) > > > > > > I heard that cross thread members will be replaced with cross thread [weak] > > > persistents. > > > > That doesn't make too much sense (to me) without context; is there a > discussion > > somewhere about the use of CrossThreadWeakPersistent<> in this CL? > > Design doc about "Per-thread Heap Design Doc" [1] says, "When one thread wants > to reference an object that is owned by another thread’s ThreadHeap, it must use > the a special handle, CrossThreadPersistent, instead of a Member.". (+keishi@ > who is an author of the doc) > > [1] > https://docs.google.com/document/d/14MzxMuUV9e5ZgxbZL3-jCMKa3S-UJGsbYSmRodLh9... > > In this CL, Bridge is owned by the worker thread and Peer is owned by the main > thread, so I think they need to reference each other via > CrossThread(Weak)Persistent. Yeah, let's add the description to BlinkGCAPIReferences.md. My only concern of using CrossThreadPersistent for cross-origin pointers is that it may introduce new leaks, but we already have ASSERT(persistentCount == 0) at the end of a thread-termination GC, which would help catch the leaks.
Message was sent while issue was closed.
https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h (right): https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThreadLifecycleObserver.h:28: // and contextDestroyed() is never notified. This looks like a strange semantics. Other lifecycle observers don't have wasContextDestroyedBeforeObserverCreation(). If the context is already gone, LifecycleObserver should not be instantiated in the first place. In the particular case of the worker context, WorkerThread::workerThreadContext() should return nullptr after the context is destroyed. Then WorkerWebSocketChannel should not create a LifecycleObserver if WorkerThread::workerThreadContext() returns nullptr. https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h (right): https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:121: CrossThreadWeakPersistent<Bridge> m_bridge; This changes the lifetime semantics. Before, it was a strong reference. Now, it is a weak reference. Is this expected?
Message was sent while issue was closed.
https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h (right): https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:121: CrossThreadWeakPersistent<Bridge> m_bridge; On 2016/06/10 00:45:36, haraken wrote: > > This changes the lifetime semantics. Before, it was a strong reference. Now, it > is a weak reference. Is this expected? It's intended. Previously Bridge and Peer referenced each other with Members. That was OK because Members can handle reference cycles. Persistents cannot. The WebSocket object in the worker has the websocket channel in the main thread to communicate with the server. The back reference can be weak.
Message was sent while issue was closed.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
Message was sent while issue was closed.
some nits https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:61: class CORE_EXPORT WorkerThreadContext final : public GarbageCollectedFinalized<WorkerThreadContext>, public LifecycleNotifier<WorkerThreadContext, WorkerThreadLifecycleObserver> { nit: This very generic, yet another -context class which doesn't do much concerns me a bit. Could this be given more specific, descriptive name? Maybe- WorkerThreadLifecycleContext or WorkerLifecycleContext? https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkerThread.h:143: WorkerThreadContext* workerThreadContext() const { return m_workerThreadContext; } nit: I know this file is consistently in the old style, but workerThreadContext should be getWorkerThreadContext https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp (right): https://codereview.chromium.org/2025783002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp:198: if (wasContextDestroyedBeforeObserverCreation()) Yeah this feels weird. should just check lifecycleContext() returns nullptr or not in this case.
Message was sent while issue was closed.
Thanks for your comments! I'll make a follow-up CL to change the semantics etc soon.
Message was sent while issue was closed.
On 2016/06/10 00:30:53, haraken wrote: > On 2016/06/09 23:04:41, nhiroki wrote: > > On 2016/06/09 18:00:10, sof wrote: > > > On 2016/06/09 16:01:34, yhirano wrote: > > > > On 2016/06/09 15:31:32, sof wrote: > > > > > On 2016/06/09 15:19:58, sof wrote: > > > > > > On 2016/06/09 15:00:35, nhiroki wrote: > > > > > > > On 2016/06/09 14:24:30, sof wrote: > > > > > > > > On 2016/06/09 14:15:07, haraken wrote: > > > > > > > > > > Thank you for the information. On the patchset 8, > > > > > > > > CrossThread(Weak)Persistents > > > > > > > > > > are traced but the plugin doesn't complain. > > > > > > > > > > > > > > > > > > +sigbjorn > > > > > > > > > > > > > > > > Thanks, the gc plugin hasn't caught up with these newer > *Persistent > > > > > > variants. > > > > > > > > > > > > > > Acknowledged. Thanks! > > > > > > > > > > > > I'll update the plugin, but why doesn't the class use WeakMember<> ? > > > > > > > > > > iow, WorkerWebSocketChannel should fail to compile in its current form > -- > > we > > > > > don't allow persistent references from heap-allocated objects (unless we > > > > declare > > > > > them as exemptions.) > > > > > > > > I heard that cross thread members will be replaced with cross thread > [weak] > > > > persistents. > > > > > > That doesn't make too much sense (to me) without context; is there a > > discussion > > > somewhere about the use of CrossThreadWeakPersistent<> in this CL? > > > > Design doc about "Per-thread Heap Design Doc" [1] says, "When one thread wants > > to reference an object that is owned by another thread’s ThreadHeap, it must > use > > the a special handle, CrossThreadPersistent, instead of a Member.". (+keishi@ > > who is an author of the doc) > > > > [1] > > > https://docs.google.com/document/d/14MzxMuUV9e5ZgxbZL3-jCMKa3S-UJGsbYSmRodLh9... > > > > In this CL, Bridge is owned by the worker thread and Peer is owned by the main > > thread, so I think they need to reference each other via > > CrossThread(Weak)Persistent. > > Yeah, let's add the description to BlinkGCAPIReferences.md. > > My only concern of using CrossThreadPersistent for cross-origin pointers is that > it may introduce new leaks, but we already have ASSERT(persistentCount == 0) at > the end of a thread-termination GC, which would help catch the leaks. CrossThreadPersistent<>s aren't covered by thread-termination GCs & that assert, i think.
Message was sent while issue was closed.
On 2016/06/10 05:11:06, sof wrote: > On 2016/06/10 00:30:53, haraken wrote: > > On 2016/06/09 23:04:41, nhiroki wrote: > > > On 2016/06/09 18:00:10, sof wrote: > > > > On 2016/06/09 16:01:34, yhirano wrote: > > > > > On 2016/06/09 15:31:32, sof wrote: > > > > > > On 2016/06/09 15:19:58, sof wrote: > > > > > > > On 2016/06/09 15:00:35, nhiroki wrote: > > > > > > > > On 2016/06/09 14:24:30, sof wrote: > > > > > > > > > On 2016/06/09 14:15:07, haraken wrote: > > > > > > > > > > > Thank you for the information. On the patchset 8, > > > > > > > > > CrossThread(Weak)Persistents > > > > > > > > > > > are traced but the plugin doesn't complain. > > > > > > > > > > > > > > > > > > > > +sigbjorn > > > > > > > > > > > > > > > > > > Thanks, the gc plugin hasn't caught up with these newer > > *Persistent > > > > > > > variants. > > > > > > > > > > > > > > > > Acknowledged. Thanks! > > > > > > > > > > > > > > I'll update the plugin, but why doesn't the class use WeakMember<> ? > > > > > > > > > > > > iow, WorkerWebSocketChannel should fail to compile in its current form > > -- > > > we > > > > > > don't allow persistent references from heap-allocated objects (unless > we > > > > > declare > > > > > > them as exemptions.) > > > > > > > > > > I heard that cross thread members will be replaced with cross thread > > [weak] > > > > > persistents. > > > > > > > > That doesn't make too much sense (to me) without context; is there a > > > discussion > > > > somewhere about the use of CrossThreadWeakPersistent<> in this CL? > > > > > > Design doc about "Per-thread Heap Design Doc" [1] says, "When one thread > wants > > > to reference an object that is owned by another thread’s ThreadHeap, it must > > use > > > the a special handle, CrossThreadPersistent, instead of a Member.". > (+keishi@ > > > who is an author of the doc) > > > > > > [1] > > > > > > https://docs.google.com/document/d/14MzxMuUV9e5ZgxbZL3-jCMKa3S-UJGsbYSmRodLh9... > > > > > > In this CL, Bridge is owned by the worker thread and Peer is owned by the > main > > > thread, so I think they need to reference each other via > > > CrossThread(Weak)Persistent. > > > > Yeah, let's add the description to BlinkGCAPIReferences.md. > > > > My only concern of using CrossThreadPersistent for cross-origin pointers is > that > > it may introduce new leaks, but we already have ASSERT(persistentCount == 0) > at > > the end of a thread-termination GC, which would help catch the leaks. > > CrossThreadPersistent<>s aren't covered by thread-termination GCs & that assert, > i think. Ideally I think we should cover them, but I'm not sure how many callsites we need to fix to enable the assert...
Message was sent while issue was closed.
On 2016/06/10 05:24:05, haraken wrote: > On 2016/06/10 05:11:06, sof wrote: > > On 2016/06/10 00:30:53, haraken wrote: > > > On 2016/06/09 23:04:41, nhiroki wrote: .... > > > https://docs.google.com/document/d/14MzxMuUV9e5ZgxbZL3-jCMKa3S-UJGsbYSmRodLh9... > > > > > > > > In this CL, Bridge is owned by the worker thread and Peer is owned by the > > main > > > > thread, so I think they need to reference each other via > > > > CrossThread(Weak)Persistent. > > > > > > Yeah, let's add the description to BlinkGCAPIReferences.md. > > > > > > My only concern of using CrossThreadPersistent for cross-origin pointers is > > that > > > it may introduce new leaks, but we already have ASSERT(persistentCount == 0) > > at > > > the end of a thread-termination GC, which would help catch the leaks. > > > > CrossThreadPersistent<>s aren't covered by thread-termination GCs & that > assert, > > i think. > > Ideally I think we should cover them, but I'm not sure how many callsites we > need to fix to enable the assert... Co-opting a global registry of persistent pointers & the abstraction CrossThreadPersistent<>, to represent a pointer between heap objects on heaps N & M, is worrisome. |
