wildart / FLANN.jl

A Julia wrapper for Fast Library for Approximate Nearest Neighbors (FLANN)
Other
16 stars 14 forks source link

Issue with Memory ownership of FLANN data points in C #27

Closed MAminSFV closed 2 years ago

MAminSFV commented 2 years ago

I am trying to use the FLANNIndex struct in my codebase, and I am receiving the wrong outputs for inrange calls. I think it has to do with how the data in C is being handled, but I am not sure.

The getindex calls also return wrong outputs.

This doesn't happen in a simple example usage of FLANN, and I think it has to do with the change in scope in Julia code and how the FLANNIndex related pointers are handled in memory.

@wildart I wondered if you could help me with this issue and provide more insight.

wildart commented 2 years ago

Can you clarify on wrong outputs? FYI, FLANN does approximate NN, so what you end up with not necessarily the same result when you use exact NN.

MAminSFV commented 2 years ago

I am sorry for the short description.

To be more exact, I am trying to run knn and inrange on a dynamic data structure by mutating a FLANNIndex. I am using addpoints! to add points to FLANNIndex, which I initialize with one (one column matrix).

A simple example in REPL works fine with the general scope. However, I get some unexplainable results in my more sophisticated codebase that stores the FLANNIndex as a part of a struct. For instance, a pair of points that are in the range of each other are not returned by inrange, and I get neighbors with zero distance.

I suspect that the data structure created in C and FLANNIndex has the pointer to is being garbage collected and not preserved or owned by Julia.

It's the same case for calling getindex on FLANNIndex types. Most of the time, in my code base, I retrieve some small random value that is approximately zero. (1.25e-340 something)

Would you think that the points in C might be deleted if there are a lot of scope changes in Julia, or could it be something else?

Unfortunately, I don't understand how this happens, but I will try to make a code snippet to replicate it.

wildart commented 2 years ago

I suspect that the data structure created in C and FLANNIndex has the pointer to is being garbage collected and not preserved or owned by Julia.

It looks that way. After quick look at the FLANN code., it seems that the new points are added as pointers to the index, so if original data structure (points' matrix) is collected then you loose referenced data in the index. You need to anchor your data before adding it to the index to prevent its collection. E.g. try to keep it along with an FLANNIndex object.

FYI: https://discourse.julialang.org/t/avoid-gc-freeing-ptr-nothing/38664/21

MAminSFV commented 2 years ago

Thank you for the explanation and the discussion you shared.

This sheds more light on the process.

Currently, I have the added points and the initial points as a field in a struct, so I am not sure if they are being GCed. The current problem seems to be related to adding points. I have the points stored in a Vector{SVector{N,T}} dynamically and at each iteration add the new point to FLANN in the following way:

# Update points
push!(struct.vec_of_points, new_ponit)

# Update FLANNIndex
FLN.addpoints!(struct.FLANN, struct.vec_of_points)

In the discussion you shared, it seems that there are two options, one is using GC.@preserve and the other is defining a Base.cconvert method. I am not sure if the first approach suits my code since there are a lot of different function calls and changes in scope.

Would you think it is possible to use the second approach and how would that be like? Should we add a Base.cconvert to FLANN.jl or rather I should implement something similar in my codebase.

I really appreciate your thoughts on this.

MAminSFV commented 2 years ago

I just found the problem, and it was rather a detail. It seems that SVectors won't work with FLANN.jl. I don't exactly know why and how, but my problem got fixed after storing my points as Vectors rather than SVectors.

wildart commented 2 years ago

I guess with SVectors you would need, in addpoints! call, to pass an underlying tuple storage data field of SArray and not a structure object itself.

MAminSFV commented 2 years ago

That's a good idea! The only problem is the function call addpoints! doesn't have a dispatch on tuple type.

MAminSFV commented 2 years ago

Thank you for your help. @wildart I got everything to work. (with normal vectors)

Should I close this issue and open another one for adding SVector support?

wildart commented 2 years ago

I see SVector support as a marginal case, so I won't do it. This package basically is in a maintenance mode, so no new features.