FFT Window Threadsafety #7145
FFT Window Threadsafety #7145
Conversation
marcusmueller
force-pushed
the
fft_window_threadsafety
branch
from
3199bf0
to
85b437c
Compare
Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
…antics Also, remove unused config.h Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
marcusmueller
force-pushed
the
fft_window_threadsafety
branch
from
ff7844d
to
2840a0c
Compare
There was a problem hiding this comment.
The code looks good to me, but performance-wise I'm wondering if it wouldn't be better to get the window lock at the beginning of the work() function and hold it for the whole duration of work(). I'm slightly worried that if the FFT size is small, locking and unlocking the mutex for each individual FFT might introduce too much overhead.
|
CI stopped on Python formatting: |
There was a problem hiding this comment.
I tend to agree with @daniestevez, although I don't know about the performance overhead. But what if we flip it around: If we unlock sooner, we get better concurrency. To which I say "so what", what's the value of being able to call set_window() more often from a different thread. So if we lock for the duration of work(), we get maybe some, maybe very little performance gains, but we lose nothing of value.
|
@mbr0wn that's exactly what I had in mind. |
Co-authored-by: Martin Braun <mail@martin-braun.org>
|
I'm fine either way, but I remember I had to be finer in locking granularity in one place, because otherwise I could deadlock our CI, then made it consistent in every place. Will have to check what the precise reason was, and whether it affects work, probably not. Anyways, currently no time, so this becomes a draft for now. |
| """ | ||
| for fftlength in [1, 2, 3] + [2**i for i in range(2, 10)] + list(primes): | ||
| self.assertTrue(self._set_length_and_test(fftlength, fftlength)) | ||
| self.assertFalse(self._set_length_and_test(fftlength, fftlength+2)) |
There was a problem hiding this comment.
Not sure if this will solve ./gr-fft/python/fft/qa_fft.py:288:76: E226 missing whitespace around arithmetic operator but probability it will, according to E226
| self.assertFalse(self._set_length_and_test(fftlength, fftlength+2)) | |
| self.assertFalse(self._set_length_and_test(fftlength, fftlength + 2 )) |
Description
I add locks around window setting/usage.
As a drive-by improvement, let's not use floor, ceil to implement (inherent) integer division semantics.
Related Issue
Fixes #7144
Which blocks/areas does this affect?
FFT
Testing Done
Checklist