perf(history): detect duplicate row IDs without a per-ID map
Row IDs are not required to be monotonic: AppendWithID accepts any unused
ID and the codec only rejects zero/reserved values and duplicates, so an
ordering check would change accept/reject results. Replace the
map[RowID]struct{} in parseHistory and validateRestoredHistoryView with
rowIDSet, an exact set that keeps IDs in an ascending slice while views
arrive in increasing order and falls back to a map on interleaving.
Per-chunk IDs are sorted and checked, then committed, so the accepted
inputs are identical (duplicate rows are still rejected before any cell
of the chunk is decoded).- commit
- 96f49005db3a6adb7229a6e38517336d7d300b35
- author
- brice · · 2026-10-02 09:31 UTC
- parent
- dca8245
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +76 -15
M history_codec.go +7 -5
| @@ -234,7 +234,7 @@ func parseHistory(data []byte, populate bool) (HistoryView, historyDecodeStats, | |||
| 234 | 234 | return invalid() | |
| 235 | 235 | } | |
| 236 | 236 | stats := historyDecodeStats{chunks: uint64(chunkCount)} | |
| 237 | - | seenIDs := make(map[RowID]struct{}) | |
| 237 | + | var seenIDs rowIDSet | |
| 238 | 238 | var maxID RowID | |
| 239 | 239 | var chunks []*HistoryChunk | |
| 240 | 240 | if populate { | |
| @@ -280,6 +280,7 @@ func parseHistory(data []byte, populate bool) (HistoryView, historyDecodeStats, | |||
| 280 | 280 | return invalid() | |
| 281 | 281 | } | |
| 282 | 282 | usedPayloads := make([]bool, npayloads) | |
| 283 | + | seenIDs.reserve(int(rows)) | |
| 283 | 284 | rowIDs := make([]RowID, rows) | |
| 284 | 285 | bounds := make([]LineBound, rows) | |
| 285 | 286 | for row := range rows { | |
| @@ -288,13 +289,14 @@ func parseHistory(data []byte, populate bool) (HistoryView, historyDecodeStats, | |||
| 288 | 289 | if !ok || !bok || id == 0 || id >= math.MaxUint64-1 || !validHistoryBound(bound, int(width)) { | |
| 289 | 290 | return invalid() | |
| 290 | 291 | } | |
| 291 | - | if _, duplicate := seenIDs[RowID(id)]; duplicate { | |
| 292 | - | return invalid() | |
| 293 | - | } | |
| 294 | - | seenIDs[RowID(id)] = struct{}{} | |
| 292 | + | seenIDs.collect(RowID(id)) | |
| 295 | 293 | maxID = max(maxID, RowID(id)) | |
| 296 | 294 | rowIDs[row], bounds[row] = RowID(id), bound | |
| 297 | 295 | } | |
| 296 | + | // Reject duplicate IDs before decoding any cells, as the map did. | |
| 297 | + | if !seenIDs.commit() { | |
| 298 | + | return invalid() | |
| 299 | + | } | |
| 298 | 300 | lastStyleRow := make([]int, nstyles) | |
| 299 | 301 | for i := range lastStyleRow { | |
| 300 | 302 | lastStyleRow[i] = -1 | |
| @@ -234,7 +234,7 @@ func parseHistory(data []byte, populate bool) (HistoryView, historyDecodeStats, | |||||
| 234 | return invalid() | 234 | return invalid() | ||
| 235 | } | 235 | } | ||
| 236 | stats := historyDecodeStats{chunks: uint64(chunkCount)} | 236 | stats := historyDecodeStats{chunks: uint64(chunkCount)} | ||
| 237 | - | seenIDs := make(map[RowID]struct{}) | 237 | + | var seenIDs rowIDSet |
| 238 | var maxID RowID | 238 | var maxID RowID | ||
| 239 | var chunks []*HistoryChunk | 239 | var chunks []*HistoryChunk | ||
| 240 | if populate { | 240 | if populate { | ||
| @@ -280,6 +280,7 @@ func parseHistory(data []byte, populate bool) (HistoryView, historyDecodeStats, | |||||
| 280 | return invalid() | 280 | return invalid() | ||
| 281 | } | 281 | } | ||
| 282 | usedPayloads := make([]bool, npayloads) | 282 | usedPayloads := make([]bool, npayloads) | ||
| 283 | + | seenIDs.reserve(int(rows)) | |||
| 283 | rowIDs := make([]RowID, rows) | 284 | rowIDs := make([]RowID, rows) | ||
| 284 | bounds := make([]LineBound, rows) | 285 | bounds := make([]LineBound, rows) | ||
| 285 | for row := range rows { | 286 | for row := range rows { | ||
| @@ -288,13 +289,14 @@ func parseHistory(data []byte, populate bool) (HistoryView, historyDecodeStats, | |||||
| 288 | if !ok || !bok || id == 0 || id >= math.MaxUint64-1 || !validHistoryBound(bound, int(width)) { | 289 | if !ok || !bok || id == 0 || id >= math.MaxUint64-1 || !validHistoryBound(bound, int(width)) { | ||
| 289 | return invalid() | 290 | return invalid() | ||
| 290 | } | 291 | } | ||
| 291 | - | if _, duplicate := seenIDs[RowID(id)]; duplicate { | 292 | + | seenIDs.collect(RowID(id)) |
| 292 | - | return invalid() | |||
| 293 | - | } | |||
| 294 | - | seenIDs[RowID(id)] = struct{}{} | |||
| 295 | maxID = max(maxID, RowID(id)) | 293 | maxID = max(maxID, RowID(id)) | ||
| 296 | rowIDs[row], bounds[row] = RowID(id), bound | 294 | rowIDs[row], bounds[row] = RowID(id), bound | ||
| 297 | } | 295 | } | ||
| 296 | + | // Reject duplicate IDs before decoding any cells, as the map did. | |||
| 297 | + | if !seenIDs.commit() { | |||
| 298 | + | return invalid() | |||
| 299 | + | } | |||
| 298 | lastStyleRow := make([]int, nstyles) | 300 | lastStyleRow := make([]int, nstyles) | ||
| 299 | for i := range lastStyleRow { | 301 | for i := range lastStyleRow { | ||
| 300 | lastStyleRow[i] = -1 | 302 | lastStyleRow[i] = -1 | ||
A history_rowids.go +61 -0
| @@ -0,0 +1,61 @@ | |||
| 1 | + | package vt | |
| 2 | + | ||
| 3 | + | import "slices" | |
| 4 | + | ||
| 5 | + | // rowIDSet detects duplicate row IDs across one or more decoded blobs without | |
| 6 | + | // a hash map in the common case. Row IDs are NOT required to be monotonic | |
| 7 | + | // (History.AppendWithID accepts any unused ID, so persisted blobs may list IDs | |
| 8 | + | // out of order), so this is an exact set, not an ordering check: it accepts | |
| 9 | + | // and rejects precisely the inputs a map[RowID]struct{} would. | |
| 10 | + | // | |
| 11 | + | // While every view contributes IDs strictly above everything seen so far, IDs | |
| 12 | + | // are kept in an ascending slice. The first view that interleaves with earlier | |
| 13 | + | // IDs converts the set to a map; from then on it behaves like a plain set. | |
| 14 | + | type rowIDSet struct { | |
| 15 | + | sorted []RowID | |
| 16 | + | members map[RowID]struct{} | |
| 17 | + | scratch []RowID | |
| 18 | + | } | |
| 19 | + | ||
| 20 | + | // reserve makes room for n more collected IDs. | |
| 21 | + | func (s *rowIDSet) reserve(n int) { s.scratch = slices.Grow(s.scratch, n) } | |
| 22 | + | ||
| 23 | + | // collect records one ID of the view being collected. | |
| 24 | + | func (s *rowIDSet) collect(id RowID) { s.scratch = append(s.scratch, id) } | |
| 25 | + | ||
| 26 | + | // commit reports whether the collected IDs are distinct from each other and | |
| 27 | + | // from every previously committed ID, and if so adds them to the set. | |
| 28 | + | func (s *rowIDSet) commit() bool { | |
| 29 | + | ids := s.scratch | |
| 30 | + | s.scratch = s.scratch[:0] | |
| 31 | + | if len(ids) == 0 { | |
| 32 | + | return true | |
| 33 | + | } | |
| 34 | + | slices.Sort(ids) // the scratch copy is private, so sorting in place is safe | |
| 35 | + | ||
| 36 | + | for i := 1; i < len(ids); i++ { | |
| 37 | + | if ids[i] == ids[i-1] { | |
| 38 | + | return false | |
| 39 | + | } | |
| 40 | + | } | |
| 41 | + | if s.members == nil { | |
| 42 | + | if len(s.sorted) == 0 || ids[0] > s.sorted[len(s.sorted)-1] { | |
| 43 | + | s.sorted = append(s.sorted, ids...) | |
| 44 | + | return true | |
| 45 | + | } | |
| 46 | + | s.members = make(map[RowID]struct{}, len(s.sorted)+len(ids)) | |
| 47 | + | for _, id := range s.sorted { | |
| 48 | + | s.members[id] = struct{}{} | |
| 49 | + | } | |
| 50 | + | s.sorted = nil | |
| 51 | + | } | |
| 52 | + | for _, id := range ids { | |
| 53 | + | if _, duplicate := s.members[id]; duplicate { | |
| 54 | + | return false | |
| 55 | + | } | |
| 56 | + | } | |
| 57 | + | for _, id := range ids { | |
| 58 | + | s.members[id] = struct{}{} | |
| 59 | + | } | |
| 60 | + | return true | |
| 61 | + | } | |
| @@ -0,0 +1,61 @@ | |||||
| 1 | + | package vt | |||
| 2 | + | ||||
| 3 | + | import "slices" | |||
| 4 | + | ||||
| 5 | + | // rowIDSet detects duplicate row IDs across one or more decoded blobs without | |||
| 6 | + | // a hash map in the common case. Row IDs are NOT required to be monotonic | |||
| 7 | + | // (History.AppendWithID accepts any unused ID, so persisted blobs may list IDs | |||
| 8 | + | // out of order), so this is an exact set, not an ordering check: it accepts | |||
| 9 | + | // and rejects precisely the inputs a map[RowID]struct{} would. | |||
| 10 | + | // | |||
| 11 | + | // While every view contributes IDs strictly above everything seen so far, IDs | |||
| 12 | + | // are kept in an ascending slice. The first view that interleaves with earlier | |||
| 13 | + | // IDs converts the set to a map; from then on it behaves like a plain set. | |||
| 14 | + | type rowIDSet struct { | |||
| 15 | + | sorted []RowID | |||
| 16 | + | members map[RowID]struct{} | |||
| 17 | + | scratch []RowID | |||
| 18 | + | } | |||
| 19 | + | ||||
| 20 | + | // reserve makes room for n more collected IDs. | |||
| 21 | + | func (s *rowIDSet) reserve(n int) { s.scratch = slices.Grow(s.scratch, n) } | |||
| 22 | + | ||||
| 23 | + | // collect records one ID of the view being collected. | |||
| 24 | + | func (s *rowIDSet) collect(id RowID) { s.scratch = append(s.scratch, id) } | |||
| 25 | + | ||||
| 26 | + | // commit reports whether the collected IDs are distinct from each other and | |||
| 27 | + | // from every previously committed ID, and if so adds them to the set. | |||
| 28 | + | func (s *rowIDSet) commit() bool { | |||
| 29 | + | ids := s.scratch | |||
| 30 | + | s.scratch = s.scratch[:0] | |||
| 31 | + | if len(ids) == 0 { | |||
| 32 | + | return true | |||
| 33 | + | } | |||
| 34 | + | slices.Sort(ids) // the scratch copy is private, so sorting in place is safe | |||
| 35 | + | ||||
| 36 | + | for i := 1; i < len(ids); i++ { | |||
| 37 | + | if ids[i] == ids[i-1] { | |||
| 38 | + | return false | |||
| 39 | + | } | |||
| 40 | + | } | |||
| 41 | + | if s.members == nil { | |||
| 42 | + | if len(s.sorted) == 0 || ids[0] > s.sorted[len(s.sorted)-1] { | |||
| 43 | + | s.sorted = append(s.sorted, ids...) | |||
| 44 | + | return true | |||
| 45 | + | } | |||
| 46 | + | s.members = make(map[RowID]struct{}, len(s.sorted)+len(ids)) | |||
| 47 | + | for _, id := range s.sorted { | |||
| 48 | + | s.members[id] = struct{}{} | |||
| 49 | + | } | |||
| 50 | + | s.sorted = nil | |||
| 51 | + | } | |||
| 52 | + | for _, id := range ids { | |||
| 53 | + | if _, duplicate := s.members[id]; duplicate { | |||
| 54 | + | return false | |||
| 55 | + | } | |||
| 56 | + | } | |||
| 57 | + | for _, id := range ids { | |||
| 58 | + | s.members[id] = struct{}{} | |||
| 59 | + | } | |||
| 60 | + | return true | |||
| 61 | + | } | |||
M snapshot.go +8 -10
| @@ -88,22 +88,22 @@ func HistoryFromBlobs(config HistoryConfig, sealed [][]byte, tail []byte) (*Hist | |||
| 88 | 88 | } | |
| 89 | 89 | ||
| 90 | 90 | func decodeRestoredHistoryBlobs(sealed [][]byte, tail []byte, extra ...HistoryView) ([]HistoryView, HistoryView, error) { | |
| 91 | - | seen := make(map[RowID]struct{}) | |
| 91 | + | var seen rowIDSet | |
| 92 | 92 | sealedViews := make([]HistoryView, len(sealed)) | |
| 93 | 93 | for i, blob := range sealed { | |
| 94 | 94 | view, err := UnmarshalHistory(blob) | |
| 95 | - | if err != nil || len(view.chunks) != 1 || view.chunks[0].len() == 0 || !validateRestoredHistoryView(view, seen) { | |
| 95 | + | if err != nil || len(view.chunks) != 1 || view.chunks[0].len() == 0 || !validateRestoredHistoryView(view, &seen) { | |
| 96 | 96 | return nil, HistoryView{}, fmt.Errorf("restore sealed history: %w", errInvalidHistory) | |
| 97 | 97 | } | |
| 98 | 98 | sealedViews[i] = view | |
| 99 | 99 | } | |
| 100 | 100 | ||
| 101 | 101 | tailView, err := UnmarshalHistory(tail) | |
| 102 | - | if err != nil || !validateRestoredHistoryView(tailView, seen) { | |
| 102 | + | if err != nil || !validateRestoredHistoryView(tailView, &seen) { | |
| 103 | 103 | return nil, HistoryView{}, fmt.Errorf("restore history tail: %w", errInvalidHistory) | |
| 104 | 104 | } | |
| 105 | 105 | for _, view := range extra { | |
| 106 | - | if !validateRestoredHistoryView(view, seen) { | |
| 106 | + | if !validateRestoredHistoryView(view, &seen) { | |
| 107 | 107 | return nil, HistoryView{}, fmt.Errorf("restore recovery transcript: %w", errInvalidHistory) | |
| 108 | 108 | } | |
| 109 | 109 | } | |
| @@ -174,7 +174,7 @@ func (h *History) boundedRestoredChunk(chunk *HistoryChunk) *HistoryChunk { | |||
| 174 | 174 | return bounded | |
| 175 | 175 | } | |
| 176 | 176 | ||
| 177 | - | func validateRestoredHistoryView(view HistoryView, seen map[RowID]struct{}) bool { | |
| 177 | + | func validateRestoredHistoryView(view HistoryView, seen *rowIDSet) bool { | |
| 178 | 178 | if view.nextRowID == 0 || view.nextRowID == ^RowID(0) { | |
| 179 | 179 | return false | |
| 180 | 180 | } | |
| @@ -183,18 +183,16 @@ func validateRestoredHistoryView(view HistoryView, seen map[RowID]struct{}) bool | |||
| 183 | 183 | if chunk == nil || chunk.len() != len(chunk.rowIDs) || chunk.len() != len(chunk.bounds) { | |
| 184 | 184 | return false | |
| 185 | 185 | } | |
| 186 | + | seen.reserve(len(chunk.rowIDs)) | |
| 186 | 187 | for _, id := range chunk.rowIDs { | |
| 187 | 188 | if id == 0 || id >= ^RowID(0)-1 { | |
| 188 | 189 | return false | |
| 189 | 190 | } | |
| 190 | - | if _, duplicate := seen[id]; duplicate { | |
| 191 | - | return false | |
| 192 | - | } | |
| 193 | - | seen[id] = struct{}{} | |
| 191 | + | seen.collect(id) | |
| 194 | 192 | maxID = max(maxID, id) | |
| 195 | 193 | } | |
| 196 | 194 | } | |
| 197 | - | return view.nextRowID > maxID | |
| 195 | + | return view.nextRowID > maxID && seen.commit() | |
| 198 | 196 | } | |
| 199 | 197 | ||
| 200 | 198 | // NewScreenWithRecoveryTranscript constructs a fresh blank screen whose | |
| @@ -88,22 +88,22 @@ func HistoryFromBlobs(config HistoryConfig, sealed [][]byte, tail []byte) (*Hist | |||||
| 88 | } | 88 | } | ||
| 89 | 89 | ||||
| 90 | func decodeRestoredHistoryBlobs(sealed [][]byte, tail []byte, extra ...HistoryView) ([]HistoryView, HistoryView, error) { | 90 | func decodeRestoredHistoryBlobs(sealed [][]byte, tail []byte, extra ...HistoryView) ([]HistoryView, HistoryView, error) { | ||
| 91 | - | seen := make(map[RowID]struct{}) | 91 | + | var seen rowIDSet |
| 92 | sealedViews := make([]HistoryView, len(sealed)) | 92 | sealedViews := make([]HistoryView, len(sealed)) | ||
| 93 | for i, blob := range sealed { | 93 | for i, blob := range sealed { | ||
| 94 | view, err := UnmarshalHistory(blob) | 94 | view, err := UnmarshalHistory(blob) | ||
| 95 | - | if err != nil || len(view.chunks) != 1 || view.chunks[0].len() == 0 || !validateRestoredHistoryView(view, seen) { | 95 | + | if err != nil || len(view.chunks) != 1 || view.chunks[0].len() == 0 || !validateRestoredHistoryView(view, &seen) { |
| 96 | return nil, HistoryView{}, fmt.Errorf("restore sealed history: %w", errInvalidHistory) | 96 | return nil, HistoryView{}, fmt.Errorf("restore sealed history: %w", errInvalidHistory) | ||
| 97 | } | 97 | } | ||
| 98 | sealedViews[i] = view | 98 | sealedViews[i] = view | ||
| 99 | } | 99 | } | ||
| 100 | 100 | ||||
| 101 | tailView, err := UnmarshalHistory(tail) | 101 | tailView, err := UnmarshalHistory(tail) | ||
| 102 | - | if err != nil || !validateRestoredHistoryView(tailView, seen) { | 102 | + | if err != nil || !validateRestoredHistoryView(tailView, &seen) { |
| 103 | return nil, HistoryView{}, fmt.Errorf("restore history tail: %w", errInvalidHistory) | 103 | return nil, HistoryView{}, fmt.Errorf("restore history tail: %w", errInvalidHistory) | ||
| 104 | } | 104 | } | ||
| 105 | for _, view := range extra { | 105 | for _, view := range extra { | ||
| 106 | - | if !validateRestoredHistoryView(view, seen) { | 106 | + | if !validateRestoredHistoryView(view, &seen) { |
| 107 | return nil, HistoryView{}, fmt.Errorf("restore recovery transcript: %w", errInvalidHistory) | 107 | return nil, HistoryView{}, fmt.Errorf("restore recovery transcript: %w", errInvalidHistory) | ||
| 108 | } | 108 | } | ||
| 109 | } | 109 | } | ||
| @@ -174,7 +174,7 @@ func (h *History) boundedRestoredChunk(chunk *HistoryChunk) *HistoryChunk { | |||||
| 174 | return bounded | 174 | return bounded | ||
| 175 | } | 175 | } | ||
| 176 | 176 | ||||
| 177 | - | func validateRestoredHistoryView(view HistoryView, seen map[RowID]struct{}) bool { | 177 | + | func validateRestoredHistoryView(view HistoryView, seen *rowIDSet) bool { |
| 178 | if view.nextRowID == 0 || view.nextRowID == ^RowID(0) { | 178 | if view.nextRowID == 0 || view.nextRowID == ^RowID(0) { | ||
| 179 | return false | 179 | return false | ||
| 180 | } | 180 | } | ||
| @@ -183,18 +183,16 @@ func validateRestoredHistoryView(view HistoryView, seen map[RowID]struct{}) bool | |||||
| 183 | if chunk == nil || chunk.len() != len(chunk.rowIDs) || chunk.len() != len(chunk.bounds) { | 183 | if chunk == nil || chunk.len() != len(chunk.rowIDs) || chunk.len() != len(chunk.bounds) { | ||
| 184 | return false | 184 | return false | ||
| 185 | } | 185 | } | ||
| 186 | + | seen.reserve(len(chunk.rowIDs)) | |||
| 186 | for _, id := range chunk.rowIDs { | 187 | for _, id := range chunk.rowIDs { | ||
| 187 | if id == 0 || id >= ^RowID(0)-1 { | 188 | if id == 0 || id >= ^RowID(0)-1 { | ||
| 188 | return false | 189 | return false | ||
| 189 | } | 190 | } | ||
| 190 | - | if _, duplicate := seen[id]; duplicate { | 191 | + | seen.collect(id) |
| 191 | - | return false | |||
| 192 | - | } | |||
| 193 | - | seen[id] = struct{}{} | |||
| 194 | maxID = max(maxID, id) | 192 | maxID = max(maxID, id) | ||
| 195 | } | 193 | } | ||
| 196 | } | 194 | } | ||
| 197 | - | return view.nextRowID > maxID | 195 | + | return view.nextRowID > maxID && seen.commit() |
| 198 | } | 196 | } | ||
| 199 | 197 | ||||
| 200 | // NewScreenWithRecoveryTranscript constructs a fresh blank screen whose | 198 | // NewScreenWithRecoveryTranscript constructs a fresh blank screen whose | ||