Skip to content

Make threadLocalSupplier() return a closeable supplier and close it at build call sites - #715

Open
ikaitist wants to merge 1 commit into
datastax:mainfrom
ikaitist:fix/thread-local-copies-lifecycle
Open

Make threadLocalSupplier() return a closeable supplier and close it at build call sites#715
ikaitist wants to merge 1 commit into
datastax:mainfrom
ikaitist:fix/thread-local-copies-lifecycle

Conversation

@ikaitist

Copy link
Copy Markdown

Fixes #635

What

threadLocalSupplier() caches per-thread RAVV copies in an ExplicitThreadLocal that callers could never release, leaking AutoCloseable copies (e.g. mmap-backed) for the lifetime of the supplier.

  • threadLocalSupplier() now returns a ThreadLocalCopies supplier that implements AutoCloseable: close() invokes close() on every AutoCloseable copy created so far and drops the per-thread cache. Un-shared RAVVs keep returning () -> this.
  • GraphIndexBuilder.build() and the merge path close the supplier in a finally block when construction completes.

How verified

  • New TestThreadLocalCopies (3 tests): close() closes all per-thread copies, the supplier remains usable after close, un-shared RAVVs return the source.
  • GraphIndexBuilderTest regression: 5 tests, 0 failures, 0 errors.
  • mvn -pl jvector-tests -am -Dtest=TestThreadLocalCopies -DskipScalar=false -DskipSIMD=true test -> 3 tests, 0 failures (SIMD execution skipped locally: the JDK here has no jdk.incubator.vector module).

@github-actions

Copy link
Copy Markdown
Contributor

Before you submit for review:

  • Does your PR follow guidelines from CONTRIBUTIONS.md?
  • Did you summarize what this PR does clearly and concisely?
  • Did you include performance data for changes which may be performance impacting?
  • Did you include useful docs for any user-facing changes or features?
  • Did you include useful javadocs for developer oriented changes, explaining new concepts or key changes?
  • Did you rebase your branch onto the latest main for regression testing and PR submission?
  • Did you trigger regression testing via Run Bench Main and review results?
  • Did you adhere to the code formatting guidelines (TBD)
  • Did you group your changes for easy review, providing meaningful descriptions for each commit?
  • Did you ensure that all files contain the correct copyright header?
  • Did you add documentation for this feature to the release notes directory?

If you did not complete any of these, then please explain below.

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.

RandomAccessVectorValues::threadLocalSupplier leaks objects

1 participant