Stream ballots into a compact per-race projection for tabulation - #1464
Stream ballots into a compact per-race projection for tabulation#1464jacksonloper wants to merge 4 commits into
Conversation
Tabulating a large election loaded every ballot as a full verbose row before handing it to the tabulators, which is one of the main drivers of star-server OOMKills. The algorithms are batch, so they genuinely need the whole election — but "the whole election" doesn't have to mean 30k verbose JSON ballot objects. getElectionResults now streams ballots from a db cursor (selecting only the votes column) and projects each row into a CompactVoteStore: flat typed arrays holding one race's marks positionally against its candidate order. The verbose row is dropped immediately, and the tabulator's own input is expanded from the store one race at a time. The positional layout is the same one bulk uploads already use, so it's now defined once in shared/domain_model/OrderedVoteCodec: the frontend uploader encodes with it, castVoteController's mapOrderedNewBallot decodes with it, and the projection stores it unrolled. Measured on a synthetic 31k-ballot election with 10 candidates/race, peak retained memory (heap + external): 1 race: 38.1 MB -> 10.0 MB 3 races: 72.1 MB -> 16.3 MB The compact store is 3.2 MB/race and lives off the JS heap, so it also stops adding GC-tracing pressure. What remains is the expanded rawVote cvr the tabulator API requires. A bug in a projection layer miscounts elections silently rather than failing loudly, so BallotProjection.test.ts pins the new path against a verbatim copy of the old inline projection: for every voting method, the tabulator inputs must match and tabulating both must give identical results. Fixtures cover null vs. absent marks, write-ins (approved, unapproved, unrecognized, aliased, shared aliases), duplicate scores, overvote_rank, has_duplicate_rank, skipped ranks and multi-race ballots. The distinction that most needed pinning: a candidate the ballot never mentions is not the same as one left explicitly blank. STAR treats an all-equal ballot as an abstention, so filling an approved write-in's missing slot with 0 would silently reclassify those ballots. Closes #1425 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things kept peak memory higher than it needed to be. takeRawVotes built the whole rawVote[] before calling release(), so for the duration of that one call a race's compact store and its full expansion both sat in memory. The store now keeps marks in fixed-size blocks and consumeMarks drops each block as soon as it's read, so the overlap is the expansion plus one block instead of the expansion plus the whole store. Blocks also mean appending never copies — the old doubling buffer transiently held the old and new copies at once. Reading a Float64Array always yields a double, and V8 boxes a double stored into an object property as a HeapNumber. That was 310k HeapNumbers for a 31k-ballot race with 10 candidates, which more than doubled the expanded cvr (10.3 MB, against 5.2 MB for the same data built straight from JSON). Marks are almost always small integers, so hand those back as int32s and let V8 keep them as tagged small ints. Peak retained (heap + external), 31k ballots, 10 candidates/race: 1 race: 36.5 MB -> 5.3 MB (was 10.0 MB) 3 races: 70.5 MB -> 11.6 MB (was 16.3 MB) The compact store now costs less than the expansion it feeds, so it earns its place for single-race elections too, not just multi-race ones. Adds a projection test spanning three blocks — the block seam is exactly where an indexing mistake would attribute one ballot's marks to another, and both plausible off-by-one mutations there are caught by it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getElectionResultsController calls shuffleCandidatesForRandomTiebreak, which sorts `candidates` in place, and takeRawVotes was reading the mark-to-candidate mapping off that same array. The two calls happened to be in the only order that works. Swapping them attributes every ballot's marks to the wrong candidate — and the whole test suite still passed, which is exactly the silent miscount this projection is supposed to be guarded against. RaceProjection now snapshots candidateIds at construction and indexes the store by that, so callers can mutate `candidates` freely. The new test shuffles the list before expanding and checks the cvr against the reference projection; it fails against the pre-fix code. Also from the review: - CompactVoteStore rejects commitRow after consumeMarks. The blocks are handed out and nulled by then, so appending would push a fresh block at the wrong index and misplace the ballot. - The codec test now asserts the thrown error's class, not just its message. castVoteController maps OrderedVoteFormatError to a 400 via instanceof, so a break there would turn a bad upload into a 500. - startRow's comment claimed to clear the scratch row when it only clears the tags; explained why that's sufficient instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Deploy Preview for bettervoting ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Tabulation had almost no coverage above the unit level. Stubbing the ballot stream to yield nothing failed exactly one test, and the controller could read the wrong race's projection with the whole suite still green — the multi-race and precinct tests only cover ballot submission and never fetch results, and the one test that does tabulate uses a single-race election. That left the fan-out unpinned: projectBallots builds one projection per race and the controller indexes them alongside election.races, and a mixup there yields plausible results for the wrong race rather than an obvious failure. Adds a two-race election that goes through submission and /API/ElectionResult, asserting each race's candidate list, winner and tally count. Both races reuse the same candidate ids on purpose, so a mixup produces marks that still resolve instead of an empty tally. Reading the wrong projection, or matching votes by position instead of race_id, each fail it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Looks great, thanks for doing this! I was impressed by the optimizations in the BallotProjection & CompactVoteStore pattern.
I left some clarifying questions and optional fixes, but none of them are blockers. Approved! Feel free to merge whenever you're ready
| // Reference implementation (the pre-streaming code path, unchanged) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const referenceProjection = (race: Race, ballots: BallotVotes[]) => { |
There was a problem hiding this comment.
At first glance I feel like there's too much logic here. Like the test is confirming the production logic by reimplementing the production logic
There was a problem hiding this comment.
NOTE: after reading the production logic, I see how this is much smaller, so I'm fine with this as is
| return ((t ^ (t >>> 14)) >>> 0) / 4294967296; | ||
| }; | ||
|
|
||
| const shuffled = <T,>(items: T[], rand: () => number): T[] => { |
There was a problem hiding this comment.
We have a couple of adhoc shuffle functions spread around. I think the TinyRand implementation should be the default. That could also be cleaned up in more places, but that's probably out of scope for the PR
| return {reference, cvr, candidates: projection.candidates}; | ||
| }; | ||
|
|
||
| const compareTabulation = async (race: Race, ballots: BallotVotes[]) => { |
There was a problem hiding this comment.
I wonder if we can just remove compareProjection and referenceProjection and just do compare tabulation 👀
| * Expand this race's compact store into the tabulator's input and release | ||
| * the store. Called one race at a time so only a single race's verbose | ||
| * tabulator input is ever alive; consumeMarks frees the store block by block | ||
| * as it goes, so the two don't both sit at full size. |
There was a problem hiding this comment.
Could you clarify if this optimization would help for single race polls? I'm thinking of the Indivisible CA race with 11k votes
2 participants
Closes #1425.
Tabulating a large election loaded every ballot as a full verbose row before handing it to the tabulators — one of the main drivers of star-server OOMKills. The algorithms are batch, so they genuinely need the whole election at once, but "the whole election" doesn't have to mean 30k verbose JSON ballot objects.
What changed
getElectionResultsnow streams ballots from a DB cursor (selecting only thevotescolumn) and projects each row into aCompactVoteStore— flat typed arrays holding one race's marks positionally against its candidate order. The verbose row is dropped immediately, and the tabulator's own input is expanded from the store one race at a time, with the store freeing itself block by block as it's consumed.Tabulator inputs and outputs are unchanged; this is purely how the input gets built.
DRY with the bulk-upload format
The positional layout is the same one bulk uploads already use, so it's now defined once in
shared/domain_model/OrderedVoteCodec:UploadElections.tsx)castVoteController'smapOrderedNewBallotdecodes with it, including the length validation and its exact error messagesOrderedVotewidened fromnumber[]to(number | null | undefined)[], which is what it already carried at runtime —JSON.stringifywritesnullfor unmarked candidates and absent ranks.Memory
Peak retained (heap + external), synthetic 31k ballots, 10 candidates/race:
Two things mattered beyond streaming. The store keeps marks in fixed-size blocks and drops each as it's read, so the store and its expansion never both sit at full size. And marks are handed back as int32s rather than raw
Float64Arraydoubles — V8 boxes a double stored into an object property as a HeapNumber, which was 310k HeapNumbers per race and more than doubled the expanded cvr.Testing
A bug in a projection layer miscounts elections silently rather than failing loudly, so
BallotProjection.test.tspins the new path against a verbatim copy of the old inline projection: for every voting method, the tabulator inputs must match and tabulating both must give identical results. Fixtures cover null vs. absent marks, write-ins (approved, unapproved, unrecognized, aliased, shared aliases), duplicate scores,overvote_rank,has_duplicate_rank, skipped ranks, multi-race ballots, and ballots spanning three storage blocks.The distinction that most needed pinning: a candidate the ballot never mentions is not the same as one left explicitly blank. STAR treats an all-equal ballot as an abstention, so filling an approved write-in's missing slot with 0 would silently reclassify those ballots. That's why marks carry a three-state tag rather than a NaN sentinel.
Each encoding rule was mutation-tested — collapsing absent→null, null→0, last-alias-wins, dropping the unprocessed-write-in counter, and both block-indexing off-by-ones each fail the suite.
Worth a reviewer's attention
streamVotesByElectionIDdeliberately matchesgetBallotsByElectionID's filter (head only, any status) rather than the submitted-only filter used bystreamSubmittedBallotsByElectionID. Narrowing it here would silently change election results.shuffleCandidatesForRandomTiebreaksortscandidatesin place, and an earlier revision read the store's mark→candidate mapping off that array — the two calls were in the only order that works, and swapping them misattributed every ballot's marks with the entire suite still green.candidateIdsis now immune to that.anonymizedBallotsalready holds one across a whole HTTP response, but it's a new way for concurrent results requests to contend.getWriteInNamesControllerstill loads all ballots verbosely. It's admin-only and needswrite_in_name, which the narrow select drops, so it wants its own query — left out of scope.🤖 Generated with Claude Code