diff --git a/CHANGELOG.md b/CHANGELOG.md index afa8034..0cd1741 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,84 @@ 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: **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: + + | 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,