Integration robustness contracts - #710
Conversation
Introduce an opt-in control surface a host can install on long-running
jvector operations, without touching any existing call site:
- WorkStage / WorkLimiter / ProgressTracker: the admission + reporting
primitives (throttle down, progress up).
- ProgressLimiter: the single combined facet a host implements.
- LeakyBucketLimiter: a default rate-limited WorkLimiter.
Purely additive. Nothing in the library calls these yet; they establish
the seam that host-driven operations (e.g. compaction) can later be
rewritten against.
Add a host-supplied destination abstraction so compaction output can be
written through a caller-owned channel instead of always allocating its
own file:
- SeekableSink / FileChannelSeekableSink: a minimal seekable byte sink
and its file-backed implementation.
- CompactionDestination / FileCompactionDestination: the compaction
output target, resolvable to a SeekableSink.
Purely additive interface types; no existing code is wired to them on
this branch.
Add ParallelExecutor, a minimal parallel-for abstraction that lets an embedding host supply its own execution strategy -- its own pool, or caller-runs on the calling thread -- instead of jvector reaching for ForkJoinPool.commonPool(): - forkJoin(pool): run on a caller-supplied ForkJoinPool - callerRuns(): run inline, no work escaping to a shared pool - forEachInt / forEach: the parallel-for entry points Additive: a standalone interface with no consumers on this branch. It establishes the execution seam that build, quantization, and compaction paths can be rewritten against later.
Add RuntimeMode, a process-level switch (jvector.mode) separating production from diagnostic runs. Unset is production; diagnostic-only work (e.g. verification walks) sits behind this gate and must be opted into explicitly. Additive: a standalone type with no call sites on this branch. It establishes the gate that diagnostic code paths can consult once wired.
Harden the read path for embedding hosts that manage their own mmap
lifecycle, where a stale offset or a close racing an in-flight read
faults the JVM (SIGSEGV) instead of throwing:
- OnDiskGraphIndex: bound record reads. A node id outside the graph
would become a wild offset into the mapped file (garbage, or a
fault); requireValidNode() rejects it up front, and a stale/corrupt
neighbor block now fails with IllegalStateException instead of
consuming garbage ints.
- ReaderSupplier / SimpleMappedReader: document the close() contract --
the raw-release (immediate unmap -> SIGSEGV) vs coordinated
(liveness handshake -> IllegalStateException) families -- so a host
knows a supplier must not be closed until every vended reader is
quiescent.
Non-breaking: internal bounds checks and documentation only, no
signature or call-path changes. The compactor-side memory safety
(drain-on-unwind, truncate-reused-outputs) stays with the compaction
work, where it lives.
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
There was a problem hiding this comment.
This PR adds in some helpful comments and checks (in OnDiskGraphIndex, ReaderSupplier, SimpleMappedReader) while adding some new interfaces. IMO those are different concerns: it might be better to split the comments and checks off into a separate PR while leaving the new interfaces here.
| * hands one out is {@code io.github.jbellis.jvector.graph.disk.CompactionDestination}. | ||
| */ | ||
| @Experimental | ||
| public interface SeekableSink extends AutoCloseable { |
There was a problem hiding this comment.
This interface appears to serve the same purpose as RandomAccessReader/ReaderSupplier + RandomAccessWriter/IndexWriter. Is this intention to completely replace those existing interfaces with this one? If so, it would be helpful to jot down the advantages of this approach over what we already have.
There was a problem hiding this comment.
This is mostly captured in the javadoc above, but the main benefit here is to virtualize the output stream as logically owned by the jvector writer and physically owned by the embedding system. Specifically, this allows jvector to persist its indexes "care of" the owning system, which allows the writes to only happen once.
The alternative is that jvector owns the raw file management, and presumes everything to be jvector-only, full file ownership, 0-indexed. This is not true in practice, and by forcing it, we have caused other systems to have to copy and recopy data in order to properly virtualize it into the owning system's data formats and filesystem conventions.
The operative benefit is described in the javadoc above as:
An embedder uses it to hand a compactor (or other writer) a bounded window inside a larger container file: positions are region-relative and the implementation adds the container's base offset, so the writer never needs to know the absolute offset.
There was a problem hiding this comment.
Here is a more mechanical explanation of some of the decision points, with help from analysis:
RandomAccessReader/Writer are stateful cursor APIs (seek, then read/write sequentially), documented as not threadsafe, which is why ReaderSupplier exists to create one per thread. IndexWriter is a typed DataOutput serialization surface on top of that.
SeekableSink is a stateless positional-I/O primitive: writeAt/readAt with no cursor, where a single instance must support concurrent reads and writes to disjoint ranges. It also addresses a region — positions are relative to a base offset inside a caller-owned channel, and close() doesn't close the channel. That combination is what the CompactionDestination extension point needs: an embedder hands the compactor a bounded window inside its own container file (write the body, read it back for checksumming, on one handle) without exposing absolute offsets or giving up channel lifecycle.
None of that fits the existing interfaces without changing their documented threading/addressing contracts for every current implementation, so this is additive rather than a migration. If anything, the relationship is layered: a format-aware writer like IndexWriter could be implemented over a SeekableSink region.
| * e.g. one thread per compaction — instead of a jvector-owned all-core pool. It is the build/finalize | ||
| * counterpart to the caller-runs executor injection already available on the compaction merge path. | ||
| */ | ||
| public interface ParallelExecutor { |
There was a problem hiding this comment.
This interface takes care of the forEach operations for generic Streams and IntStreams, but there are plenty of other terminal operations that are not expressed, for example collect and reduce for generic streams, count, min, max, toArray for primitive streams etc. There are whole other stream types like ByteStream which are not covered. Even if we don't use those terminal operations now, this interface locks us out of using any of those operations in the future.
In theory you could add all those operations to the ParallelExecutor, but that just leaves us with a huge amount of boilerplate to maintain.
The solution used elsewhere in JV is to accept a ForkJoinPool directly, run all parallel stream operations within that pool. This approach has certain downsides (for example, if you want to run a parallel task single-threaded, it'll probably run on the ForkJoin worker thread while the current thread is blocked and idle), but is rather flexible in terms of the operations that are supported.
Are the trade-offs involved in using this interface really worth it?
There was a problem hiding this comment.
The reason this interface exists is explicitly to get around forcing the ForkJoinPool as the interface type to the embedding system. This has caused some issues in that it doesn't map properly (Liskov) to the higher level generic interface prescribed and presumed by most systems, which will assume the Executor level interfaces are used. By pulling the interface types up to the common layer of what is expected, embedding systems can use the "provide an executor or threadpool interface" facilities that they already have. You could take that and internally use a ForkJoinPool if you needed, but we shouldn't force the type inversion back to the caller/embedder.
What's good for JV is not always good for the embedder, and I believe we should start favoring the callers's view over JV when it comes to embedding ergonomics and conventions.
There was a problem hiding this comment.
You're making two valid points:
ForkJoinPooldoesn't necessarily map well to the Executor-style abstractions used by caller.- The caller's view should be preferred over JV for ergonomics purposes.
However, both of these requirements can be satisfied by switching the ForkJoinPool to the more fundamental ExexcutorService instead. We would still need to build any stream-style operations we want to use, but unlike the proposed ParallelExecutor, this would be JV's problem instead of the client. Moreover, this would allow us to implement new stream operations without breaking the API every time.
There was a problem hiding this comment.
I looked into using more directly an ExecutorService here, but taking that route comes with a ton of complexity trade-offs. In particular, that the executor service lifecycle itself is fraught with ownership responsibilities which are inherently stateful and error-prone. Further, it doesn't actually provide an in-built data-parallel service on its own, and the streaming calls you refer to (partitioned by data) are only really parallel when implemented by a forkJoinPool (which is itself an ExecutorService, but not always the other way around). What this means is that if we provide ExecutorService as the one and true prescribed interface, then it may silently fall back to run on the common pool, which is leaky and a potential surprise.
Regarding being locked out of the other patterns, they are all easily derivable from the canonical forEachInt above and could be added as a default method without breaking any implementors, should one become important enough.
So the interface we have here effectively defines a contract which can be fulfilled over a couple very specific and key patterns in whatever way the caller runtime deems appropriate without forcing the forkjoin pool into it. This allows concurrency and parallelism to be managed in a specific way (via data partitioning, etc) should the caller choose, and the wrapper types provided streamline this based on the chosen trade-offs.
Still, for the purposes of illustration and testing, I'm adding another call path adapter (ParallelExecutor.over(...)) which can take an executor service, but further study of it will be required to know if it brings more in usability and adaptation than it does in implicit complexity. (some scale testing in an embedded scenario)
There was a problem hiding this comment.
I'm still concerned about exposing this kind of API to our end users. What if we do something like the following instead:
- Rename this interface to
ParallelExecutorInternaland make it non-public. - Define the public
ParallelExecutorinterface as a sealed, marker interface with no methodspublic sealed interface ParallelExecutor permits `ForkJoinPE`, `SameThreadPE`, `ExecutorServicePE` {}
- Provide methods to allow the user to create one of these three by passing in a
ForkJoinPoolorExecutorServiceas required.
This gives us complete freedom to optimize our abstractions for maximum performance when given, say, a ForkJoinPool, while still allowing ordinary ExecutorService implementations to be used. It also doesn't tie us down to any contract we might be tempted to revoke at some point.
use Objects.requireNonNull Co-authored-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Rework the progress SPI per review: startPhase(WorkStage) is now
ProgressTracker's single abstract method, and onProgress(completed,
total) lives on the returned PhaseScope, making the scope the
capability to report:
- progress for a never-started phase is unrepresentable, and
per-phase implementer state (bars, timers) lives in the scope
instance instead of a map keyed by WorkStage
- concurrent phases of the same stage are distinguished by scope
identity
- lambda ergonomics preserved: PhaseScope.close() defaults to a
no-op, so a progress-only tracker is still one expression:
stage -> (completed, total) -> ...
ProgressLimiter.acquire stays unscoped (throttling is aggregate rate,
not phase identity); the logging combinator now routes phase start,
progress, and completion through one scope wrapper.
The GitHub-applied Objects.requireNonNull suggestion in FileCompactionDestination landed without the java.util.Objects import, breaking compilation. Add the import, and keep the "path" message via the two-arg form so the NPE stays as descriptive as the code it replaced (matching the requireNonNull(sink, "sink") idiom in ProgressLimiter).
Per review discussion: 'Target' reads as a static descriptor a caller could construct up front and pass in, which is exactly the confusion the API is meant to prevent. An OutputReservation is inherently live, single-use, per-run state made against the destination — reserve() a fresh region per compaction, commit(bodyLength) fulfills it, close without commit releases it and discards partial output. The destination stays stateless configuration an embedder builds once. CompactionDestination.open() becomes reserve() to match.
Add ParallelExecutor.over(ExecutorService, int parallelism) so embedders
holding a plain ExecutorService get a one-liner alongside forkJoin(...)
and callerRuns(). Parallel streams cannot be hosted on a generic ES
(inside its workers they silently run on the common pool), so the
adapter chunks on the calling thread and submits, with:
- bounded in-flight window (2x parallelism) on the stream paths and
even range-splitting for forEachInt
- nested use from a body degrading to inline execution instead of
starving a bounded pool into deadlock
- drain-before-unwind: on failure or interrupt, unstarted chunks
skip via a cooperative abort flag and every started chunk is
waited out, so the caller never unwinds beneath a running body.
Future.cancel is deliberately unused: cancel(false) succeeds on a
RUNNING FutureTask and get() then returns while the body still
executes (caught by the new tests before it shipped)
- a ForkJoinPool argument delegates to forkJoin(...), where
whole-pipeline stream decomposition is strictly better
The class javadoc gains a 'Choosing a factory' section spelling out the
relative caveats of the three implementations, including that over(...)
distributes only the body while the source is traversed on the calling
thread.
Synopsis
This is a set of changes to make the embedding layer between JVector an hosting runtimes more robust, visible, and efficient. The aspects addressed here are those which we haven't been prescriptive enough about for embedding systems.
Most essentially, these contracts and supporting types and wrappers are used by embeding systems to instruct JVector where and how to do certain things, like writing index outputs, or using thread pools.
The aspects covered in this set include execution controls, rate-limiting, output vectoring (virtualization), runtime-safety checks on non-runtime debugging features, and memory safety.
Why
Jvector today assumes it owns its execution, its output files, and its process. When it runs embedded in a host that manages its own thread pools, IO budget, mmap lifecycle, and cancellation — e.g. Cassandra's SAI vector indexing driving compaction — those assumptions leak:
ForkJoinPool.commonPool()/PhysicalCoreExecutor.pool(), outside the host's budget;This PR adds the extension points a host needs to close those gaps. It is purely additive and opt-in — no existing behavior changes, and nothing in jvector consumes these seams yet. It's the foundation an embedding/compaction effort builds on; a follow-up PR illustrates how the compactor's calling conventions convert onto these seams.
What's in it, in detail
Five contracts, one per commit:
util/work/) —ProgressLimiter=ProgressTracker(progress up) +WorkLimiter(throttle down), withWorkStagefor phase scoping andLeakyBucketLimiteras a default rate-limited limiter. A host installs one to observe and rate-limit a long jvector operation, and to checkpoint cancellation at phase boundaries.disk/) —SeekableSink+CompactionDestination(with file-backed defaults) let a host redirect compaction output through a caller-owned channel — e.g. a slot inside a larger container after a reserved header — instead of jvector always allocating its own file.Targethas an explicitopen → commit → closelifecycle (nocommit()⇒ aborted, partial output discarded).ParallelExecutor(graph/) — a minimal parallel-for abstraction so a host supplies its own pool (forkJoin(pool)) or runs inline (callerRuns()), instead of jvector reaching for the common pool.RuntimeMode(util/) — a process-leveljvector.modegate. Unset is production; diagnostic-only work is opt-in behind it.disk/) — bounded record reads inOnDiskGraphIndex(an out-of-range node id fails withIllegalArgumentExceptioninstead of becoming a wild offset into the mapped file), plus documenting theclose()contract onReaderSupplier/SimpleMappedReader(the raw-release-immediate-unmap vs. coordinated-liveness-handshake families) so a host knows not to close a supplier while any vended reader is live.Design principles
--release 11. Everything lands injvector-base(bytecode v55), so it runs on every supported JDK (11–25). NoMETA-INF/versions/*overrides, no incubator Vector API use — no per-runtime divergence.Deliberately not here
No changes to the compactor or its algorithm, no new call sites, no host-specific code. Adoption is the follow-up PR.
Testing
jvector-basecompiles at--release 11. New unit tests coverProgressLimiter,SeekableSink, andRuntimeMode. The only edits to existing files are non-breaking (Javadoc onReaderSupplier/SimpleMappedReader, an internal bounds check inOnDiskGraphIndex).Stats: 17 files, +1346/−3 (the 3 deletions are lines replaced by the
OnDiskGraphIndexbounds check).Commits
util/work: cooperative work-limiting and progress interfacedisk: pluggable output sink for compaction writesgraph: ParallelExecutor abstraction for host-provided executionutil: RuntimeMode gate for opt-in diagnostic workdisk: memory-safety guards for host-managed reads