Skip to content

[Java] Tidy up MemorySegments lifecycle #1069

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Jul 15, 2025
Merged

[Java] Tidy up MemorySegments lifecycle #1069

merged 15 commits into from
Jul 15, 2025

Conversation

ldematte
Copy link
Contributor

@ldematte ldematte commented Jun 30, 2025 •
edited
Loading

This PR tidies up the lifecycle of MemorySegments by using specific confined Arenas where possible, and specific index-bound Arenas where the lifetime needs to be bound to the lifetime of an index.

It addresses all remaining usages of CuVSResources#arena after #1024 and #1045; as such, we can remove CuVSResources#arena completely, and force native memory usages to be tracked and dealt with specifically.

This would towards the goal of #1037

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 30, 2025

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
ChrisHegarty approved these changes Jun 30, 2025
View reviewed changes
Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

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

This is a good change. We should certainly be using more local arenas where possible, and cleaning up more eagerly. 👍

java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/common/Util.java
var returnValue = cuvsRMMAlloc(resourceHandle, datasetMemorySegment, datasetBytes);
checkCuVSError(returnValue, "cuvsRMMAlloc");

return datasetMemorySegment.get(C_POINTER, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@cjnolet cjnolet added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jun 30, 2025
ldematte added 4 commits July 2, 2025 15:55
@ldematte
Merge remote-tracking branch 'upstream/branch-25.08' into java/reduce…
7f2f5e1 
…-memorysegment-scope
@ldematte
Merge remote-tracking branch 'upstream/branch-25.08' into java/reduce…
178ec0b 
…-memorysegment-scope

# 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/HnswIndexImpl.java
@ldematte
Final removal of CuVSResourcesImpl#getArena
c3309fe
@ldematte
Cleanup remaining redundant cuvsStreamGet
a93a8b4
@ldematte ldematte mentioned this pull request Jul 7, 2025
Merged
@chatman
Copy link
Contributor

chatman commented Jul 7, 2025

FYI, https://termbin.com/ac7k3 here's a patch on top of this branch. It adds a synchronized() block around the search() call, and the test from #1082 (added in this patch) passes.

@ldematte
Copy link
Contributor Author

ldematte commented Jul 7, 2025

FYI, https://termbin.com/ac7k3 here's a patch on top of this branch. It adds a synchronized() block around the search() call, and the test from #1082 (added in this patch) passes.

For the record: we discussed this on slack and decided to keep the 2 separate, as they address different problems

@ldematte ldematte mentioned this pull request Jul 8, 2025
Merged
@ldematte ldematte changed the title [WIP][Java] Tidy up MemorySegments lifecycle [Review][Java] Tidy up MemorySegments lifecycle Jul 10, 2025
@mythrocks
Copy link
Contributor

/ok to test 250ca15

@ldematte ldematte mentioned this pull request Jul 11, 2025
Merged
mythrocks
mythrocks approved these changes Jul 11, 2025
View reviewed changes
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

LGTM! Waiting on CI now.

@ldematte ldematte changed the title [Review][Java] Tidy up MemorySegments lifecycle [Java] Tidy up MemorySegments lifecycle Jul 12, 2025
@ldematte ldematte requested a review from a team as a code owner July 12, 2025 06:49
@ldematte
Copy link
Contributor Author

@mythrocks is anything blocking this from getting merged? Should I do something?

@mythrocks
Copy link
Contributor

/ok to test 9010448

@mythrocks
Copy link
Contributor

@mythrocks is anything blocking this from getting merged? Should I do something?

Nothing. CI hadn't passed at the time. I'll check this in, once it does.

@mythrocks
Copy link
Contributor

The CI builds should be fixed once #1114 is resolved.

@benfred
Copy link
Member

benfred commented Jul 15, 2025

/ok to test 109b481

@mythrocks
Copy link
Contributor

Will prioritize #1034 first, to address the test failures.

@mythrocks
Copy link
Contributor

/ok to test 3ebc564

@mythrocks
Copy link
Contributor

/ok to test d7badfd

@cjnolet cjnolet moved this to In Progress in Elasticsearch + cuVS Team Jul 15, 2025
@mythrocks
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit f61a27d into rapidsai:branch-25.08 Jul 15, 2025
53 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Elasticsearch + cuVS Team Jul 15, 2025
@mythrocks
Copy link
Contributor

This has been merged. Thank you for this contribution, @ldematte .

@ldematte
Copy link
Contributor Author

Thank you for assisting through the process, @mythrocks!

@ldematte ldematte deleted the java/reduce-memorysegment-scope branch July 16, 2025 06:46
punAhuja pushed a commit to SearchScale/cuvs that referenced this pull request Jul 16, 2025
@ldematte @punAhuja
[Java] Tidy up MemorySegments lifecycle (rapidsai#1069)
aee68a9 
This PR tidies up the lifecycle of `MemorySegment`s by using specific confined `Arena`s where possible, and specific index-bound `Arena`s where the lifetime needs to be bound to the lifetime of an index.

It addresses all remaining usages of `CuVSResources#arena` after rapidsai#1024 and rapidsai#1045; as such, we can remove `CuVSResources#arena` completely, and force native memory usages to be tracked and dealt with specifically.

This would towards the goal of rapidsai#1037

Authors:
  - Lorenzo Dematté (https://github.com/ldematte)
  - MithunR (https://github.com/mythrocks)
  - Ben Frederickson (https://github.com/benfred)

Approvers:
  - Chris Hegarty (https://github.com/ChrisHegarty)
  - MithunR (https://github.com/mythrocks)

URL: rapidsai#1069
@cjnolet cjnolet moved this from Todo to Done in Elasticsearch + cuVS Team Board Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mythrocks mythrocks mythrocks approved these changes

+1 more reviewer

@ChrisHegarty ChrisHegarty ChrisHegarty approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@ldematte ldematte

Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
Elasticsearch + cuVS Team
Status: Done
Vector Search, ML, & Data Mining Release Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ldematte @chatman @mythrocks @benfred @ChrisHegarty @cjnolet