Optional: validate one file's lines across all cores - #27
Open
dionmcm wants to merge 1 commit into
Open
Conversation
dionmcm
force-pushed
the
upstream/structural-split-files
branch
from
August 31, 2026 00:36
469ee6c to
25d0dd2
Compare
OPTIONAL. This buys 1.38x on an 8-core host for a meaningful amount of concurrency machinery in a hot path, and the judgement of whether that trade is worth taking is yours. Raised separately from the previous PR for exactly that reason - that one is 2.86x with none of this risk. After the previous PR the phase runs at about 2.2 of 8 cores, because work is parallel across FILES and RF2 file sizes are extremely skewed: the largest of 76 files is 20% of all bytes. Once the small files are done, two or three threads grind through the giants while the rest idle, so the largest single file sets the floor. A zip entry cannot be seeked, so byte ranges are not available. Instead each file keeps ONE sequential reader that hands batches of 2,000 lines to a worker pool, so a single file's lines use every core. Measured on the same host and release: ColumnPatternTester 31.8s -> 21.7s structural phase 38.5s -> 27.8s 1.38x Both runs processed 75 files and 45,311,214 lines. What the complexity actually is, so it can be weighed: * Two pools. The outer, unbounded, holds one task per file and mostly reads and waits; the inner is fixed at the core count and validates. Nothing on the inner pool submits further work, so an outer task waiting on inner tasks cannot starve them - the deadlock a single shared pool would risk. * A semaphore caps lines held in memory across all files at once, so a reader cannot outrun the validators and fill the heap with a 1GB file. * The header is now processed separately and before any data batch, because populateExtendedRefsetAdditionalFieldNames REDEFINES the field list that every data line is validated against. That ordering is correctness-critical and is easy for a later edit to break; it did not matter when everything was sequential. * Line numbers are carried into each batch rather than counted as lines are read. * Report rows for one file are no longer produced in line order. They carry line numbers, so anything sorting by them is unaffected, but anything relying on arrival order would notice. Suite: 233 tests run, 0 failures, 142 errors, 21 skipped - identical to the previous PR and the same 142 as clean develop. Worth knowing before deciding: the gain scales with the cores left idle by the tail, so 1.38x on eight cores would be considerably more on a 32 or 64-core node and considerably less on four. If RVF workers are sized at 4-8 cores - which is what our measurements recommend - this is close to its least favourable case.
dionmcm
force-pushed
the
upstream/structural-split-files
branch
from
August 31, 2026 00:48
25d0dd2 to
0f19305
Compare
dionmcm
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Optional. 1.38x for a meaningful amount of concurrency in a hot path. Raised separately from #26 so that trade is a separate decision; #26 is 2.86x without any of this.
Stacked on #26. One file, +122/-27.
Problem
After #26 the structural phase runs at about 2.2 of 8 cores. Work is parallel across files, and file sizes are skewed — the largest of 76 files is 20% of all bytes. Once the small files finish, two or three threads grind through the giants, so the largest file sets the floor.
A zip entry cannot be seeked, so byte ranges are not available. Each file keeps one sequential reader that hands batches of 2,000 lines to a worker pool, so one file's lines use every core.
Measured
Same host, same release, 8 cores:
Both runs processed 75 files and 45,311,214 lines. Cumulatively from upstream: 110.3s to 27.8s.
Cost
populateExtendedRefsetAdditionalFieldNamesredefines the field list every data line is validated against. Correctness-critical ordering that did not exist when the loop was sequential.Verification
Case against
The gain scales with cores left idle by the tail, so 1.38x on eight cores would be more on a 32 or 64-core node and less on four. Our sizing measurements recommend RVF workers at 4-8 cores, which is close to this change's least favourable case. #26 is worth merging regardless of what happens to this one.