Skip to content

refactor(speculation): drop snapshot validation from bestfirst - #524

Open
behinddwalls wants to merge 2 commits into
mainfrom
preetam/cleanup-best-first
Open

refactor(speculation): drop snapshot validation from bestfirst#524
behinddwalls wants to merge 2 commits into
mainfrom
preetam/cleanup-best-first

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Why?

Generate re-validated the queue snapshot on every run: seven checks covering empty and duplicate batch IDs, empty, duplicate, and self dependencies, dependencies missing from the snapshot, and unknown dependency states — plus a nil-scorer panic in New. Those are properties of a correctly assembled snapshot, established wherever the snapshot is built, so checking them again on the hot path of every run spreads one contract across two places and buys nothing a well-formed snapshot could ever trip. They were also the bulk of Generate's branching.

The scorer range check was different in kind: a score arrives from an injected extension, so no earlier stage can vet it. But sinking the whole run on one bad number is the wrong response when a default will do.

What?

Snapshot validation is gone. The Generator interface doc and the best-first RFC now state the well-formedness rules as a caller precondition — a generator may assume them, and a malformed snapshot yields undefined candidates rather than an error. Nothing consumes the generator yet, so whoever assembles the snapshot will own the check.

One behavior worth flagging for reviewers: a head that repeats a dependency was previously rejected and now produces paths that assume the same batch both succeeds and fails. That is undefined input under the new contract, but it fails quietly rather than loudly.

A score outside [0, 1], or NaN, is now replaced with a default of 0.95 rather than rejected. The default is optimistic on purpose: a dependency nobody could estimate keeps its head's preferred path near the front instead of being buried. Both comparisons in asProbability are false for NaN, so it needs no separate case.

Generate is now three linear steps — index, speculatingHeads, score — and seeding the global heap appends and heapifies once. Previously heap.Init ran on an empty slice and each head was pushed individually, costing a sift and an interface boxing per head; building the slice first is linear. slices.Sorted(maps.Keys(...)) replaces the collect-then-sort loop, and slices.Clone replaces three make+copy pairs.

Net 143 lines removed, 80 added.

Test Plan

go test ./submitqueue/extension/speculation/...
go vet ./submitqueue/extension/speculation/...
gofmt -l submitqueue/extension/speculation/ — clean

The malformed-snapshot table test is removed. The score-range test now asserts the default is applied and both of the head's paths still come out, ranked as the default and its complement, rather than asserting an error.

@behinddwalls
behinddwalls marked this pull request as ready for review August 6, 2026 04:09
@behinddwalls
behinddwalls requested review from a team and sbalabanov as code owners August 6, 2026 04:09
## Summary

### Why?

`Generate` re-validated the queue snapshot on every run: seven checks covering empty and duplicate batch IDs, empty, duplicate, and self dependencies, dependencies missing from the snapshot, and unknown dependency states — plus a nil-scorer panic in `New`. Those are properties of a correctly assembled snapshot, established wherever the snapshot is built, so checking them again on the hot path of every run spreads one contract across two places and buys nothing a well-formed snapshot could ever trip. They were also the bulk of `Generate`'s branching.

The scorer range check was different in kind: a score arrives from an injected extension, so no earlier stage can vet it. But sinking the whole run on one bad number is the wrong response when a default will do.

### What?

Snapshot validation is gone. The `Generator` interface doc and the best-first RFC now state the well-formedness rules as a caller precondition — a generator may assume them, and a malformed snapshot yields undefined candidates rather than an error. Nothing consumes the generator yet, so whoever assembles the snapshot will own the check.

One behavior worth flagging for reviewers: a head that repeats a dependency was previously rejected and now produces paths that assume the same batch both succeeds and fails. That is undefined input under the new contract, but it fails quietly rather than loudly.

A score outside `[0, 1]`, or `NaN`, is now replaced with a default of 0.95 rather than rejected. The default is optimistic on purpose: a dependency nobody could estimate keeps its head's preferred path near the front instead of being buried. Both comparisons in `asProbability` are false for `NaN`, so it needs no separate case.

`Generate` is now three linear steps — index, `speculatingHeads`, `score` — and seeding the global heap appends and heapifies once. Previously `heap.Init` ran on an empty slice and each head was pushed individually, costing a sift and an interface boxing per head; building the slice first is linear. `slices.Sorted(maps.Keys(...))` replaces the collect-then-sort loop, and `slices.Clone` replaces three `make`+`copy` pairs.

Net 143 lines removed, 80 added.

## Test Plan

✅ `go test ./submitqueue/extension/speculation/...`
✅ `go vet ./submitqueue/extension/speculation/...`
✅ `gofmt -l submitqueue/extension/speculation/` — clean

The malformed-snapshot table test is removed. The score-range test now asserts the default is applied and both of the head's paths still come out, ranked as the default and its complement, rather than asserting an error.
@behinddwalls
behinddwalls force-pushed the preetam/cleanup-best-first branch from dffa1d5 to 3166fea Compare August 6, 2026 05:30
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.

1 participant