DTolm / VkFFT

Vulkan/CUDA/HIP/OpenCL/Level Zero/Metal Fast Fourier Transform library
MIT License
1.54k stars 93 forks source link

Arrays of buffer sizes lifetime #184

Open CoffeeExterminator opened 1 month ago

CoffeeExterminator commented 1 month ago

Hi, I have found out (the hard way) that the buffer sizes (bufferSize, inputBufferSize, etc.) provided in the configuration as pointers are required even after initialization and thus must be kept alive. Meaning, that if I do something like this

config.bufferSize = (uint64_t *) &size

The size variable must exist even after the initialization is completed, which, at least to me, was not the expected behavior (I understand they are in fact arrays since it's possible to use multiple buffers, but still): my assumption was that I can forget about config entirely after the initialization. Could you please clarify if there are any other parameters passed as pointers required after initialization in a similar manner? What about VkFFTLaunchParams? Are the structure or any of its fields required after VkFFTAppend completion? I have done some tests and it seems like they aren't, but asking just in case anyway.

fixgoats commented 1 month ago

I was literally just figuring out this was the problem I was having with my code and I feel kind of silly for not having figured it out earlier. I think you just need to apply the normal lifetime heuristics: when you assign any pointer to a field in the config, if the data it points to is stack allocated and you go out of the scope where the allocation occurs then you have a dangling pointer. This kind of stuff is why RAII is not just a buzzword but actually a useful practice that makes life a whole lot easier.

DTolm commented 1 month ago

Hello,

I made a change in the last update on develop branch, that VkFFT now stores the copy of all data that is passed as a pointer. This should solve the issue and now all contents of the config can be deallocated after completion of initializeVkFFT (unless I messed up something). The underlying structs that are passed (like VkDevice) still must not be released before the call to deleteVkFFT.

Best regards, Dmitrii

CoffeeExterminator commented 4 weeks ago

Hi, I have tested the develop branch and the issue seems fixed now, thank you.

The underlying structs that are passed (like VkDevice) still must not be released before the call to deleteVkFFT.

Got a little confused about this. What about VkQueue, VkCommandPool and VkFence used in the config?

DTolm commented 4 weeks ago

I just checked, VkQueue, VkCommandPool and VkFence can be released after the initialization call as they are only used for internal GPU calculations of LUT. Will update the documentation with that. Thanks!

CoffeeExterminator commented 4 weeks ago

Thanks for clarifying!

mohawk2 commented 2 days ago

None of my business, but... is this now fixed?

CoffeeExterminator commented 19 hours ago

@mohawk2 It is, in the develop branch.