Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2913303002:
Avoid unsafe heap access from audio thread. (Closed)
DescriptionAvoid unsafe heap access from audio thread.
The audio thread tries to touch main thread Blink GCed objects in a
select few places, which isn't safe as a GC might concurrently run.
Avoid such cross off-thread usage, rearrange the processing of
finished nodes from the audio thread to the main thread.
R=hongchan,rtoy,haraken
BUG=727091, 729275
Review-Url: https://codereview.chromium.org/2913303002
Cr-Commit-Position: refs/heads/master@{#478084}
Committed: https://chromium.googlesource.com/chromium/src/+/548a4efe0d1cf9aa4958b1b73d53a9f0b1126a89
Patch Set 1 #
Total comments: 8
Patch Set 2 : clean up stoppable nodes on the main thread #Patch Set 3 : rebased upto r477617 #Patch Set 4 : bring back comment #
Total comments: 2
Patch Set 5 : support optional GC locking #Patch Set 6 : experiment with moving post-render finished node cleanup to main thread #Patch Set 7 : adjust source handler locking to actually work #Patch Set 8 : more ThreadState undo #Patch Set 9 : remove unwanted assert #Patch Set 10 : fix comment spelling #
Total comments: 3
Patch Set 11 : improve method documentation #
Messages
Total messages: 63 (37 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look (% bots coming through green) A fallout from enabling vector arena compaction; I do wonder if it helps explain why issue 682945 surfaced (for a heap hash set), but that's just a thought.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
haraken@chromium.org changed reviewers: + haraken@chromium.org
Thanks for looking into this. Hmm. In general it's not encouraged to let one thread touch data structures on another thread. Blink discourages a shared-memory programming model. Would there be any option to stop HandleStoppableSourceNodes from touching active_source_nodes_?
Description was changed from ========== Prevent unsafe heap access from audio thread while a GC runs. The audio thread has to touch main thread Blink GCed objects in a select few places. It is not safe for it to do so while a GC is ongoing, as that GC can end up moving objects. To ensure that there is no such access happening, the audio thread is now required to grab a GCLockScope before attempting access. Such a lock is now held per-thread while a GC runs. R= BUG=727091 ========== to ========== Prevent unsafe heap access from audio thread while a GC runs. The audio thread has to touch main thread Blink GCed objects in a select few places. It is not safe for it to do so while a GC is ongoing, as that GC can end up moving objects. To ensure that there is no such access happening, the audio thread is now required to grab a GCLockScope before attempting access. Such a lock is now held per-thread while a GC runs. R= BUG=727091 ==========
On 2017/06/01 01:16:27, haraken wrote: > Thanks for looking into this. > > Hmm. In general it's not encouraged to let one thread touch data structures on > another thread. Blink discourages a shared-memory programming model. > > Would there be any option to stop HandleStoppableSourceNodes from touching > active_source_nodes_? The conventional way would be to (conditionally) post to the main thread, but I don't know if that alters the stopping of nodes too much. +Cc: rtoy@ and hongchan@ for some input, hopefully :)
hongchan@chromium.org changed reviewers: + hongchan@chromium.org
Thanks for working on this! (and few drive-by comments) https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:685: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); rtoy@ If we have this lock, perhaps we don't need the graph owner lock anymore? https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/ThreadState.h:325: // to the heap while it is in a non-GCing state. From what I understand, this should not be abused and the general policy should be applied for most cases. (i.e. don't touch other threads' objects.) Perhaps we need some warnings here?
rtoy@chromium.org changed reviewers: + rtoy@chromium.org
https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:685: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); On 2017/06/01 16:08:55, hongchan wrote: > rtoy@ If we have this lock, perhaps we don't need the graph owner lock anymore? Based on the name of the class, I think we still need the graph owner lock in general. Can't have both the audio thread and the main thread modifying the graph at the same time. This lock seems to held if GC is happening on the main thread. Is that right sof@ and haraken@? https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:724: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); Is it possible to make this a trylock where we can just return if we can't get the lock? It's ok if we don't process the nodes right now in the audio thread. There's no requirement that the processing needs to happen right now as long as it happens eventually.
https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:685: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); On 2017/06/01 18:20:47, Raymond Toy wrote: > On 2017/06/01 16:08:55, hongchan wrote: > > rtoy@ If we have this lock, perhaps we don't need the graph owner lock > anymore? > > Based on the name of the class, I think we still need the graph owner lock in > general. Can't have both the audio thread and the main thread modifying the > graph at the same time. This lock seems to held if GC is happening on the main > thread. > > Is that right sof@ and haraken@? That's correct, it prevents the audio thread from accessing the heap while the GC runs (and vice versa), when the main thread isn't touching the audio node graph. https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:724: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); On 2017/06/01 18:20:47, Raymond Toy wrote: > Is it possible to make this a trylock where we can just return if we can't get > the lock? It's ok if we don't process the nodes right now in the audio thread. > There's no requirement that the processing needs to happen right now as long as > it happens eventually. I see, thanks - a tryLock() operation could be provided. But, if promptly stopping isn't vital, would it be plausible to move this active_source_nodes_ iteration to an expanded version of RemoveFinishedSourceNodesOnMainThread() instead? Thereby being performed by a task on the main thread, which avoids having to use this GC lock.
On 2017/06/01 21:10:05, sof wrote: > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): > > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:685: > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > On 2017/06/01 18:20:47, Raymond Toy wrote: > > On 2017/06/01 16:08:55, hongchan wrote: > > > rtoy@ If we have this lock, perhaps we don't need the graph owner lock > > anymore? > > > > Based on the name of the class, I think we still need the graph owner lock in > > general. Can't have both the audio thread and the main thread modifying the > > graph at the same time. This lock seems to held if GC is happening on the > main > > thread. > > > > Is that right sof@ and haraken@? > > That's correct, it prevents the audio thread from accessing the heap while the > GC runs (and vice versa), when the main thread isn't touching the audio node > graph. > > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:724: > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > On 2017/06/01 18:20:47, Raymond Toy wrote: > > Is it possible to make this a trylock where we can just return if we can't get > > the lock? It's ok if we don't process the nodes right now in the audio > thread. > > There's no requirement that the processing needs to happen right now as long > as > > it happens eventually. > > I see, thanks - a tryLock() operation could be provided. But, if promptly > stopping isn't vital, would it be plausible to move this active_source_nodes_ > iteration to an expanded version of RemoveFinishedSourceNodesOnMainThread() > instead? Thereby being performed by a task on the main thread, which avoids > having to use this GC lock. Yes, we can probably do this. Is it critical to fix this right away? I won't get to this for a few days.
https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:685: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); On 2017/06/01 21:10:05, sof wrote: > On 2017/06/01 18:20:47, Raymond Toy wrote: > > On 2017/06/01 16:08:55, hongchan wrote: > > > rtoy@ If we have this lock, perhaps we don't need the graph owner lock > > anymore? > > > > Based on the name of the class, I think we still need the graph owner lock in > > general. Can't have both the audio thread and the main thread modifying the > > graph at the same time. This lock seems to held if GC is happening on the > main > > thread. > > > > Is that right sof@ and haraken@? > > That's correct, it prevents the audio thread from accessing the heap while the > GC runs (and vice versa), when the main thread isn't touching the audio node > graph. I'm a bit confused. Is it possible that a GC runs on the main thread while the audio thread is holding the graph owner lock? I guess it would be problematic. The fact that the audio thread is holding the graph owner lock would mean that it may touch objects shared with the main thread.
https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:685: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); On 2017/06/02 02:42:25, haraken wrote: > On 2017/06/01 21:10:05, sof wrote: > > On 2017/06/01 18:20:47, Raymond Toy wrote: > > > On 2017/06/01 16:08:55, hongchan wrote: > > > > rtoy@ If we have this lock, perhaps we don't need the graph owner lock > > > anymore? > > > > > > Based on the name of the class, I think we still need the graph owner lock > in > > > general. Can't have both the audio thread and the main thread modifying the > > > graph at the same time. This lock seems to held if GC is happening on the > > main > > > thread. > > > > > > Is that right sof@ and haraken@? > > > > That's correct, it prevents the audio thread from accessing the heap while the > > GC runs (and vice versa), when the main thread isn't touching the audio node > > graph. > > I'm a bit confused. > > Is it possible that a GC runs on the main thread while the audio thread is > holding the graph owner lock? > Yes, that can happen - the lock is over an off-heap graph of nodes, however. Coordination between main thread (mutator) & audio thread about updating that node graph is handled by way of that lock. But as the graph is off-heap, the main thread GC may proceed without knowing anything about that lock while it operates (which now includes moving objects.)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/01 22:35:33, Raymond Toy wrote: > On 2017/06/01 21:10:05, sof wrote: > > > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): > > > > > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:685: > > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > On 2017/06/01 18:20:47, Raymond Toy wrote: > > > On 2017/06/01 16:08:55, hongchan wrote: > > > > rtoy@ If we have this lock, perhaps we don't need the graph owner lock > > > anymore? > > > > > > Based on the name of the class, I think we still need the graph owner lock > in > > > general. Can't have both the audio thread and the main thread modifying the > > > graph at the same time. This lock seems to held if GC is happening on the > > main > > > thread. > > > > > > Is that right sof@ and haraken@? > > > > That's correct, it prevents the audio thread from accessing the heap while the > > GC runs (and vice versa), when the main thread isn't touching the audio node > > graph. > > > > > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:724: > > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > On 2017/06/01 18:20:47, Raymond Toy wrote: > > > Is it possible to make this a trylock where we can just return if we can't > get > > > the lock? It's ok if we don't process the nodes right now in the audio > > thread. > > > There's no requirement that the processing needs to happen right now as long > > as > > > it happens eventually. > > > > I see, thanks - a tryLock() operation could be provided. But, if promptly > > stopping isn't vital, would it be plausible to move this active_source_nodes_ > > iteration to an expanded version of RemoveFinishedSourceNodesOnMainThread() > > instead? Thereby being performed by a task on the main thread, which avoids > > having to use this GC lock. > > Yes, we can probably do this. Is it critical to fix this right away? I won't > get to this for a few days. >= ps#2 does this now (time available is limited on this end also); I don't see how something similar can be done for the post render action check (ReleaseFinishedSourceNodes()), so it still requires&sets up a GC lock scope.
On 2017/06/07 at 12:47:57, sigbjornf wrote: > On 2017/06/01 22:35:33, Raymond Toy wrote: > > On 2017/06/01 21:10:05, sof wrote: > > > > > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > > > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): > > > > > > > > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:685: > > > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > > On 2017/06/01 18:20:47, Raymond Toy wrote: > > > > On 2017/06/01 16:08:55, hongchan wrote: > > > > > rtoy@ If we have this lock, perhaps we don't need the graph owner lock > > > > anymore? > > > > > > > > Based on the name of the class, I think we still need the graph owner lock > > in > > > > general. Can't have both the audio thread and the main thread modifying the > > > > graph at the same time. This lock seems to held if GC is happening on the > > > main > > > > thread. > > > > > > > > Is that right sof@ and haraken@? > > > > > > That's correct, it prevents the audio thread from accessing the heap while the > > > GC runs (and vice versa), when the main thread isn't touching the audio node > > > graph. > > > > > > > > https://codereview.chromium.org/2913303002/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:724: > > > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > > On 2017/06/01 18:20:47, Raymond Toy wrote: > > > > Is it possible to make this a trylock where we can just return if we can't > > get > > > > the lock? It's ok if we don't process the nodes right now in the audio > > > thread. > > > > There's no requirement that the processing needs to happen right now as long > > > as > > > > it happens eventually. > > > > > > I see, thanks - a tryLock() operation could be provided. But, if promptly > > > stopping isn't vital, would it be plausible to move this active_source_nodes_ > > > iteration to an expanded version of RemoveFinishedSourceNodesOnMainThread() > > > instead? Thereby being performed by a task on the main thread, which avoids > > > having to use this GC lock. > > > > Yes, we can probably do this. Is it critical to fix this right away? I won't > > get to this for a few days. > > >= ps#2 does this now (time available is limited on this end also); I don't see how something similar can be done for the post render action check (ReleaseFinishedSourceNodes()), so it still requires&sets up a GC lock scope. Let's go with this CL for now then. It will take a bit of time to think over how to make all of these things happen on the main thread instead of the audio thread. It this locking is a problem, I hope we can make it a tryLock so as not to block the audio thread. At least until we can move the operations to the main thread. Thanks for your help on this.
lgtm
This doesn't seem like a good solution but I'll stamp LGTM... https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); Add a TODO to remove this somehow. We should refactor code so that the audio thread doesn't need to access data structures on Oilpan's heap. GCLockScope is not nice because the audio thread needs to stop while the main thread is running a GC. This breaks the realtime-ness of the audio thread. Also it violates Blink's messaging passing model. In Blink a thread should not access data structures held by a different thread. https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.h:326: class GCLockScope final { Let's add a caution that this should not be used in other places.
On 2017/06/08 at 01:07:55, haraken wrote: > This doesn't seem like a good solution but I'll stamp LGTM... > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765: ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > Add a TODO to remove this somehow. We should refactor code so that the audio thread doesn't need to access data structures on Oilpan's heap. > > GCLockScope is not nice because the audio thread needs to stop while the main thread is running a GC. This breaks the realtime-ness of the audio thread. > > Also it violates Blink's messaging passing model. In Blink a thread should not access data structures held by a different thread. > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/ThreadState.h (right): > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/ThreadState.h:326: class GCLockScope final { > > Let's add a caution that this should not be used in other places. How hard would it be to add tryLock support for this? As stated earlier, it's ok if we skip processing if we can't get the GC lock. That should take care of the issue of causing the audio thread to pause.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/08 03:06:15, Raymond Toy wrote: > On 2017/06/08 at 01:07:55, haraken wrote: > > This doesn't seem like a good solution but I'll stamp LGTM... > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765: > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > > > Add a TODO to remove this somehow. We should refactor code so that the audio > thread doesn't need to access data structures on Oilpan's heap. > > > > GCLockScope is not nice because the audio thread needs to stop while the main > thread is running a GC. This breaks the realtime-ness of the audio thread. > > > > Also it violates Blink's messaging passing model. In Blink a thread should not > access data structures held by a different thread. > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/ThreadState.h (right): > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/ThreadState.h:326: class GCLockScope > final { > > > > Let's add a caution that this should not be used in other places. > > How hard would it be to add tryLock support for this? As stated earlier, it's > ok if we skip processing if we can't get the GC lock. That should take care of > the issue of causing the audio thread to pause. Certainly, ps#5 does this. As there is general unhappiness with having to introduce GC locking functionality, ps#6 moves the post-render finished node processing to the main thread. That might be acceptable.. (asking the trybots first :) ) Re: audio thread being "off heap" and the complexity that this brings -- this design was a solution adopted at a time when we didn't have per-thread heaps. Now that we do, c/should that be reconsidered?
On 2017/06/08 08:22:56, sof wrote: > On 2017/06/08 03:06:15, Raymond Toy wrote: > > On 2017/06/08 at 01:07:55, haraken wrote: > > > This doesn't seem like a good solution but I'll stamp LGTM... > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp > (right): > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765: > > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > > > > > Add a TODO to remove this somehow. We should refactor code so that the audio > > thread doesn't need to access data structures on Oilpan's heap. > > > > > > GCLockScope is not nice because the audio thread needs to stop while the > main > > thread is running a GC. This breaks the realtime-ness of the audio thread. > > > > > > Also it violates Blink's messaging passing model. In Blink a thread should > not > > access data structures held by a different thread. > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/heap/ThreadState.h (right): > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/heap/ThreadState.h:326: class GCLockScope > > final { > > > > > > Let's add a caution that this should not be used in other places. > > > > How hard would it be to add tryLock support for this? As stated earlier, it's > > ok if we skip processing if we can't get the GC lock. That should take care > of > > the issue of causing the audio thread to pause. > > Certainly, ps#5 does this. > > As there is general unhappiness with having to introduce GC locking > functionality, ps#6 moves the post-render finished node processing to the main > thread. That might be acceptable.. (asking the trybots first :) ) > > Re: audio thread being "off heap" and the complexity that this brings -- this > design was a solution adopted at a time when we didn't have per-thread heaps. > Now that we do, c/should that be reconsidered? That could be an option. However, I still think that the best solution would be to make the audio thread not touch Oilpan's heaps at all. Regardless of whether we have Oilpan or not, it's not a good idea to violate the message passing model in Blink. Thoughts?
On 2017/06/08 08:34:23, haraken wrote: > On 2017/06/08 08:22:56, sof wrote: > > On 2017/06/08 03:06:15, Raymond Toy wrote: > > > On 2017/06/08 at 01:07:55, haraken wrote: > > > > This doesn't seem like a good solution but I'll stamp LGTM... > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765: > > > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > > > > > > > Add a TODO to remove this somehow. We should refactor code so that the > audio > > > thread doesn't need to access data structures on Oilpan's heap. > > > > > > > > GCLockScope is not nice because the audio thread needs to stop while the > > main > > > thread is running a GC. This breaks the realtime-ness of the audio thread. > > > > > > > > Also it violates Blink's messaging passing model. In Blink a thread should > > not > > > access data structures held by a different thread. > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/platform/heap/ThreadState.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/platform/heap/ThreadState.h:326: class > GCLockScope > > > final { > > > > > > > > Let's add a caution that this should not be used in other places. > > > > > > How hard would it be to add tryLock support for this? As stated earlier, > it's > > > ok if we skip processing if we can't get the GC lock. That should take care > > of > > > the issue of causing the audio thread to pause. > > > > Certainly, ps#5 does this. > > > > As there is general unhappiness with having to introduce GC locking > > functionality, ps#6 moves the post-render finished node processing to the main > > thread. That might be acceptable.. (asking the trybots first :) ) > > > > Re: audio thread being "off heap" and the complexity that this brings -- this > > design was a solution adopted at a time when we didn't have per-thread heaps. > > Now that we do, c/should that be reconsidered? > > That could be an option. > > However, I still think that the best solution would be to make the audio thread > not touch Oilpan's heaps at all. Regardless of whether we have Oilpan or not, > it's not a good idea to violate the message passing model in Blink. Thoughts? We have to work with real-time constraints and other practical concerns at times. As far as it goes, ps#6 avoids the audio thread usage of active source nodes (I just left the ThreadState changes around for now), but I expect there to be more such non-ideal heap accesses around elsewhere in webaudio-land.
On 2017/06/08 08:42:09, sof wrote: > On 2017/06/08 08:34:23, haraken wrote: > > On 2017/06/08 08:22:56, sof wrote: > > > On 2017/06/08 03:06:15, Raymond Toy wrote: > > > > On 2017/06/08 at 01:07:55, haraken wrote: > > > > > This doesn't seem like a good solution but I'll stamp LGTM... > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765: > > > > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > > > > > > > > > Add a TODO to remove this somehow. We should refactor code so that the > > audio > > > > thread doesn't need to access data structures on Oilpan's heap. > > > > > > > > > > GCLockScope is not nice because the audio thread needs to stop while the > > > main > > > > thread is running a GC. This breaks the realtime-ness of the audio thread. > > > > > > > > > > Also it violates Blink's messaging passing model. In Blink a thread > should > > > not > > > > access data structures held by a different thread. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/platform/heap/ThreadState.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/platform/heap/ThreadState.h:326: class > > GCLockScope > > > > final { > > > > > > > > > > Let's add a caution that this should not be used in other places. > > > > > > > > How hard would it be to add tryLock support for this? As stated earlier, > > it's > > > > ok if we skip processing if we can't get the GC lock. That should take > care > > > of > > > > the issue of causing the audio thread to pause. > > > > > > Certainly, ps#5 does this. > > > > > > As there is general unhappiness with having to introduce GC locking > > > functionality, ps#6 moves the post-render finished node processing to the > main > > > thread. That might be acceptable.. (asking the trybots first :) ) > > > > > > Re: audio thread being "off heap" and the complexity that this brings -- > this > > > design was a solution adopted at a time when we didn't have per-thread > heaps. > > > Now that we do, c/should that be reconsidered? > > > > That could be an option. > > > > However, I still think that the best solution would be to make the audio > thread > > not touch Oilpan's heaps at all. Regardless of whether we have Oilpan or not, > > it's not a good idea to violate the message passing model in Blink. Thoughts? > > We have to work with real-time constraints and other practical concerns at > times. > > As far as it goes, ps#6 avoids the audio thread usage of active source nodes (I > just left the ThreadState changes around for now), but I expect there to be more > such non-ideal heap accesses around elsewhere in webaudio-land. Thanks for working on this! PS6 looks good to me from the oilpan perspective (% removing GCLockScope).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/08 08:57:16, haraken wrote: > On 2017/06/08 08:42:09, sof wrote: > > On 2017/06/08 08:34:23, haraken wrote: > > > On 2017/06/08 08:22:56, sof wrote: > > > > On 2017/06/08 03:06:15, Raymond Toy wrote: > > > > > On 2017/06/08 at 01:07:55, haraken wrote: > > > > > > This doesn't seem like a good solution but I'll stamp LGTM... > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > > > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:765: > > > > > ThreadState::GCLockScope gc_lock(ThreadState::MainThreadState()); > > > > > > > > > > > > Add a TODO to remove this somehow. We should refactor code so that the > > > audio > > > > > thread doesn't need to access data structures on Oilpan's heap. > > > > > > > > > > > > GCLockScope is not nice because the audio thread needs to stop while > the > > > > main > > > > > thread is running a GC. This breaks the realtime-ness of the audio > thread. > > > > > > > > > > > > Also it violates Blink's messaging passing model. In Blink a thread > > should > > > > not > > > > > access data structures held by a different thread. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > > > File third_party/WebKit/Source/platform/heap/ThreadState.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2913303002/diff/60001/third_party/WebKit/Sour... > > > > > > third_party/WebKit/Source/platform/heap/ThreadState.h:326: class > > > GCLockScope > > > > > final { > > > > > > > > > > > > Let's add a caution that this should not be used in other places. > > > > > > > > > > How hard would it be to add tryLock support for this? As stated > earlier, > > > it's > > > > > ok if we skip processing if we can't get the GC lock. That should take > > care > > > > of > > > > > the issue of causing the audio thread to pause. > > > > > > > > Certainly, ps#5 does this. > > > > > > > > As there is general unhappiness with having to introduce GC locking > > > > functionality, ps#6 moves the post-render finished node processing to the > > main > > > > thread. That might be acceptable.. (asking the trybots first :) ) > > > > > > > > Re: audio thread being "off heap" and the complexity that this brings -- > > this > > > > design was a solution adopted at a time when we didn't have per-thread > > heaps. > > > > Now that we do, c/should that be reconsidered? > > > > > > That could be an option. > > > > > > However, I still think that the best solution would be to make the audio > > thread > > > not touch Oilpan's heaps at all. Regardless of whether we have Oilpan or > not, > > > it's not a good idea to violate the message passing model in Blink. > Thoughts? > > > > We have to work with real-time constraints and other practical concerns at > > times. > > > > As far as it goes, ps#6 avoids the audio thread usage of active source nodes > (I > > just left the ThreadState changes around for now), but I expect there to be > more > > such non-ideal heap accesses around elsewhere in webaudio-land. > > Thanks for working on this! > > PS6 looks good to me from the oilpan perspective (% removing GCLockScope). Latest PS fixes that and a couple of other wrinkles noticed by the trybots; now in a ready state. This change will overall cause more tasks to be posted by the audio thread, so the question is if that is acceptable & workable.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Prevent unsafe heap access from audio thread while a GC runs. The audio thread has to touch main thread Blink GCed objects in a select few places. It is not safe for it to do so while a GC is ongoing, as that GC can end up moving objects. To ensure that there is no such access happening, the audio thread is now required to grab a GCLockScope before attempting access. Such a lock is now held per-thread while a GC runs. R= BUG=727091 ========== to ========== Avoid unsafe heap access from audio thread. The audio thread tries to touch main thread Blink GCed objects in a select few places, which isn't safe as a GC might concurrently run. Avoid such cross off-thread usage, rearrange the processing of finished nodes from the audio thread to the main thread. R= BUG=727091 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit. Thanks so much for doing this work! https://codereview.chromium.org/2913303002/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:875: void BaseAudioContext::ResolvePromisesForResume() { nit: This does more than resolver promises for resume now. Can we change the name?
https://codereview.chromium.org/2913303002/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:875: void BaseAudioContext::ResolvePromisesForResume() { On 2017/06/08 15:06:20, Raymond Toy wrote: > nit: This does more than resolver promises for resume now. Can we change the > name? We can, but what did you have in mind? It schedules a main thread task if promises need to be resumed. [ That the same task performs periodic cleanup of finished nodes, is an implementation detail that doesn't need to be reflected in this method's name -- that's how I reason(ed). ]
lgtm Thanks for doing this!
Description was changed from ========== Avoid unsafe heap access from audio thread. The audio thread tries to touch main thread Blink GCed objects in a select few places, which isn't safe as a GC might concurrently run. Avoid such cross off-thread usage, rearrange the processing of finished nodes from the audio thread to the main thread. R= BUG=727091 ========== to ========== Avoid unsafe heap access from audio thread. The audio thread tries to touch main thread Blink GCed objects in a select few places, which isn't safe as a GC might concurrently run. Avoid such cross off-thread usage, rearrange the processing of finished nodes from the audio thread to the main thread. R=hongchan,rtoy,haraken BUG=727091,729275 ==========
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks for the reviews, will go ahead and try landing. Should it cause perf problems (or otherwise), we do have a fallback alternative, albeit undesirable. https://codereview.chromium.org/2913303002/diff/170001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2913303002/diff/170001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:875: void BaseAudioContext::ResolvePromisesForResume() { On 2017/06/08 15:24:39, sof wrote: > On 2017/06/08 15:06:20, Raymond Toy wrote: > > nit: This does more than resolver promises for resume now. Can we change the > > name? > > We can, but what did you have in mind? It schedules a main thread task if > promises need to be resumed. > > [ That the same task performs periodic cleanup of finished nodes, is an > implementation detail that doesn't need to be reflected in this method's name -- > that's how I reason(ed). ] Tried to address this issue via method documentation (for PerformCleanupOnMainThread()).
On 2017/06/08 at 18:46:34, sigbjornf wrote: > thanks for the reviews, will go ahead and try landing. > > Should it cause perf problems (or otherwise), we do have a fallback alternative, albeit undesirable. > > https://codereview.chromium.org/2913303002/diff/170001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): > > https://codereview.chromium.org/2913303002/diff/170001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:875: void BaseAudioContext::ResolvePromisesForResume() { > On 2017/06/08 15:24:39, sof wrote: > > On 2017/06/08 15:06:20, Raymond Toy wrote: > > > nit: This does more than resolver promises for resume now. Can we change the > > > name? > > > > We can, but what did you have in mind? It schedules a main thread task if > > promises need to be resumed. > > > > [ That the same task performs periodic cleanup of finished nodes, is an > > implementation detail that doesn't need to be reflected in this method's name -- > > that's how I reason(ed). ] > > Tried to address this issue via method documentation (for PerformCleanupOnMainThread()). This is fine. If it bothers me enough I'll fix it later if I can up with an appropriate name. (Which I can't think of any right now.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, rtoy@chromium.org, hongchan@chromium.org Link to the patchset: https://codereview.chromium.org/2913303002/#ps190001 (title: "improve method documentation")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 190001, "attempt_start_ts": 1496955991172260,
"parent_rev": "0751b8db7ce498600b35c3203225d3e1408f4dd3", "commit_rev":
"548a4efe0d1cf9aa4958b1b73d53a9f0b1126a89"}
Message was sent while issue was closed.
Description was changed from ========== Avoid unsafe heap access from audio thread. The audio thread tries to touch main thread Blink GCed objects in a select few places, which isn't safe as a GC might concurrently run. Avoid such cross off-thread usage, rearrange the processing of finished nodes from the audio thread to the main thread. R=hongchan,rtoy,haraken BUG=727091,729275 ========== to ========== Avoid unsafe heap access from audio thread. The audio thread tries to touch main thread Blink GCed objects in a select few places, which isn't safe as a GC might concurrently run. Avoid such cross off-thread usage, rearrange the processing of finished nodes from the audio thread to the main thread. R=hongchan,rtoy,haraken BUG=727091,729275 Review-Url: https://codereview.chromium.org/2913303002 Cr-Commit-Position: refs/heads/master@{#478084} Committed: https://chromium.googlesource.com/chromium/src/+/548a4efe0d1cf9aa4958b1b73d53... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:190001) as https://chromium.googlesource.com/chromium/src/+/548a4efe0d1cf9aa4958b1b73d53... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
