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

Fix resize! for JLVector #546

Merged
merged 3 commits into from
Jul 5, 2024
Merged

Conversation

jipolanco
Copy link
Contributor

This fixes a reference counting issue when using resize! on a JLVector.

The issue is illustrated by the following example:

using JLArrays

u = JLArray{Float64}(undef, 0)
data_prev = u.data
resize!(u, 16)
data = u.data

GC.gc()
data_prev.rc.count[]  # should be 0 (old data was freed)
data.rc.count[]       # should be 1 (new data is still available)

Without this PR, the last two lines incorrectly give 1 and 0 when it should be the opposite.

This means in particular that doing things like copy(u) after resize! failed with ArgumentError: Attempt to use a freed reference..

Copy link
Member

@maleadt maleadt left a comment

Choose a reason for hiding this comment

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

Thanks. Calling finalize is too expensive of an operation though, so we should probably do something like CUDA.jl: https://github.com/JuliaGPU/CUDA.jl/blob/14de0097ff7c26932cc4a175840961cc7d3f396e/src/array.jl#L831-L861

@jipolanco
Copy link
Contributor Author

Thanks for your quick response.

If I understand correctly, finalize just calls unsafe_free!, which is also used in the linked CUDA.jl implementation, so I'm not sure I see how one could avoid such an expensive operation. Or would it make sense to simply replace the call to finalize with unsafe_free! to be more explicit?

@maleadt
Copy link
Member

maleadt commented Jun 28, 2024

Or would it make sense to simply replace the call to finalize with unsafe_free! to be more explicit?

Yeah. finalize invokes the GC, which is slow. It's not that unsafe_free! is slow.

@jipolanco
Copy link
Contributor Author

Right, I didn't think about that. I'll use unsafe_free! then.

@maleadt maleadt merged commit 80b8226 into JuliaGPU:master Jul 5, 2024
14 checks passed
@jipolanco jipolanco deleted the jlarray-resize branch July 5, 2024 10:32
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.

2 participants