[Java] Introduce scoped resource access #1089

ldematte · 2025-07-08T08:14:16Z

This PR wraps access to the cuVS resources handle into a autocloseable accessor. The reason is to provide a clear scope for resource access, with the possibility to add application logic to the scope begin/end.

An example of this logic is the CheckedCuVSResources decorator introduced in the test suite, which checks that the CuVSResources it decorates has not been destroyed yet, and that access to the underlying cuVS resources handle is not concurrent (is not done by more than 1 thread at every time).

I preferred to write it as a decorator, as this gives the API consumers more freedom: if they want the check, they can use CheckedCuVSResources (or a similar class); if they want direct, straight, fast access, they can just use the accessor; if they need more logic (e.g. logging or anything else), that is possible too.

Tests are updated to use CheckedCuVSResources, so we can spot issues quickly in tests without burdening production code with additional logic.

copy-pr-bot · 2025-07-08T08:14:19Z

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.

ChrisHegarty · 2025-07-09T07:51:51Z

I do very much like this approach and API. It closely aligns with the intent of the C API, while allowing to enforce higher-level constraints through typical java patterns, e.g. the decorator pattern. The consume is free to decide if, and how to, enforce thread semantics.

…-check-on-resource-access # Conflicts: # java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/BruteForceIndexImpl.java # java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/CagraIndexImpl.java # java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/CuVSResourcesImpl.java # java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/HnswIndexImpl.java # java/cuvs-java/src/test/java/com/nvidia/cuvs/CagraBuildAndSearchIT.java

ldematte · 2025-07-16T15:02:45Z

@mythrocks I've rebased this PR to include the latest merges (TY!)
I think this should be the next one to go in, as it is touching several files - changes should be minor, but somehow git merge does not like them (too much whitespace change).

mythrocks · 2025-07-16T16:48:02Z

/ok to test 4ba4537

cjnolet · 2025-07-16T21:39:27Z

Linking cjnolet/nv_elastic#22

ldematte · 2025-07-18T08:20:52Z

@mythrocks I merged #1028 and fixed/cleaned up a few things besides adding resource access:

used Dataset.ofArray to avoid passing around float[][] and reduce duplication and complexity
used named constants where possible to increase readability (e.g. cudaMemcpyHostToDevice)
used allocateRMMSegment to reduce duplication
fixed dataset memory leak

Some of the changes (e.g. Dataset.ofArray and avoid passing float[][] down) were necessary for this to change (so I could use CuVSResources without the need to cast up to a specific implementation). Others could go to another cleanup PR, but I wanted to get this right ASAP.

ldematte · 2025-07-18T08:23:47Z

Still missing: the new code is allocating params directly (e.g. cuvsTieredIndexParams.allocate(arena);). It should use cuvsTieredIndexParamsCreate + Destroy like I did in #1109 and #1110. It's OK, I can fix it later, but for the future can we point this in the review of the PRs, so that old code/old patterns does not keep sneaking in?

punAhuja · 2025-07-18T08:46:23Z

Still missing: the new code is allocating params directly (e.g. cuvsTieredIndexParams.allocate(arena);). It should use cuvsTieredIndexParamsCreate + Destroy like I did in #1109 and #1110. It's OK, I can fix it later, but for the future can we point this in the review of the PRs, so that old code/old patterns does not keep sneaking in?

Will raise a PR and change this. @ldematte

ldematte · 2025-07-18T10:08:40Z

Will raise a PR and change this. @ldematte

That would be awesome @punAhuja, thanks! To keep things consistent, would you mind using CloseableHandle from my PRs, so we keep a single pattern to do that?

punAhuja · 2025-07-18T10:20:23Z

Will raise a PR and change this. @ldematte

That would be awesome @punAhuja, thanks! To keep things consistent, would you mind using CloseableHandle from my PRs, so we keep a single pattern to do that?

Yes, have looked at your PRs, will follow a similar pattern for consistency. @ldematte

mythrocks · 2025-07-18T11:05:09Z

It's OK, I can fix it later, but for the future can we point this in the review of the PRs, so that old code/old patterns does not keep sneaking in?

Agreed. I had held up #1028 long enough; I was hoping that could be handled in a follow-up. Thank you for pointing it out, @ldematte. Also, thank you for picking that up, @punAhuja.

…matte/cuvs into java/thread-check-on-resource-access

mythrocks · 2025-07-18T17:01:49Z

