Use base::Lock instead of RecursiveMutex in DeferredTaskHandler [chromium/src : main]
Michael Wilson (Gerrit)
Michael Wilson added 1 comment![]( "Open in Gerrit")
Please take a look. The main part of this change is in the following files:
- third_party/blink/renderer/modules/webaudio/audio_context.cc
- third_party/blink/renderer/modules/webaudio/base_audio_context.h
- third_party/blink/renderer/modules/webaudio/base_audio_context.cc
- third_party/blink/renderer/modules/webaudio/deferred_task_handler.h
- third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc
Everything else is mechanical cleanup. If you would like me to split this into multiple CLs please let me know.
Related details
- Jeremy Roman
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
+hongchan for extra WebAudio experience
If this works, I'm happy with it. At some point in the past a straightforward replacement like this was impossible because WebAudio relied on recursive acquisition of the lock in subtle ways, which this CL turns into a thread deadlocking itself.
How confident are we that this (acquisition of a lock already held by the same thread) cannot in fact happen today? Would it be worth doing some additional legwork to validate that this is the case, or feature-guard it (and maybe even roll it out via Finch)?
Related details
- Hongchan Choi
- Michael Wilson
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 4 comments![]( "Open in Gerrit")
+hongchan for extra WebAudio experience
If this works, I'm happy with it. At some point in the past a straightforward replacement like this was impossible because WebAudio relied on recursive acquisition of the lock in subtle ways, which this CL turns into a thread deadlocking itself.
How confident are we that this (acquisition of a lock already held by the same thread) cannot in fact happen today? Would it be worth doing some additional legwork to validate that this is the case, or feature-guard it (and maybe even roll it out via Finch)?
+1
I really can't say I am confident on possible fallout, but this CL can achieve a decent and meaningful clean up.
mjwilson@ - should we consider flag guarding this or doing a finch roll out?
#include "third_party/blink/renderer/platform/wtf/threading_primitives.h"Is this directly relevant to the nature of this patch?
#include "third_party/blink/renderer/platform/wtf/threading_primitives.h"Ditto.
#include "third_party/blink/renderer/platform/wtf/threading_primitives.h"Ditto.
Related details
- Jeremy Roman
- Michael Wilson
Michael Wilson (Gerrit)
Michael Wilson added 1 comment![]( "Open in Gerrit")
Hongchan Choi+hongchan for extra WebAudio experience
If this works, I'm happy with it. At some point in the past a straightforward replacement like this was impossible because WebAudio relied on recursive acquisition of the lock in subtle ways, which this CL turns into a thread deadlocking itself.
How confident are we that this (acquisition of a lock already held by the same thread) cannot in fact happen today? Would it be worth doing some additional legwork to validate that this is the case, or feature-guard it (and maybe even roll it out via Finch)?
+1
I really can't say I am confident on possible fallout, but this CL can achieve a decent and meaningful clean up.
mjwilson@ - should we consider flag guarding this or doing a finch roll out?
When I wrote this I was fairly confident, but I was looking more today and saw some discussion about issues when objects go out of scope, etc. that have made me less confident. I will do a bit more analysis and I also agree that a flag guard seems appropriate if I can't strongly prove it.
(Also, I will write a separate CL to remove the unused includes which should reduce the number of people who will get emails about this issue; sorry.)
Related details
- Michael Wilson
Michael Wilson (Gerrit)
Michael Wilson added 4 comments![]( "Open in Gerrit")
Please take a look. The main part of this change is in the following files:
- third_party/blink/renderer/modules/webaudio/audio_context.cc
- third_party/blink/renderer/modules/webaudio/base_audio_context.h
- third_party/blink/renderer/modules/webaudio/base_audio_context.cc
- third_party/blink/renderer/modules/webaudio/deferred_task_handler.h
- third_party/blink/renderer/modules/webaudio/deferred_task_handler.cc
Everything else is mechanical cleanup. If you would like me to split this into multiple CLs please let me know.
Done
#include "third_party/blink/renderer/platform/wtf/threading_primitives.h"Is this directly relevant to the nature of this patch?
No, handled in crrev.com/c/5405863
#include "third_party/blink/renderer/platform/wtf/threading_primitives.h"Michael WilsonDitto.
Done
#include "third_party/blink/renderer/platform/wtf/threading_primitives.h"Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 5 comments![]( "Open in Gerrit")
It looks pretty clean! My comments are mostly question - we can discuss offline if that's better.
// Graph locking.Revise the comment:
The mutex for exclusive graph manipulation. It can be held from either the main or the audio thread when the graph is changed. (e.g. connection or disconnection of AudioNodes)
void Lock() EXCLUSIVE_LOCK_FUNCTION(context_graph_mutex_);
bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true, context_graph_mutex_);
void Unlock() UNLOCK_FUNCTION(context_graph_mutex_);Could you explain what these macros do?
void DeferredTaskHandler::Lock() {Also this "Lock" method seems vague. Can we name it to "GraphLock()"? Or "LockGraph()"?
// In release build treat tryLock() as lock() (since above
// DCHECK(isAudioThread) never fires) - this is the best we can do.This logic is a bit weird. TryLock shouldn't be needed on the main thread. Right?
Related details
- Michael Wilson
Michael Wilson (Gerrit)
Michael Wilson added 3 comments![]( "Open in Gerrit")
Hongchan Choi+hongchan for extra WebAudio experience
If this works, I'm happy with it. At some point in the past a straightforward replacement like this was impossible because WebAudio relied on recursive acquisition of the lock in subtle ways, which this CL turns into a thread deadlocking itself.
How confident are we that this (acquisition of a lock already held by the same thread) cannot in fact happen today? Would it be worth doing some additional legwork to validate that this is the case, or feature-guard it (and maybe even roll it out via Finch)?
Michael Wilson+1
I really can't say I am confident on possible fallout, but this CL can achieve a decent and meaningful clean up.
mjwilson@ - should we consider flag guarding this or doing a finch roll out?
When I wrote this I was fairly confident, but I was looking more today and saw some discussion about issues when objects go out of scope, etc. that have made me less confident. I will do a bit more analysis and I also agree that a flag guard seems appropriate if I can't strongly prove it.
(Also, I will write a separate CL to remove the unused includes which should reduce the number of people who will get emails about this issue; sorry.)
I tried a few things to get the thread analysis tools to verify the lock is not used recursively, but I think the lock is accessed through too many layers of indirection for the analysis to work.
The team has a plan to work on threading and architecture later this year so I will put this down for now and plan to pick it up again then. Set back to WIP.
void Lock() EXCLUSIVE_LOCK_FUNCTION(context_graph_mutex_);
bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true, context_graph_mutex_);
void Unlock() UNLOCK_FUNCTION(context_graph_mutex_);Could you explain what these macros do?
The thread annotations are described here:
https://crsrc.org/c/base/thread_annotations.h
They are to document threading constraints and also serve as hints to the compiler thread safety analysis to help catch some (but not all) multi-threading bugs.
// In release build treat tryLock() as lock() (since above
// DCHECK(isAudioThread) never fires) - this is the best we can do.This logic is a bit weird. TryLock shouldn't be needed on the main thread. Right?
Right.
This is actually a violation of the coding standard: we should either remove the `DCHECK`, remove the handling of `!IsAudioThread()`, or just remove both.
But, since I'm not confident that this is never called from the main thread, I left it here. I think if we want to remove this it should be a separate change so it could be rolled back independently if there is a problem.
Related details
Michael Wilson (Gerrit)
Michael Wilson added 4 comments![]( "Open in Gerrit")
Revise the comment:
The mutex for exclusive graph manipulation. It can be held from either the main or the audio thread when the graph is changed. (e.g. connection or disconnection of AudioNodes)
Done
void Lock() EXCLUSIVE_LOCK_FUNCTION(context_graph_mutex_);
bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true, context_graph_mutex_);
void Unlock() UNLOCK_FUNCTION(context_graph_mutex_);Michael WilsonCould you explain what these macros do?
The thread annotations are described here:
https://crsrc.org/c/base/thread_annotations.hThey are to document threading constraints and also serve as hints to the compiler thread safety analysis to help catch some (but not all) multi-threading bugs.
Done
void DeferredTaskHandler::Lock() {Also this "Lock" method seems vague. Can we name it to "GraphLock()"? Or "LockGraph()"?
This is the standard naming for this type of method throughout the codebase. I would prefer to keep it just as `Lock`.
// In release build treat tryLock() as lock() (since above
// DCHECK(isAudioThread) never fires) - this is the best we can do.Michael WilsonThis logic is a bit weird. TryLock shouldn't be needed on the main thread. Right?
Right.
This is actually a violation of the coding standard: we should either remove the `DCHECK`, remove the handling of `!IsAudioThread()`, or just remove both.
But, since I'm not confident that this is never called from the main thread, I left it here. I think if we want to remove this it should be a separate change so it could be rolled back independently if there is a problem.
I ended up removing it; the DCHECK has been in place for a while and we can verify statically that this private method is only called from the audio thread.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson added 3 comments![]( "Open in Gerrit")
Hongchan Choi+hongchan for extra WebAudio experience
If this works, I'm happy with it. At some point in the past a straightforward replacement like this was impossible because WebAudio relied on recursive acquisition of the lock in subtle ways, which this CL turns into a thread deadlocking itself.
How confident are we that this (acquisition of a lock already held by the same thread) cannot in fact happen today? Would it be worth doing some additional legwork to validate that this is the case, or feature-guard it (and maybe even roll it out via Finch)?
Michael Wilson+1
I really can't say I am confident on possible fallout, but this CL can achieve a decent and meaningful clean up.
mjwilson@ - should we consider flag guarding this or doing a finch roll out?
Michael WilsonWhen I wrote this I was fairly confident, but I was looking more today and saw some discussion about issues when objects go out of scope, etc. that have made me less confident. I will do a bit more analysis and I also agree that a flag guard seems appropriate if I can't strongly prove it.
(Also, I will write a separate CL to remove the unused includes which should reduce the number of people who will get emails about this issue; sorry.)
I tried a few things to get the thread analysis tools to verify the lock is not used recursively, but I think the lock is accessed through too many layers of indirection for the analysis to work.
The team has a plan to work on threading and architecture later this year so I will put this down for now and plan to pick it up again then. Set back to WIP.
I have added a flag guard. I don't think Finch is appropriate, because there's no metric to compare (either we will observe deadlocks or not). Once the change is verified we can remove the remaining files along with the flag.
PTAL again. I worked with Gemini and am more confident in the change now, especially with the flag guard in place.
// In release build treat tryLock() as lock() (since above
// DCHECK(isAudioThread) never fires) - this is the best we can do.Michael WilsonThis logic is a bit weird. TryLock shouldn't be needed on the main thread. Right?
Michael WilsonRight.
This is actually a violation of the coding standard: we should either remove the `DCHECK`, remove the handling of `!IsAudioThread()`, or just remove both.
But, since I'm not confident that this is never called from the main thread, I left it here. I think if we want to remove this it should be a separate change so it could be rolled back independently if there is a problem.
I ended up removing it; the DCHECK has been in place for a while and we can verify statically that this private method is only called from the audio thread.
- Hongchan Choi
- Jeremy Roman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 8 comments![]( "Open in Gerrit")
This is the last usage of RecursiveMatrix, so also remove theRecursiveMutex?
BASE_FEATURE(kWebAudioUseBaseLock, base::FEATURE_ENABLED_BY_DEFAULT);Can we have a comment on this flag?
// rendering graph state (from main thread changes). It's OK if the tryLock()It needs to be updated. (I'd just rewrite this comment)
// Must use a tryLock() here too. Don't worry, the lock will very rarely beDitto. Let's just rewrite this comment.
class MODULES_EXPORT GraphAutoTryLocker {Does adding "SCOPED_LOCKABLE" make sense here?
void DeferredTaskHandler::Lock() {Michael WilsonAlso this "Lock" method seems vague. Can we name it to "GraphLock()"? Or "LockGraph()"?
This is the standard naming for this type of method throughout the codebase. I would prefer to keep it just as `Lock`.
Acknowledged
void DeferredTaskHandler::Lock() NO_THREAD_SAFETY_ANALYSIS {Why is this necessary? I think it makes sense to add a comment on why it's needed here.
Also it looks like we don't need "NO_THREAD_SAFETY_ANALYSIS_FIXME" anymore on this annotation? If you have an explanation, it would be easier for me to review.
// In release build treat tryLock() as lock() (since above
// DCHECK(isAudioThread) never fires) - this is the best we can do.Michael WilsonThis logic is a bit weird. TryLock shouldn't be needed on the main thread. Right?
Michael WilsonRight.
This is actually a violation of the coding standard: we should either remove the `DCHECK`, remove the handling of `!IsAudioThread()`, or just remove both.
But, since I'm not confident that this is never called from the main thread, I left it here. I think if we want to remove this it should be a separate change so it could be rolled back independently if there is a problem.
Michael WilsonI ended up removing it; the DCHECK has been in place for a while and we can verify statically that this private method is only called from the audio thread.
(unresolve for visibility).
- Jeremy Roman
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson added 7 comments![]( "Open in Gerrit")
Hongchan Choi+hongchan for extra WebAudio experience
If this works, I'm happy with it. At some point in the past a straightforward replacement like this was impossible because WebAudio relied on recursive acquisition of the lock in subtle ways, which this CL turns into a thread deadlocking itself.
How confident are we that this (acquisition of a lock already held by the same thread) cannot in fact happen today? Would it be worth doing some additional legwork to validate that this is the case, or feature-guard it (and maybe even roll it out via Finch)?
Michael Wilson+1
I really can't say I am confident on possible fallout, but this CL can achieve a decent and meaningful clean up.
mjwilson@ - should we consider flag guarding this or doing a finch roll out?
Michael WilsonWhen I wrote this I was fairly confident, but I was looking more today and saw some discussion about issues when objects go out of scope, etc. that have made me less confident. I will do a bit more analysis and I also agree that a flag guard seems appropriate if I can't strongly prove it.
(Also, I will write a separate CL to remove the unused includes which should reduce the number of people who will get emails about this issue; sorry.)
Michael WilsonI tried a few things to get the thread analysis tools to verify the lock is not used recursively, but I think the lock is accessed through too many layers of indirection for the analysis to work.
The team has a plan to work on threading and architecture later this year so I will put this down for now and plan to pick it up again then. Set back to WIP.
I have added a flag guard. I don't think Finch is appropriate, because there's no metric to compare (either we will observe deadlocks or not). Once the change is verified we can remove the remaining files along with the flag.
Now I am working on some pre-work CLs that enable thread annotations and compile-time checking. If we can get those strong enough we may not need to land behind a flag after all.
This is the last usage of RecursiveMatrix, so also remove theMichael WilsonRecursiveMutex?
Whoops, done.
BASE_FEATURE(kWebAudioUseBaseLock, base::FEATURE_ENABLED_BY_DEFAULT);Can we have a comment on this flag?
Done
// rendering graph state (from main thread changes). It's OK if the tryLock()It needs to be updated. (I'd just rewrite this comment)
Done
// Must use a tryLock() here too. Don't worry, the lock will very rarely beDitto. Let's just rewrite this comment.
Done
Does adding "SCOPED_LOCKABLE" make sense here?
Yes, see https://crrev.com/c/7831498
void DeferredTaskHandler::Lock() NO_THREAD_SAFETY_ANALYSIS {Why is this necessary? I think it makes sense to add a comment on why it's needed here.
Also it looks like we don't need "NO_THREAD_SAFETY_ANALYSIS_FIXME" anymore on this annotation? If you have an explanation, it would be easier for me to review.
This was to disable the analysis when the flag was off, since the same mutex was not acquired. But with the new pre-work CL this may not be necessary anymore.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |