Repository navigation
Repros 781 - #786
Open
jshook wants to merge 11 commits into
Open
Repros 781#786jshook wants to merge 11 commits into
jshook wants to merge 11 commits into
Conversation
Count each cached fused-PQ source object and its code-buffer storage. Sum InlineSource byte estimates directly instead of multiplying them by the integer element width. Keep compressor ownership separate. Add PQ-only regression tests for source storage overhead and byte units through an actual serialized/loaded fused graph. Both tests fail on the unmodified main branch and pass with this fix. Validation: JDK 22, 65 tests each under scalar and Panama profiles; new tests also pass with compressed and uncompressed object references.
Handle independent reference/class-pointer settings and compact headers. Use VM field and array offsets; include full byte-sequence, segment, scope, and backing-array storage.
Count graph metadata, allocated cache capacity, adjacency arrays, and fused source storage once. Exclude separately owned models, worker scratch, and shared JVM metadata. Add independent instrumentation checks across JDK 22 and 25 layouts and document the reporting cost.
Skip the compressed-class-pointer/wide-reference combination unavailable before JDK 15. Preserve all eight JDK 22 and twelve JDK 25 layouts; verify the six supported JDK 11 layouts.
Count on-disk cache payloads while loading and size only bounded header metadata. Cache built-in PQ totals at chunk/reference-array growth, preserve shared immutable backing storage, and reuse native buffer fields for allocation-free size reporting.
Check independent JVM footprints, concurrent chunk growth, capacity reuse, and zero-allocation reporting. Add a reproducible timing/allocation exercise and document immutable cache ownership and initialization cost. Update the 4.0.3 bug-fix release note.
… memory access Fork a JVM and read every built-in retained-memory figure. With --sun-misc-unsafe-memory-access=deny (JDK 23+, the planned future default) RamUsageEstimator's static initializer throws from Unsafe.arrayBaseOffset, so creating any ByteSequence fails with ExceptionInInitializerError. On JDK 24+ with default settings the first use prints the JEP 498 terminal-deprecation warning naming RamUsageEstimator.
RamUsageEstimator: drop sun.misc.Unsafe and the reflective object-graph walker. Shallow sizes are header plus field widths, cached per class. arrayHeaderBytes(elementSize) models the array header: heap-word aligned before JDK 23, element-width aligned from JDK 23 on. PQVectors: sum the chunk array and each allocated chunk's ramBytesUsed(); ImmutablePQVectors caches the result at construction. OnDiskGraphIndex: keep the format's feature map (V6 orders fused features last), copy the layer list into an exactly sized ArrayList, and sum the metadata explicitly. Feature.ramBytesUsed() defaults to the shallow size; NVQ features add their scorer. TestRetainedMemoryAccounting: exact on JDK 15+, at most 8 bytes per object below the measurement on older HotSpot; the shared-slice case becomes distinct chunks.
TestMemoryAccountingCost asserted that the largest of five samples allocated nothing and charged the ThreadMXBean read to the loop. On JDK 11 that read boxes the thread id into two long[] per call (48 bytes per sample), and on JDK 20 one sample can catch C2 recompiling the measuring loop after its call site turns megamorphic (128 bytes), so the test failed on both in every run. A per-call allocation shows in every sample, so measure the counter's own cost with back-to-back reads, subtract it, and assert on the quietest sample; print both extremes and the sampler cost for diagnosis.
jshook
requested review from
MarkWolters,
ashkrisk and
tlwillke
as code owners
November 4, 2026 21:36
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: repro and proposed fix for the Unsafe dependency
Reviewed at
4526e1b7. The repro test and a proposed fix are onrepros-781, one commit each. The fix follows the design in the inline comment above (explicit sums, nosun.misc.Unsafe) and keeps the footprint matrix exact on JDK 15+.sun.misc.Unsafememory access: JDK 24 warns on first use of anyByteSequence;--sun-misc-unsafe-memory-access=denyfails every byte sequence, PQ store and index load withExceptionInInitializerError0e963b814e14ccaeTestMemoryAccountingCostfails on JDK 11 (the allocation counter itself allocates 48 bytes per read) and JDK 20 (one 128-byte sample during a JIT recompile of the loop), on the unmodified head51d055921.
RamUsageEstimator.<clinit>callsUnsafe.arrayBaseOffset, andArrayByteSequence/MemorySegmentByteSequencereach it from their own static initializers, so the warning (and underdeny, the failure) shows up as soon as any byte sequence is created. The reflective walker then needsobjectFieldOffset/getObject/staticFieldBasefor JDK-internal fields (EnumMap,UnmodifiableMap,jdk.internal.foreign.*), so without UnsafeImmutablePQVectorsandOnDiskGraphIndex.loadthrow over an estimate. The walk is also eager: throughcreateCompressedVectors(Object[])(one chunk per vector) 1M vectors cost ~1 s and ~150 MB of temporary allocation in the constructor, versus 1 ms and 0.1 MB at 1024 vectors per chunk. The repro forks a JVM that exercises every accounting path throughSimpleReader, once underdenyand once checking for the JEP 498 warning.The fix drops Unsafe entirely. Shallow sizes are header plus declared field widths (cached per class), array headers follow HotSpot's rule explicitly (heap-word aligned before JDK 23, element-width aligned from JDK 23 on, which
arrayBaseOffsethad been tracking silently),PQVectorssums its chunks' own reports, andOnDiskGraphIndexsums its metadata explicitly (an owned exactly-sizedArrayListof layers, and the format's own feature map,EnumMapbefore V6 and the fused-lastLinkedHashMapfrom V6, with all views prepared) plus a newFeature.ramBytesUsed()default. The oracle test keeps exact equality on JDK 15+; on older HotSpot, which pads superclass boundaries, it allows up to 8 bytes per object.2. The check asserts the largest of five samples allocated nothing and charges the
ThreadMXBeanread to the loop. JDK 11 boxes the thread id into twolong[]per read, and on JDK 20.0.2 oneslice-17sample catches C2 recompiling the loop (median drops 28 ns to 10 ns). Both fail two of two runs on the head; the CI jobs passing means it depends on the JDK build and timing. The fix subtracts the counter's measured cost and asserts on the quietest sample, printing min, max and sampler bytes.With both fixes: JDK 24 full reactor suite green (534 tests) with the footprint matrix exact across 8 layouts; JDK 25 exact across 12 layouts including compact headers; JDK 22 exact across 8; JDK 20 (Panama) exact across 8; JDK 11 within 8 bytes per object over 6 layouts.
Not fixed, worth addressing:
Unsafe.invokeCleaneris in the same deprecated set and main already calls it fromSimpleMappedReader.Supplier.close(),HubMapandPreEncodedCodeCache: JDK 24 warns on the first mapped close, anddenythrows there. Not this PR's regression (the doc line about Unsafe being "already used by JVector" refers to these calls), but it deserves its own issue before the default flips.ramBytesUsed()on native byte sequences now reports the whole backing array (a 48-byte code slice of a 1024-vector chunk reports ~49 KB). Intended and documented, but a visible change for anything summingpqVectors.get(i).ramBytesUsed()under the now-default native provider; worth a line in the release note.MutablePQVectors.encodeAndSet(-1, v)/setZero(-1)now throwIllegalArgumentExceptioninstead ofIndexOutOfBoundsException.docs/release notes/4.1.0/, and the description still says 4.0.3.