[Review][Java] Fix random segabort/segfault/double free problems

[ldematte]

The cuVS C API we are leveraging uses a common C pattern for "object creation": you pass down a pointer, and that gets populated. Often, this is a **, and memory gets allocated on the other side:

create(voi**ptrPtr) {
   *ptrPtr = new Something;
}
void** ptrPtr = // allocate space for a pointer
create(ptrPtr);
void* ptr = *ptrPtr;

In Panama, this means

• allocating a small pointer size MemorySegment, very short lived
• call create
• extract a MemorySegment from the MemorySegment

Existing code sometimes skipped the extraction bit (the dereference), and/or created the structure directly instead of using the C create functions; this lead to some segfaults (addressing the wrong memory/wrong size) and to some double frees (the JVM freed MemorySegments we allocated via an arena, but we also called the destroy functions, so the C side tried to free them too).

Plus, this PR tries to reduce the lifetime of some allocations, avoiding to use the CuVSResources arena when it is clearly not needed -- but I did not want to expand the scope too much, so I just did the more obvious ones. Optimizing memory allocation is still a WIP, especially on the search side, but this I think will deserve its own PR.

These changes makes the test suite pass reliably - I have not seen a segmentation fault over hundreds of executions locally, and lowers the native memory occupied a little.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[benfred]

/ok to test b26ff62

[mythrocks]

create(voi**ptrPtr) {
   *ptrPtr = new Something;
}
void** ptrPtr = // allocate space for a pointer
create(ptrPtr);
void* ptr = *ptrPtr;

Correct me if I'm wrong, but I thought the code flow was more like:

create(void** ptrPtr) {
   *ptrPtr = new Something;
}
void* ptr; // (Automatically) allocate space for a pointer.
create(&ptr);

Edit: I think I might be nitpicking. I'll examine the crux of your fix shortly.

[ldematte]

Correct me if I'm wrong, but I thought the code flow was more like:

create(void** ptrPtr) {
   *ptrPtr = new Something;
}
void* ptr; // (Automatically) allocate space for a pointer.
create(&ptr);

You are absolutely correct, I was writing "C the way we can use in Java": if we cannot generate a "pointer" (a MemorySegment) referencing a stack variable, void* ptr; create(&ptr); becomes

void** ptrPtr = (void*)malloc(sizeof(void*)); 
create(ptrPtr);
void* ptr = *ptrPtr;
free(ptrPtr);

or, equivalent in Java

try (var localArena = Arena.ofConfined) {
   MemorySegment ptrPtr = localArena.allocate(C_POINTER); // malloc
   create(ptrPtr)
   MemorySegment ptr = ptrPtr.get(...); // dereference
} // free

My point is: the original code was missing the indirection and dereference.

[chatman]

Excellent catch, thanks a lot @ldematte.

Optimizing memory allocation is still a WIP, especially on the search side, but this I think will deserve its own PR.

We can use this issue, if needed: #1037
Some thoughts I had around this was to perform some tests/checks for memory leaks (esp. non-heap) in our tests. Not sure how easy it would be without diving deeper into it.

[mythrocks]

I'm generally positive on this change.

There are a couple of minor quibbles, and some clarifying questions.

[benfred]

/ok to test 0fb438e

[ChrisHegarty]

LGTM. Thanks for fixing this intermittent failure.

[mythrocks]

/ok to test a87d96e

[mythrocks]

LGTM! Good catch, @ldematte. 👏

[mythrocks]

/merge