Run CI on Linux, macOS and Windows; fix Tier 2 bugs - #14
Merged
Merged
Conversation
- Run the tests on ubuntu-latest, macos-latest and windows-latest. golangci-lint stays on ubuntu. Windows calls go test directly because make is not reliably available there. - Add a make test-race target with a 10 minute timeout (the root package takes over a minute under -race) and run it on ubuntu. - Fix make examples: stop at the first failing example instead of reporting only the last one, and detect directories with more than one .go file (the old [ -f dir*.go ] test broke on them). Run it on ubuntu. - Skip TestTempFileCreationFailure and TestTempFileCreationFailureStrings on Windows and as root, where a 0555 directory does not block file creation. - Fill the temp dir cache on first use in buildCandidateList and buildAdditionalFallbacks, so TestGetAdditionalFallbacks no longer depends on another test having called GetTempDir first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
buildChunks used a plain break when the input channel closed, which only left the select, so it kept polling the closed channel up to ChunkSize times, then allocated and polled one more empty chunk. With the default ChunkSize of 1M this added about 50 ms to every sort. Use a labeled break and stop after the last chunk. A 10-record sort drops from 51.8 ms to 0.05 ms (BenchmarkSortTenRecords) and allocates one chunk instead of two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- mergeConfig wrote defaults into the caller's *Config, which raced when one Config was shared by several sorters. It now normalizes a copy and never modifies the caller's struct. - Document the zero-value rules, which were misdescribed: a nil Config means DefaultConfig(), a ChunkSize or NumWorkers below 1 and a negative buffer size use the default, and a zero ChanBuffSize or SortedChanBuffSize means an unbuffered channel. The "Must be > 1" notes on ChunkSize and NumWorkers were also wrong: 1 is valid. - Change the ChanBuffSize default from 16 to 1, matching the docs and README. It only sizes the channel between reading the input and the sort workers, which holds whole chunks: 16 let up to 25 chunks sit in memory at once for no gain in wall time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
compareFunc is called from several sort and merge goroutines at once, and since the parallel merge (v1.1.0) so is fromBytes. None of this was documented. Say so on the function types in types.go and on Generic, MockGeneric, New and NewMock. toBytes is documented the same way so that serializing records in the sort workers stays possible. The parallel merge broke legacy FromBytes functions written for v1.0, which were only ever called from one goroutine. New and NewMock now guard FromBytes with a per-sorter mutex. Generic stays lock-free. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- diff read the error channel of the stream that ended first twice, so it hung if the caller never closed that channel, and error reads ignored ctx, so the hang outlasted the ctx deadline. Each error channel is now read once, and the read gives up when ctx is done. - StringResultChan's send ignored ctx, so a diff whose results were no longer read hung. Add StringResultChanContext, whose result function returns ctx.Err() once ctx is done. StringResultChan keeps its signature and behavior. - BREAKING: NEW and OLD are now typed Delta constants instead of untyped integers, so they print as ">" and "<". Code that uses them as plain ints no longer compiles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
UniqStringChan took a bidirectional chan string, so it could not be passed the <-chan string that Strings returns, its main use. Take <-chan string instead; callers passing a chan string still compile. The output channel is now buffered like the sorter's output (1000). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Save called Sync on a file that is only read back by this process and, on Unix, is already unlinked, so durability buys nothing. Writing and saving 64 MiB drops from 20.4 ms to 7.3 ms on macOS (BenchmarkWriteAndSave). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Save always called Next, so after the sorter's Next for its last chunk it appended an empty section. The merge then counted one chunk more than was written, which chose the parallel merge for exactly NumWorkers chunks and gave one worker an empty range. Save now ends the current section only if anything was written to it (or nothing was written at all, which keeps one empty section), and Size reports the count Save will produce. Same for MockFileWriter. TestTempFileRepeat and TestMockFileMultiSection expected the extra section; they now expect one section per Next. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Save allocated a 64 KiB bufio.Reader for every section up front, so merge memory grew by 64 KiB per chunk: 10k chunks took 640 MB before reading a byte. Readers are now created on the first Read of each section, with a buffer that shrinks to the section length for small sections and to an equal share of a 64 MiB budget (at least 4 KiB) when there are many sections. Sorting 200k records in 2,000 chunks allocates 6.9 MB instead of 136.7 MB (BenchmarkSortManyChunks). Multi-pass merging for very high chunk counts is still out of scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ordered created a new gob encoder and decoder for every record, which made it about 2x slower than Generic and allocated 1.74 GB per 1M ints. Encode cmp.Ordered values directly, choosing the codec once from the underlying kind of T, so named types like `type ID int64` work too: integers as varints, floats as their exact IEEE 754 bits (NaN and -0 round-trip), and strings as their bytes. Decoding rejects truncated, oversized and out-of-range data. The codecs access T through a pointer to its underlying type, which the kind check makes safe. Sorting 1M ints with Ordered drops from 748 ms and 1.74 GB to 367 ms and 46.7 MB (BenchmarkOrderedInts), the same as Generic with a varint codec. Add round-trip tests for every kind, named types, NaN and -0, invalid input, and sorts through disk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TestLargeDataElements, TestMixedTypeComparison and TestOrderedSerializationError sorted 2 to 4 records with the default 1M ChunkSize, so everything stayed in one in-memory chunk and the codecs were never called. Use one record per chunk so the 1 MB elements, both mixed types and the Ordered codec go through disk. Co-Authored-By: Claude Opus 5.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
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.
No description provided.