Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2951903003:
Hold global GC heap lock while making audio thread access. (Closed)
DescriptionHold global GC heap lock while making audio thread access.
For auxillary threads that rarely need to gain access to another
thread's GC heap, we have to ensure that this happens while the
heap-owning thread isn't concurrently GCing the heap. Otherwise there
is the possibility that heap objects might be relocated or mutated
while the auxillary thread tries to access.
A CrossThreadPersistent<T> (CTP) ensures reference liveness across
threads, but isn't sufficient to handle the wanted exclusive access
after a non-attached thread has deref'ed the persistent. So, for
this to happen, keep the global CTP lock while accessing a heap
object during offline audio rendering -- it specifically accessing
heap objects while a GC runs. As all GCs hold the lock on the global
CTP region while they run, this ensures exclusion.
It is clearly desirable to have all heap access be under the control
of the heap-owning thread, and threaded code should try hard to avoid
accessing another thread's heap objects. The CTP global lock is the
mechanism to use when that isn't practically feasible -- feel free to
add a "TODO(foo): avoid using" next to any instances that you end up
introducing.
As regards audio thread cross-thread usage, the code needs auditing to
check if there are other places where setting up this CTP lock is
required.
R=haraken,rtoy,hongchan
BUG=732511
Review-Url: https://codereview.chromium.org/2951903003
Cr-Commit-Position: refs/heads/master@{#481809}
Committed: https://chromium.googlesource.com/chromium/src/+/2ad39770967ebd93314070776ffe92d3eaeb8a6b
Patch Set 1 #Patch Set 2 : repurpose CrossThreadPersistentRegion::LockScope #Patch Set 3 : comment capitalization #Patch Set 4 : take GC lock earlier #Patch Set 5 : move GC locking outwards #Patch Set 6 : go back to GCLockScope #Patch Set 7 : revert CTP::LockScope changes #Patch Set 8 : verify LockScope timeouts #Patch Set 9 : revert ps#8 changes #Patch Set 10 : final version #Patch Set 11 : consistent lock scope wrt heap object in question #
Messages
Total messages: 52 (38 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: + hongchan@chromium.org, oilpan-reviews@chromium.org, rtoy@chromium.org
please take a look.
> threaded code should try hard to avoid accessing the a thread's heap from another. Yeah, I hope this but agree that it's sometimes to avoid it. Instead of introducing GCLockScope, would it be possible to reuse CrossThreadPersistentRegion::LockScope? These two scopes are doing the same thing (i.e., acquire a global lock).
On 2017/06/21 11:00:40, haraken wrote: > > threaded code should try hard to avoid accessing the a thread's heap from > another. > > Yeah, I hope this but agree that it's sometimes to avoid it. > > Instead of introducing GCLockScope, would it be possible to reuse > CrossThreadPersistentRegion::LockScope? These two scopes are doing the same > thing (i.e., acquire a global lock). That lock is across all threads having a heap (and all accesses to CTPs); GCLockScope is specific to one thread. To my mind, it seemed unnatural to co-opt the CTP lock here.
This is going to be weird anyway, so I don't have any strong opinion but... On 2017/06/21 11:11:12, sof wrote: > On 2017/06/21 11:00:40, haraken wrote: > > > threaded code should try hard to avoid accessing the a thread's heap from > > another. > > > > Yeah, I hope this but agree that it's sometimes to avoid it. > > > > Instead of introducing GCLockScope, would it be possible to reuse > > CrossThreadPersistentRegion::LockScope? These two scopes are doing the same > > thing (i.e., acquire a global lock). > > That lock is across all threads having a heap (and all accesses to CTPs); > GCLockScope is specific to one thread. The downside of this is 1) it is introducing a new lock scope and 2) it lets a thread access ThreadState of another thread (in general this should be forbidden). > To my mind, it seemed unnatural to co-opt > the CTP lock here. I don't think this is that strange if we interpret it as "a thread must acquire the CTP lock when the thread accesses a heap of another thread".
On 2017/06/21 11:16:50, haraken wrote: > This is going to be weird anyway, so I don't have any strong opinion but... > > On 2017/06/21 11:11:12, sof wrote: > > On 2017/06/21 11:00:40, haraken wrote: > > > > threaded code should try hard to avoid accessing the a thread's heap from > > > another. > > > > > > Yeah, I hope this but agree that it's sometimes to avoid it. > > > > > > Instead of introducing GCLockScope, would it be possible to reuse > > > CrossThreadPersistentRegion::LockScope? These two scopes are doing the same > > > thing (i.e., acquire a global lock). > > > > That lock is across all threads having a heap (and all accesses to CTPs); > > GCLockScope is specific to one thread. > > The downside of this is 1) it is introducing a new lock scope and 2) it lets a > thread access ThreadState of another thread (in general this should be > forbidden). > 1 - agree; 2 - that's the nature of this very bug, so not unexpected that the ThreadState is involved/accessed. > > To my mind, it seemed unnatural to co-opt > > the CTP lock here. > > I don't think this is that strange if we interpret it as "a thread must acquire > the CTP lock when the thread accesses a heap of another thread". It increases contention on this all-global lock. I want this bug off my plate, so will change.
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_...)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
On 2017/06/21 11:22:57, sof wrote: > On 2017/06/21 11:16:50, haraken wrote: > > This is going to be weird anyway, so I don't have any strong opinion but... > > > > On 2017/06/21 11:11:12, sof wrote: > > > On 2017/06/21 11:00:40, haraken wrote: > > > > > threaded code should try hard to avoid accessing the a thread's heap > from > > > > another. > > > > > > > > Yeah, I hope this but agree that it's sometimes to avoid it. > > > > > > > > Instead of introducing GCLockScope, would it be possible to reuse > > > > CrossThreadPersistentRegion::LockScope? These two scopes are doing the > same > > > > thing (i.e., acquire a global lock). > > > > > > That lock is across all threads having a heap (and all accesses to CTPs); > > > GCLockScope is specific to one thread. > > > > The downside of this is 1) it is introducing a new lock scope and 2) it lets a > > thread access ThreadState of another thread (in general this should be > > forbidden). > > > > 1 - agree; 2 - that's the nature of this very bug, so not unexpected that the > ThreadState is involved/accessed. > > > > To my mind, it seemed unnatural to co-opt > > > the CTP lock here. > > > > I don't think this is that strange if we interpret it as "a thread must > acquire > > the CTP lock when the thread accesses a heap of another thread". > > It increases contention on this all-global lock. I want this bug off my plate, > so will change. Done.
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...
LGTM
rtoy@chromium.org changed reviewers: + haraken@chromium.org
lgtm
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_...)
This might affect the timing of suspend/resume call when it happens, but we can't avoid that anyway. lgtm
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add ThreadState::GCLockScope for safe non-attached heap access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access. So, to have this happen in an orderly fashion, introduce a per-thread GC lock (i.e., wrt a ThreadState). Access into a thread's heap from the "outside" should be done while holding a GCLockScope. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing the a thread's heap from another. GCLockScope is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any uses that you end up introducing. R= BUG=732511 ========== to ========== Hold global GC heap lock while making audio thread access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> (CTP) ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access after a non-attached thread has deref'ed the persistent. So, for this to happen, keep the global CTP lock while accessing a heap object during offline audio rendering -- it specifically accessing heap objects while a GC runs. As all GCs hold the lock on the global CTP region while they run, this ensures exclusion. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing the a thread's heap from another. The CTP global lock is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any instances that you end up introducing. As regards audio thread cross-thread usage, the code needs auditing to check if there are other places where setting up this CTP lock is required. R= BUG=732511 ==========
Description was changed from ========== Hold global GC heap lock while making audio thread access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> (CTP) ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access after a non-attached thread has deref'ed the persistent. So, for this to happen, keep the global CTP lock while accessing a heap object during offline audio rendering -- it specifically accessing heap objects while a GC runs. As all GCs hold the lock on the global CTP region while they run, this ensures exclusion. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing the a thread's heap from another. The CTP global lock is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any instances that you end up introducing. As regards audio thread cross-thread usage, the code needs auditing to check if there are other places where setting up this CTP lock is required. R= BUG=732511 ========== to ========== Hold global GC heap lock while making audio thread access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> (CTP) ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access after a non-attached thread has deref'ed the persistent. So, for this to happen, keep the global CTP lock while accessing a heap object during offline audio rendering -- it specifically accessing heap objects while a GC runs. As all GCs hold the lock on the global CTP region while they run, this ensures exclusion. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing another thread's heap objects. The CTP global lock is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any instances that you end up introducing. As regards audio thread cross-thread usage, the code needs auditing to check if there are other places where setting up this CTP lock is required. R= BUG=732511 ==========
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/21 14:55:35, hongchan (OOO Jun 19-22) wrote: > This might affect the timing of suspend/resume call when it happens, but we > can't avoid that anyway. > Indeed, ran into timeout failures while having the lock taking reside inside the offline rendering loop. Moved it outwards; ptal at the latest patchset. (I thought I saw some extra timeout flakes locally when using the CTP lock over a ThreadState-specific lock, but testing on the bots didn't bear that out.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Hold global GC heap lock while making audio thread access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> (CTP) ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access after a non-attached thread has deref'ed the persistent. So, for this to happen, keep the global CTP lock while accessing a heap object during offline audio rendering -- it specifically accessing heap objects while a GC runs. As all GCs hold the lock on the global CTP region while they run, this ensures exclusion. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing another thread's heap objects. The CTP global lock is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any instances that you end up introducing. As regards audio thread cross-thread usage, the code needs auditing to check if there are other places where setting up this CTP lock is required. R= BUG=732511 ========== to ========== Hold global GC heap lock while making audio thread access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> (CTP) ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access after a non-attached thread has deref'ed the persistent. So, for this to happen, keep the global CTP lock while accessing a heap object during offline audio rendering -- it specifically accessing heap objects while a GC runs. As all GCs hold the lock on the global CTP region while they run, this ensures exclusion. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing another thread's heap objects. The CTP global lock is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any instances that you end up introducing. As regards audio thread cross-thread usage, the code needs auditing to check if there are other places where setting up this CTP lock is required. R=haraken,rtoy,hongchan BUG=732511 ==========
landing; will followup, should there be more feedback.
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/2951903003/#ps200001 (title: "consistent lock scope wrt heap object in question")
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": 200001, "attempt_start_ts": 1498195538583600,
"parent_rev": "cde1b24dea0e95b2da254954f86a079c7522a80a", "commit_rev":
"2ad39770967ebd93314070776ffe92d3eaeb8a6b"}
Message was sent while issue was closed.
Description was changed from ========== Hold global GC heap lock while making audio thread access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> (CTP) ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access after a non-attached thread has deref'ed the persistent. So, for this to happen, keep the global CTP lock while accessing a heap object during offline audio rendering -- it specifically accessing heap objects while a GC runs. As all GCs hold the lock on the global CTP region while they run, this ensures exclusion. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing another thread's heap objects. The CTP global lock is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any instances that you end up introducing. As regards audio thread cross-thread usage, the code needs auditing to check if there are other places where setting up this CTP lock is required. R=haraken,rtoy,hongchan BUG=732511 ========== to ========== Hold global GC heap lock while making audio thread access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> (CTP) ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access after a non-attached thread has deref'ed the persistent. So, for this to happen, keep the global CTP lock while accessing a heap object during offline audio rendering -- it specifically accessing heap objects while a GC runs. As all GCs hold the lock on the global CTP region while they run, this ensures exclusion. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing another thread's heap objects. The CTP global lock is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any instances that you end up introducing. As regards audio thread cross-thread usage, the code needs auditing to check if there are other places where setting up this CTP lock is required. R=haraken,rtoy,hongchan BUG=732511 Review-Url: https://codereview.chromium.org/2951903003 Cr-Commit-Position: refs/heads/master@{#481809} Committed: https://chromium.googlesource.com/chromium/src/+/2ad39770967ebd93314070776ffe... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/2ad39770967ebd93314070776ffe...
Message was sent while issue was closed.
On 2017/06/23 05:29:07, commit-bot: I haz the power wrote: > Committed patchset #11 (id:200001) as > https://chromium.googlesource.com/chromium/src/+/2ad39770967ebd93314070776ffe... Hi, By looking at the flakiness dashboard here: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType... This CL seems to cause the webaudio/internals/audiosource-premature-gc.html to timeout. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
