JuliaMath / FFTW.jl

Julia bindings to the FFTW library for fast Fourier transforms
https://juliamath.github.io/FFTW.jl/stable
MIT License
273 stars 55 forks source link

reduce allocations in dims_howmany #269

Closed jishnub closed 1 year ago

jishnub commented 1 year ago

This reduces some unnecessary allocations in dims_howmany. This may be reduced further by using StaticArrays, but that's a separate discussion as it'll increase the load time.

On master

julia> X = rand(100, 100);

julia> @btime FFTW.dims_howmany($X, $X, $(collect(size(X))), 1)
  689.000 ns (21 allocations: 1.59 KiB)
([100; 1; 1;;], [100; 100; 100;;])

This PR Updated after https://github.com/JuliaMath/FFTW.jl/pull/269/commits/e15a9b8c8576a714f95d70cf32edf093c0b647a1

julia> @btime FFTW.dims_howmany($X, $X, size($X), 1);
  73.707 ns (2 allocations: 160 bytes)
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 96.00% and project coverage change: +1.59 :tada:

Comparison is base (82a99dc) 69.09% compared to head (270c27a) 70.68%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #269 +/- ## ========================================== + Coverage 69.09% 70.68% +1.59% ========================================== Files 5 5 Lines 495 522 +27 ========================================== + Hits 342 369 +27 Misses 153 153 ``` | [Impacted Files](https://codecov.io/gh/JuliaMath/FFTW.jl/pull/269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath) | Coverage Δ | | |---|---|---| | [src/fft.jl](https://codecov.io/gh/JuliaMath/FFTW.jl/pull/269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath#diff-c3JjL2ZmdC5qbA==) | `68.75% <96.00%> (+2.16%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

stevengj commented 1 year ago

Thanks, this is ancient code that needed a fresh look.

There's nothing StaticArrays can do that couldn't be done with tuples, perhaps with slightly more code, and I'd prefer to avoid the StaticArrays dependency.

jishnub commented 1 year ago

Thanks! I've updated it now, and all the unnecessary allocations are now avoided if region is an Int or a Tuple

jishnub commented 1 year ago

This should be ready now

jishnub commented 1 year ago

Gentle bump

jishnub commented 1 year ago

266 should ideally have been v1.7.0 instead of v1.6.1 as it is mildly breaking, so I'm bumping the version in this PR instead.

jishnub commented 1 year ago

Gentle bump @stevengj

stevengj commented 1 year ago

LGTM