perf: use value BuildMap slices instead of pointer slices#2319
Conversation
Eliminates per-mapping heap allocation by switching []*BuildMap to []BuildMap throughout the header package. Also sorts mappings on deserialization to protect against unsorted legacy (V<3) headers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use consistent value receiver on BuildMap (recvcheck) - Merge var declaration with assignment (staticcheck S1021) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aafe6e0bbd
ℹ️ 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".
|
I would remove the explicit sorting as the mappings must already be sorted and we are validating that on diff creation. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical pointer-to-value refactor with consistent test coverage and a sensible defensive sort on deserialization.
Extended reasoning...
Overview
Changes are scoped entirely to the packages/shared/pkg/storage/header package (plus two minor call-site updates). The core transformation is replacing []*BuildMap with []BuildMap everywhere, eliminating per-mapping heap allocations. A secondary fix tightens NormalizeMappings to require contiguous BuildStorageOffset (not just matching BuildId) before merging adjacent mappings, which is more semantically correct. A defensive sort is added in DeserializeBytes for legacy headers.
Security risks
No security-sensitive code is touched. The changes do not affect auth, crypto, or permissions.
Level of scrutiny
This is a performance/correctness refactor with no external API surface changes. The logic in the hot path (getMapping's binary search) is equivalent to before — the i = sort.Search(...) - 1 form is just cleaner. The MergeMappings pointer aliasing (taking base := &baseMapping[baseIdx]) is safe because every branch that mutates baseMapping[baseIdx] also immediately continues the loop, so base is always re-fetched at the next iteration top. The test suite comprehensively covers all merge/normalize cases.
Other factors
No outstanding reviewer comments. The NormalizeMappings semantic tightening (requiring contiguous BuildStorageOffset) is a correctness improvement — the old code would have incorrectly merged mappings that share a BuildId but point to non-contiguous storage regions. All tests pass for both the new stricter merging behavior and the legacy zero-length mapping edge case.
The BuildStorageOffset contiguity check is a correctness fix that belongs in a separate PR, not in this perf-only pointer-to-value change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # packages/shared/pkg/storage/header/header.go
…into lev-value-buildmap
The initial merge left unresolved conflict markers and didn't account for main's []*BuildMap → []BuildMap migration (PR #2319) or the simplified mapping resolution (PR #2318). This properly reconciles our compression branch (FrameTable support, BuildFiles, V4 format) with those changes: value-type BuildMap slices, sort.Search lookup, and ApplyFrames as a standalone function to avoid mixed receivers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Eliminates per-mapping heap allocation by switching []*BuildMap to []BuildMap throughout the header package.