P.S. Apologies for not having caught some of this in the review of #1028.

Thanks for making the fixes in TieredIndexImpl, @ldematte.

mythrocks · 2025-07-18T22:09:29Z

/ok to test e2e044d

mythrocks · 2025-07-18T23:04:33Z

java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java

      int returnValue = cuvsTieredIndexDestroy(tieredIndexReference.getMemorySegment());
      checkCuVSError(returnValue, "cuvsTieredIndexDestroy");
+      if (dataset != null) {
+        dataset.close();


Ah, this is resource leak you mentioned.

mythrocks · 2025-07-18T23:06:40Z

java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java

+        // Create tensor from device memory
+        long[] datasetShape = {rows, cols};
+        MemorySegment datasetTensor =
+            prepareTensor(localArena, datasetDP, datasetShape, kDLFloat(), 32, 2, kDLCUDA(), 1);


Ok, there's the magic numbers you mentioned. Yep, your way is better.

mythrocks · 2025-07-18T23:30:32Z

java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java

-      checkCuVSError(returnValue, "cuvsRMMAlloc");
+        // TieredIndex REQUIRES device memory - allocate it
+        long datasetSize = C_FLOAT_BYTE_SIZE * rows * cols;
+        MemorySegment datasetDP = allocateRMMSegment(cuvsRes, datasetSize);


Nitpick: Not to be handled in the current PR.

Do we stand the risk of leaking the allocation from RMM if there is an exception before the cleanup?

Might need to look at this more closely, separately.

mythrocks · 2025-07-18T23:32:43Z

/ok to test 0a620fb

mythrocks · 2025-07-19T04:46:41Z

/merge

…as missed for tieredIndex tests

#1089 Scoped Resource access changes were missed in the TieredIndex test causing them to fail. - Using CheckedCuVSResources.create() now - Skip TieredIndex tests if CuVS native support is unavailable Authors: - Puneet Ahuja (https://github.com/punAhuja) Approvers: - Mayya Sharipova (https://github.com/mayya-sharipova) - MithunR (https://github.com/mythrocks) URL: #1156

As a follow up to #1089 and #1082, this PR introduces a decorator to ensure that access to a shared CuVSResource is synchronized. This could be useful in cases where application logic is complex and it's not easy to efficiently and cleanly guarantee that access to a CuVSResources is done by at most one thread at each given time. Authors: - Lorenzo Dematté (https://github.com/ldematte) - MithunR (https://github.com/mythrocks) Approvers: - MithunR (https://github.com/mythrocks) URL: #1209

…#1156) rapidsai#1089 Scoped Resource access changes were missed in the TieredIndex test causing them to fail. - Using CheckedCuVSResources.create() now - Skip TieredIndex tests if CuVS native support is unavailable Authors: - Puneet Ahuja (https://github.com/punAhuja) Approvers: - Mayya Sharipova (https://github.com/mayya-sharipova) - MithunR (https://github.com/mythrocks) URL: rapidsai#1156

) As a follow up to rapidsai#1089 and rapidsai#1082, this PR introduces a decorator to ensure that access to a shared CuVSResource is synchronized. This could be useful in cases where application logic is complex and it's not easy to efficiently and cleanly guarantee that access to a CuVSResources is done by at most one thread at each given time. Authors: - Lorenzo Dematté (https://github.com/ldematte) - MithunR (https://github.com/mythrocks) Approvers: - MithunR (https://github.com/mythrocks) URL: rapidsai#1209

…#1156) rapidsai#1089 Scoped Resource access changes were missed in the TieredIndex test causing them to fail. - Using CheckedCuVSResources.create() now - Skip TieredIndex tests if CuVS native support is unavailable Authors: - Puneet Ahuja (https://github.com/punAhuja) Approvers: - Mayya Sharipova (https://github.com/mayya-sharipova) - MithunR (https://github.com/mythrocks) URL: rapidsai#1156

) As a follow up to rapidsai#1089 and rapidsai#1082, this PR introduces a decorator to ensure that access to a shared CuVSResource is synchronized. This could be useful in cases where application logic is complex and it's not easy to efficiently and cleanly guarantee that access to a CuVSResources is done by at most one thread at each given time. Authors: - Lorenzo Dematté (https://github.com/ldematte) - MithunR (https://github.com/mythrocks) Approvers: - MithunR (https://github.com/mythrocks) URL: rapidsai#1209

Thread check on resource access (WIP)

7b78d76

