filt perf optimizations
#516
Conversation
|
managed to squeeze performance of |
wheeheee
changed the title
filt: create si only once, and reusefilt perf optimizations
|
Bump, couldn't figure out how to make |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #516 +/- ##
==========================================
+ Coverage 97.46% 97.49% +0.03%
==========================================
Files 18 18
Lines 3080 3078 -2
==========================================
- Hits 3002 3001 -1
+ Misses 78 77 -1 ☔ View full report in Codecov by Sentry. |
|
@martinholters Ok to merge? Hope you don't mind the ping. |
There was a problem hiding this comment.
@martinholters Ok to merge? Hope you don't mind the ping.
Don't mind at all. In principle ok with me. I'm not too excited about all those muladds; I find them less readable than the straight a*b+c and don't think they bring benefits to justify that everywhere. That said, now that all the replacements have been done, I don't think it's worth it to go through them and see whether they bring any tangible benefits.
One thing I'm not sure about and would like a word of explanation is all those deleted filt(self, ...) methods. What's up with those? Why don't we need them anymore?
|
They were duplicates of each other except for the kernels so I edited the last one to accept all the |
|
Ah, yes, of course. I had missed the edited one. |
|
Merge? Also @wheeheee would you be ok to be invited to the organization to help maintain these packages? |
|
I will say I don't have serious expertise in DSP, although I don't mind helping clean up issues / add documentation occasionally, and actually have a few more minor commits saved up to contribute. Also, it might be good to do some benchmarking on other hardware for this commit. My laptop cpu (i5-1135g7) has avx512 instructions, which may possibly be the reason why the inbounds fiddling in |
|
@wheeheee Thanks for helping with the maintenance here. Since we have approval here from @martinholters, it would be great to rebase and get this one merged. |
- use Base.Cartesian.@ntuple
major speedups for N > 18, much slower if inbounds for small N
>20% faster for fir
no need to assign values to enum
- eachindex stuff - `==` to `===` nothing - `npairs = max(nz, np)`
- in `unsafe_conv_kern_os!` - probably no real impact
- reduced exceptions in `@code_llvm` for `N < 18` - `@noinline mul!`, `broadcast`, only for julia > v1.8 - name `SMALL_FILT_VECT_CUTOFF`, selective bounds check
|
Right, I think I'll just go ahead and merge to speed up CI. If the newest commits need modification, I'll open a PR later. |
|
Thanks @wheeheee, any help here is appreciated. And also thanks @ViralBShah for pushing this forward. |
|
Thanks @martinholters for all the work on this package, and @wheeheee for jumping in. We should see if there are other contributors who would like to step up as well. I'm just trying to help out - amazing how the small improvements over the last few days are making a big cosmetic difference at least. |
* create `si` only once, and reuse * no inbounds faster for N < 24 * @generated `_small_filt_fir!` - use Base.Cartesian.@ntuple * inbounds if N > 18 major speedups for N > 18, much slower if inbounds for small N * reorder store in `_filt_fir` and `_filt_iir` >20% faster for fir * maybe prevent undefvarerror no need to assign values to enum * small things - eachindex stuff - `==` to `===` nothing - `npairs = max(nz, np)` * isone, iszero, etc. * `filt` for `FIRFilter`: remove redundant definitions * extract type instability from loop * remove fft conv type instability - in `unsafe_conv_kern_os!` - probably no real impact * correct `filt` arg types * nicer loop * inv plan * cleanup filt_order * use `muladd`s in filt.jl * `_prod_freq` * more `muladd`s * `fill!`, `reverse`, don't broadcast scalar / * micro-optimizations, cosmetics - reduced exceptions in `@code_llvm` for `N < 18` - `@noinline mul!`, `broadcast`, only for julia > v1.8 - name `SMALL_FILT_VECT_CUTOFF`, selective bounds check * Compat noinline * add test for error (cherry picked from commit 9de4c0c)
* create `si` only once, and reuse * no inbounds faster for N < 24 * @generated `_small_filt_fir!` - use Base.Cartesian.@ntuple * inbounds if N > 18 major speedups for N > 18, much slower if inbounds for small N * reorder store in `_filt_fir` and `_filt_iir` >20% faster for fir * maybe prevent undefvarerror no need to assign values to enum * small things - eachindex stuff - `==` to `===` nothing - `npairs = max(nz, np)` * isone, iszero, etc. * `filt` for `FIRFilter`: remove redundant definitions * extract type instability from loop * remove fft conv type instability - in `unsafe_conv_kern_os!` - probably no real impact * correct `filt` arg types * nicer loop * inv plan * cleanup filt_order * use `muladd`s in filt.jl * `_prod_freq` * more `muladd`s * `fill!`, `reverse`, don't broadcast scalar / * micro-optimizations, cosmetics - reduced exceptions in `@code_llvm` for `N < 18` - `@noinline mul!`, `broadcast`, only for julia > v1.8 - name `SMALL_FILT_VECT_CUTOFF`, selective bounds check * Compat noinline * add test for error (cherry picked from commit 9de4c0c)
si(different from inputsi)filtcurrently errors forN>2(edit: if the trailing dimensions aren't all of length 1); this PR doesn't change that