Skip to content

Simplify build mapping resolution#2318

Merged
ValentaTomas merged 1 commit intomainfrom
simplify-mapping-resolution
Apr 10, 2026
Merged

Simplify build mapping resolution#2318
ValentaTomas merged 1 commit intomainfrom
simplify-mapping-resolution

Conversation

@ValentaTomas
Copy link
Copy Markdown
Member

@ValentaTomas ValentaTomas commented Apr 7, 2026

Remove the bitset and simplify build id lookup to reduce the memory usage.

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 7, 2026

PR Summary

Medium Risk
Mapping resolution now relies on Header.Mapping being sorted/contiguous and uses binary search; if callers ever provide unsorted or gapped mappings, lookups may return the wrong source or fail. Behavior changes could surface as read offset/mapping errors in storage paths that call GetShiftedMapping.

Overview
Simplifies Header build mapping resolution by removing the precomputed bitset/start-index map and switching getMapping to a sort.Search over Header.Mapping, computing shifts directly from the chosen range. This also trims related block-based error/log details and removes the extra allocation/indexing work done during NewHeader construction.

Reviewed by Cursor Bugbot for commit b90faf6. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Binary search assumes sorted mapping without enforcement
    • Added sort.Slice in NewHeader to guarantee mappings are sorted by Offset, ensuring binary search correctness regardless of input order.

Create PR

Or push these changes by commenting:

@cursor push 277fabe9bc
Preview (277fabe9bc)
diff --git a/packages/shared/pkg/storage/header/header.go b/packages/shared/pkg/storage/header/header.go
--- a/packages/shared/pkg/storage/header/header.go
+++ b/packages/shared/pkg/storage/header/header.go
@@ -32,6 +32,10 @@
 		}}
 	}
 
+	sort.Slice(mapping, func(i, j int) bool {
+		return mapping[i].Offset < mapping[j].Offset
+	})
+
 	return &Header{
 		Metadata: metadata,
 		Mapping:  mapping,

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b90faf6. Configure here.

@ValentaTomas ValentaTomas merged commit cdab472 into main Apr 10, 2026
48 checks passed
@ValentaTomas ValentaTomas deleted the simplify-mapping-resolution branch April 10, 2026 04:12
levb added a commit that referenced this pull request Apr 10, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants