Refactor analyser node #256
Conversation
- use shared buffer to move audio data to control thread - implement full API
This comment was marked as outdated.
This comment was marked as outdated.
|
Just added a patch for #236 too |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Hey, I took a first look and I think this very well reflects the 'safe' version we have discussed. Really nice!
One important thing to notice is that the outputs between the current version and yours differ by a lot. Try running your analyser example on main and your branch and see what I mean. You also notice this in the mic_playback example. I wonder if maybe the current version is broken?
I left some small nitpicks that could further improve performance. I will also reply to your comments in the issue in a minute.
In any case, please proceed with your adventures
src/analysis.rs
Outdated
|
|
||
| // single producer / single consumer ring buffer | ||
| pub(crate) struct AnalyserRingBuffer { | ||
| buffer: Arc<Vec<AtomicF32>>, |
There was a problem hiding this comment.
alternatively, make it like this:
#[derive(Clone)]
pub(crate) struct AnalyserRingBuffer {
buffer: Arc<[AtomicF32]>,
write_index: Arc<AtomicUsize>,
}
Then you never need to wrap the thing inside an Arc, just clone them. I think it saves a level of indirection so it is more performant
There was a problem hiding this comment.
You can create an Arc<[..]> with Arc::from(&vec_of_atomics[..]). (you need an intermediate allocation that hopefully the compiler will skip)
There was a problem hiding this comment.
ok I did the alternative but kept the Arc<Vec<AtomicF32>>. I didn't manage to create the array from the Vec, with your snippet the build failed because it gives Arc<&[AtomicF32]> rather than `Arc<[AtomicF32]>.
There was a problem hiding this comment.
It was easier than I thought: 1c29c2f
Showing a nice bit of extra performance, before:
bench_analyser_node
Instructions: 41181322 (-18.52477%)
L1 Accesses: 58665001 (-17.67864%)
L2 Accesses: 154232 (-63.80796%)
RAM Accesses: 84974 (-1.944403%)
Estimated Cycles: 62410251 (-18.34027%)
after:
bench_analyser_node
Instructions: 39385732 (-22.07732%)
L1 Accesses: 56186630 (-21.15673%)
L2 Accesses: 158969 (-62.67475%)
RAM Accesses: 84981 (-1.936325%)
Estimated Cycles: 59955810 (-21.55078%)
This comment was marked as outdated.
This comment was marked as outdated.
Hum weird indeed, I will have a look |
This comment was marked as outdated.
This comment was marked as outdated.
|
Ok, I checked the values and they are for sure wrong in So I think we are all good on this side! |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
- buffer: Arc<Vec<AtomicF32>>, + buffer: Arc<[AtomicF32]>,
|
src/analysis.rs
Outdated
| // If not wrapped into Arc<Mutex<T>>, compiler complains about thread safety: | ||
| // > `(dyn rustfft::avx::avx_planner::AvxPlannerInternalAPI<f32> + 'static)` | ||
| // > cannot be shared between threads safely | ||
| // But the Mutex is ok here as `Analyser` lives outside the audio thread. |
There was a problem hiding this comment.
Are you sure this is necessary? It works on my hardware without the Arc+Mutex.
It would also surprise me, Mutex does not magically make non-Send types Send. Being not-Send means you cannot leave the thread you were created on. Arc+Mutex specifically allows one to transfer objects from one thread to another
There was a problem hiding this comment.
I took the liberty to remove them. Let me know if building fails on your hardware
There was a problem hiding this comment.
Hah, it fails on CI. The Mutex is required to make the thing Sync
|
The Mutex is necessary on avx-enabled systems This reverts commit 589985b.
|
So to summarize, this PR
I could not be more happier |
|
Hey,
This is quite a big PR regarding the AnalyserNode and what we discussed in #253:
Arc<AtomicF32>) to ship audio data from audio thread to control threadanalyser) to complete the quite complexmic_playbackoneI'm not completely sure that what have done for thread safety (i.e. moving the node to a different thread as you have done in the
mic_playbackexample) is the cleanest / simplest possible way, but it builds and seems to work quite well. But any case having your thoughts on this point would be nice.Let me know what you think