Repository navigation
chore: simplify arrayComposite offsets and skip them for fixed size elements - #556
Merged
wemeetagain merged 3 commits intoOct 16, 2026
Conversation
readOffsetsArrayComposite() now returns n + 1 offsets with a trailing entry equal to the serialized size, so element i is always [offsets[i], offsets[i + 1]) and the deserialize loops no longer special case the last element. 🤖 Generated with AI assistance
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ac6df088d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🤖 Generated with AI assistance
Fixed size elements are now read at start + i * fixedSize in value_/tree_deserializeFromBytesArrayComposite and ProgressiveList tree_deserializeFromBytes, so offsets are only read and allocated for variable size elements. 🤖 Generated with AI assistance
wemeetagain
approved these changes
Oct 16, 2026
wemeetagain
pushed a commit
that referenced
this pull request
Oct 16, 2026
**Motivation** #556 is merged and conflicts with this branch in `readVariableOffsetsArrayComposite()`. **Description** Merges `master` into `te/serialized_list`: - `readVariableOffsetsArrayComposite()` takes master's version and stays exported. It always returns n + 1 offsets now, so `withEndOffset` is gone. Offsets are a `Float64Array` when the list is 2^32 bytes or larger, so the trailing entry doesn't wrap - `SerializedListBaseType.value_deserializeFromBytes()` drops the `true` argument - `SerializedList.offsets` is `Uint32Array | Float64Array` to match. Its doc comment no longer says that list types keep only n offsets If you'd rather keep `offsets` a plain `Uint32Array`, the alternative is to reject serialized lists of 2^32 bytes or more in `SerializedListBaseType`. That would differ from `ListCompositeType`, which accepts them. **Tests** - `pnpm test:unit`: 2405 passed - `pnpm test:spec-generic`: 5065 passed - `pnpm test:spec-static-minimal`: 13284 passed, `pnpm test:spec-static-mainnet`: 540 passed - `pnpm lint` clean 🤖 Generated with AI assistance Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
wemeetagain
added a commit
that referenced
this pull request
Oct 16, 2026
PR #557 brought master's changes into this branch with a squash commit, so the PR comparison still includes the changes from #556. Record the actual merge with master to correct the comparison base. The resulting tree is identical to ca973a6; preserve the exported readVariableOffsetsArrayComposite helper required by SerializedListType.
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.
Motivation
readOffsetsArrayComposite()returned one offset per element, so both deserialize loops inarrayComposite.tshad to special case the last element (i === length - 1 ? end : start + offsets[i + 1]). The comments next to them ("Capture length before pushing end offset", "offests include the last element end") describe an n + 1 layout that the code no longer had.The fixed size branch also built an offsets array of
i * fixedSizejust so both element kinds could share one loop, while serialization already writes fixed size elements atoffset + i * fixedSizedirectly.This also lets #555 drop the
withEndOffsetflag it adds toreadVariableOffsetsArrayComposite(), since that is the layoutSerializedListneeds.Description
start + i * fixedSizeinvalue_deserializeFromBytesArrayComposite(),tree_deserializeFromBytesArrayComposite()andProgressiveListCompositeType.tree_deserializeFromBytes(), with no offsets array. The length checks (size % fixedSize,assertValidArrayLength()) and their errors are unchangedreadVariableOffsetsArrayComposite()now returns n + 1 offsets, with a trailing entry equal toend - start. An empty array returns[0]assertValidArrayLength()still gets the element count (offsets.length - 1)endEl = start + offsets[i + 1]with no branchThe offset readers are module private and the deserialize functions above are their only callers. ProgressiveList keeps its own variable size offset reader. Validation and error order are unchanged. The trailing write happens after all offset checks.
Fixed size elements use plain number math, so they don't wrap at any size. Variable size offsets stay in a
Uint32Array, except when the array is 2^32 bytes or larger. Then they go in aFloat64Arrayso the trailing entry doesn't wrap, and the last element still ends atendas before. The SSZ spec caps the serialized size below 2^32, but the library accepted these inputs before this change.Tests
pnpm test:unit: 1814 passedpnpm test:spec-generic: 5065 passedpnpm test:spec-static-minimal: 13284 passed,pnpm test:spec-static-mainnet: 540 passedpnpm lintcleanBenchmarks
Local, node 24 (V8 13.6), at 08422c6. Medians of 6 interleaved master/branch rounds per case, with the side that runs first alternating each round.
deserialize()= value,deserializeToView()= tree. Cases up to 128 elements come from the small file (3 to 6 s per case), the rest from the large file (1.5 to 4 s per case).The fixed size cases are a few percent faster than master because nothing is allocated for offsets anymore. This also removes the n = 16 regression the first commit had (
List[Container(fixed 40B)] 16value: 2.16 us on master vs 2.89 us, fromUint32Array(17)crossing V8's 64 byte on heap limit). It's now 2.20 us on master vs 2.11 us. Variable size cases are unchanged within noise. On this machine the large cases vary by about 20% between rounds of the same code, so differences below that only show up in the small cases.Bench file (not committed). The small cases used the same file with counts 1/4 (ByteList), 1/16/128 (Container) and Vector[Bytes32, 4], without the ProgressiveList cases
🤖 Generated with AI assistance