Repository navigation
fix: packed arrays fall back to an unpacked context like the Java version - #64
Open
alexvictoor wants to merge 1 commit into
Open
alexvictoor wants to merge 1 commit into
alexvictoor wants to merge 1 commit into
Conversation
…sion A packed context addresses its entries with 16 bit short indexes. The Java version caps the physical length at Short.MAX_VALUE / 4 longs: past that, expandArrayIfNeeded throws ResizeException, resizeStorageArray rebuilds the context with the requested length, and the new context is unpacked (one long per virtual index). The port grew the same context in place, never re-evaluated isPacked, and corrupted itself once storage passed 128 KB. AssemblyScript has no try/catch, so the ResizeException is ported as a return value: expandArrayIfNeeded reports the needed length and returns false, newEntry / newLeafEntry / expandEntry / getRootEntry propagate the failure without linking anything, and getPackedIndex returns RESIZE_REQUIRED. PackedArray.set() and add() keep the Java retry loops (bytesAlreadySet, unshifted remainingValueToAdd) and call resizeStorageArray(), which rebuilds the context and re-inserts the non-zero values. setVirtualLength() follows the same path when the set trees do not fit in a packed context. Growth below the cap still happens in place. Tests: 5000 indexes of 64 bit values crashed before with a memory access out of bounds; now the array switches to unpacked with correct values, survives resize and clear, and a PackedHistogram with large counts matches Histogram64. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013Jg4gNfTZpd8eVPAvqx3js
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.
Summary
Ports the Java behaviour for packed arrays that outgrow the packed representation. Builds on #63 (same files), so this PR targets
perf/packed; GitHub will retarget it tomasteronce #63 merges.The bug
A packed context addresses entries with 16-bit short indexes. In Java,
AbstractPackedArrayContext.MAX_SUPPORTED_PACKED_COUNTS_ARRAY_LENGTH = Short.MAX_VALUE / 4caps the physical length:expandArrayIfNeededthrowsResizeException,PackedLongArray.resizeStorageArrayrebuilds the context with the requested length, and the context constructor makes it unpacked (a flatlong[virtualLength]) when that length is above the cap. This has been there since packed histograms were introduced (fe288b16, 2019-11-04) and completed two days later (ad5a4419, "Fix unpacked modes"), first shipped in 2.1.12.The port carried the constant (with a
TODO ALEX why ???) and the unpacked code paths, butexpandArrayIfNeededgrew the same context in place andisPackedwas only evaluated in the constructor. Past 128 KB of storage, short indexes wrapped and the array corrupted itself: filling 5000 indexes with 64-bit values ends in a memory access out of bounds.The port
AssemblyScript has no
try/catch, soResizeExceptionbecomes a return value:expandArrayIfNeededstill grows in place below the cap. Above it, it stores the needed length inrequestedPhysicalLengthand returnsfalse.newEntry,newLeafEntry,expandEntryandgetRootEntrypropagate the failure asENTRY_ALLOCATION_FAILED. Allocations happen before any link is written, so a failed expansion leaves the tree consistent.getPackedIndexreturnsRESIZE_REQUIRED(-2;-1still means "absent").PackedArray.set()andadd()get the Java retry loops back:bytesAlreadySetforset, an unshiftedremainingValueToAddforadd, and onRESIZE_REQUIREDthey callresizeStorageArray(), then retry (which lands in the unpacked branch).resizeStorageArray()andsetVirtualLength()sharereplaceContext(): packed to packed copies the set trees as before (now reporting if they don't fit), anything else re-inserts the non-zero values throughset(), as Java'sresizeStorageArray/setVirtualLengthdo withnonZeroValues().PackedArray.isPackedis exposed for tests.Tests
assembly/__tests__/PackedArrayUnpacked.test.ts(new), written first and failing onperf/packedwith the crash above:set/add: switches to unpacked, every value matches a plain-array model.resizeafter the switch keeps values and more values can be added past the old length.clearafter the switch zeroes everything and stays unpacked, as in Java.PackedHistogramwith 6000 large counts (> 2^20) matchesHistogram64on totalCount, maxValue and seven percentiles, and subtracting the 64-bit one brings it to zero.npm run astest: 154/154.npm test: 180/180.Performance
Same side-by-side harness as #62/#63, PR #63 binary vs this branch, packed histograms, median of 15 rounds: all ops within noise (p99 1.01x, add+subtract 0.92x to 0.98x, record 0.95x to 0.98x). The retry loops only add a compare per byte on the insert path.
🤖 Generated with Claude Code
https://claude.ai/code/session_013Jg4gNfTZpd8eVPAvqx3js