Fix thread safety in fast_tanh toggle - #178
Conversation
Use atomic<bool> and runtime check instead of modifying shared map.
sdatkinson
left a comment
sdatkinson
left a comment
There was a problem hiding this comment.
Thanks for the PR!
If you can check to make sure that this doesn't clash with the real-time safety of any of the nets, then this seems ok.
Also, some testing to cement the intent of the change would be appreciated.
| { | ||
| if (_activations.find(name) == _activations.end()) | ||
| // Return FastTanh when Tanh is requested and fast_tanh mode is enabled | ||
| if (name == "Tanh" && using_fast_tanh.load(std::memory_order_relaxed)) |
There was a problem hiding this comment.
Does this change make the WaveNet's getting of the sigmoid activation for its gating activation no longer real-time safe?
NeuralAmpModelerCore/NAM/wavenet.cpp
Line 56 in 6ea03b1
|
Thanks for the review! Real-time safety: For non-"Tanh" lookups like WaveNet's "Sigmoid": It just hits the string comparison (false), then goes straight to find() - same as before, actually one fewer map lookup since we reuse the iterator now. For "Tanh" lookups, the atomic load is lock-free with memory_order_relaxed - compiles down to a plain memory read on x86/ARM. One thing I did consider: _activations.at("Fasttanh") technically can throw, though "Fasttanh" is guaranteed to exist. If you'd prefer, I can cache the pointer or use find() instead to be more defensive. Tests: Happy to add tests - what would you like to see? Unit tests for toggle behavior, multi-threaded stress tests, or both? |
…urrent access Local fork for Absolute Stereo NAM. Investigating upstream issue sdatkinson#303 ("Loading certain A1 models after A2 models can cause audio crackling/dropouts") led here by a different route: nam::activations::Activation::_activations is a process-wide static std::unordered_map, mutated by enable_fast_tanh()/disable_fast_tanh()/enable_lut()/disable_lut() and read by get_activation() -- with no synchronization anywhere. The downstream plugin (NeuralAmpModeler.cpp) calls enable_fast_tanh() unconditionally in its constructor (inherited from the original upstream scaffold, PR sdatkinson#178). get_activation() runs during model construction on this project's background staging thread (never during real-time process() -- confirmed by checking every call site: LSTM/ConvNet/WaveNet layer constructors only). Because this state is process-wide rather than per-instance, one plugin instance being constructed (a write, on the main thread) can race with a *different*, already-running instance's background staging thread reading the same map (get_activation(), mid model load) -- an ordinary multi-instance/parallel-bus scenario, not a contrived one, and previously an unsynchronized write/read race on a std::unordered_map: undefined behavior. Added a static std::mutex guarding every access to _activations and its *_bak backup pointers: get_activation() (both overloads that touch the map directly), enable_fast_tanh(), disable_fast_tanh(), enable_lut(), disable_lut(). Never taken from any real-time process() path, so this adds no audio-thread lock. Whether this is *the* cause of sdatkinson#303 is not established -- the reporter's own plugin code isn't available to inspect, and their symptom is a real bug that could have several explanations, but it's a real design pattern (unscoped global mutable activation state, exactly the kind of thing a per-model LUT calibration bug -- "calibrate for one model, then a different model reads the same global entry" -- would look like) worth flagging as a lead upstream, independent of whether it's confirmed to be the exact mechanism. Validated: full downstream plugin build (VST3+app) + complete automated test suite pass; RealtimePathBenchmark/ModelCaptureLatencyBenchmark checksums byte-identical to baseline (adds synchronization only, no behavior change under the single-threaded call pattern every existing test exercises).
2 participants
Summary
Test plan