From 9ce6fb9de7591c4f1a244eaa945e92027e80c58d Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Sat, 5 Sep 2026 22:59:33 +0200 Subject: [PATCH 1/2] perf: one listing instead of two questions a file, and one collection instead of two Three findings from the performance report, and two of them came back with the report's own suggestion measured and refused. preflight asked the filesystem twice for every planned file. It reads the output directory once. A run of 100 000 files with --dry-run goes from 15.7-19.7 s to 0.49-0.56 s, order alternated - the check was 97% of it. Ten thousand names measured on their own: 1.937 s of stat calls against 8.9 ms for one listing. That is also a fix rather than only a speedup, and the guard for it says so. os.Stat follows a link, so a link pointing at nothing answered "no name here" and the run replaced it without a word. A directory ENTRY is what a taken name is, whatever it points at. With the old question put back, the new guard reports that the run went ahead over a name somebody else's link was holding. A directory that cannot be LISTED is still asked about file by file. Both systems allow write permission without read, a run into such a directory has always worked, and reading nothing there and calling it empty would let the run write over what is inside. That fallback has its own guard, which skips on Windows because denying a listing there needs an ACL. The plan ceiling forced a collection to take every reading, so a run of one kilobyte paid for two of them - measured with GODEBUG=gctrace=1, exactly two on every run however small. It asks /gc/heap/allocs:bytes first, at 251 ns against 519 us, and only collects when that says it might be over. The shortcut is sound by an inequality rather than by an estimate: the live heap cannot have grown by more than has been allocated. The report asked for /gc/heap/live:bytes and that metric is WRONG here. It reports the heap as of the last collection, and measured on 2026-09-05 all four existing ceiling guards stay green with it, because 25 MB of allocation makes the collector run on its own and the lagging reading catches up by luck. With the collector switched off the luck goes: a plan six times the ceiling is accepted. The new guard turns the collector off for exactly that reason. hashFile reads in 256 KiB pieces. The report asked for a 1 MB buffer through io.CopyBuffer and that does nothing at all: os.File implements io.WriterTo, so CopyBuffer hands it the whole job and throws the buffer away - 128 KiB, 256 KiB and 1 MiB with a plain file all take the same 167-172 ms that io.Copy takes. Hidden behind a reader that offers only Read, 256 KiB takes 161 ms against 199. The size is measured too: 64 KiB is 182 ms and nothing above a quarter of a megabyte can be told apart, so sixteen workers cost four megabytes. planChildren is sized up front, since the total is known from the groups. TotalBytes being walked twice is NOT done, and that is a measurement rather than an oversight: one walk over 100 000 planned files has a median of 0 s and a maximum of 541 us, so two of them cost half a millisecond of a nineteen second run. Widening a signature for that would be a change nothing can see. engine.go went past the length ceiling, and the guard asks for a split by what the parts do rather than for a bigger number - so preflight and the questions it asks about names are their own file now. Two ceilings came down with it. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 21 +++ internal/audit/audit.go | 42 +++++- internal/audit/cleanup.go | 8 +- internal/audit/parallel.go | 19 ++- internal/engine/engine.go | 122 ------------------ internal/engine/planmemory.go | 107 ++++++++++++--- internal/engine/preflight.go | 207 ++++++++++++++++++++++++++++++ internal/format/zip/children.go | 10 +- internal/guard/branching_test.go | 4 +- internal/guard/codeshape_test.go | 4 +- internal/guard/planmemory_test.go | 48 +++++++ internal/guard/safety_test.go | 115 +++++++++++++++++ 12 files changed, 549 insertions(+), 158 deletions(-) create mode 100644 internal/engine/preflight.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 72b7aeb..64e687d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,27 @@ because it turns other people's test suites red. Small runs are unaffected either way. A handful of files was already instant and still is. +- **Starting a run of many files no longer spends most of its time deciding + whether it may start.** Before writing anything, `tfg` checks that it is not + about to write over something of yours. That check asked the filesystem two + questions for every file it planned. It reads the folder once instead. + + Measured on a run of 100 000 files: `--dry-run` went from 15.7-19.7 seconds to + 0.49-0.56 seconds. The check was 97% of it. + +- **A name held by a shortcut that points at nothing now counts as taken.** The + check followed the shortcut, found nothing at the other end, and replaced it + without a word. It asks what the folder holds now, so anything already sitting + under a name your run wants is refused, whatever it points at. + +### Fixed + +- **`verify` and `cleanup` read each file in larger pieces**, which takes about + a fifth off the time spent hashing. + +- **Every run used to pause twice to tidy memory**, however small it was. It + pauses once. Nothing about what a run produces changes. + ## [0.3.0-rc1] - 2026-09-03 ### Breaking diff --git a/internal/audit/audit.go b/internal/audit/audit.go index 78a36cd..27fca96 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -219,8 +219,8 @@ func Verify(ctx context.Context, dir string, m *manifest.Manifest, skip string) return nil, err } - found, stopped := inOrder(ctx, len(claimed), func(i int) Difference { - return compare(claimed[i], full[i]) + found, stopped := inOrder(ctx, len(claimed), func(i int, scratch []byte) Difference { + return compare(claimed[i], full[i], scratch) }) for _, d := range found { if d.Kind != "" { @@ -392,7 +392,7 @@ func claimedPaths(b core.Boundary, files []manifest.File) ([]string, error) { // agreement on its own. No sentinel anybody has to remember, and no pointer // per file - a run of ten thousand would allocate ten thousand of them to say // "nothing to report" ten thousand times. -func compare(f manifest.File, full string) Difference { +func compare(f manifest.File, full string, scratch []byte) Difference { info, statErr := os.Stat(full) if statErr != nil { return Difference{Kind: Missing, Path: f.Path} @@ -408,7 +408,7 @@ func compare(f manifest.File, full string) Difference { } } - sum, hashErr := hashFile(full) + sum, hashErr := hashFile(full, scratch) if hashErr != nil { return Difference{Kind: Unreadable, Path: f.Path, Got: hashErr.Error()} } @@ -422,18 +422,48 @@ func compare(f manifest.File, full string) Difference { return Difference{} } +// hashScratch is how much of a file is read at a time. +// +// Measured 2026-09-05 on a 256 MB file, median of five interleaved runs: +// io.Copy 199 ms, 64 KiB 182 ms, 256 KiB 161 ms, 1 MiB 162 ms, 4 MiB 160 ms. +// The whole win is in by a quarter of a megabyte and nothing above it can be +// told apart, so this is the size that buys it without asking every worker for +// a megabyte. +const hashScratch = 256 << 10 + +// onlyRead hides everything but Read from io.CopyBuffer. +// +// This is not a wrapper for its own sake and without it the buffer below does +// nothing at all. os.File implements io.WriterTo, so io.CopyBuffer hands the +// whole job to the file and THROWS THE BUFFER AWAY - measured on 2026-09-05, +// passing 128 KiB, 256 KiB or 1 MiB with a plain file on the other end all take +// the same 167-172 ms that io.Copy takes, because all four are the same 32 KiB +// path inside. Hidden behind this, a 256 KiB buffer takes 150-161 ms. +// +// The report that suggested the buffer said it would "cut the syscall count 32 +// fold" and would have changed nothing as written. That is the whole reason +// this type exists rather than a comment saying the buffer is bigger. +type onlyRead struct{ io.Reader } + // hashFile streams the file rather than reading it in. A run of this tool // produces files measured in gigabytes, and verify has to survive its own // output. -func hashFile(path string) (string, error) { +// +// scratch belongs to the caller and is reused across every file that caller +// answers for. It may be nil, and then this asks for one of its own - which is +// what a sequential caller wants and what every test that reaches here gets. +func hashFile(path string, scratch []byte) (string, error) { f, err := os.Open(path) if err != nil { return "", err } defer func() { _ = f.Close() }() + if scratch == nil { + scratch = make([]byte, hashScratch) + } h := sha256.New() - if _, err := io.Copy(h, f); err != nil { + if _, err := io.CopyBuffer(h, onlyRead{f}, scratch); err != nil { return "", err } return hex.EncodeToString(h.Sum(nil)), nil diff --git a/internal/audit/cleanup.go b/internal/audit/cleanup.go index 84249e6..7e659db 100644 --- a/internal/audit/cleanup.go +++ b/internal/audit/cleanup.go @@ -79,14 +79,14 @@ func Inspect(ctx context.Context, dir string, m *manifest.Manifest) ([]Candidate // In order, because this list is what cleanup removes from and what it // printed to a person beforehand. - return inOrder(ctx, len(claimed), func(i int) Candidate { - return look(claimed[i], full[i]) + return inOrder(ctx, len(claimed), func(i int, scratch []byte) Candidate { + return look(claimed[i], full[i], scratch) }) } // look is what one claimed file comes to for cleanup: whether it may be // removed, and if not, why not in the words a person needs. -func look(f manifest.File, full string) Candidate { +func look(f manifest.File, full string, scratch []byte) Candidate { info, err := os.Stat(full) if errors.Is(err, fs.ErrNotExist) { return Candidate{Path: f.Path, Disposition: Absent} @@ -101,7 +101,7 @@ func look(f manifest.File, full string) Candidate { return Candidate{Path: f.Path, Disposition: Changed, Detail: fmt.Sprintf("it is %d B and the manifest recorded %d B", info.Size(), f.Bytes)} } - sum, err := hashFile(full) + sum, err := hashFile(full, scratch) if err != nil { return Candidate{Path: f.Path, Disposition: Unreachable, Detail: err.Error()} } diff --git a/internal/audit/parallel.go b/internal/audit/parallel.go index e0c8726..ea264b4 100644 --- a/internal/audit/parallel.go +++ b/internal/audit/parallel.go @@ -75,7 +75,7 @@ func widthFor(n int) int { // own sake: if a refusal could arrive here, stopping the other goroutines // would mean a LOWER index never got asked, and the same manifest would // name a different file on different days. -func inOrder[T any](ctx context.Context, n int, one func(i int) T) ([]T, error) { +func inOrder[T any](ctx context.Context, n int, one func(i int, scratch []byte) T) ([]T, error) { out := make([]T, n) done := make([]bool, n) @@ -111,7 +111,20 @@ func inOrder[T any](ctx context.Context, n int, one func(i int) T) ([]T, error) // out and done are written at indices no other goroutine will take, because // the counter hands each index out exactly once. They are the only things // written at all. -func drain[T any](ctx context.Context, next *atomic.Int64, out []T, done []bool, one func(i int) T) { +func drain[T any](ctx context.Context, next *atomic.Int64, out []T, done []bool, one func(i int, scratch []byte) T) { + // One buffer per goroutine, handed to every item that goroutine answers. + // + // Per worker rather than per item, because a run of a hundred thousand + // files would otherwise allocate a hundred thousand of these. And per + // worker rather than one shared, because two goroutines reading files into + // the same bytes is the data race this whole file is written to avoid. + // + // The size is measured rather than picked. Hashing a 256 MB file, median of + // five: io.Copy 199 ms, 64 KiB 182 ms, 256 KiB 161 ms, 1 MiB 162 ms, 4 MiB + // 160 ms. The win is all in by a quarter of a megabyte and nothing above it + // is distinguishable, so sixteen workers cost four megabytes rather than + // sixty four. + scratch := make([]byte, hashScratch) for { i := int(next.Add(1)) - 1 // Cancellation is asked per item rather than per pass, so a Ctrl+C @@ -120,7 +133,7 @@ func drain[T any](ctx context.Context, next *atomic.Int64, out []T, done []bool, if i >= len(out) || ctx.Err() != nil { return } - out[i] = one(i) + out[i] = one(i, scratch) done[i] = true } } diff --git a/internal/engine/engine.go b/internal/engine/engine.go index f449a0e..4c557e9 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -433,128 +433,6 @@ type Progress struct { BytesTotal int64 } -// DefaultManifestName is where the manifest lands when nothing says otherwise. -// -// It lives here rather than in the caller because the engine has to know the -// name to protect it from being written over, and two copies of a file name -// are two things to keep in step. -const DefaultManifestName = "manifest.json" - -// preflight answers whether this run may start, without writing anything. -// -// Both checks used to sit after the dry run had already returned, so -// --dry-run reported success for runs that would refuse to start. -func preflight(ctx context.Context, files []PlannedFile, opt Options) error { - // Free space first. Finding out at file five thousand of ten thousand - // leaves a half written set and a full disk on a machine somebody works on. - needed := TotalBytes(files) - if available, err := opt.availableBytes(opt.OutDir); err == nil { - if available < needed { - return &SpaceError{Needed: needed, Available: available, Path: opt.OutDir} - } - } - // A failure to read the free space is not a reason to refuse. A disk we - // cannot measure is not the same as a disk that is full. - - // Pointing --out at a file rather than a directory is a mistake somebody - // makes, and it used to arrive as two messages about one fault, the first - // of them saying "there is nothing at that path" about a path that has - // something at it. The system reports ENOTDIR and our mapping only knew - // "missing", "no permission" and "already there". - if info, err := os.Stat(opt.OutDir); err == nil && !info.IsDir() { - return &RecipeError{Setting: SettingOutDir, - Detail: fmt.Sprintf("the output directory %s is a file, not a directory", opt.OutDir), - Remedy: "Point the output directory at a directory, or at one that does not exist yet and it will be created"} - } - - // The manifest is checked with the files it would describe, and leaving it - // out cost exactly what it protects. A second run into the same directory - // wrote a fresh manifest over the old one, so every file the old one listed - // stopped being anybody's - cleanup reported nothing to remove and those - // files could never be cleaned up by this tool again. It happened on a - // successful run as readily as on a refused one, and on the successful one - // it happened in silence. - if path := ManifestPath(opt); exists(path) { - return &CollisionError{Path: path, Manifest: true} - } - - // Nothing else is written over either. This tool runs in directories that - // belong to the user, so destroying their work is the one failure that - // cannot be undone by running again. - // - // The temporary name is checked as well as the final one. It used to be - // created with os.Create, which truncates, so a file already sitting under - // that name lost its contents without a word while the collision check - // looked only at the name the file ends up with. Asking the filesystem - // instead, with O_EXCL at the write, was tried on 2026-08-25 and taken - // back out - see writeOne for what it costs on Windows. So this check is - // the whole of the protection rather than the friendlier half of it. - // - // Which run leaves such a file behind is worth being accurate about, - // because the comment here used to get it wrong. The name carries the - // process id, so a run killed outright leaves one that no later run can - // meet: the next run builds a different name and walks past it. Nor is it - // lost - verify names it as one of ours rather than as something nobody - // asked for, and cleanup leaves it alone because untouchable rule 7 makes - // the manifest the whole authority over what may be deleted, and a file - // that never finished never reached one. - for _, f := range files { - // Two questions of the filesystem per planned file, so a large run on a - // slow share spends real time in here. Until 2026-08-26 that time could - // not be interrupted: preflight took no context, so a preview had - // nothing to cancel, the window offered no button for it, and closing - // the window waited for the whole loop on the interface thread. The - // same reasoning that put a context into the directory walk in - // internal/audit on 2026-08-25. - if err := ctx.Err(); err != nil { - return err - } - if path := filepath.Join(opt.OutDir, f.Name); exists(path) { - return &CollisionError{Path: path} - } - if path := tempPathFor(opt.OutDir, f.Name); exists(path) { - return &CollisionError{Path: path} - } - } - return nil -} - -// tempPathFor is the name a file is written under before it is renamed into -// place. One definition, used by the check above and by the write below, so -// the two cannot disagree about what is being protected. -func tempPathFor(outDir, name string) string { - return fmt.Sprintf("%s%s%d", filepath.Join(outDir, name), core.PartialMarker, os.Getpid()) -} - -func exists(path string) bool { - _, err := os.Stat(path) - return err == nil -} - -// manifestNameOf is where this run's record lands. One definition, because the -// preflight check, the claim and the save all have to mean the same file. -func manifestNameOf(opt Options) string { - if opt.ManifestName == "" { - return DefaultManifestName - } - return opt.ManifestName -} - -// ManifestPath is the file this run's record lands in, for the callers that -// save it. The engine claims that name before the first file and refuses the -// run if it is taken, so a saver joining the path its own way is answering a -// question this package has already answered. -// -// Exported on 2026-08-27 because both savers did join it their own way. The -// window's did not handle an empty name at all, so a caller that left it blank -// would have asked the manifest to be written to the output directory itself - -// a rename of a file onto a directory. Nothing reached that, because all three -// screens fill the field in, and "nothing reaches it today" is the description -// of a fault waiting for a fourth screen rather than of a safe piece of code. -func ManifestPath(opt Options) string { - return filepath.Join(opt.OutDir, manifestNameOf(opt)) -} - // Run writes a planned set of files. // // Each file is written under a temporary name and only then renamed, so the diff --git a/internal/engine/planmemory.go b/internal/engine/planmemory.go index 54c2387..e9afd64 100644 --- a/internal/engine/planmemory.go +++ b/internal/engine/planmemory.go @@ -3,6 +3,7 @@ package engine import ( "fmt" "runtime" + "runtime/metrics" "github.com/donislawdev/TestingFilesGenerator/internal/core" ) @@ -61,9 +62,19 @@ const ( // not nought, and this is where it lives. type planMemory struct { baseline uint64 - seen int - nextAt int - ceiling uint64 + // allocsBaseline is the running total of bytes this process has ever + // allocated, taken beside the baseline above. + // + // It is what makes the cheap question in account possible, and the reason + // it works is an inequality rather than an estimate: every byte the live + // heap grows by has to have been allocated, so the growth this guard cares + // about can never exceed the total allocated since the baseline. Under the + // ceiling on that total means under the ceiling full stop, and no + // collection has to be paid for to find out. + allocsBaseline uint64 + seen int + nextAt int + ceiling uint64 } // newPlanMemory takes the reference point, and it is taken here because here @@ -108,17 +119,51 @@ func newPlanMemory(ceiling int64) *planMemory { if ceiling <= 0 { ceiling = core.MaxPlanBytes } - return &planMemory{baseline: heapInUse(), nextAt: 1, ceiling: uint64(ceiling)} + return &planMemory{ + baseline: heapInUse(), + allocsBaseline: allocatedSoFar(), + nextAt: 1, + ceiling: uint64(ceiling), + } } // account is called once per planned file. It returns an error only when the // plan has already passed the ceiling. +// +// The cheap question is asked first and it is the one nearly every run gets to +// stop at. Until 2026-09-05 there was no cheap question: every reading was a +// forced collection, so a run of one file of one kilobyte paid for TWO of them +// - measured with GODEBUG=gctrace=1, exactly two on every run however small. +// The cost is 519 us against 251 ns for the counter, and it grows with the live +// heap, which is precisely the run this guard exists for. +// +// What the counter cannot do is answer the question this refuses on. It counts +// everything ever allocated, including what has already been thrown away, so it +// says "no" with certainty and "maybe" otherwise. The certain half is enough, +// because it is the half every real run lands in: MaxPlanBytes is two +// gigabytes and planning ten thousand files allocates tens of megabytes. +// +// /gc/heap/live:bytes was the obvious other candidate and it is the wrong one. +// It reports the live heap as of the LAST COLLECTION, so a burst of allocation +// between two collections is invisible to it - which would turn a refusal into +// silence exactly when the plan is growing fastest. The counter has no such +// gap. func (p *planMemory) account(targetIndex, filesSoFar int) error { p.seen++ if p.seen < p.nextAt { return nil } + if allocated := allocatedSoFar() - p.allocsBaseline; allocated <= p.ceiling { + // Bring the next reading forward to land inside the headroom that is + // left, for the reason written out below - and on this figure rather + // than on the swept one, which makes the step SHORTER than it needs to + // be rather than longer, since what has been allocated is never less + // than what is still held. + p.nextAt = p.seen + stepFor(p.ceiling, allocated, p.seen) + return nil + } + grown := heapInUse() if grown <= p.baseline { // The collector handed memory back between the two readings. That is @@ -130,22 +175,7 @@ func (p *planMemory) account(targetIndex, filesSoFar int) error { } used := grown - p.baseline if used <= p.ceiling { - // Bring the next reading forward to land inside the headroom that is - // left, so an expensive file cannot carry the plan far past the - // ceiling between two readings. Half the headroom rather than all of - // it, because the average is an average - a run whose later files are - // dearer than its earlier ones would otherwise step straight over. - perFile := used / uint64(p.seen) - step := planCheckEvery - if perFile > 0 { - if room := int((p.ceiling - used) / perFile / 2); room < step { - step = room - } - } - if step < 1 { - step = 1 - } - p.nextAt = p.seen + step + p.nextAt = p.seen + stepFor(p.ceiling, used, p.seen) return nil } @@ -160,6 +190,43 @@ func (p *planMemory) account(targetIndex, filesSoFar int) error { } } +// stepFor is how many more files may go by before the next reading. +// +// It brings the reading forward to land inside the headroom that is left, so +// an expensive file cannot carry the plan far past the ceiling between two of +// them. Half the headroom rather than all of it, because the average is an +// average - a run whose later files are dearer than its earlier ones would +// otherwise step straight over. +// +// One function rather than the same arithmetic twice, since 2026-09-05 there +// are two readings it has to serve: the cheap counter and the swept heap. They +// disagree about the number they are given and agree about what to do with it. +func stepFor(ceiling, used uint64, seen int) int { + step := planCheckEvery + if perFile := used / uint64(seen); perFile > 0 { + if room := int((ceiling - used) / perFile / 2); room < step { + step = room + } + } + if step < 1 { + step = 1 + } + return step +} + +// allocatedSoFar is every byte this process has ever allocated, thrown away or +// not. +// +// Cumulative and exact - unlike the live heap, it does not wait for a +// collection to catch up, so a burst of allocation is visible the moment it +// happens. Measured 2026-09-05: 251 ns a call against 519 us for a forced +// collection and its reading. +func allocatedSoFar() uint64 { + sample := [1]metrics.Sample{{Name: "/gc/heap/allocs:bytes"}} + metrics.Read(sample[:]) + return sample[0].Value.Uint64() +} + // heapInUse is the live heap, after a collection so that the number is about // what the plan is holding rather than about what has not been swept yet. // diff --git a/internal/engine/preflight.go b/internal/engine/preflight.go new file mode 100644 index 0000000..ca85e4a --- /dev/null +++ b/internal/engine/preflight.go @@ -0,0 +1,207 @@ +package engine + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" +) + +// What has to be true before a run may start, and where the things it writes +// land. +// +// Split out of engine.go on 2026-09-05, when reading the output directory once +// instead of asking about every planned file twice took that file past the +// length ceiling. The guard's own message asks for a split by what the parts +// do rather than for a bigger number, and this is that part: no bytes are +// written from here, every function answers a question about a name. + +// DefaultManifestName is where the manifest lands when nothing says otherwise. +// +// It lives here rather than in the caller because the engine has to know the +// name to protect it from being written over, and two copies of a file name +// are two things to keep in step. +const DefaultManifestName = "manifest.json" + +// preflight answers whether this run may start, without writing anything. +// +// Both checks used to sit after the dry run had already returned, so +// --dry-run reported success for runs that would refuse to start. +func preflight(ctx context.Context, files []PlannedFile, opt Options) error { + // Free space first. Finding out at file five thousand of ten thousand + // leaves a half written set and a full disk on a machine somebody works on. + needed := TotalBytes(files) + if available, err := opt.availableBytes(opt.OutDir); err == nil { + if available < needed { + return &SpaceError{Needed: needed, Available: available, Path: opt.OutDir} + } + } + // A failure to read the free space is not a reason to refuse. A disk we + // cannot measure is not the same as a disk that is full. + + // Pointing --out at a file rather than a directory is a mistake somebody + // makes, and it used to arrive as two messages about one fault, the first + // of them saying "there is nothing at that path" about a path that has + // something at it. The system reports ENOTDIR and our mapping only knew + // "missing", "no permission" and "already there". + if info, err := os.Stat(opt.OutDir); err == nil && !info.IsDir() { + return &RecipeError{Setting: SettingOutDir, + Detail: fmt.Sprintf("the output directory %s is a file, not a directory", opt.OutDir), + Remedy: "Point the output directory at a directory, or at one that does not exist yet and it will be created"} + } + + // The manifest is checked with the files it would describe, and leaving it + // out cost exactly what it protects. A second run into the same directory + // wrote a fresh manifest over the old one, so every file the old one listed + // stopped being anybody's - cleanup reported nothing to remove and those + // files could never be cleaned up by this tool again. It happened on a + // successful run as readily as on a refused one, and on the successful one + // it happened in silence. + if path := ManifestPath(opt); exists(path) { + return &CollisionError{Path: path, Manifest: true} + } + + // Nothing else is written over either. This tool runs in directories that + // belong to the user, so destroying their work is the one failure that + // cannot be undone by running again. + // + // The temporary name is checked as well as the final one. It used to be + // created with os.Create, which truncates, so a file already sitting under + // that name lost its contents without a word while the collision check + // looked only at the name the file ends up with. Asking the filesystem + // instead, with O_EXCL at the write, was tried on 2026-08-25 and taken + // back out - see writeOne for what it costs on Windows. So this check is + // the whole of the protection rather than the friendlier half of it. + // + // Which run leaves such a file behind is worth being accurate about, + // because the comment here used to get it wrong. The name carries the + // process id, so a run killed outright leaves one that no later run can + // meet: the next run builds a different name and walks past it. Nor is it + // lost - verify names it as one of ours rather than as something nobody + // asked for, and cleanup leaves it alone because untouchable rule 7 makes + // the manifest the whole authority over what may be deleted, and a file + // that never finished never reached one. + // One listing of the directory rather than two questions of the filesystem + // for every planned file. + // + // The comment that stood here named the cost - "a large run on a slow share + // spends real time in here" - and until 2026-08-26 that time could not even + // be interrupted, which is why the context arrived. Measured on 2026-09-05, + // ten thousand planned names with half of them present: 1.937 s of stat + // calls against 8.9 ms for one listing, ranges disjoint over eight + // repetitions with a steady canary. That is a local disk. The share the old + // comment was written for pays a round trip for each of those twenty + // thousand questions. + // + // Names cannot hold a separator - checkFileName refuses both on every + // system - so one listing of the output directory covers every planned file + // and every temporary name beside it. + taken := namesIn(opt.OutDir) + + for _, f := range files { + // Asked per file rather than once, so a preview of a hundred thousand + // files can still be cancelled. That reasoning is unchanged by the + // listing above, it is only much cheaper to be interrupted now. + if err := ctx.Err(); err != nil { + return err + } + if path := filepath.Join(opt.OutDir, f.Name); isTaken(taken, path, f.Name) { + return &CollisionError{Path: path} + } + if path := tempPathFor(opt.OutDir, f.Name); isTaken(taken, path, tempNameFor(f.Name)) { + return &CollisionError{Path: path} + } + } + return nil +} + +// namesIn is every name the output directory holds, or nil when it could not be +// listed at all. +// +// nil is not the same answer as empty, and the difference is the one failure +// this check exists to prevent. A directory that cannot be READ can still be +// one a file may be created in - both systems allow that combination - and +// treating it as empty would let a run write over somebody's work. So nil means +// "ask file by file" rather than "there is nothing there". +// +// A directory that does not exist yet is a different answer and a correct one: +// it holds no names, nothing can collide, and that is the ordinary case for a +// fresh run. +func namesIn(dir string) map[string]struct{} { + entries, err := os.ReadDir(dir) + if errors.Is(err, fs.ErrNotExist) { + return map[string]struct{}{} + } + if err != nil { + return nil + } + taken := make(map[string]struct{}, len(entries)) + for _, e := range entries { + taken[e.Name()] = struct{}{} + } + return taken +} + +// isTaken answers whether a name is already in use - from the listing when +// there is one, and from the filesystem when there is not. +// +// The two are not quite the same question and the listing asks the better one. +// os.Stat follows a link, so a link pointing at nothing answered "no name +// here" and the run went on to replace it. A directory ENTRY is what a name +// being taken means, whatever it points at, and the rule this serves is that +// nothing already there is written over. So the listing refuses a little more +// than the stat did, in the direction that cannot lose somebody's work. +func isTaken(taken map[string]struct{}, path, name string) bool { + if taken == nil { + return exists(path) + } + _, ok := taken[name] + return ok +} + +// tempPathFor is the name a file is written under before it is renamed into +// place. One definition, used by the check above and by the write below, so +// the two cannot disagree about what is being protected. +func tempPathFor(outDir, name string) string { + return filepath.Join(outDir, tempNameFor(name)) +} + +// tempNameFor is that name without the directory in front of it, which is what +// a directory listing gives back. Split out so the listing and the path cannot +// disagree about how the temporary name is spelt. +func tempNameFor(name string) string { + return fmt.Sprintf("%s%s%d", name, core.PartialMarker, os.Getpid()) +} + +func exists(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +// manifestNameOf is where this run's record lands. One definition, because the +// preflight check, the claim and the save all have to mean the same file. +func manifestNameOf(opt Options) string { + if opt.ManifestName == "" { + return DefaultManifestName + } + return opt.ManifestName +} + +// ManifestPath is the file this run's record lands in, for the callers that +// save it. The engine claims that name before the first file and refuses the +// run if it is taken, so a saver joining the path its own way is answering a +// question this package has already answered. +// +// Exported on 2026-08-27 because both savers did join it their own way. The +// window's did not handle an empty name at all, so a caller that left it blank +// would have asked the manifest to be written to the output directory itself - +// a rename of a file onto a directory. Nothing reached that, because all three +// screens fill the field in, and "nothing reaches it today" is the description +// of a fault waiting for a fourth screen rather than of a safe piece of code. +func ManifestPath(opt Options) string { + return filepath.Join(opt.OutDir, manifestNameOf(opt)) +} diff --git a/internal/format/zip/children.go b/internal/format/zip/children.go index df3447c..466e155 100644 --- a/internal/format/zip/children.go +++ b/internal/format/zip/children.go @@ -24,7 +24,15 @@ import ( // seed of a member does not move when a group above it changes count. That is // untouchable rule 2 applied one level down. func planChildren(r format.Request, groups []format.Content, layout archive.Layout) ([]child, error) { - var out []child + // Sized up front, because the total is known before the walk starts: it is + // what the groups add up to. Growing by append instead reallocates and + // copies the whole slice fourteen times on the way to ten thousand entries, + // which is the ceiling this format declares. + total := 0 + for _, g := range groups { + total += g.Count + } + out := make([]child, 0, total) index := 0 // Numbering runs per format rather than per group, so two groups of the // same format do not both start at 0001 and collide inside the archive. diff --git a/internal/guard/branching_test.go b/internal/guard/branching_test.go index 7735c16..9a9fcd7 100644 --- a/internal/guard/branching_test.go +++ b/internal/guard/branching_test.go @@ -45,7 +45,9 @@ const ( // either, and one handles neither. Flattening it in TIFF alone would make // two functions that answer the same question look different, which costs // more than the depth does. - crowdedDepthFunctions = 52 + // Lowered from 52 on 2026-09-05: splitting preflight out took one function + // out of the band. The ratchet only tightens. + crowdedDepthFunctions = 51 // An axis this set does not watch. crowding() asks n >= band, so nothing // reaches it. diff --git a/internal/guard/codeshape_test.go b/internal/guard/codeshape_test.go index 2c92515..14a2c0c 100644 --- a/internal/guard/codeshape_test.go +++ b/internal/guard/codeshape_test.go @@ -39,7 +39,9 @@ const ( // how big a target was, because the budget stopped needing to be told - it // takes its reference point when it is built. A ratchet goes down when work // makes it lowerable. - longestFile = 502 + // Lowered from 502 on 2026-09-05: preflight and the questions it asks about + // names moved into their own file. The ratchet only tightens. + longestFile = 457 // Depth answers a different question than length, and it is the better // question of the two. A hundred line function that is flat reads top to diff --git a/internal/guard/planmemory_test.go b/internal/guard/planmemory_test.go index ee657aa..9e6315c 100644 --- a/internal/guard/planmemory_test.go +++ b/internal/guard/planmemory_test.go @@ -1,6 +1,7 @@ package guard import ( + "runtime/debug" "strings" "testing" @@ -207,3 +208,50 @@ func asAddressed(err error, target *interface{ AboutSetting() string }) bool { } return false } + +// The cheap question the ceiling asks first is about ALLOCATION, not about the +// last collection, and the difference only shows when the collector is idle. +// +// Since 2026-09-05 account reads /gc/heap/allocs:bytes before it forces a +// collection, because that costs 251 ns against 519 us and a run of one file +// used to pay for two collections however small it was. What makes the +// shortcut safe is an inequality rather than an estimate: the live heap cannot +// have grown by more than has been allocated, so under the ceiling on the +// total is under the ceiling full stop. +// +// /gc/heap/live:bytes was the other candidate - the report that asked for this +// named it - and it is unsound here. It reports the heap as of the LAST +// COLLECTION, so a plan growing between two of them is invisible. +// +// Measured on 2026-09-05: swapping the metric leaves all four guards above +// GREEN, because twenty five megabytes of allocation makes the collector run on +// its own and the lagging reading catches up by luck. Turning the collector off +// takes the luck away - the live reading then never moves at all, a build +// asking that question accepts a plan of any size, and this is the only thing +// here that would say so. +func TestThePlanCeilingAsksAboutAllocationRatherThanTheLastCollection(t *testing.T) { + was := debug.SetGCPercent(-1) + t.Cleanup(func() { debug.SetGCPercent(was) }) + + // The shape the short run guard uses, which is one file measured at + // 25 194 908 B of plan against a ceiling of four megabytes. + targets := []engine.Target{{ + ID: "one", + Format: "pdf", + Sizes: engine.Uniform(1, 20<<20), + Properties: map[string]string{"pages": "5000"}, + }} + + _, err := engine.Plan(targets, engine.Options{ + OutDir: "out", + ManifestName: "manifest.json", + MaxPlanBytes: 4 << 20, + }) + if err == nil { + t.Fatal("with the collector switched off a plan six times the ceiling was accepted, " + + "so the cheap reading is answering about the last collection rather than about what was allocated") + } + if !strings.Contains(err.Error(), "ceiling") { + t.Errorf("the refusal does not mention the ceiling: %s", err) + } +} diff --git a/internal/guard/safety_test.go b/internal/guard/safety_test.go index b245b6b..ef66c28 100644 --- a/internal/guard/safety_test.go +++ b/internal/guard/safety_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "strings" "testing" @@ -174,3 +175,117 @@ func TestAFreshRunIntoAnEmptyDirectoryStillWorks(t *testing.T) { t.Errorf("produced %d files, expected 3", res.Manifest.Summary.Materialized) } } + +// A name is taken by whatever holds it, including a link that points nowhere. +// +// Until 2026-09-05 preflight asked os.Stat whether the path existed, and +// os.Stat follows a link - so a link pointing at nothing answered "no name +// here" and the run replaced the entry without a word. It reads one listing of +// the directory now, and a directory ENTRY is what a taken name is, whatever +// it points at. +// +// That difference is why the change is not only about speed. Replacing a link +// somebody put there loses their work exactly as replacing a file does, and it +// happened on the quiet path rather than the loud one. +func TestANameTakenByALinkPointingNowhereIsStillTaken(t *testing.T) { + dir := t.TempDir() + dangling := filepath.Join(dir, "files_0001.txt") + if err := os.Symlink(filepath.Join(dir, "nothing-is-here"), dangling); err != nil { + t.Skipf("this system will not create a link here, so the case cannot be built: %v", err) + } + + // Asserted rather than assumed, because the whole case is a name that IS + // there and that os.Stat cannot see. A fixture that quietly resolved would + // leave this guard green about the old behaviour (O118). + if _, err := os.Stat(dangling); err == nil { + t.Fatal("the link resolves, so this is not the state being guarded") + } + if _, err := os.Lstat(dangling); err != nil { + t.Fatalf("there is no entry at all, so there is nothing for a run to collide with: %v", err) + } + + opt := engine.Options{OutDir: dir, Seed: 7741, Command: "test"} + planned, err := engine.Plan([]engine.Target{txtTarget("files", 1, 4096)}, opt) + if err != nil { + t.Fatalf("planning: %v", err) + } + + _, runErr := engine.Run(context.Background(), planned, opt) + if runErr == nil { + t.Fatal("the run went ahead over a name somebody else's link was holding") + } + var collision *engine.CollisionError + if !errors.As(runErr, &collision) { + t.Errorf("refused with %T, expected a CollisionError so the caller answers with the right exit code", runErr) + } +} + +// A directory that cannot be LISTED is still asked about every name, one at a +// time. +// +// The listing that made preflight cheap has an answer it cannot give: a +// directory with permission to write and none to read. Both systems allow that +// combination, and a run into one has always worked. Reading nothing there and +// calling it empty would let the run write over whatever is inside - the one +// failure here that running again cannot undo - so a listing that fails means +// "ask file by file" rather than "there is nothing there". +// +// Without this the fallback is a branch nothing can turn red, and this project +// has removed seven of those. +func TestADirectoryThatCannotBeListedIsStillAskedAboutEveryName(t *testing.T) { + if runtime.GOOS == "windows" { + // The same reason environment_test.go gives for the sibling of this + // guard: os.Chmod on Windows moves the read only bit and nothing else, + // and denying a listing needs an ACL, which is not something a test + // should be installing. + t.Skip("a directory that refuses a listing needs an ACL on Windows") + } + + dir := t.TempDir() + out := filepath.Join(dir, "writeonly") + if err := os.Mkdir(out, 0o755); err != nil { + t.Fatalf("making the directory: %v", err) + } + const precious = "work that took an afternoon" + victim := filepath.Join(out, "files_0001.txt") + if err := os.WriteFile(victim, []byte(precious), 0o644); err != nil { + t.Fatalf("preparing the file: %v", err) + } + if err := os.Chmod(out, 0o300); err != nil { + t.Fatalf("taking the read permission away: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(out, 0o755) }) + + // Asserted rather than assumed. Root ignores the permission bits, so on a + // container running as root this state does not exist and the guard would + // otherwise pass while testing the fast path twice. + if _, err := os.ReadDir(out); err == nil { + t.Skip("this process can list a directory with no read permission, so the fallback cannot be reached") + } + + opt := engine.Options{OutDir: out, Seed: 7741, Command: "test"} + planned, err := engine.Plan([]engine.Target{txtTarget("files", 1, 4096)}, opt) + if err != nil { + t.Fatalf("planning: %v", err) + } + + _, runErr := engine.Run(context.Background(), planned, opt) + if runErr == nil { + t.Fatal("a run into a directory it could not list went ahead over what was inside") + } + var collision *engine.CollisionError + if !errors.As(runErr, &collision) { + t.Errorf("refused with %T, expected a CollisionError", runErr) + } + + if err := os.Chmod(out, 0o755); err != nil { + t.Fatalf("putting the permission back to read the file: %v", err) + } + after, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("reading the file back: %v", err) + } + if string(after) != precious { + t.Error("the existing file was destroyed - this is the one failure that cannot be undone by running again") + } +} From 26f7ce6a1a4276406edc93f760e985c4e92384d5 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Sun, 6 Sep 2026 00:43:15 +0200 Subject: [PATCH 2/2] perf: one filler instead of thirteen, and eight bytes per draw instead of one Thirteen packages each carried their own copy of the bulk random filler loop, in four different shapes. Measured over 64 MiB through a 32 KiB buffer, interleaved with the order reversed between repetitions: one draw per byte 182 MB/s zip, targz, wav eight via a temporary array 1468 MB/s bmp, gif, ico, opc, png, tiff eight via a shift loop 845 MB/s avif, jpg, jxl, webp one store into the buffer 2499 MB/s the shape they all use now All thirteen now call core.FillRandomBE or core.FillRandomLE. Two functions rather than one with a flag, because choosing the wrong byte order is not a style mistake - it silently rewrites every file a format has ever produced, and an argument would put that one typo away. BREAKING: zip, targz and wav files have different bytes. Their padding is where those formats spend almost the whole file, so almost every byte changes. Size, structure and readability are untouched. End to end on 64 MB, ranges disjoint: zip 2.81x, targz 2.74-3.32x. wav is in that list for uniformity and not for speed, by the owner's decision after the measurement: its padding is audio modulo the frame size, so exactly two bytes of a WAV differ and the time is unchanged. The other ten packages moved with no byte change at all - the shift loop IS little endian, which the performance report did not notice, so those four collapse to one store for free. Checked across 24 formats at five sizes and two seeds: three moved, twenty identical. Also closes a blind spot the golden set had. Forcing the filler to emit a constant moved 33 of the 54 pinned cases and not one WAV: wav_32kib lands on a size the audio fills exactly and never reaches the filler, so that path had no pinned witness. wav_with_the_padding_chunk is that witness. Guard: TestBulkRandomBytesComeFromOnePlace, two mutations, both caught. It names the one honest UintN caller as an exception and fails if that exception outlives its code. A third mutation proves the golden set covers the shared filler's short tail. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 23 ++++ internal/core/fill.go | 59 +++++++++ internal/format/avif/avif.go | 11 +- internal/format/bmp/bmp.go | 6 +- internal/format/gif/gif.go | 17 +-- internal/format/ico/ico.go | 6 +- internal/format/jpg/jpg.go | 9 +- internal/format/jxl/jxl.go | 11 +- internal/format/opc/opc.go | 7 +- internal/format/png/png.go | 6 +- internal/format/targz/size.go | 4 +- internal/format/tiff/tiff.go | 6 +- internal/format/wav/wav.go | 4 +- internal/format/webp/webp.go | 14 +-- internal/format/zip/zip.go | 4 +- internal/guard/fillerprimitive_test.go | 113 ++++++++++++++++++ internal/guard/generatorbytes_test.go | 11 ++ internal/guard/testdata/generator-golden.json | 29 ++++- 18 files changed, 244 insertions(+), 96 deletions(-) create mode 100644 internal/core/fill.go create mode 100644 internal/guard/fillerprimitive_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 64e687d..843c0a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,29 @@ because it turns other people's test suites red. ## [Unreleased] +### Breaking + +- **ZIP, TAR.GZ and WAV files have different bytes.** The padding these three + formats write is now drawn from the random generator eight bytes at a time + instead of one byte at a time. For a ZIP or a TAR.GZ that padding is almost + the whole file, so almost every byte changes. + + Nothing a reader can see is different. The size is the same to the byte, the + structure is the same, the same tools open the same files. What changes is the + content of the padding, so a hash you recorded from an earlier version will + not match one you generate now. + + This is what makes large archives faster to produce. Measured on 64 MB files, + runs interleaved: ZIP 2.8 times faster, TAR.GZ 2.7 to 3.3 times faster. + + WAV is on this list for consistency rather than for speed. Its padding is at + most a few bytes per file, so only those bytes differ and the time to produce + one is unchanged. + + Every other format keeps exactly the bytes it had. Twelve of them share the + same new code and were checked against their recorded hashes and across every + format at five sizes and two seeds. + ### Changed - **`verify` and `cleanup` read the files over several threads, so checking a diff --git a/internal/core/fill.go b/internal/core/fill.go new file mode 100644 index 0000000..00fd815 --- /dev/null +++ b/internal/core/fill.go @@ -0,0 +1,59 @@ +package core + +import ( + "encoding/binary" + // D11 promises the same bytes from the same seed, so a deliberate, + // reproducible generator is the product rather than a weakness. Nothing + // here ever makes a secret. + // nosemgrep: go.lang.security.audit.crypto.math_random.math-random-used + "math/rand/v2" +) + +// Bulk random bytes are drawn eight at a time, and the two byte orders are two +// functions rather than one function with a flag. +// +// Eight at a time rather than one is the whole point. Measured 2026-09-06 over +// 64 MiB through a 32 KiB buffer, repetitions interleaved with the order +// reversed: one draw per byte runs at 182 MB/s, eight bytes per draw at +// 2499 MB/s. The write path of an archive is slower than the disk underneath it +// when it draws a byte at a time (P2 in PERFORMANCE-REVIEW-2026-09-05.md). +// +// The orders stay separate because picking the wrong one is not a style +// mistake - it silently rewrites every file a format has ever produced, which +// is exactly what D11 forbids. A boolean argument would put that mistake one +// typo away and would read the same in review either way. Two names cannot be +// confused by accident, and the compiler cannot help with a flag. +// +// Neither function draws anything for an empty slice, and both spend exactly +// one draw on a trailing group shorter than eight bytes. That is what the ten +// call sites did before they were folded into here, so folding them in moved no +// bytes except where the owner asked for them to move. + +// FillRandomBE fills b with random bytes, most significant byte of each draw +// first. This is the order bmp, gif, ico, opc, png, tiff and - since 0.3.0 - +// targz, wav and zip write. +func FillRandomBE(b []byte, rng *rand.Rand) { + i := 0 + for ; i+8 <= len(b); i += 8 { + binary.BigEndian.PutUint64(b[i:], rng.Uint64()) + } + if i < len(b) { + var eight [8]byte + binary.BigEndian.PutUint64(eight[:], rng.Uint64()) + copy(b[i:], eight[:]) + } +} + +// FillRandomLE fills b with random bytes, least significant byte of each draw +// first. This is the order avif, jpg, jxl and webp write. +func FillRandomLE(b []byte, rng *rand.Rand) { + i := 0 + for ; i+8 <= len(b); i += 8 { + binary.LittleEndian.PutUint64(b[i:], rng.Uint64()) + } + if i < len(b) { + var eight [8]byte + binary.LittleEndian.PutUint64(eight[:], rng.Uint64()) + copy(b[i:], eight[:]) + } +} diff --git a/internal/format/avif/avif.go b/internal/format/avif/avif.go index 5077e86..aa740dd 100644 --- a/internal/format/avif/avif.go +++ b/internal/format/avif/avif.go @@ -515,7 +515,7 @@ func writeFiller(ctx context.Context, w io.Writer, buf []byte, rng *rand.Rand, s return ctx.Err() default: } - fillBytes(buf[:n], rng) + core.FillRandomLE(buf[:n], rng) if _, err := w.Write(buf[:n]); err != nil { return err } @@ -523,12 +523,3 @@ func writeFiller(ctx context.Context, w io.Writer, buf []byte, rng *rand.Rand, s } return nil } - -func fillBytes(b []byte, rng *rand.Rand) { - for i := 0; i < len(b); i += 8 { - v := rng.Uint64() - for j := 0; j < 8 && i+j < len(b); j++ { - b[i+j] = byte(v >> (8 * uint(j))) - } - } -} diff --git a/internal/format/bmp/bmp.go b/internal/format/bmp/bmp.go index 612af04..153c3ee 100644 --- a/internal/format/bmp/bmp.go +++ b/internal/format/bmp/bmp.go @@ -372,11 +372,7 @@ func writeGap(ctx context.Context, w io.Writer, seed uint64, n int64) error { take = remaining } chunk := buf[:take] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/gif/gif.go b/internal/format/gif/gif.go index 0703482..ee62db6 100644 --- a/internal/format/gif/gif.go +++ b/internal/format/gif/gif.go @@ -358,11 +358,7 @@ func writeComment(ctx context.Context, w io.Writer, seed uint64, blocks, payload } buf[0] = byte(size) chunk := buf[1 : 1+size] - for j := 0; j < len(chunk); j += 8 { - var eight [8]byte - putUint64(eight[:], rng.Uint64()) - copy(chunk[j:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(buf[:1+size]); err != nil { return err } @@ -372,17 +368,6 @@ func writeComment(ctx context.Context, w io.Writer, seed uint64, blocks, payload return err } -func putUint64(b []byte, v uint64) { - b[0] = byte(v >> 56) - b[1] = byte(v >> 48) - b[2] = byte(v >> 40) - b[3] = byte(v >> 32) - b[4] = byte(v >> 24) - b[5] = byte(v >> 16) - b[6] = byte(v >> 8) - b[7] = byte(v) -} - // sizeLadder is tried from the largest down when the recipe names no picture // size, exactly as PNG does. The first rung that leaves a reachable remainder // wins, so a small file gets a small picture instead of being refused. diff --git a/internal/format/ico/ico.go b/internal/format/ico/ico.go index f6b469c..813ffd8 100644 --- a/internal/format/ico/ico.go +++ b/internal/format/ico/ico.go @@ -333,11 +333,7 @@ func writeGap(ctx context.Context, w io.Writer, seed uint64, n int64) error { take = remaining } chunk := buf[:take] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/jpg/jpg.go b/internal/format/jpg/jpg.go index cd9b652..7c1a094 100644 --- a/internal/format/jpg/jpg.go +++ b/internal/format/jpg/jpg.go @@ -543,14 +543,7 @@ func writeComments(ctx context.Context, w io.Writer, seed uint64, segments int, size = remaining } chunk := buf[:size] - for j := 0; j < len(chunk); j += 8 { - var eight [8]byte - v := rng.Uint64() - for k := 0; k < 8; k++ { - eight[k] = byte(v >> (8 * k)) - } - copy(chunk[j:], eight[:]) - } + core.FillRandomLE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/jxl/jxl.go b/internal/format/jxl/jxl.go index 83b4a9c..8b5ed0b 100644 --- a/internal/format/jxl/jxl.go +++ b/internal/format/jxl/jxl.go @@ -566,7 +566,7 @@ func writeFiller(ctx context.Context, w io.Writer, buf []byte, rng *rand.Rand, s return ctx.Err() default: } - fillBytes(buf[:n], rng) + core.FillRandomLE(buf[:n], rng) if _, err := w.Write(buf[:n]); err != nil { return err } @@ -574,12 +574,3 @@ func writeFiller(ctx context.Context, w io.Writer, buf []byte, rng *rand.Rand, s } return nil } - -func fillBytes(b []byte, rng *rand.Rand) { - for i := 0; i < len(b); i += 8 { - v := rng.Uint64() - for j := 0; j < 8 && i+j < len(b); j++ { - b[i+j] = byte(v >> (8 * uint(j))) - } - } -} diff --git a/internal/format/opc/opc.go b/internal/format/opc/opc.go index 5dced06..097b2e0 100644 --- a/internal/format/opc/opc.go +++ b/internal/format/opc/opc.go @@ -24,7 +24,6 @@ import ( "bytes" "compress/flate" "context" - "encoding/binary" "fmt" "hash/crc32" "io" @@ -412,11 +411,7 @@ func writeFiller(ctx context.Context, w io.Writer, seed uint64, n int64) error { take = remaining } chunk := buf[:take] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/png/png.go b/internal/format/png/png.go index 42b8db6..5fde3a7 100644 --- a/internal/format/png/png.go +++ b/internal/format/png/png.go @@ -470,11 +470,7 @@ func writePaddingChunk(ctx context.Context, w io.Writer, kind string, seed uint6 size = remaining } chunk := buf[:size] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) crc.Write(chunk) if _, err := w.Write(chunk); err != nil { diff --git a/internal/format/targz/size.go b/internal/format/targz/size.go index 9cfd9ba..b07ff10 100644 --- a/internal/format/targz/size.go +++ b/internal/format/targz/size.go @@ -418,9 +418,7 @@ func writeFiller(ctx context.Context, w io.Writer, seed uint64, n int64) error { size = remaining } chunk := buf[:size] - for i := range chunk { - chunk[i] = byte(rng.UintN(256)) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/tiff/tiff.go b/internal/format/tiff/tiff.go index f36f98a..220733b 100644 --- a/internal/format/tiff/tiff.go +++ b/internal/format/tiff/tiff.go @@ -471,11 +471,7 @@ func writeGap(ctx context.Context, w io.Writer, seed uint64, n int64) error { take = remaining } chunk := buf[:take] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/wav/wav.go b/internal/format/wav/wav.go index a584c30..340b400 100644 --- a/internal/format/wav/wav.go +++ b/internal/format/wav/wav.go @@ -481,9 +481,7 @@ func fill(ctx context.Context, w io.Writer, seed uint64, n int64) error { size = remaining } chunk := buf[:size] - for i := range chunk { - chunk[i] = byte(rng.UintN(256)) - } + core.FillRandomBE(chunk, rng) if err := writeAll(w, chunk); err != nil { return err } diff --git a/internal/format/webp/webp.go b/internal/format/webp/webp.go index 0c75318..8ca881c 100644 --- a/internal/format/webp/webp.go +++ b/internal/format/webp/webp.go @@ -37,7 +37,6 @@ import ( "encoding/binary" "fmt" "io" - "math/rand/v2" "strconv" "github.com/donislawdev/TestingFilesGenerator/internal/core" @@ -433,7 +432,7 @@ func writeFiller(ctx context.Context, w io.Writer, name string, seed uint64, siz return ctx.Err() default: } - fillBytes(buf[:n], rng) + core.FillRandomLE(buf[:n], rng) if err := writeAll(w, buf[:n]); err != nil { return err } @@ -447,19 +446,10 @@ func writeFiller(ctx context.Context, w io.Writer, name string, seed uint64, siz func filler(seed uint64, size int64) []byte { out := make([]byte, size) - fillBytes(out, core.NewRand(seed)) + core.FillRandomLE(out, core.NewRand(seed)) return out } -func fillBytes(b []byte, rng *rand.Rand) { - for i := 0; i < len(b); i += 8 { - v := rng.Uint64() - for j := 0; j < 8 && i+j < len(b); j++ { - b[i+j] = byte(v >> (8 * uint(j))) - } - } -} - func writeUint32(w io.Writer, v uint32) error { var b [4]byte binary.LittleEndian.PutUint32(b[:], v) diff --git a/internal/format/zip/zip.go b/internal/format/zip/zip.go index 9b03496..423ad73 100644 --- a/internal/format/zip/zip.go +++ b/internal/format/zip/zip.go @@ -626,9 +626,7 @@ func writeFiller(ctx context.Context, w io.Writer, seed uint64, n int64) error { size = remaining } chunk := buf[:size] - for i := range chunk { - chunk[i] = byte(rng.UintN(256)) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/guard/fillerprimitive_test.go b/internal/guard/fillerprimitive_test.go new file mode 100644 index 0000000..11938cf --- /dev/null +++ b/internal/guard/fillerprimitive_test.go @@ -0,0 +1,113 @@ +package guard + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +// Bulk random bytes come from core, and no format keeps its own copy of the +// loop. +// +// Until 2026-09-06 thirteen packages each carried their own version of the same +// filler, in four different shapes, and the drift between them was not +// cosmetic. Three of them - zip, targz and wav - drew one byte per call to the +// generator and ran at 182 MB/s, which is slower than the disk they were +// feeding. Six drew eight bytes but through a temporary array. Four wrote the +// same eight bytes with a hand rolled shift loop. Measured over 64 MiB: the +// slow shape 182 MB/s, the temporary array 1468 MB/s, the shift loop 845 MB/s, +// and one store straight into the buffer 2499 MB/s. +// +// The performance report that started this named three of the thirteen. The +// other ten were found by grep, and two of the four shapes were nobody's +// finding at all. That is the argument for this guard rather than for a note in +// a document: the copies were not written by one person on one day, they +// accumulated one format at a time, and each one looked reasonable beside the +// format next to it. +// +// Uint64 is the bulk draw. Every one of the thirteen reached for it, so asking +// that no format calls it directly is asking the question the copies actually +// answer. UintN is here for the one legitimate caller, which picks a word out +// of a list rather than filling a buffer. +// +// What this does NOT see, stated because a guard that hides its edges is worse +// than none: a filler written with IntN, Uint32 or Float64 would walk straight +// past it. Two such loops already exist and are deliberately left alone - the +// salt and header fills in internal/format/archive, twelve and sixteen bytes +// once per archive, where the shape costs nothing and changing it would move +// bytes for no gain. +func TestBulkRandomBytesComeFromOnePlace(t *testing.T) { + // Every call to UintN allowed to live outside core, and why. An entry that + // stops naming real code is a failure below, not a comment nobody reads. + allowed := map[string]string{ + "internal/format/opc/opc.go": "picks a word from a list, it does not fill a buffer", + } + + seen := map[string]bool{} + + root := filepath.Join("..", "format") + err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".go") { + return nil + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + return err + } + rel := filepath.ToSlash(filepath.Join("internal", "format", + strings.TrimPrefix(filepath.ToSlash(path), filepath.ToSlash(root)+"/"))) + + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + where := fset.Position(call.Pos()) + switch sel.Sel.Name { + case "Uint64": + t.Errorf("%s:%d calls Uint64 directly.\n"+ + "Bulk random bytes come from core.FillRandomBE or core.FillRandomLE, so that "+ + "every format draws them the same way and at the same speed. Thirteen packages "+ + "each had their own copy of this loop until 2026-09-06, in four shapes, the "+ + "slowest of them thirteen times slower than the fastest. Call the core helper "+ + "whose byte order this format already writes - and if the order is genuinely "+ + "new, add it there rather than here.", rel, where.Line) + case "UintN": + if _, ok := allowed[rel]; !ok { + t.Errorf("%s:%d calls UintN directly.\n"+ + "If this fills a buffer, use core.FillRandomBE or core.FillRandomLE. If it "+ + "draws a single value for something else, add it to the allowed map above "+ + "with the reason.", rel, where.Line) + return true + } + seen[rel] = true + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walking the format packages: %v", err) + } + + // An exception that outlived its code is an exception nobody granted. + for rel, why := range allowed { + if !seen[rel] { + t.Errorf("%s is listed as allowed to call UintN (%s), but it does not call it.\n"+ + "Delete the entry. A standing exception for code that has gone will quietly "+ + "cover the next thing that lands in that file.", rel, why) + } + } +} diff --git a/internal/guard/generatorbytes_test.go b/internal/guard/generatorbytes_test.go index 5b575b7..f79e752 100644 --- a/internal/guard/generatorbytes_test.go +++ b/internal/guard/generatorbytes_test.go @@ -191,6 +191,17 @@ func goldenCases() map[string]engine.Target { "jpg_192kib_many_segments": {ID: "g", Format: "jpg", Sizes: engine.Uniform(1, 196608), Label: true, Properties: map[string]string{"width": "64", "height": "64", "quality": "90"}}, "wav_32kib": {ID: "g", Format: "wav", Sizes: engine.Uniform(1, 32768), Label: true}, + + // WAV only pads when the audio does not land on the size exactly, and + // the padding is audio modulo the frame size - two bytes here, never a + // frame's worth. wav_32kib happens to be a size the audio fills + // exactly, so it walks past the filler without touching it, and until + // 2026-09-06 no case pinned that path at all. + // + // Found by measurement rather than by reading: making the filler emit a + // constant moved 33 of the 54 cases and not one of them was a WAV. + "wav_with_the_padding_chunk": {ID: "g", Format: "wav", Sizes: engine.Uniform(1, 102400), Label: true}, + "zip_16kib": {ID: "g", Format: "zip", Sizes: engine.Uniform(1, 16384), Label: true}, "md_8kib": {ID: "g", Format: "md", Sizes: engine.Uniform(1, 8192), Label: true}, "log_8kib": {ID: "g", Format: "log", Sizes: engine.Uniform(1, 8192), Label: true}, diff --git a/internal/guard/testdata/generator-golden.json b/internal/guard/testdata/generator-golden.json index 504af8b..8c2ad46 100644 --- a/internal/guard/testdata/generator-golden.json +++ b/internal/guard/testdata/generator-golden.json @@ -164,8 +164,8 @@ }, "targz_past_the_comment_limit": { "bytes": 262144, - "sha256": "e0ea316d5fd0d3dee866a551660cd7626fc1902d5d36dfdb2e1ca8e240395e13", - "measured_on": "2026-09-01" + "sha256": "34dae598e18f6ab8f0abf07358353b014feb7b6e1f831dc6b0f3e0f84c4e6b1a", + "measured_on": "2026-09-06" }, "targz_with_three_pdfs": { "bytes": 65536, @@ -199,6 +199,11 @@ "bytes": 32768, "sha256": "7f1de29ead6fd51cad8675133054f77f760c9f867cc26351260e5340e5201b3a" }, + "wav_with_the_padding_chunk": { + "bytes": 102400, + "sha256": "55f453ce6cc46a6d856d62825327169683283b22b98b0a4e06effe3a3aaafffe", + "measured_on": "2026-09-06" + }, "webp_64kib": { "bytes": 65536, "sha256": "de9034bd6315f0a51fd8fe1b99adfa5719597f7c3d7cdaba1d3b8624754bf89e", @@ -234,15 +239,18 @@ }, "zip_16kib": { "bytes": 16384, - "sha256": "480b35575b9cec5bcdcdf02d066f4ad8b4b967969aecbeb2e49d7a451a14fdaf" + "sha256": "4e02a3eb7c2cad250ec98d4b03e1393b503c344cda1fc1d16c0a3296d9eed9a5", + "measured_on": "2026-09-06" }, "zip_past_the_comment_limit": { "bytes": 262144, - "sha256": "fae918bc961420dd675dd9a90005d319c93856d56b5c3f9aeeb1150baf41a6df" + "sha256": "19f8a63a28a0cd930caa95300bc4b70b164d35befde65bc919a3a9dbbbc37e7e", + "measured_on": "2026-09-06" }, "zip_with_three_pdfs": { "bytes": 65536, - "sha256": "2f45c46662ab65d2f598d65446ba0476067fb5c6297dc497d580a0fcd1752fbe" + "sha256": "fa73baa3d6fa5a3cdd9ee024c027c0b34e95664d2a99c3b6c15deea10a2a3751", + "measured_on": "2026-09-06" }, "csv_8kib_quote_all": { "bytes": 8192, @@ -315,6 +323,17 @@ "files": [ "csv_8kib_seventeen_columns" ] + }, + { + "on": "2026-09-06", + "why": "The three formats whose filler drew one byte per RNG call - zip, targz and wav - now draw eight, which is where their padding bytes come from and so is a breaking change under D11. Measured 2026-09-06, interleaved with the order reversed and the exit code checked on every run: zip 64 MB 2.81x, targz 64 MB 2.74-3.32x, ranges disjoint in both. The filler loop itself goes from 182 MB/s to 2499 MB/s over 64 MiB. Ten further packages moved onto the same core.FillRandomBE/LE and did NOT move a byte - checked across 24 formats by 5 sizes by 2 seeds, twenty formats identical. wav gains no speed at all and the owner took it anyway for uniformity: its padding is audio modulo the frame size, which is two bytes, so only two bytes of a WAV differ. That is also why wav_with_the_padding_chunk had to be added - wav_32kib lands on a size the audio fills exactly and never reaches the filler, so the path had no golden value at all.", + "files": [ + "targz_past_the_comment_limit", + "wav_with_the_padding_chunk", + "zip_16kib", + "zip_past_the_comment_limit", + "zip_with_three_pdfs" + ] } ] }