Repository navigation
Repros 780 - #787
Open
jshook wants to merge 4 commits into
Open
Repros 780#787jshook wants to merge 4 commits into
jshook wants to merge 4 commits into
Conversation
Resolve an NVQParameters compressor through Grid.getCompressor twice, clearing the in-memory memo in between so the second call takes the on-disk path. loadFromCache reads every cache file with ProductQuantization.load, which takes NVQ's leading version int as a centroid length and runs off the end of the file: IndexOutOfBoundsException from the mapped reader.
Each compressor type has its own on-disk format, so loadFromCache now calls CompressorParameters.loadCompressor instead of ProductQuantization.load; PQ and NVQ parameters read back their own type. Same shape as the loader in #776 so the two merge cleanly.
Grid.cachedCompressors is an IdentityHashMap keyed by strings that idStringFor formats afresh on every call, so a second resolution of the same key misses, reloads the compressor from disk, and adds another entry. Resolve one key twice and expect the same instance and one entry.
idStringFor formats a new String on every call, so the IdentityHashMap never matched an earlier key: each resolution reloaded the compressor from disk and kept the previous instance in the map. Use a HashMap.
jshook
requested review from
MarkWolters,
ashkrisk and
tlwillke
as code owners
November 4, 2026 21:55
Contributor
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review: repros and fixes for the cache path this makes reachable
Reviewed at
f8ca7e51. The format fix is right and the key test passes. Two things in the cache path it enables, each with a failing test and a fix onrepros-780, one commit each:ProductQuantization.loadand fails (the point in the approving comment; #776 is still open)fa932e2163c32578Grid.cachedCompressorsis anIdentityHashMapkeyed by freshly formatted strings, so the memo never hits: every resolution reloads from disk and keeps the previous instance (pre-existing, since #244)f90f1f56cc5fd20d1.
GridCompressorCacheTest.cachedNvqCompressorLoadsBackAsNvqresolves anNVQParameterscompressor throughGrid.getCompressortwice, clearing the memo in between so the second call takes the on-disk path. On the head it dies inProductQuantization.loadreading NVQ's version int as a centroid length (IndexOutOfBoundsExceptionfrom the mapped reader on a small dataset; a corrupt PQ on a large one). The fix addsCompressorParameters.loadCompressor(RandomAccessReader), overridden for PQ and NVQ, and hasloadFromCachecall it. Same name and shape as the loader in #776, so whichever merges second resolves trivially.2.
cachedCompressors.computeIfAbsent(cp.idStringFor(ds), ...)never finds an earlier key becauseidStringForformats a newStringeach call and the map compares by identity.sameKeyReturnsTheMemoizedCompressorresolves one key twice and expects the same instance and one entry; on the head it gets two instances and two entries. Fix:HashMap.getCompressoris only called from the sequential configuration loop.With both fixes the cache tests, the PR's key test, and the full reactor suite pass on JDK 24 with the native provider (531 tests).
Minor:
CompressorParametersTest's anonymousDataSetomits@Override, so a method removed from the interface leaves a dead stub instead of a compile error.