This is now working locally for me, and tests are passing, when used with FFTW_jll v3.3.9+7.

Can you bump the version requirement on FFTW_jll in Project.toml?

Yeah I wasn't sure what to do about that... I don't think you can specify a build number in Project.toml. If I just change the compat section so that FFTW_jll's version is "3.3.9+7" I get the following error when trying to use FFTW:

ERROR: Could not parse compatibility version for dependency `FFTW_jll`

As long as people update their packages this should work since they will get the most recent build of FFTW_jll, but I don't know how to make sure they get v3.3.9+7 (or later).

I can update the version requirement for FFTW_jll to be 3.3.9, but that won't always work if people are using an older FFTW_jll build. Would that suffice?

I don't think you can specify a build number in Project.toml.

That's another reason why we want to change the versioning system of JLL package (and the reason why I was waiting to merge the PR, I knew we'd have had this situation)

I imagine that not being able to specify the build number will not be a problem for people, unless they have pinned or dev'd FFTW_jll.

In order to make sure that it works, it seems like you would need to create a new FFTW_jll version v3.3.10 that corresponds to fftw3 v3.3.9, because FFTW_jll v.3.3.9 has been wrapping a pre-release version of fftw v3.3.9 for a while (likely for a good reason), meaning that there is currently no way to specify the released fftw 3.3.9 artifact with FFTW_jll's version.

I'm still not clear on how package maintainers will be able to specify which artifact version they want if the _jll package version number is not the same as the underlying artifact, even though I understand why it is necessary for them to not be the same. Will there be a lookup table in the README of each _jll github project page? Or will there be some other programmatic way to do it, which might even have Pkg support?

I imagine that not being able to specify the build number will not be a problem for people, unless they have pinned or dev'd FFTW_jll.

There are weird situations where users may end up updating FFTW but not FFTW_jll. Not really common, but not totally impossible.

I'm still not clear on how package maintainers will be able to specify which artifact version they want if the _jll package version number is not the same as the underlying artifact, even though I understand why it is necessary for them to not be the same. Will there be a lookup table in the README of each _jll github project page? Or will there be some other programmatic way to do it, which might even have Pkg support?

We haven't worked this out yet because it isn't a trivial problem. If you have some ideas please share in Yggdrasil.

Not really common, but not totally impossible.

Exactly, it seems like a pretty unlikely problem.

I think that this PR should be merged before there is both consensus about, and implementation of, and improved version system in Yggdrasil—which may take quite some time. The change here simply provides access to a function in the most recent version of fftw. I have updated the Project.toml for this package so that it requires that version of fftw (3.3.9), which should just work for the vast majority of users, and indeed worked for CI here. The potential downside of adding this function is that due to their local Pkg state, it's possible that a user will have a fftw library that reflects the state of fftw around Sep 17 2019, instead of the released 3.3.9 version, causing this new function to error. However, this Pkg state seems unlikely, even though it is possible. The fix for any affected users would be to simply run pkg> update, and possibly unpin their FFTW_jll, if they had it pinned.

Furthermore, it seems like the reason that FFTW_jll was shipping a pre-released version of fftw was to provide access to fftw_threads_set_callback, enabling mutlithreaded FFTW plans using Julia's partr threads. I see substantial performance improvements in DSP.jl's convolutions when using fftw's multithreaded transformations, but different sized convolutions seem to perform best with different numbers of threads (often one is best). While packages can change the number of threads being used by fftw with set_num_threads, doing so currently changes the global state of Julia since there is no way to see what that number was beforehand. This new function, get_num_threads, allows package authors to programatically change the number of fftw threads without changing global sate, and in a way would unlock the benefits of the (great) change that @stevengj made to enable fftw to use Julia's threads, even though that change is causing some consternation with package versions at the moment.

I added the vendor check.

It occurs to me that the way I would want to use this would benefit from support in this package. I would want to acquire the fftwlock, change the number of threads, make some plans, restore the original number of threads, and finally release the fftwlock. To minimize version numbers I could add it to this PR, if you approve.

The function would look something like:

@exclusive function set_num_threads(f, nthreads::Integer)
    orig_nthreads = get_num_threads()
    try
        ccall((:fftw_plan_with_nthreads,libfftw3), Cvoid, (Int32,), nthreads)
        ccall((:fftwf_plan_with_nthreads,libfftw3f), Cvoid, (Int32,), nthreads)
        f()
    finally
        ccall((:fftw_plan_with_nthreads,libfftw3), Cvoid, (Int32,), orig_nthreads)
        ccall((:fftwf_plan_with_nthreads,libfftw3f), Cvoid, (Int32,), orig_nthreads)
    end
end

and would enable planning like:

plan = FFTW.set_num_threads(4) do
    plan_rfft(...)
end

The function argument would have to be careful to avoid a context switch, e.g. no IO. However, it seems like you would want to hold the lock while making plans to ensure that the plans were made with the intended number of threads.

The function argument would have to be careful to avoid a context switch, e.g. no IO.

Why?

Yes, adding that set_num_threads method seems reasonable.

(You wouldn't need to ccall though — just call set_num_threads(::Integer).)

Or we could add an nthreads keyword argument to the plan_fft functions.

You wouldn't need to ccall though — just call set_num_threads(::Integer)

Wouldn't set_num_threads(::Integer) try to acquire the lock again?

Or we could add an nthreads keyword argument to the plan_fft functions.

That also sounds great

Codecov Report

Merging #171 (1af16ec) into master (683a6e8) will decrease coverage by 2.16%.
The diff coverage is 42.30%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   63.24%   61.07%   -2.17%     
==========================================
  Files           5        5              
  Lines         438      483      +45     
==========================================
+ Hits          277      295      +18     
- Misses        161      188      +27     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 683a6e8...1af16ec. Read the comment docs.

I have implemented the suggestions in your code reviews, as well as your suggestion to add a num_threads keyword to the plan_... functions. My approach to adding the num_threads keyword argument here is pretty naive, but I couldn't think of a better way of adding the keyword argument to all of these functions. I haven't yet added tests for this new keyword argument, since I'm not sure this is the way you'd want to proceed. If so, I'd be happy to try to add some tests.

Gentle bump on this. Not sure what's left to be done but I would love to see this merged.

bump. The depending package NFFT.jl is multi-threaded and it would be nice to chose the same number of threads all operations without changing global state.

Another friendly bump 🙏.
This would be nice to have.

I merged master onto this branch, I hope @galenlynch won't mind 🙂