ldematte added 4 commits July 8, 2025 14:30

Moving checked access to tests

4871095

With lambda

ba7f993

Back to AutoCloseable

065ba68

Javadoc

e2d2c97

ldematte mentioned this pull request Jul 8, 2025

[Java][Fix] Multithreaded querying fails without synchronization #1082

Merged

Add tests

e9dfd76

ldematte changed the title ~~[WIP][Java] Thread check on resource access (WIP)~~ [WIP][Java] Thread check on resource access Jul 10, 2025

github-project-automation bot added this to Vector Search, ML, & Data Mining Release Board Jul 10, 2025

github-project-automation bot moved this to Todo in Vector Search, ML, & Data Mining Release Board Jul 10, 2025

ldematte mentioned this pull request Jul 11, 2025

[Java] Add Dataset based on MemorySegment #1034

Merged

cjnolet added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jul 11, 2025

cjnolet assigned ldematte Jul 11, 2025

cjnolet added this to Elasticsearch + cuVS Team Jul 15, 2025

cjnolet moved this to In Progress in Elasticsearch + cuVS Team Jul 15, 2025

ldematte changed the title ~~[WIP][Java] Thread check on resource access~~ [Review][Java] Thread check on resource access Jul 16, 2025

ldematte marked this pull request as ready for review July 16, 2025 07:45

ldematte requested a review from a team as a code owner July 16, 2025 07:45

Fix test

e91bd75

Merge branch 'branch-25.08' into java/thread-check-on-resource-access

4ba4537

cjnolet added this to Elasticsearch + cuVS Team Board Jul 16, 2025

cjnolet moved this to In Progress in Elasticsearch + cuVS Team Board Jul 16, 2025

mythrocks self-requested a review July 17, 2025 18:00

Merge branch 'java/thread-check-on-resource-access' of github.com:lde…

f58f765

…matte/cuvs into java/thread-check-on-resource-access

Merge branch 'branch-25.08' into java/thread-check-on-resource-access

e2e044d

mythrocks reviewed Jul 18, 2025

View reviewed changes

mythrocks approved these changes Jul 18, 2025

View reviewed changes

Merge branch 'branch-25.08' into java/thread-check-on-resource-access

0a620fb

rapids-bot bot merged commit 533ce9c into rapidsai:branch-25.08 Jul 19, 2025
53 checks passed

github-project-automation bot moved this from Todo to Done in Vector Search, ML, & Data Mining Release Board Jul 19, 2025

github-project-automation bot moved this from In Progress to Done in Elasticsearch + cuVS Team Board Jul 19, 2025

github-project-automation bot moved this from In Progress to Done in Elasticsearch + cuVS Team Jul 19, 2025

punAhuja added a commit to SearchScale/cuvs that referenced this pull request Jul 23, 2025

Fix breakage caused by rapidsai#1089; Scoped resource access change w…

f904d50

…as missed for tieredIndex tests

punAhuja mentioned this pull request Jul 23, 2025

Fix TieredIndexIT, Scoped resource access change was missed #1156

Merged

ldematte mentioned this pull request Aug 4, 2025

[Java] Add decorator for synchronized CuVSResource access #1209

Merged

[Java] Introduce scoped resource access #1089

[Java] Introduce scoped resource access #1089

Uh oh!

Conversation

ldematte commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

copy-pr-bot bot commented Jul 8, 2025

Uh oh!

ChrisHegarty commented Jul 9, 2025

Uh oh!

ldematte commented Jul 16, 2025

Uh oh!

mythrocks commented Jul 16, 2025

Uh oh!

cjnolet commented Jul 16, 2025

Uh oh!

ldematte commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldematte commented Jul 18, 2025

Uh oh!

punAhuja commented Jul 18, 2025

Uh oh!

ldematte commented Jul 18, 2025

Uh oh!

punAhuja commented Jul 18, 2025

Uh oh!

mythrocks commented Jul 18, 2025

Uh oh!

mythrocks commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mythrocks commented Jul 18, 2025

Uh oh!

mythrocks Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

mythrocks Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mythrocks Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

mythrocks commented Jul 18, 2025

Uh oh!

mythrocks commented Jul 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldematte commented Jul 8, 2025 •

edited

Loading

ldematte commented Jul 18, 2025 •

edited

Loading

mythrocks commented Jul 18, 2025 •

edited

Loading

mythrocks Jul 18, 2025 •

edited

Loading