Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport filt fix (#516 and #539) and bump version to 0.7.10 #566

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

martinholters
Copy link
Member

Backports #539 and, as a prerequisite, #516. (A more targeted subset of the latter might be in order, but including it wholesale was simplest to do.)

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 98.06763% with 4 lines in your changes missing coverage. Please review.

Please upload report for BASE (release-0.7@1737340). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/Filters/stream_filt.jl 81.81% 2 Missing ⚠️
src/Filters/remez_fir.jl 94.73% 1 Missing ⚠️
src/dspbase.jl 98.14% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##             release-0.7     #566   +/-   ##
==============================================
  Coverage               ?   97.03%           
==============================================
  Files                  ?       18           
  Lines                  ?     3104           
  Branches               ?        0           
==============================================
  Hits                   ?     3012           
  Misses                 ?       92           
  Partials               ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@martinholters
Copy link
Member Author

martinholters commented Sep 13, 2024

Tests fail on nightly, but that's unrelated, pre-existing on v0.7.9, and should be "solved" with #565.

wheeheee and others added 2 commits September 13, 2024 10:06
* 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)
In some cases when `filt(::FIRFilter{FIRArbitrary}, x)` is called
with certain values of `x`, `filt!(buffer, ::FIRFilter{FIRArbitrary}, x)`
tries to write one sample too many to the buffer and a `BoundsError`
is thrown.  Add one extra sample to the buffer to catch these
exceptional cases.

(This code is really a hack and simply works around the deeper problem
of calculating the correct output buffer length; this should instead be
addressed properly in a future change.)

Closes #317.

(cherry picked from commit 73d3214)
@martinholters martinholters changed the title Backport filt fix (#516 and #539) Backport filt fix (#516 and #539) and bump version to 0.7.10 Sep 13, 2024
@martinholters martinholters marked this pull request as ready for review September 13, 2024 08:16
@martinholters martinholters mentioned this pull request Sep 13, 2024
Copy link
Member

@wheeheee wheeheee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may also be worth noting again (in release notes?) that a portion of #516's fix is for 1.9/1.10, and kind of trashes performance for at least 1.6...
Some time ago I was working on a sort of multi-versioning inside a generated function, but iirc it was rather finicky.

@martinholters
Copy link
Member Author

Ah, that's a bit unfortunate, but with 1.10 becoming LTS soonish (fingers crossed), I wouldn't worry too much. That is, if anyone steps up to backport the bugfix of #539 without the performance changes of #516, that would be welcome, otherwise I'll merge this one in a couple of days.

@wheeheee
Copy link
Member

Ok, I think there's a way to mitigate the optimized _filt_fir! regressions in 1.6.

So, on my tweaks branch, I previously believed that I had addressed the issue for large-ish lengths (near SMALL_FILT_CUTOFF, about 50), but worsened it for N~15.
I've just found the offending commit (wheeheee/DSP.jl@407d30b), which adds a line making the optimized _filt_fir! return a tuple, meant to enable reuse for stateful filters. This works as intended on 1.10, but on 1.6 it causes that regression.
These performance hacks are maybe a bit voodoo, but would it help if that branch was cleaned up and merged, and that one commit reverted here?

For clarity, the regression I'm talking about, all on Julia 1.6:

julia> x = rand(10_000); out = similar(x);

julia> function testvars(n)
           a, b = 1., rand(n)
           si, bs = zeros(n - 1), Val(n)
           return (a, b, si, bs)
       end
testvars (generic function with 1 method)

julia> @btime filt!($out, b, a, $x, si) setup=((a, b, si) = testvars(15));
  18.400 μs (2 allocations: 96 bytes)  # master
  32.300 μs (0 allocations: 0 bytes)   # tweaks branch
  19.000 μs (0 allocations: 0 bytes)   # tweaks branch, revert commit 407d30bd

julia> @btime filt!($out, b, a, $x, si) setup=((a, b, si) = testvars(18));
  18.600 μs (2 allocations: 96 bytes)
  47.600 μs (0 allocations: 0 bytes)
  18.900 μs (0 allocations: 0 bytes)

julia> @btime filt!($out, b, a, $x, si) setup=((a, b, si) = testvars(30));
  36.100 μs (2 allocations: 96 bytes)
  34.400 μs (0 allocations: 0 bytes)
  21.500 μs (0 allocations: 0 bytes)

julia> @btime filt!($out, b, a, $x, si) setup=((a, b, si) = testvars(50));
  85.500 μs (2 allocations: 96 bytes)
  47.300 μs (0 allocations: 0 bytes)
  32.000 μs (0 allocations: 0 bytes)

@martinholters
Copy link
Member Author

I was hoping for something which makes the backport simpler, not tack even more stuff on which hasn't even landed in master yet and might itself warrant some detailed review/discussion. If you open a PR, we discuss, backport (hoping it backports cleanly) ... that will take quite a while and the point is to do a timely release of v0.7.10 to get the bugfix out.

So, I'll go ahead with this as is. If you PR your tweaks we can always think about backporting them and doing a v0.7.11, but finally cutting v0.8.0 would also be quite something...

@martinholters martinholters merged commit c6663ab into release-0.7 Sep 27, 2024
9 checks passed
@martinholters martinholters deleted the mh/backport-539 branch September 27, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants