diff --git a/CHANGELOG.md b/CHANGELOG.md index db3003a..db252b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,83 @@ served it. ### Changed +- **go-python-packaging is now `v0.6.0`**, which brings a packed integer version + comparison key. **Cold resolution is 2.0x to 13.4x faster and warm 1.2x to 1.9x**, + with no change to any resolution this module produces. + + Nothing in this repository changed but the dependency pin. `version.Version` + comparison now runs off a packed 4×uint64 order-preserving key rather than a + field-by-field walk, for the 97.3% of real versions that fit one. + + Medians of five interleaved rounds, ten iterations, against the production + snapshot (932,861 packages, dated 2026-08-04), on an Apple M4 Max: + + | entry | cold before | cold after | | warm before | warm after | | + |---|---|---|---|---|---|---| + | `single-no-deps` | 1.18 ms | 0.24 ms | 4.9x | 0.24 ms | 0.17 ms | 1.42x | + | `small-tree` | 4.87 ms | 1.21 ms | 4.0x | 1.22 ms | 0.91 ms | 1.34x | + | `extras` | 6.47 ms | 1.65 ms | 3.9x | 1.71 ms | 1.32 ms | 1.29x | + | `app-set` | 20.67 ms | 5.50 ms | 3.8x | 5.40 ms | 4.22 ms | 1.28x | + | `wide-versions` | 167.35 ms | 12.46 ms | 13.4x | 14.15 ms | 7.61 ms | 1.86x | + | `backtracking` | 11.16 ms | 5.61 ms | 2.0x | 3.10 ms | 2.59 ms | 1.20x | + | `unsatisfiable` | 2.33 ms | 0.47 ms | 4.9x | 0.45 ms | 0.30 ms | 1.50x | + + **Cold gains far exceed warm, and that asymmetry is the whole story.** Building a + package's sorted version order is comparison-bound, and it happens once per package + per index — so it lands entirely in cold, where `wide-versions` (botocore, over ten + thousand releases) drops 13.4x. Warm reuses that order through the memo added in + 0.5.0, so what the packed key can still reach warm is only the residual comparison + inside `pep440set` containment and candidate filtering. That the warm figure is + 1.2–1.9x rather than flat says those residual comparisons were still material after + #39 and #40; that it is not larger says #40 had already removed most of them. + + ⚠️ **This does not compose multiplicatively with #40 — the two CONTEND**, and that is + measured rather than assumed. #40 removed most comparisons; the packed key makes the + remainder cheap. Both attack the same cost, so each is worth less once the other has + landed. A 2×2 in one interleaved session, warm, medians of three: + + | entry | packed gain BEFORE #40 | packed gain AFTER #40 | #40 gain at v0.5.0 | #40 gain at v0.6.0 | + |---|---|---|---|---| + | `single-no-deps` | 5.04x | 1.42x | 6.14x | 1.73x | + | `small-tree` | 2.37x | 1.32x | 3.26x | 1.81x | + | `extras` | 3.10x | 1.30x | 5.44x | 2.28x | + | `app-set` | 5.33x | 1.28x | 12.34x | 2.95x | + | `wide-versions` | 5.93x | 1.88x | 5.42x | 1.72x | + | `backtracking` | 4.01x | 1.23x | 7.49x | 2.29x | + | `unsatisfiable` | 1.99x | 1.48x | 1.43x | 1.07x | + + Both directions shrink, which is what "substitutes" means. On `app-set`, multiplying + the two isolated headlines predicts 12.34 × 5.33 = **65.8x**; the measured end-to-end + from `73d820a` + v0.5.0 to `276fe91` + v0.6.0 is **15.7x**. ⚠️ This is the opposite of + the #39/#40 interaction, where `Contains` became *more* valuable after the rank memo + (1.02x → 1.29x) — so the sign of these interactions cannot be guessed and has to be + measured each time. + + It also explains an apparent discrepancy with go-python-packaging's own release notes, + which claim 2.1–5.2x warm on this benchmark: that was measured at `73d820a`, before + #40 landed 46 minutes later. The pre-#40 column above reproduces it (1.99–5.93x). The + figures in the table at the top of this entry are the ones that apply to `main`. + + `candvers`, `metadata` and the pin set are **identical** on every entry, as they must + be: this changes what a comparison costs, not what it answers. Allocation churn falls + with the wall clock (`app-set` cold 23.1 MB / 475,428 allocs → 10.9 MB / 118,447). + Peak heap is flat to better — `wide-versions` 18.7 MB → 12.3 MB over baseline — with + one exception measured and kept rather than dropped: `backtracking` retains slightly + *more* (6.0 MB → 6.5 MB) and allocates slightly more bytes warm, while still + allocating fewer objects and finishing faster. + + `pypirsf.Open` plus `NewRSFIndex` over the same snapshot is unchanged at ~220 ms + (220.0 ms → 216.6 ms, within noise): it builds a name-to-offset table and parses no + versions, so the key cannot reach it. + + **Equivalence, since a comparison key change touches every ordering decision.** + 4,007 resolutions against the production snapshot — the seven corpus entries plus + 4,000 sampled package names, seed 1 — produce byte-identical transcripts before and + after: identical pins, identical decision ORDER, identical activated extras, and + identical failure report text on the 1,607 that fail. 36 cases where either side hit + an 8-second wall-clock deadline are excluded (35 on both sides, 1 on the baseline + only, 0 on the bumped side only). + - **Resolution is 1.4x to 11.8x faster warm**, with no change to any resolution it produces. `Provider.Candidates` no longer re-ranks a package's versions on every call, and `candidate.Rank` no longer sorts a list that is already ordered. @@ -154,6 +231,30 @@ served it. `version.Specifiers.Check` over 20,000 production packages, and 8.6M fuzz executions. +### Fixed + +- **Sharing one parsed `version.Version` between goroutines is no longer a data + race**, which this module inherits from the go-python-packaging v0.6.0 bump above + rather than fixing itself. Upstream's `Compare` now pads into a fresh slice instead + of appending into spare capacity a by-value copy shares. + + This matters here because the restriction was **documented on an exported API**: + `index.PackageMetadata.SupportsPython` told callers "DO NOT SHARE ONE PARSED target + BETWEEN GOROUTINES … give each goroutine its own `version.Parse`", and that guidance + is now unnecessary work. That doc and the canonical account in `RSFIndex.Versions` + are corrected; the historical explanation is kept, since it is still why several + types memoize version KEYS rather than parsed values. + + Re-verified against both pins with the exact eight-goroutine repro those docs + specify (target `3.11.0`, constraint `>3.9.1`): v0.5.0 reports `WARNING: DATA RACE`, + v0.6.0 is clean. + + ⚠️ **No behaviour in this module changed.** Memoizing parsed versions is now + *available* but is not taken here — that is a performance change owing its own + measurement, not something to ride along with a dependency bump. The remaining + internal rationales in `provider`, `index/mock.go` and `resolver/bench_test.go` + still describe the constraint as current and are swept with that work. + ### Added - `TestEveryReasonIsRecordedWhenNOTHINGIsUsable`, pinning the claim the whole diff --git a/go.mod b/go.mod index 046d9ff..a381a90 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.25.0 require ( github.com/klauspost/compress v1.19.2 github.com/posit-dev/go-pubgrub v0.2.0 - github.com/posit-dev/go-python-packaging v0.5.0 + github.com/posit-dev/go-python-packaging v0.6.0 github.com/rstudio/repository-snapshot-format v0.12.0 ) diff --git a/go.sum b/go.sum index 95b1377..ffa14f8 100644 --- a/go.sum +++ b/go.sum @@ -6,8 +6,8 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/posit-dev/go-pubgrub v0.2.0 h1:RWGbZl0x1s1T/7kF1e1Q4CKyy+gMBqEh1rCaZwIzdLs= github.com/posit-dev/go-pubgrub v0.2.0/go.mod h1:5ks/krPJaOu6GmbBqaeOiFR+cBa4lh5lkYIxun6cWW4= -github.com/posit-dev/go-python-packaging v0.5.0 h1:9hozlTpf3a3u5rX0A0j/aH92nh/8Nfei+odBnfTA2jI= -github.com/posit-dev/go-python-packaging v0.5.0/go.mod h1:NkELXOh/eRDT7c+LKFxJd+HkyGpKFsUIxpTYaq5xdkU= +github.com/posit-dev/go-python-packaging v0.6.0 h1:bqiHnvfeFN7TV7QWxwgCyTrZ75oJhg+hr0K41S6l67c= +github.com/posit-dev/go-python-packaging v0.6.0/go.mod h1:NkELXOh/eRDT7c+LKFxJd+HkyGpKFsUIxpTYaq5xdkU= github.com/rstudio/go-version v0.0.2 h1:ihU0xaF+Yuya0p2J6C8dfyB4gu/YehM1d6cTpabf/7A= github.com/rstudio/go-version v0.0.2/go.mod h1:Xfuma+m4R9L0P+Hof8iDDiL4/TJ8VjpdDXWq8qGXHL4= github.com/rstudio/repository-snapshot-format v0.12.0 h1:a6gRfXPLOR+5YxN7irO097lpWGOPMe8ZoATGkC9hcWE= diff --git a/index/rsfindex.go b/index/rsfindex.go index 5a672b8..683f706 100644 --- a/index/rsfindex.go +++ b/index/rsfindex.go @@ -436,8 +436,18 @@ func cloneMetadata(m PackageMetadata, ver version.Version) PackageMetadata { // it" the same choice by construction rather than by two call sites agreeing on // preferKey. See resolveStoredKey. // -// ⚠️ A version.Version MUST NOT BE SHARED BETWEEN GOROUTINES, so memoizing the -// parsed values is not available. Version.Compare pads the shorter operand's +// ⚠️ UPDATE, go-python-packaging v0.6.0: the upstream defect this whole section +// rests on is FIXED. Compare now pads into a fresh slice, so a parsed Version is +// safe to share and memoizing parsed values IS available. Re-verified against both +// pins: v0.5.0 races under eight goroutines, v0.6.0 is clean. +// +// This code still memoizes KEYS. That remains correct -- it is now a choice rather +// than a requirement -- and taking the memo is a performance change with its own +// measurement, not something to ride along with a dependency bump. Tracked +// separately; the account below is kept because it explains the current shape. +// +// A version.Version MUST NOT BE SHARED BETWEEN GOROUTINES under v0.5.0 and earlier, +// so memoizing the parsed values was not available. Version.Compare pads the shorter operand's // release segment with `append`, and cmpkey builds that segment by RESLICING // away trailing zeros -- so "3.0.0" carries a Parts of len 1 and cap 3, and // padding it back to three segments writes into spare capacity in the backing @@ -451,12 +461,13 @@ func cloneMetadata(m PackageMetadata, ver version.Version) PackageMetadata { // benign in practice today -- but it is a data race the Go memory model gives // no guarantee about, and this type documents itself as safe for concurrent use. // -// The defect is upstream, in rstudio/go-version v0.0.2 (part.Parts.Padding +// The defect was upstream, in rstudio/go-version v0.0.2 (part.Parts.Padding // appending into shared capacity) as reached through go-python-packaging v0.5.0 -// (version.Version.Compare). It is not introduced here and it is not fixable +// (version.Version.Compare). It was not introduced here and was not fixable // here: key.release is unexported, so this module cannot hand out a Version -// whose backing array it has clipped. When it is fixed upstream, memoizing the -// parsed values becomes available and recovers the remaining parse cost. +// whose backing array it has clipped. go-python-packaging v0.6.0 sidesteps it by +// padding into a fresh slice, so the condition in the note at the top of this +// comment is now met and the parsed-version memo is available to be taken. func (idx *RSFIndex) Versions(ctx context.Context, pkg PackageName) ([]version.Version, error) { if err := ctx.Err(); err != nil { return nil, err diff --git a/index/types.go b/index/types.go index ef6a900..68fbcf8 100644 --- a/index/types.go +++ b/index/types.go @@ -122,10 +122,22 @@ type PackageMetadata struct { // So the leniency the parser intends is only actually lenient if callers come // through here. // -// # ⚠️ DO NOT SHARE ONE PARSED target BETWEEN GOROUTINES +// # Sharing one parsed target between goroutines: FIXED as of go-python-packaging v0.6.0 // -// Parse the interpreter version once and fan the work out across goroutines and -// this is a DATA RACE, in go-version rather than here. version.Version.Compare +// ⚠️ This section described a real data race under go-python-packaging v0.5.0 and +// earlier, and it told callers to give each goroutine its own version.Parse. That +// restriction is LIFTED: v0.6.0's version.Compare pads into a fresh slice rather +// than into shared spare capacity, so a parsed Version is safe to share. This +// module requires v0.6.0, so callers of this method need do nothing. +// +// The account below is kept because it is the reason several types in this package +// memoize version KEYS rather than parsed values -- those choices are still in the +// code and still correct, they are simply no longer forced. Removing them, and the +// parsed-version memo the fix now unblocks, is tracked separately. +// +// Under v0.5.0 and earlier, parse the interpreter version once and fan the work out +// across goroutines and this was a DATA RACE, in go-version rather than here. +// version.Version.Compare // pads the shorter operand's release segment with append, and cmpkey builds that // segment by reslicing trailing zeros away -- so "3.11.0" carries spare capacity // that a by-value copy shares, and two goroutines comparing two copies write to @@ -144,13 +156,12 @@ type PackageMetadata struct { // // `>` and `<` use the prospective version directly; `>=`, `<=`, `==` and `!=` // re-parse it through Public() first and are immune. So a target of "3.11" or a -// corpus that happens to use `>=` hides it completely, and it appears the day -// someone passes "3.11.0" to a package pinned with `<`. +// corpus that happens to use `>=` hides it completely, and it appeared the day +// someone passed "3.11.0" to a package pinned with `<`. // -// A fix is being filed upstream. Until it lands there is nothing this method can -// do about it -- the padding happens inside a value it does not own -- so the -// warning is the mitigation. The same reasoning is why RSFIndex memoizes version -// KEYS rather than parsed values. +// The fix landed upstream in go-python-packaging v0.6.0. Re-verified here against +// both pins with the exact table above: v0.5.0 reports WARNING: DATA RACE under +// eight goroutines, v0.6.0 is clean. func (m PackageMetadata) SupportsPython(target version.Version) bool { if m.RequiresPython.String() == "" { return true