From 163240938e93a0b119e6708f366834e16b1b7260 Mon Sep 17 00:00:00 2001 From: Jonathan Yoder Date: Fri, 14 Aug 2026 14:04:29 -0400 Subject: [PATCH 1/2] perf(pep440set): take the release group key from gpp, not from a render MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contains needs each candidate version's (epoch, release) position, and it got it by calling BaseVersion() -- which renders the version through a bytes.Buffer with one math/big decimal conversion PER SEGMENT -- then split the text back into digit runs. Once per candidate version per Contains call. version.ReleaseKey, new in go-python-packaging v0.7.0, derives the same key from the parsed Version's own epoch and release fields: 16 ns and no allocation against 220 ns and 10 allocations. releaseKey, canonDigits, cmpDigits and cmpSegments go with it. isDigits stays; construct.go uses it. Warm resolution against the production snapshot is 1.19x to 1.39x faster and allocates 1.25x to 2.27x less on the six corpus entries that resolve anything; cold is 1.11x to 1.28x. The allocation column is the one to read -- what the render was buying was garbage. Contains/cross-group, the common case, drops from 304 ns and 8 allocations to 104 ns and none, and Set.Contains falls from 41% of resolver.Resolve to 24% on app-set. Full tables in resolver/bench_test.go. ⚠️ That share is against Resolve, not against total samples: removing this allocation shrinks the profile's GC share too, so "% of samples" would credit this change with that as well. And strings.Split leaves the profile while writeRelease and math/big.nat.itoa do NOT -- ensurePub still renders the public spelling, which this change does not touch. Both were stated wrongly in an earlier draft of these comments. TestBoundOrderingGrid gains a pair differing only past the SIXTH release segment, where gpp's packed fast path gives way to arbitrary precision. Truncating gpp's packer there instead of refusing makes that row fail; without it, nothing in this module would notice. ⚠️ unsatisfiable is NOT measurably faster. Its 1.07x is a median of three and a fourth confirmation round put it at 0.96x, slower; read it as no change. That is the prediction met rather than a disappointment -- it fails before enumerating many candidates and so makes the fewest Contains calls in the corpus. Its allocations do fall by 1.25x, which is the effect appearing where the mechanism says it should. ⚠️ This is the THIRD change to this cost and the one that removes it. #33 derived a bound's key once instead of per comparison and #39 replaced the probe bound with a stack-held verPos, but both left the derivation itself a string render, in newPosKey and in verPos.init. Fixing releaseKey fixes both and leaves no fourth site. A memo over the old function was measured and rejected: it would have made memory larger where this makes it smaller. TestCanonDigits is replaced rather than deleted. It existed because the key was built from DIGIT RUNS compared length-first, where "007" would sort above "7". The key now comes from math/big integers, where "007" is 7 before a key exists and there is no run to strip -- the concern is gone rather than moved. But the behaviour it protected is not optional, so TestLeadingZeroSpellings asserts it through cmpBound at widths either side of int64 and uint64, and also asserts the spelling has not flattened a real difference: 1.007 is 1.7, not 1.70 and not 1.0.7. Held by the production differential at PEP440SET_CORPUS_PACKAGES=20000 -- 33,918,235 (specifier, version) pairs, 0 mismatches against Check -- the rendering differential over the same 20,000 packages, FuzzFromSpecifiers for 120 s with no new input, the ordering suites under -race, and on the gpp side a migration differential against this very algorithm across 348,752 corpus versions. --- CHANGELOG.md | 72 ++++++++++++ go.mod | 2 +- go.sum | 4 +- pep440set/bound.go | 171 ++++++++++------------------- pep440set/bound_invariants_test.go | 29 +++-- pep440set/bound_test.go | 63 ++++++----- pep440set/construct.go | 17 +++ pep440set/fuzz_test.go | 3 +- pep440set/verpos.go | 34 +++--- pep440set/verpos_race_test.go | 31 ++++-- resolver/bench_test.go | 87 ++++++++++++++- 11 files changed, 327 insertions(+), 186 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afa8034..b173cf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,78 @@ served it. ## [Unreleased] +### Changed + +- **`pep440set` no longer renders a version back to text to find its release + group.** Warm resolution is **1.19x to 1.39x faster and allocates 1.25x to + 2.27x less** on the six corpus entries that resolve anything; cold is 1.11x to + 1.28x. The seventh, `unsatisfiable`, does not move — see below. No resolution + this module produces changes. + + `Contains` needs each candidate version's `(epoch, release)` position, and it + derived that by calling `BaseVersion()` — which renders the version through a + `bytes.Buffer` with one `math/big` decimal conversion **per segment** — and + then splitting the result back into digit runs. Once per candidate version per + `Contains` call. It now asks go-python-packaging for + `version.ReleaseKey` instead, which reads the parsed `Version`'s own epoch and + release fields: ~16 ns and no allocation, against ~220 ns and 10 allocations + for the render-and-split. + + Medians of three interleaved rounds, ten iterations, against the production + snapshot (932,861 packages, dated 2026-08-04), on an Apple M4 Max: + + | entry | warm before | warm after | | warm allocs before | after | | + |---|---|---|---|---|---|---| + | `single-no-deps` | 0.21 ms | 0.15 ms | 1.38x | 3,637 | 2,260 | 1.61x | + | `small-tree` | 1.06 ms | 0.80 ms | 1.32x | 19,393 | 10,908 | 1.78x | + | `extras` | 1.45 ms | 1.08 ms | 1.35x | 29,391 | 15,102 | 1.95x | + | `app-set` | 4.54 ms | 3.27 ms | 1.39x | 96,318 | 42,415 | 2.27x | + | `wide-versions` | 8.11 ms | 6.74 ms | 1.20x | 197,228 | 127,659 | 1.54x | + | `backtracking` | 2.78 ms | 2.33 ms | 1.19x | 43,304 | 21,015 | 2.06x | + | `unsatisfiable` | 0.33 ms | 0.31 ms | 1.07x | 6,448 | 5,168 | 1.25x | + + | entry | cold before | cold after | | cold allocs before | after | | + |---|---|---|---|---|---|---| + | `single-no-deps` | 0.27 ms | 0.23 ms | 1.17x | 4,365 | 2,982 | 1.46x | + | `small-tree` | 1.41 ms | 1.10 ms | 1.28x | 23,609 | 15,107 | 1.56x | + | `extras` | 1.82 ms | 1.46 ms | 1.25x | 34,630 | 20,354 | 1.70x | + | `app-set` | 5.94 ms | 4.70 ms | 1.26x | 118,465 | 64,533 | 1.84x | + | `wide-versions` | 13.14 ms | 11.86 ms | 1.11x | 276,776 | 207,206 | 1.34x | + | `backtracking` | 6.22 ms | 5.26 ms | 1.18x | 89,097 | 66,832 | 1.33x | + | `unsatisfiable` | 0.55 ms | 0.51 ms | 1.08x | 9,256 | 7,974 | 1.16x | + + **The allocation column is the one to read.** The saving is not the arithmetic + — it is the garbage. `app-set` allocates 2.27x less warm while running 1.39x + faster. + + In a CPU profile of the warm resolution, `Set.Contains` falls from **41% of + `resolver.Resolve` to 24%** on `app-set`, and from 33% to 14% on + `wide-versions` — 3.1x and 3.8x fewer samples in absolute terms. Measured + against `Resolve` rather than against total samples deliberately: removing + this allocation shrinks the profile's garbage-collection share too, so a + "% of samples" figure would credit the change with that as well. + + `strings.Split` leaves the profile entirely. `writeRelease` and + `math/big.nat.itoa` do **not** — `ensurePub` still renders the public + spelling, which this change deliberately does not touch — but they fall from + 2.3% and 1.6% of samples under `Contains` to below the sampling floor. + + ⚠️ **`unsatisfiable` is not measurably faster.** Its 1.07x is the median of + three rounds, and a fourth confirmation round put it at **0.96x** — slower. + Read it as no change, not as a small win. That is the predicted result rather + than a disappointment: it fails before enumerating many candidates, so it + makes the fewest `Contains` calls of the corpus and has the least to gain. + Its allocations do fall, by 1.25x, which is the effect showing up where the + mechanism says it should. + + ⚠️ The measurement is against `v0.6.0` **without** the parsed-version memo. The + two are not additive — after this change the largest remaining warm cost is + `version.Parse` inside `index.Versions` (16.9% of samples on `wide-versions`), + which is exactly what that memo removes. + +- **go-python-packaging is now `v0.7.0`**, for `version.ReleaseKey`. Purely + additive; nothing else in this module changed with the pin. + ## [0.6.0] - 2026-08-14 ### Changed diff --git a/go.mod b/go.mod index a381a90..4c45d04 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.6.0 + github.com/posit-dev/go-python-packaging v0.7.0 github.com/rstudio/repository-snapshot-format v0.12.0 ) diff --git a/go.sum b/go.sum index ffa14f8..dbb415c 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.6.0 h1:bqiHnvfeFN7TV7QWxwgCyTrZ75oJhg+hr0K41S6l67c= -github.com/posit-dev/go-python-packaging v0.6.0/go.mod h1:NkELXOh/eRDT7c+LKFxJd+HkyGpKFsUIxpTYaq5xdkU= +github.com/posit-dev/go-python-packaging v0.7.0 h1:pxyI2MmsP2iLqcwHkjZiXMPvvagywbslXid35czAh3o= +github.com/posit-dev/go-python-packaging v0.7.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/pep440set/bound.go b/pep440set/bound.go index 2da03ae..0733680 100644 --- a/pep440set/bound.go +++ b/pep440set/bound.go @@ -50,21 +50,22 @@ type bound struct { // posKey is everything cmpBound needs from a bound's version. // -// ⚠️ DERIVE IT AT CONSTRUCTION, NOT PER COMPARISON. Every field here costs at -// least one string render: version.Version holds its release as big.Ints and -// its public spelling only as something String() builds, and Public() then -// needs a re-parse to be comparable. cmpBound runs in the innermost loop of the -// set algebra, which is itself in the solver's hot loop, so it is called orders -// of magnitude more often than a bound is built. One resolution against a -// curated-shaped index -- packages present, transitive dependencies absent -- -// took 17.5 s and allocated 25 GB CUMULATIVELY with this work on the comparison -// side. Cumulatively, not concurrently: peak heap stayed under 120 MB, so what -// the churn bought was garbage collection. The cost is latency, not footprint. +// ⚠️ DERIVE IT AT CONSTRUCTION, NOT PER COMPARISON. version.Version holds its +// public spelling only as something String() builds, and Public() then needs a +// re-parse to be comparable, so the public fields below cost a render each. +// cmpBound runs in the innermost loop of the set algebra, which is itself in +// the solver's hot loop, so it is called orders of magnitude more often than a +// bound is built. One resolution against a curated-shaped index -- packages +// present, transitive dependencies absent -- took 17.5 s and allocated 25 GB +// CUMULATIVELY with this work on the comparison side. Cumulatively, not +// concurrently: peak heap stayed under 120 MB, so what the churn bought was +// garbage collection. The cost is latency, not footprint. type posKey struct { - // epoch and release are the canonical (leading-zero-free, trailing-zero- - // stripped) decimal digit runs releaseKey produces. - epoch string - release []string + // rel is v's release group: its epoch and its trailing-zero-stripped + // release segments, ignoring the pre/post/dev/local suffix. gpp derives it + // from the parsed fields, so unlike the rest of this struct it costs no + // render -- see the note above cmpBound. + rel version.ReleaseKey // public is v.Public(), and pub is that spelling parsed, which is v with // any local label removed. pubOK is false when the spelling does not parse, // which leaves the public comparison out entirely, exactly as it was when @@ -76,8 +77,7 @@ type posKey struct { // newPosKey derives the key for v. func newPosKey(v version.Version) *posKey { - epoch, release := releaseKey(v) - k := &posKey{epoch: epoch, release: release, public: v.Public()} + k := &posKey{rel: v.ReleaseKey(), public: v.Public()} if k.public == "" { // An uninitialized Version. Parsing its (empty) public spelling failed // when cmpBound did it inline, so it stays out of the comparison. @@ -134,102 +134,50 @@ func (b bound) tier() int { } } -// isDigits reports whether s is a non-empty run of ASCII digits. -func isDigits(s string) bool { - if s == "" { - return false - } - for i := 0; i < len(s); i++ { - if s[i] < '0' || s[i] > '9' { - return false - } - } - return true -} - -// canonDigits strips leading zeros from a digit run, keeping one digit for an -// all-zero segment, so "007" and "7" produce the same key component. -func canonDigits(s string) string { - i := 0 - for i < len(s)-1 && s[i] == '0' { - i++ - } - return s[i:] -} - -// releaseKey returns the (epoch, release) of b's version, with trailing zeros -// stripped so 1.0 and 1.0.0.0 share a key. BaseVersion renders "1!3.4.5" for -// an epoch, so the epoch is split off here. +// cmpBound reports whether a is before (-1), at (0) or after (+1) b. +// +// # Where the release group key comes from, and why it is not derived here +// +// The (epoch, release) group key is version.ReleaseKey, derived by gpp from the +// parsed Version's own epoch and release fields. // -// ⚠️ THE KEY COMPONENTS ARE DECIMAL STRINGS, NOT ints. DO NOT "SIMPLIFY" THIS -// BACK TO strconv.Atoi. +// ⚠️ DO NOT REDERIVE IT FROM A RENDERED STRING. This package used to: it called +// BaseVersion(), which renders "1!3.4.5" through a bytes.Buffer with one +// math/big decimal conversion PER SEGMENT, and then split the result back into +// digit runs -- once per candidate version per Contains call. // -// PEP 440 puts no ceiling on an epoch or a release segment -- the grammar is -// `[0-9]+` -- and gpp stores both as arbitrary-precision part.BigInt, so it -// orders 1.99999999999999999999 above 1.5 correctly. The earlier key parsed -// each segment with strconv.Atoi and BROKE out of the loop on error, so a -// segment at or above 2^63 was dropped along with every segment AFTER it: the -// key became a PREFIX of the real release, and 99999999999999999999.0 keyed as -// the empty release, sorting below every version in existence. That made +// That derivation was 82% of Set.Contains on the warm app-set resolution and +// 74% on wide-versions, measured with pprof against the production snapshot. +// +// ⚠️ Measured as a share of resolver.Resolve, NOT as a share of total samples. +// The denominator has to be something the change does not move: removing this +// allocation shrinks the profile's garbage-collection share too, so "% of +// samples" would credit this change with that as well. Against Resolve, +// Contains falls from 41% to 24% on app-set and from 33% to 14% on +// wide-versions -- 3.1x and 3.8x fewer samples in absolute terms. +// +// End to end that is 1.39x warm on app-set and 1.20x on wide-versions, with +// 2.27x fewer allocations. BenchmarkContains/cross-group -- the common case, +// probing a span that anchors some other release -- went from 304 ns and 8 +// allocations to 104 ns and none. +// +// ⚠️ strings.Split leaves the profile entirely, but writeRelease and +// math/big.nat.itoa DO NOT: ensurePub still renders the public spelling, which +// this change deliberately does not touch. They drop from 2.3% and 1.6% of +// samples under Contains to below the sampling floor, which is not the same as +// gone. What is left inside Contains is mostly verPos.init, and most of that is +// the two Version copies it makes rather than the key. +// +// ⚠️ AND DO NOT REPLACE IT WITH ints. An even earlier key parsed each segment +// with strconv.Atoi and BROKE out of the loop on error, so a segment at or +// above 2^63 was dropped along with every segment AFTER it: the key became a +// PREFIX of the real release, and 99999999999999999999.0 keyed as the empty +// release, sorting below every version in existence. That made // `>99999999999999999999.0` admit everything while Check admitted nothing, and -// `<1.5` admit 1.99999999999999999999. Comparing the digit runs directly -// (length first, then byte-wise) is exact at every magnitude and needs no -// math/big. -func releaseKey(v version.Version) (epoch string, release []string) { - base := v.BaseVersion() - epoch = "0" - if i := strings.Index(base, "!"); i >= 0 { - // A non-numeric epoch cannot come out of BaseVersion; if one ever did, - // treating it as 0 keeps this a total order rather than a panic. - if isDigits(base[:i]) { - epoch = canonDigits(base[:i]) - } - base = base[i+1:] - } - for _, part := range strings.Split(base, ".") { - if !isDigits(part) { - break - } - release = append(release, canonDigits(part)) - } - for len(release) > 0 && release[len(release)-1] == "0" { - release = release[:len(release)-1] - } - return epoch, release -} - -// cmpDigits orders two canonical (leading-zero-free) digit runs by value. The -// shorter run is the smaller number, and equal-length runs compare byte-wise, -// which for ASCII digits is the same as comparing values. -func cmpDigits(a, b string) int { - switch { - case len(a) < len(b): - return -1 - case len(a) > len(b): - return 1 - } - return strings.Compare(a, b) -} - -// cmpSegments orders two release keys segment by segment, the shorter being -// smaller when it is a prefix of the longer. releaseKey has already stripped -// trailing zeros, so a shorter key means a genuinely shorter release. -func cmpSegments(a, b []string) int { - for i := 0; i < len(a) && i < len(b); i++ { - if c := cmpDigits(a[i], b[i]); c != 0 { - return c - } - } - switch { - case len(a) < len(b): - return -1 - case len(a) > len(b): - return 1 - } - return 0 -} - -// cmpBound reports whether a is before (-1), at (0) or after (+1) b. +// `<1.5` admit 1.99999999999999999999. PEP 440 puts no ceiling on an epoch or a +// release segment -- the grammar is `[0-9]+` -- and ReleaseKey is exact at +// every magnitude, comparing arbitrary-precision integers for the keys that do +// not fit its packed fast path. func cmpBound(a, b bound) int { if a.inf != 0 || b.inf != 0 { switch { @@ -242,10 +190,7 @@ func cmpBound(a, b bound) int { } ak, bk := a.pos(), b.pos() - if c := cmpDigits(ak.epoch, bk.epoch); c != 0 { - return c - } - if c := cmpSegments(ak.release, bk.release); c != 0 { + if c := ak.rel.Compare(bk.rel); c != 0 { return c } diff --git a/pep440set/bound_invariants_test.go b/pep440set/bound_invariants_test.go index b3539aa..709b9ab 100644 --- a/pep440set/bound_invariants_test.go +++ b/pep440set/bound_invariants_test.go @@ -83,6 +83,18 @@ func ascendingPositions(t *testing.T) []orderEntry { below("1.0.1"), at("1.0.1"), aboveRelease("1.0.1"), + + // ⚠️ A pair differing ONLY past the sixth release segment. gpp packs a + // release into six 32-bit fields and falls back to arbitrary-precision + // comparison beyond that, so these two straddle the fast path's edge: + // a packer that silently truncated at six segments, or whose fields + // overlapped, would order them EQUAL and nothing else in this module + // would notice. gpp has its own tests for the layout; this is the row + // that makes a regression in it fail HERE, where the consequence is a + // resolver picking the wrong version. + at("1.0.1.2.3.4.5"), + at("1.0.1.2.3.4.6"), + below("1.1"), at("1.1"), below("1.5"), @@ -177,15 +189,14 @@ func TestBoundEqualPositions(t *testing.T) { bound{v: v("1!1.0"), edge: edgeAt}, bound{v: v("01!1.0"), edge: edgeAt}}, {"at(0!1.0) == at(1.0)", bound{v: v("0!1.0"), edge: edgeAt}, bound{v: v("1.0"), edge: edgeAt}}, - // ⚠️ THESE TWO DO NOT REACH canonDigits' STRIPPING, so do not read them - // as its guard. releaseKey derives from BaseVersion(), which renders - // each segment through big.Int.String() and is therefore already - // leading-zero-free: "1.00000000000000000001" arrives here as "1.1". - // What they guard is the layer above -- that a segment past 2^63 - // survives the parse at full precision, and that cmpDigits orders two - // runs by VALUE (length first) rather than truncating to an int or - // comparing rendered text. canonDigits is covered directly by - // TestCanonDigits. + // ⚠️ THESE TWO ARE NOT ABOUT LEADING ZEROS, so do not read them as that + // guard. The group key comes from gpp's parsed release segments, which + // are math/big integers: "1.00000000000000000001" is the integer 1 + // before a key exists, and no zero-stripping runs at all. What they + // guard is the layer above -- that a segment past 2^63 survives the + // parse at full precision, and that the key orders it by VALUE rather + // than truncating to an int or comparing rendered text. Leading-zero + // spellings are covered directly by TestLeadingZeroSpellings. {"at(1.00000000000000000001) == at(1.1)", bound{v: v("1.00000000000000000001"), edge: edgeAt}, bound{v: v("1.1"), edge: edgeAt}}, diff --git a/pep440set/bound_test.go b/pep440set/bound_test.go index 09fb492..1803a70 100644 --- a/pep440set/bound_test.go +++ b/pep440set/bound_test.go @@ -125,41 +125,44 @@ func TestBoundOrderingPastInt64(t *testing.T) { } } -// TestCanonDigits exercises the leading-zero stripping DIRECTLY, because -// nothing else does. +// TestLeadingZeroSpellings replaces the leading-zero stripping this package +// used to do for itself. // -// releaseKey's only caller feeds it BaseVersion(), which renders every segment -// through big.Int.String() and so is already leading-zero-free -- no bound -// built from a parsed version can reach the stripping below. That makes -// canonDigits defensive code against a future caller that hands releaseKey a -// spelling gpp has not normalized, and defensive code with no test is how a -// "simplification" that drops it passes review. cmpDigits compares -// length-first, so an unstripped run would sort "007" above "7". -func TestCanonDigits(t *testing.T) { - cases := []struct{ in, want string }{ - {"7", "7"}, - {"007", "7"}, - {"0", "0"}, - {"000", "0"}, - {"0100", "100"}, - {"10", "10"}, - {"", ""}, - {"00000000000000000001", "1"}, +// It stripped because it built its key out of DIGIT RUNS, compared +// length-first, where "007" would have sorted above "7". The key now comes from +// gpp's parsed release segments, which are math/big integers: "007" and "7" are +// the same integer before a key exists, and there is no run to strip. The +// concern is gone rather than moved, but the BEHAVIOUR it protected is not +// optional, so it is asserted here through the real path instead of through a +// helper. +// +// Every pair below must occupy one position: same value, different spelling, +// at widths on both sides of int64 and uint64. +func TestLeadingZeroSpellings(t *testing.T) { + pairs := [][2]string{ + {"1.7", "1.007"}, + {"1.100", "1.0100"}, + {"1.1", "1.00000000000000000001"}, + {"1.099999999999999999990", "1.0099999999999999999990"}, + {"0!1.0", "00!1.0"}, + {"7!1.0", "007!1.0"}, } - for _, tc := range cases { - if got := canonDigits(tc.in); got != tc.want { - t.Errorf("canonDigits(%q) = %q, want %q", tc.in, got, tc.want) + for _, p := range pairs { + a := bound{v: mustV(t, p[0]), edge: edgeAt} + b := bound{v: mustV(t, p[1]), edge: edgeAt} + if got := cmpBound(a, b); got != 0 { + t.Errorf("cmpBound(at(%s), at(%s)) = %d, want 0", p[0], p[1], got) } } - // The point of stripping: the canonical runs must compare equal, and the - // unstripped ones would not. - if got := cmpDigits(canonDigits("007"), canonDigits("7")); got != 0 { - t.Errorf("cmpDigits(canon 007, canon 7) = %d, want 0", got) - } - if got := cmpDigits("007", "7"); got == 0 { - t.Error("cmpDigits compares length first, so unstripped runs should NOT " + - "compare equal; this test no longer shows what canonDigits is for") + // ...and the spelling must not have flattened a real difference either: + // 1.007 is 1.7, NOT 1.70 or 1.0.7. + for _, p := range [][2]string{{"1.007", "1.70"}, {"1.007", "1.0.7"}} { + a := bound{v: mustV(t, p[0]), edge: edgeAt} + b := bound{v: mustV(t, p[1]), edge: edgeAt} + if got := cmpBound(a, b); got == 0 { + t.Errorf("cmpBound(at(%s), at(%s)) = 0; the two are different releases", p[0], p[1]) + } } } diff --git a/pep440set/construct.go b/pep440set/construct.go index 385c954..b9298ad 100644 --- a/pep440set/construct.go +++ b/pep440set/construct.go @@ -431,6 +431,23 @@ func compatibleUpperBound(operand string) (hi bound, ok bool, err error) { } } +// isDigits reports whether s is a non-empty run of ASCII digits. +// +// It lived in bound.go while the release-group key was derived from rendered +// text. That derivation is gone -- the key comes from gpp's parsed fields now +// -- and this is the only remaining caller, so it lives here. +func isDigits(s string) bool { + if s == "" { + return false + } + for i := 0; i < len(s); i++ { + if s[i] < '0' || s[i] > '9' { + return false + } + } + return true +} + // incrementLastSegment turns "1.2" into version 1.3, preserving any epoch. // // ⚠️ The arithmetic is done on the DIGIT STRING, not through strconv. A diff --git a/pep440set/fuzz_test.go b/pep440set/fuzz_test.go index eb07fb0..f8c4c95 100644 --- a/pep440set/fuzz_test.go +++ b/pep440set/fuzz_test.go @@ -85,7 +85,8 @@ func fuzzVersions() []string { // >99999999999999999999.0 // ~=1.0.99999999999999999999 // -// bound.go's releaseKey built its comparison key with strconv.Atoi and BROKE +// bound.go's release-group key -- then a local `releaseKey` helper, since +// replaced by gpp's version.ReleaseKey -- was built with strconv.Atoi and BROKE // out of the loop on error, so a release segment at or above 2^63 // (9223372036854775808) was dropped along with every segment after it. The // key became a PREFIX of the real release -- for the first seed, the empty key diff --git a/pep440set/verpos.go b/pep440set/verpos.go index 91aceb6..194baa9 100644 --- a/pep440set/verpos.go +++ b/pep440set/verpos.go @@ -14,19 +14,29 @@ import ( // A bound's posKey is built eagerly because it is long-lived and SHARED: it // rides along every copy of the bound through the set algebra, so deriving it // lazily would be a data race. A verPos is neither -- Contains builds one, -// probes each span with it, and drops it -- so it can defer the expensive part -// of the derivation, the public-spelling render (and, for a version carrying a -// local label, the re-parse), until a comparison actually descends into a -// release group. Cross-group comparisons, the common case when probing a span -// whose bounds anchor other releases, never pay it. +// probes each span with it, and drops it -- so it can defer the one remaining +// expensive part of the derivation, the public-spelling render (and, for a +// version carrying a local label, the re-parse), until a comparison actually +// descends into a release group. Cross-group comparisons, the common case when +// probing a span whose bounds anchor other releases, never pay it. +// +// The group key is NOT deferred, and no longer needs to be: version.ReleaseKey +// reads the parsed Version's own fields and allocates nothing. It was +// deferred-worthy when this package derived the key by rendering the version +// back to text; see the note above cmpBound. +// +// ⚠️ init is nonetheless the largest thing left inside Contains, and the key is +// not why. It copies version.Version TWICE -- once into p.v, once again for +// ReleaseKey's by-value receiver -- and a parsed Version is a few hundred bytes. +// If this function is ever worth optimizing again, that is the cost to go +// after, not the derivation. // // ⚠️ DO NOT COPY a verPos after init and DO NOT SHARE ONE ACROSS GOROUTINES. // ensurePub mutates it in place; that is safe precisely because exactly one // containment test owns it. type verPos struct { - v version.Version - epoch string - release []string + v version.Version + rel version.ReleaseKey // public, pub and pubOK mirror posKey's fields, filled by ensurePub. // pubDone marks them filled, so an empty public spelling (the zero // Version) is not re-derived per comparison. @@ -45,8 +55,7 @@ type verPos struct { // the hoist-one-out-of-the-loop micro-optimization that would start re-initing // is exactly the kind of change that skips re-reading this file. func (p *verPos) init(v version.Version) { - *p = verPos{v: v} - p.epoch, p.release = releaseKey(v) + *p = verPos{v: v, rel: v.ReleaseKey()} } // ensurePub fills the public-version fields, exactly as newPosKey does. @@ -81,10 +90,7 @@ func cmpVerBound(p *verPos, b bound) int { } bk := b.pos() - if c := cmpDigits(p.epoch, bk.epoch); c != 0 { - return c - } - if c := cmpSegments(p.release, bk.release); c != 0 { + if c := p.rel.Compare(bk.rel); c != 0 { return c } diff --git a/pep440set/verpos_race_test.go b/pep440set/verpos_race_test.go index 44ae5a2..8fac24d 100644 --- a/pep440set/verpos_race_test.go +++ b/pep440set/verpos_race_test.go @@ -15,15 +15,24 @@ import ( // // ⚠️ WHAT A GREEN RUN HERE DOES AND DOES NOT PROVE. This test demonstrates // that concurrent Contains calls on a shared Set are race-free ON THIS PATH. -// It does NOT cover the known upstream hazard -- rstudio/go-version v0.0.2's -// Parts.Padding appending into shared spare capacity in place (go-version -// PR #5, unmerged) -- because on this path that hazard is UNREACHABLE: -// reaching pub.Compare requires cmpSegments(p.release, bk.release) == 0, and -// releaseKey's trailing-zero stripping yields the same segment count as gpp's -// Parts.Normalize, so Padding always sees a zero-length difference and never -// appends. Do not read a green run here as evidence the padding race is -// covered; resolver/concurrency_test.go is the test that reaches it, through -// cross-group Compare in ranking. +// It does NOT cover the historical upstream hazard -- rstudio/go-version +// v0.0.2's Parts.Padding appending into shared spare capacity in place +// (go-version PR #5, unmerged). Two independent reasons, and the first now +// dominates: +// +// - gpp no longer calls Parts.Padding AT ALL. compareVersions pads through +// padParts, into fresh slices. Parts.Padding is still defined in +// go-version v0.0.2 and has no caller anywhere in the graph, so there is +// nothing left in this module's dependencies to race on. +// - On this path it was unreachable even before that: reaching pub.Compare +// requires the two release keys to tie, and version.ReleaseKey strips a +// trailing segment exactly when Parts.Normalize does (both drop a segment +// whose big.Int is zero), so a tie means equal segment counts and Padding +// would have seen a zero-length difference. +// +// Do not read a green run here as evidence about that hazard either way. The +// ".0" probe shapes below are kept because they are what makes this test +// descend to pub.Compare at all, which is the path it does cover. // // The probes still end in ".0" (the spare-capacity shape) and share the // bounds' release group so the ladder genuinely descends to pub.Compare -- @@ -72,9 +81,7 @@ func TestContainsConcurrent(t *testing.T) { continue } bk := b.pos() - if cmpDigits(p.epoch, bk.epoch) != 0 || - cmpSegments(p.release, bk.release) != 0 || - b.tier() != 1 { + if p.rel.Compare(bk.rel) != 0 || b.tier() != 1 { continue } if p.public != bk.public && p.pubOK && bk.pubOK { diff --git a/resolver/bench_test.go b/resolver/bench_test.go index e8c0055..7a2868f 100644 --- a/resolver/bench_test.go +++ b/resolver/bench_test.go @@ -241,6 +241,80 @@ // module's provider. The next honest measurement is of go-pubgrub's conflict // resolution, not of Candidates. // +// ⚠️ THAT SECTION IS SUPERSEDED IN ONE ROW. "CONSTRUCTING the bound for each +// version, 83% of Contains" was the diagnosis; #39 removed the bound and the +// release-key change below removed what remained under it. Contains is now +// 0.6% of samples on app-set, and the "probe memoized alongside the ranked +// list" idea it proposes has nothing left to buy. The rest of the section +// stands. +// +// # THE RELEASE-KEY RESULT, measured 2026-08-14 against 6c13230 (v0.6.0) +// +// pep440set derived a version's (epoch, release) group key by rendering the +// version back to TEXT with BaseVersion() and splitting it apart again, once +// per candidate version per Contains call. It now asks gpp for +// version.ReleaseKey, which reads the parsed Version's own fields. +// +// Medians of three interleaved rounds, ten iterations, full snapshot. A fourth +// round was run afterwards against the PUBLISHED gpp v0.7.0 rather than a local +// replace, and reproduced every entry within noise (app-set 1.40x, wide-versions +// 1.24x) except unsatisfiable; see the warning below it. +// +// entry warm ms warm allocs/op +// before after before after +// single-no-deps 0.21 0.15 3,637 2,260 1.38x / 1.61x fewer +// small-tree 1.06 0.80 19,393 10,908 1.32x / 1.78x +// extras 1.45 1.08 29,391 15,102 1.35x / 1.95x +// app-set 4.54 3.27 96,318 42,415 1.39x / 2.27x +// wide-versions 8.11 6.74 197,228 127,659 1.20x / 1.54x +// backtracking 2.78 2.33 43,304 21,015 1.19x / 2.06x +// unsatisfiable 0.33 0.31 6,448 5,168 1.07x / 1.25x +// +// entry cold ms cold allocs/op +// before after before after +// single-no-deps 0.27 0.23 4,365 2,982 1.17x / 1.46x +// small-tree 1.41 1.10 23,609 15,107 1.28x / 1.56x +// extras 1.82 1.46 34,630 20,354 1.25x / 1.70x +// app-set 5.94 4.70 118,465 64,533 1.26x / 1.84x +// wide-versions 13.14 11.86 276,776 207,206 1.11x / 1.34x +// backtracking 6.22 5.26 89,097 66,832 1.18x / 1.33x +// unsatisfiable 0.55 0.51 9,256 7,974 1.08x / 1.16x +// +// ⚠️ READ THE ALLOCATION COLUMN. The saving is not arithmetic, it is garbage: +// app-set allocates 2.27x less warm while running 1.39x faster. +// +// In the warm profile Set.Contains falls from 41% of resolver.Resolve to 24% on +// app-set, and from 33% to 14% on wide-versions -- 3.1x and 3.8x fewer samples +// in absolute terms. The group key was 82% of Contains on app-set and 74% on +// wide-versions before. +// +// ⚠️ AGAINST Resolve, NOT against total samples. The denominator has to be +// something the change does not move, and total samples is not: removing this +// allocation shrinks the profile's garbage-collection share too, so a +// "% of samples" figure would credit this change with that as well. The same +// binary's Contains share swings by 7x purely by changing -benchtime. +// +// ⚠️ strings.Split leaves the profile entirely, but math/big.nat.itoa and +// version.writeRelease DO NOT: ensurePub still renders the public spelling, +// which this change does not touch. They fall from 2.3% and 1.6% of samples +// under Contains to below the sampling floor, which is not the same as gone. +// +// ⚠️ unsatisfiable IS NOT MEASURABLY FASTER. Its 1.07x is a median of three, and +// a fourth confirmation round put it at 0.96x -- slower. Read it as no change. +// That is the prediction met rather than a disappointment: it fails before +// enumerating many candidates, so it makes the fewest Contains calls in the +// corpus and has the least to gain. Its allocations do fall, by 1.25x, which is +// the effect appearing where the mechanism says it should. An entry that gained +// where it has no mechanism to would mean the measurement, not the change, was +// doing the work. +// +// ⚠️ THIS IS MEASURED WITHOUT THE PARSED-VERSION MEMO, and the two are NOT +// additive. After this change the largest remaining warm cost is version.Parse +// inside index.Versions -- 16.9% of samples on wide-versions -- which is exactly +// what that memo removes. Whichever lands second must re-measure against the +// first rather than carrying this column forward, for the same reason #39 and the +// ranking change needed a 2x2. +// // # ⚠️ THE found/rank RESULT, measured 2026-08-13 // // Both sides below were measured in ONE session on this machine, warm, ten @@ -504,10 +578,10 @@ // releaseKey 3.35 s // // Index calls are 11,029 and barely register. The work is version-set algebra -// inside the solver's conflict resolution, and under it releaseKey, which -// re-derives its ordering key from version.BaseVersion() -- a string render -- -// on every bound comparison. So the two regimes have two different dominant -// costs, and a fix aimed at one does nothing for the other. +// inside the solver's conflict resolution, and under it a `releaseKey` helper +// that re-derived its ordering key from version.BaseVersion() -- a string +// render -- on every bound comparison. So the two regimes have two different +// dominant costs, and a fix aimed at one does nothing for the other. // // ⚠️ Those figures are the PRE-FIX baseline: they were taken before // rstudio/package-manager#19713 derived a bound's key once, which took the same @@ -515,6 +589,11 @@ // that an incomplete index has a completely different dominant cost from a // complete one, so the memo above does nothing for it -- survives the fix. // +// ⚠️ `releaseKey` no longer exists at all: the release-key change above replaced +// it with gpp's version.ReleaseKey, which derives the same key from the parsed +// fields without rendering anything. The profile is kept as history, and the +// frame name in it will not be found in the tree. +// // The measurement remains the deliverable of rstudio/package-manager#18651. The // two optimizations now in the module (#19713's set algebra, and the parsed memo // this file re-baselines) were each landed because this benchmark named them, From 4fd18676ca03ca7c68cb618924823d42c2399b88 Mon Sep 17 00:00:00 2001 From: Jonathan Yoder Date: Fri, 14 Aug 2026 14:05:47 -0400 Subject: [PATCH 2/2] docs(changelog): the nanosecond figures are load-sensitive, the allocation counts are not An unbriefed review seat measured 23 ns / 520 ns for gpp's ReleaseKey benchmark where this entry quotes 16 ns / 220 ns. Both are real: mine was an idle machine, theirs was running mutation tests. The ratio survives either way and the allocation counts -- 0 against 10 -- are identical on both, so those are what the entry now leads with. A reader who needs a time should run the benchmark. --- CHANGELOG.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b173cf3..0cd1741 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,8 +27,14 @@ served it. then splitting the result back into digit runs. Once per candidate version per `Contains` call. It now asks go-python-packaging for `version.ReleaseKey` instead, which reads the parsed `Version`'s own epoch and - release fields: ~16 ns and no allocation, against ~220 ns and 10 allocations - for the render-and-split. + release fields: **no allocation, against 10** for the render-and-split, and + roughly an order of magnitude less time. + + ⚠️ The allocation counts are exact and machine-independent; the times are + not. gpp's `BenchmarkReleaseKeyVsBaseVersionSplit` reads 16 ns vs 213 ns on + an idle M4 Max and 23 ns vs 520 ns on a loaded one — the ratio survives, a + quoted nanosecond figure does not. Run the benchmark rather than trusting a + number written down here. Medians of three interleaved rounds, ten iterations, against the production snapshot (932,861 packages, dated 2026-08-04), on an Apple M4 Max: