-
Notifications
You must be signed in to change notification settings - Fork 664
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
add fftw_copy_plan #314
add fftw_copy_plan #314
Conversation
My first reaction is to say that this is a bad idea. If the use case is C++ wrappers, C++ already has standard facilities for wrapping plain old pointers into reference-counted objects, with a menu of mutually exclusive semantics (shared vs unique vs move). I don't see what problem is solved by reimplementing this functionality at a lower level. Is there some case where std::make_shared doesn't do what you need? Or perhaps you have some other language in mind? |
I was actually thinking of Julia and Python, if they want to implement a It's not a huge deal, just a slight annoyance that there is no way to copy a plan in the FFTW API, and easy for us to provide. And of course it might be useful in C and low-level languages as well, which don't have built-in reference counting or garbage collection, if you want to pass plan pointers around without having to keep track of who owns the pointer. |
I'm not sure how you can use |
I don't know, it still smells like a bad idea to me, but if you think this is needed, go ahead and push it. My main objection is that this change doesn't solve any real problems, since I am afraid that every real use case will end up wrapping a plan into some other high-level object, at which point you may as well use native facilities to resource-manage the object. You allude to this problem yourself when you say that the object must contain some other information anyway, e.g. array size. Your comment on make_shared is valid. I had in mind the case where the plan is wrapped in a C++ object, because no self-respecting C++ programmer would be caught dead using a raw C pointer. But I don't feel strongly about this issue. If you push it, maybe add a test somewhere. |
There is already a test (for every plan the test program creates) |
We should probably set up GitHub CI at some point… |
I'm going to go ahead and merge. |
I also badly need to make copies, but not in this form. I looked at the implementation; this is just a reference counter "copy"; this defeats the purpose of having a copy. The idea of having independent copies of the plans is not about multiple objects that share a reference; In my reading, this PR is not in the spirit of the original request: #106. PS1: I am also implementing yet another FFTW C++ wrapper; in my case, I want to make the wrapper thread-compatible (not necessarily thread-safe, but thread-friendly). PS2: I always assumed this would be eventually implemented with a |
@correaa fftw plans are already "thread safe" in the sense that the plan is read-only when executing the plan, and thus you can execute the same plan from multiple threads. Plans do not store buffers, but they do store immutable arrays of trigonometric constants and other read-only information. So I don't think that the problem you mention needs to be solved. |
OK, I have been awfully mistaken for a while then. I was convinced that plan execution (over disjoint targets) was not thread-safe. In my mind, it made sense because I assumed that the plan creation would allocate buffers to perform the ffts, so executions themselves wouldn't need to do it at that point. I was so convinced about this that I misread the documentation: In that case, I wonder if there are buffers behind the scenes in some global state of the library. Anyhow, given that:
Thank you for the rapid answer. |
There are no buffers in plans. When a plan needs a buffer it calls malloc(), or alloca() when the buffer is really small. I think I agree with all your points 1-5 (see my Mar 2 comment), but other people feel otherwise. |
I am surprised about that also. (I am surprised often). Unless I am missing something, once the library decides what plan parameters to use, the amount of memory needed for execution seems to be determined.
Yes, this is IMO why it is so hard to write a good C++ wrapper for FFTW, because there are very subtle semantic implications that are hard to capture with simple classes. The other problem with naive reference counting is that it is not thread-safe itself. Anyway, food for thought for FFTW4 :) Cheers! |
Closes #106.
Since plan execution is read-only, it seemed sufficient to implement this simply by adding an internal reference count to
fftw_plan
, so thatfftw_plan
just increments the counter andfftw_destroy
plan decrements the counter, only de-allocating the plan when the reference count drops to zero.@matteo-frigo, what do you think?