perf: the files of a run are written over several threads - #62
Merged
Conversation
Writing the files is what a run is. After planning stopped encoding the picture twice, planning a run of 300 PNGs is 51 ms and writing it is 2741 ms, so the write loop was all that was left to parallelise. Measured end to end on built binaries, variants interleaved and their order reversed between repetitions. 240 png files of 200 kB go from 2.03 s to 0.44 s, which is 4.62x. zip 2.88x, docx 2.03x, and two thousand txt files of 4 kB 1.39x - files that small spend their time in what a run does once, and that stays serial. One file of any size is unchanged, and the measurement says so rather than the reasoning: the two ranges overlap there. The bytes do not move, and that was checked rather than argued. All 23 formats compared between the two binaries at ten combinations of size and seed each, 48 malformed recipes through two commands character for character with their exit codes, and the manifest of the same run identical block for block including the order. Everything that runs beside anything else is in one new file, listed in the concurrency gate with its reason, and the race detector is run for it. The whole suite under -race finds zero races. Two things a person can observe. OnProgress no longer promises which goroutine it is called from. It promises never two at once, which the engine gives with a lock. Both callers move unguarded state on the strength of that contract and neither needed a line of change. The lock was priced on the most talkative generator in the tree rather than assumed to be free: 319 840 callbacks in one run through one mutex, ranges overlap, no cost claimed. A run stopped part way now names every file that finished, which can leave a gap where a writer was cut off. Recording only the leading run of them would leave a finished file with no manifest entry, and untouchable rule 7 makes that a file nothing here can remove. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Closes the second half of
P1from the performance report, which was the last open item in it.Why here rather than in planning
The report's own costing said to parallelise
Plan, "because that is where all the cost is". That stopped being true when planning stopped encoding the picture twice: planning a run of 300 PNGs is 51 ms and writing it is 2741 ms, so the write loop is 98% of a run and the plan is 2%.Measured, on built binaries
tools/probes/abgen.py, variants interleaved with their order reversed between repetitions, exit code checked on every run, five repetitions each.pngof 200 kBzipof 2 MBdocxof 200 kBtxtof 4 kBpngof 20 MBThe last row is the control and it is meant to overlap. A single file cannot be written beside itself, so the change has to cost nothing there, and it does not.
txtgains least and that is the honest shape of this change rather than a defect in it: at 4 kB a file, most of a run is what happens once, and that stays serial.What was asked before writing any of it
The shared heap was the thing that could have invalidated the whole item, because the earlier headroom measurement used separate processes which have a heap each. A new probe,
tools/probes/writeparallel, wrote a real planned run with N goroutines in one process:-raceand looped over all 24 formats, eight goroutines each: zero races, which is what says the generators are safe beside each other at all.It also disproved a note left beside the older probe:
zipdoes not plateau at four because of the disk. That plateau belonged to eight separate processes each paying its own startup.Two things a person can observe
OnProgressno longer promises which goroutine calls it. It promises never two at once, which the engine gives with a lock. Both callers - the command line bar and the window's throttle - move unguarded state on the strength of that contract, and the lock gives them the same happens-before a single goroutine did, so neither needed a line of change. The lock is taken once per write inside a file, so it was priced rather than assumed: ongif, the most talkative generator in the tree at one call per 260 bytes, 319 840 callbacks in one run through one mutex and the timings with and without it overlap.A run stopped part way now names every file that finished, which can leave a gap where a writer was cut off. The sequential loop could only ever leave a leading run of files. Recording only that leading run would leave a finished file with no manifest entry - and untouchable rule 7 makes that a file no command here can remove, and one
verifyreports for good.Nothing else moved, and it was checked
bytesweep.py, two binaries, ten size and seed combinations per formatrefusalcorpus.py, 48 malformed recipes through two commandspngfilesandsummaryblocks identical, same order-race,-timeout 40mGuards
Three new or reshaped, each proven by a mutation. One older guard - the one that pinned the old progress contract - went red on its own on the first parallel build, naming six goroutines, which is exactly what its comment said it would do.
The mutation runner also found a hole in one of the new guards: it planned 400 files of 4 kB, which every writer finishes in one pass, so no writer was ever cut off half way and the gap it exists to describe never occurred. The plan is now built to guarantee the gap, and the guard asserts it reached it.
One mutation was withdrawn rather than fixed, and that is a result too: taking the cancellation check out of the worker loop cannot be caught, because every generator takes the context and refuses on it as well. The reason is written beside that line.
Merging
.github/workflows/ci.yml, sogh pr mergewill refuse - the merge has to happen from the browser.🤖 Generated with Claude Code