From ab741fb229dd00e2ab27e250dd4808b0379ebba1 Mon Sep 17 00:00:00 2001 From: Jonathan Yoder Date: Fri, 14 Aug 2026 13:18:31 -0400 Subject: [PATCH] perf(index): memoize parsed versions, not just version keys (#42) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RSFIndex.Versions memoized the sorted order of a package's version keys and re-parsed every one of them on every call. It held strings because it had to: until go-python-packaging v0.6.0 a version.Version could not be shared between goroutines, since Version.Compare padded the shorter operand's release segment with append into spare capacity a by-value copy shares. v0.6.0 fixed that -- packable versions never touch part.Parts, and the fallback pads by copying -- and this takes the memo the fix unblocks. Warm resolution is 1.13x to 3.23x faster and allocates 41% to 94% fewer objects against the production snapshot, measured at base 11da678 over nine interleaved rounds. Cold does not improve, as expected: the first call already parsed the keys in order to sort them. candvers, metadata and the pin set are identical on every corpus entry, and 4,007 resolutions produce byte-identical transcripts with zero one-sided timeout exclusions. ⚠️ These figures are larger than the same change measured against 6c13230 (1.24x and 2.27x). #46 shrank the denominator rather than the parse: parseKeys went from 47.4% of wide-versions' Resolve to 69.1%. Neither figure is quotable against the other's base. The returned slice is a COPY of the memo's, because cmd/pyresolve's `versions` subcommand sorts the result in place. Measured at -0.3% to +9.7% warm against a variant that shares -- higher than the "under 2%" this was scoped with, for the same denominator reason. It is also redundant on this module's own resolution path, since provider hands the result straight to candidate.Rank, which copies again; it protects the exported contract, and an internal accessor that skips it is a follow-up worth taking at that price. The cost is retained heap rather than churn, measured rather than estimated: a warmed index keeps roughly three times as much alive after a resolve (app-set 0.35 -> 0.99 MB, wide-versions 1.31 -> 4.41 MB), identical at both bases. The memo is left unbounded to match the blob cache's rationale, and the code now says plainly that a bound is a prerequisite for embedding this in a long-lived server. index/shared_memo_test.go shares one warmed memo across eight goroutines over fixtures chosen for the padding hazard's actual shape, covering both of v0.6.0's safety arguments and also driving version.ReleaseKey, which #46 made a second reader of a shared parsed version. Every fixture reports WARNING: DATA RACE under a v0.5.0 pin, 20 fresh processes each. That guard runs in CI on the -race step #44 added, which this is rebased onto. Without it these tests are a green no-op: measured on a tree with the defensive copy deleted, the pre-existing concurrency check catches it 8 times in 20 under -race and 0 times in 20 without. Sweeps the rationales that asserted the memo was impossible, in index/rsfindex.go, index/mock.go, provider/provider.go and resolver/bench_test.go. The CHANGELOG diff is pure insertion; no dated section is touched. --- CHANGELOG.md | 185 ++++++++++ index/mock.go | 75 +++- index/rsfindex.go | 356 +++++++++++-------- index/rsfmemo_test.go | 62 +++- index/shared_memo_test.go | 702 +++++++++++++++++++++++++++++++++++++ provider/provider.go | 50 ++- resolver/bench_test.go | 29 +- resolver/peak_heap_test.go | 111 ++++++ 8 files changed, 1371 insertions(+), 199 deletions(-) create mode 100644 index/shared_memo_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a1e3cca..d1ab651 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,190 @@ served it. ### Changed +- **`index.RSFIndex` memoizes PARSED versions, not just version keys. Warm + resolution is 1.13x to 3.23x faster and allocates 41% to 94% fewer objects**, + with no change to any resolution this module produces. + + `RSFIndex.Versions` memoized the sorted order of a package's version keys and + re-parsed every one of them on every call. It held strings rather than parsed + values because it had to: until go-python-packaging v0.6.0, a `version.Version` + could not be shared between goroutines. That was fixed upstream in 0.6.0, and + this takes the memo the fix unblocks. + + Re-parsing was worth taking. Profiled against the production snapshot as a + share of `resolver.Resolve`'s own cumulative cost, `parseKeys` was **22.7% of a + warm `app-set` resolution and 69.1% of a warm `wide-versions` one**, and + **58.4% and 94.8% of the objects** those resolutions allocated. + + Medians of **nine** interleaved rounds, 500 iterations, against the production + snapshot (932,861 packages, dated 2026-08-04), on an Apple M4 Max, base + `11da678`: + + | entry | warm before | warm after | | allocs/op before | after | | + |---|---|---|---|---|---|---| + | `single-no-deps` | 0.164 ms | 0.090 ms | 1.82x | 2,258 | 542 | −76.0% | + | `small-tree` | 0.795 ms | 0.555 ms | 1.43x | 10,901 | 4,634 | −57.5% | + | `extras` | 1.110 ms | 0.836 ms | 1.33x | 15,114 | 7,143 | −52.7% | + | `app-set` | 3.164 ms | 2.318 ms | 1.37x | 42,400 | 18,906 | −55.4% | + | `wide-versions` | 6.271 ms | 1.944 ms | 3.23x | 127,661 | 7,181 | −94.4% | + | `backtracking` | 2.235 ms | 1.985 ms | 1.13x | 21,020 | 12,395 | −41.0% | + | `unsatisfiable` | 0.325 ms | 0.221 ms | 1.47x | 5,170 | 2,054 | −60.3% | + + ⚠️ **These figures are LARGER than the same change measured against `6c13230`, + and that is not a mistake in either.** Measured before 0.7.0 landed, this was + 1.24x on `app-set` and 2.27x on `wide-versions`. 0.7.0 removed a render-and-split + from `pep440set.Contains`, which does not make the parse cheaper — it makes the + parse a **bigger share of what is left**, and a proportional saving is worth + more against a smaller denominator. `parseKeys` went from 47.4% of + `wide-versions`' `Resolve` to 69.1% on exactly that account. The two changes + are not additive in either direction, and neither figure is quotable against the + other's base. + + Amdahl reconciles almost exactly on the entry where the parse dominates: + removing 69.1% predicts 3.24x and `wide-versions` measures 3.23x. On `app-set` + it under-predicts — 22.7% predicts 1.29x against a measured 1.37x — because the + change also removes 58.4% of the objects allocated and the collector's share + goes with them. + + **Cold does not improve**, 0.96x to 1.01x with allocation flat to four + significant figures. That is the expected shape: the first call per package + already parsed its keys in order to sort them, so there is nothing to save, and + what remains is the copy this now makes on the way out. `wide-versions` + (botocore, over ten thousand releases) gains most warm for the same reason it + gained most from the packed comparison key — one package dominates its own + version list. + + ⚠️ **`unsatisfiable` moves here (1.47x) and 0.7.0's note that it "is not + measurably faster" still stands.** That note is about `Contains` calls, which + this entry makes few of. It makes version-key parses like everything else, and + that is what this removes. Two different mechanisms, and the earlier caveat is + not superseded. + + The RFD 0001 Phase 3 warm gate (under 1 ms) is now met by **4 of 7** corpus + entries against 3 of 7 at the base: `extras` crosses at 0.836 ms. + + `candvers`, `metadata` and the pin set are **identical** on every entry. A memo + changes what a call costs, never how many calls there are or what they answer. + + ⚠️ **The cost is RETAINED HEAP, and it is not small in the shape that matters.** + The parsed versions of every package ever asked about now live for the life of + the index, where they used to be transient garbage collected between calls. + Measured rather than estimated, by `TestIndexRetainedHeapAfterResolve` (new, + `GPR_RETAIN=1`), as the live heap one warmed `RSFIndex` keeps alive after a + resolution completes — medians of five interleaved rounds, which agreed to the + centibyte on every entry, and which came out **identical at both `6c13230` and + `11da678`**: this is a live-heap measurement, so unlike the timings it does not + move with the base or with machine load. + + | entry | retained before | retained after | | + |---|---|---|---| + | `single-no-deps` | 0.01 MB | 0.06 MB | 6.0x | + | `small-tree` | 0.07 MB | 0.24 MB | 3.4x | + | `extras` | 0.09 MB | 0.30 MB | 3.3x | + | `app-set` | 0.35 MB | 0.99 MB | 2.8x | + | `wide-versions` | 1.31 MB | 4.41 MB | 3.4x | + | `backtracking` | 0.32 MB | 0.56 MB | 1.8x | + | `unsatisfiable` | 0.04 MB | 0.12 MB | 3.0x | + + Roughly **triple**, on closures of seven to eighteen packages. Every caller in + this module builds an `RSFIndex` per resolve and drops it, so for them this is a + few megabytes that never accumulate, and the memo is left unbounded to match the + blob cache's existing rationale. + + ⚠️ **For a long-lived server that rationale does not carry, and this change is + what makes the difference material.** "Bounded by the corpus" is a property of + the KEY SET, not of the memory: the corpus is 932,861 packages, and this memo + multiplies what each cached package costs. A bound over the index's caches is a + **prerequisite** for embedding this in a server process, not an optimization to + revisit later. That is written into `index/rsfindex.go` beside the memo rather + than left in a changelog, because the previous version of this rationale was + true of the CLI and quietly wrong for a server, twice. + + **Equivalence**, since this changes which parsed values a resolution sees. + 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,608 that + fail. 3,973 cases were compared, of which 2,365 pinned something. The 34 cases + excluded by the 8-second per-case deadline timed out on **both** sides, so + nothing was dropped from one build's column and not the other's. + + **Concurrency.** Sharing one parsed `version.Version` across goroutines is the + thing 0.6.0 made legal, and `index/shared_memo_test.go` is new and pins it: + eight goroutines share a warmed memo and compare the same parsed value, across + four fixtures covering **both** of 0.6.0's independent safety arguments (the + packed integer key, and the `padParts` fallback that a local label, a non-zero + epoch or a long release segment forces). Each was confirmed to report + `WARNING: DATA RACE` with the dependency pinned back to v0.5.0, 20 fresh + processes each, 80 for 80. It also drives `version.ReleaseKey`, which + `pep440set` made a second reader of a shared parsed version in 0.7.0. + + **These run in CI**, on the `Race` step the entry below adds. That matters more + than it sounds: without `-race` they assert nothing about the hazard, because + the racing writes store the same bytes at the same address. Measured on a tree + with the defensive copy deleted — the pre-existing concurrency test detects it + 8 times in 20 under `-race` and **0 times in 20 without it**. + + ⚠️ Two claims corrected here after review, both of which had been stated as + measured. Under a v0.5.0 pin these are **not** the only tests that object: + `TestMemoIsSafeUnderConcurrentUse` reports a race in **8 of 20** fresh + processes. The earlier claim came from a single process, which is exactly the + race-detector deduplication trap documented three paragraphs down. And the + general comparison path's allocations are **not** `padParts` copying — a pair + whose release lengths already match still allocates 17 per comparison — so the + allocation guard discriminates the two paths, but the count does not measure the + copy. + + ⚠️ Three findings from doing that. First, the fixtures have to be chosen for the + hazard or the test is decorative: the shared operand must end in a stripped + trailing zero (that is where the spare capacity comes from) **and** be the + shorter of the pair. `3.11` is immune; `3.11.0` races. Second, the pre-existing + `TestMemoIsSafeUnderConcurrentUse` does **not** reliably cover this: every + goroutine there calls `Versions` once against a COLD memo, so most build their + own plan and share nothing, and with the defensive copy deleted it notices + **8 runs in 20** against **20 in 20** for the warm-memo test. Third, Go's race + detector deduplicates by stack within a process, so verifying the four subtests + with `-count=8` in one process makes fixtures that detect the race every time + look one-in-eight flaky. Each verification run must be its own process. + + **The returned slice is a copy.** `cmd/pyresolve`'s `versions` subcommand sorts + the result of `Versions` in place, so handing back the memo's own slice would + have been a silent breaking change. Its cost is measured, warm, medians of nine + interleaved rounds against a variant that returns the memo's own slice: **−0.3% + to +9.7%**, worst on `wide-versions` (+9.7%) and `backtracking` (+8.1%), where + one package carries over ten thousand releases and a `version.Version` is a + large struct. That cost is already inside every figure in the table above. + + ⚠️ That is **not** the "effectively free, under 2%" this change was scoped with, + and the difference is the base rather than the copy: against `6c13230` it + measured −0.1% to +4.0%. The copy costs the same microseconds; warm resolution + got faster, so the same microseconds are a larger fraction. A cost quoted as a + percentage has a denominator, and this one moved. + `TestVersionsMemoIsNotAliasedByTheCaller` was written for this day and could not + fail until now. + + ⚠️ It is **redundant on the resolution path** — `provider` hands the result + straight to `candidate.Rank`, which copies unconditionally, so a resolution + copies twice. The copy is there for the **exported** contract: `cmd/pyresolve`'s + `versions` subcommand and external consumers. + + ⚠️ That does **not** make an internal no-copy accessor free, which an earlier + draft of this entry implied. `FilteredIndex.Versions` and `MultiIndex.Versions` + also call it, and `FilteredIndex` returns the inner index's slice **by + reference** when its policy filters neither pre-releases nor files. Routing a + no-copy accessor through that fast path would hand the memo's own slice to an + arbitrary caller. The optimization is real and it has a prerequisite. + + ⚠️ Deleting the copy is caught by three tests and **all three are in `index/`**. + Nothing outside that package notices, `cmd/pyresolve`'s own tests included — and + `cmd/pyresolve` is the caller that sorts the result in place. It gets away with + it only because one invocation calls `Versions` once. + + Stale rationales asserting that a parsed `version.Version` cannot be shared are + swept from `index/rsfindex.go`, `index/mock.go`, `provider/provider.go` and + `resolver/bench_test.go`. Dated changelog sections are left alone: they were + accurate when written. + - **CI now runs `go test -race`, and the shareability guarantee is a test rather than a paragraph.** No behaviour change; this is test and CI only. @@ -64,6 +248,7 @@ served it. excluded from **both**: on this 139-package excerpt it failed to finish under bounds of 20 s, 60 s, 3 min and 10 min, so no deterministic transcript entry for it exists at any deadline. + ## [0.7.0] - 2026-08-14 ### Changed diff --git a/index/mock.go b/index/mock.go index f316c04..9ac1eb3 100644 --- a/index/mock.go +++ b/index/mock.go @@ -43,15 +43,37 @@ type mockPackage struct { // order preserves insertion order so Versions can return a deterministic // but deliberately unsorted result. See MockIndex.Versions. // - // ⚠️ NORMALIZED VERSION STRINGS, NOT PARSED VALUES, for the same reason - // RSFIndex.Versions memoizes keys: a version.Version MUST NOT BE SHARED - // BETWEEN GOROUTINES. Version.Compare pads the shorter operand's release - // segment with append, into spare capacity that a by-value copy shares, so - // handing every caller a copy of one stored Version means two goroutines - // ranking candidates write to the same backing array. Holding these as - // parsed values made eight concurrent resolutions against one shared - // MockIndex fail `go test -race` inside candidate.Rank -- see - // resolver/concurrency_test.go, which is what found it. + // NORMALIZED VERSION STRINGS, NOT PARSED VALUES. + // + // ⚠️ HISTORICAL as of go-python-packaging v0.6.0, and kept because it is the + // reason this field has the type it has. It used to be forced: a + // version.Version could not be shared between goroutines, because + // Version.Compare padded the shorter operand's release segment with append + // into spare capacity that a by-value copy shares -- so handing every caller + // a copy of one stored Version meant two goroutines ranking candidates wrote + // to the same backing array. Holding these as parsed values made eight + // concurrent resolutions against one shared MockIndex fail `go test -race` + // inside candidate.Rank. + // + // v0.6.0 fixed that upstream, RSFIndex now memoizes parsed versions, and + // strings here are a choice rather than a constraint. Changing it would be a + // pure convenience change with nothing behind it -- a mock's version lists + // are a handful of entries -- so it has not been made. + // + // ⚠️ resolver/concurrency_test.go found the race in the VARIANT described + // above -- a mock that stored parsed values -- and it does not guard the + // hazard on shipped code, which is a different claim than the credit reads + // as. Measured at go-pyresolver 6c13230 with the dependency pinned back to + // v0.5.0, 20 fresh processes: that test passes, and so do provider, candidate + // and pep440set. What objects is index/shared_memo_test.go (20/20), this + // package's own TestMemoIsSafeUnderConcurrentUse (8/20, a lottery), and + // index/shared_version_test.go, which #44 added for the same hazard one level + // down. Nothing that ever shipped shared a parsed Version between goroutines + // until the parsed-version memo did. + // + // ⚠️ "pep440set passes" is a statement about THAT tree. On the current base a + // v0.5.0 pin does not compile pep440set at all -- it calls version.ReleaseKey, + // which is gpp v0.7.0. See the scope note in shared_memo_test.go. order []string // versions holds per-version state, keyed by normalized version string. @@ -260,10 +282,25 @@ func (m *MockIndex) lookup(pkg PackageName, ver version.Version) (*mockVersion, // here and then fail against a real index. Reverse insertion order breaks that // assumption without the flakiness a shuffle would introduce. // -// ⚠️ Each version is RE-PARSED per call, so no two callers ever hold copies of -// one version.Version. See mockPackage.order for the data race that costs, and -// note that RSFIndex re-parses here for exactly the same reason -- the two -// implementations agreeing is the point of the mock. +// Each version is RE-PARSED per call, so no two callers ever hold copies of one +// version.Version. +// +// ⚠️ That used to be a REQUIREMENT and is now merely what this does. Sharing a +// parsed version.Version between goroutines was a data race until +// go-python-packaging v0.6.0; it is not one now, and RSFIndex has stopped +// re-parsing -- it memoizes the parsed versions and returns a copy of the memo's +// slice. So the two implementations no longer agree on this point, which matters +// because agreeing is the point of a mock. +// +// The divergence is deliberate and it is safe in ONE direction only. What a +// caller may do with the returned slice is identical: both hand back a slice +// nothing else holds, so sorting or overwriting it is fine against either. What +// differs is that a mock-backed test can no longer detect a caller that depends +// on getting a FRESH parse each call -- against RSFIndex two calls hand out +// copies of one parse. Nothing in this module does that, and no interface +// promises it. If MockIndex ever needs to model the real thing more closely, the +// change is to memoize here too; do not make RSFIndex re-parse to restore the +// symmetry. // // A parse failure is impossible: every string in order was produced by // Version.String() on a value Parse accepted. Reported rather than swallowed @@ -323,11 +360,13 @@ func (m *MockIndex) Metadata(ctx context.Context, pkg PackageName, ver version.V // missing from both for exactly as long as it was missing from either, so a // caller mutating it was invisible to the mock as well. // - // ⚠️ Version comes from the CALLER'S OWN value, not from the stored - // metadata, and that is the same concurrency requirement RSFIndex's - // cloneMetadata documents: a stored version.Version handed to every caller - // is one shared between goroutines. Not observable, because lookup matched - // on ver.String() and the setup methods force the stored Version to the key. + // Version comes from the CALLER'S OWN value, not from the stored metadata, + // matching RSFIndex's cloneMetadata. ⚠️ That was a concurrency requirement + // until go-python-packaging v0.6.0 and is not one now -- see the note on + // mockPackage.order. It is kept because cloneMetadata keeps it, for + // cloneMetadata's own second reason, and because the two must agree. Not + // observable either way here, because lookup matched on ver.String() and the + // setup methods force the stored Version to the key. out := *mv.metadata out.Version = ver out.RequiresDist = append([]requirement.Requirement(nil), mv.metadata.RequiresDist...) diff --git a/index/rsfindex.go b/index/rsfindex.go index 683f706..7c95f4b 100644 --- a/index/rsfindex.go +++ b/index/rsfindex.go @@ -65,9 +65,10 @@ type RSFIndex struct { parsed map[PackageName]map[string]memoEntry // versionList memoizes what Versions computed for a package: the winning - // stored key of each PEP 440 equality class, sorted, plus the alias index - // Metadata resolves through. Keys rather than parsed versions on purpose -- - // see Versions. One entry per package, so it is bounded by the corpus. + // stored key of each PEP 440 equality class, sorted, the PARSED version of + // each of those keys, and the alias index Metadata resolves through. One + // entry per package, so its key set is bounded by the corpus -- but see the + // retention note above deps for what the parsed half costs per entry. versionList map[PackageName]versionPlan } @@ -209,10 +210,11 @@ func (idx *RSFIndex) deps(pkg PackageName) (map[string]pypirsf.VersionDeps, erro // requirements at all (types.go: "re-parsing per candidate during resolution // is pure waste"). // -// - versionList holds only the ORDER Versions computed -- stored keys, as -// strings -- and every call re-parses them. It cannot hold the parsed -// versions, because a version.Version cannot be shared between goroutines. -// See Versions for the measurement behind that. +// - versionList holds the ORDER Versions computed -- the stored keys -- AND +// the parsed version of each. It held keys alone until go-python-packaging +// v0.6.0, because a version.Version could not then be shared between +// goroutines; that constraint is gone and the parsed half is memoized. See +// Versions for the measurement, and the retention note below for the cost. // // # What this does NOT change // @@ -251,8 +253,31 @@ func (idx *RSFIndex) deps(pkg PackageName) (map[string]pypirsf.VersionDeps, erro // a deep copy of a parsed requirement graph would cost more than the parse it // replaces. // -// Versions needs no such copy: it re-parses, so what it returns was never in the -// memo. +// Versions makes the same copy, and for the same caller. It used to need none, +// because it re-parsed and so what it returned was never in the memo; now that +// the memo holds the parsed values, handing back plan.versions would let +// cmd/pyresolve's `versions` subcommand sort the cache. +// +// It is a memmove of already-parsed values, and its cost is measured rather than +// waved away -- see the CHANGELOG for the warm figures against a variant that +// hands back the memo's own slice. +// +// ⚠️ It is REDUNDANT on the RESOLUTION path specifically, and saying more than +// that would be overclaiming. provider.Provider passes what it gets straight into +// candidate.Rank, which copies unconditionally and never writes to its argument, +// so a resolution copies twice. The copy is there for the EXPORTED contract: +// cmd/pyresolve's `versions` subcommand, and whatever an external consumer does +// with a slice an exported method handed it. +// +// ⚠️ AN INTERNAL NO-COPY ACCESSOR IS NOT THEREFORE FREE, and an earlier draft of +// this note said it was, on the strength of "provider is the only library-side +// caller". It is not the only one. FilteredIndex.Versions and MultiIndex.Versions +// both call it, and FilteredIndex has a PASS-THROUGH fast path: with a policy +// that filters neither pre-releases nor files it returns the inner index's slice +// by reference (filtered.go). Wiring a no-copy accessor through it would hand the +// memo's own slice to an arbitrary external caller -- the exact silent breaking +// change this copy exists to prevent. MultiIndex is safe because it always +// rebuilds. Whoever takes that optimization owns FilteredIndex's fast path first. // // # Bounded by the corpus, like the blob cache // @@ -270,6 +295,28 @@ func (idx *RSFIndex) deps(pkg PackageName) (map[string]pypirsf.VersionDeps, erro // for. What a memo adds is the CONSTANT: parsed requirements alongside the raw // strings, for the subset of (package, version) actually asked about. // +// ⚠️ A BOUNDED KEY SET IS NOT BOUNDED MEMORY, and versionList's parsed half is +// where that distinction stops being academic. It holds a version.Version per +// stored key where it used to hold a string header, and a version.Version is a +// large struct. Measured rather than estimated, as the live heap one warmed index +// keeps alive after a resolution finishes (resolver/peak_heap_test.go, +// TestIndexRetainedHeapAfterResolve, medians of five interleaved rounds against +// the production snapshot): app-set 0.35 MB -> 0.99 MB, wide-versions 1.31 MB -> +// 4.41 MB. Roughly triple, on closures of seven to eighteen packages. +// +// For a CLI that builds an index per resolve and drops it, a few megabytes that +// never accumulate is a rounding error. For a long-lived server holding one index +// over the whole corpus it is not: a production PyPI snapshot carries 932,861 +// packages, so "bounded by the corpus" must not be read as "small". +// +// ⚠️ And the retention is not paid only by callers who benefit from it. +// versionPlanFor builds the same plan on the Metadata path, so a consumer that +// calls Metadata and never calls Versions pays the full increase and gets none of +// the speedup -- findEqualKey still re-parses plan.order rather than probing +// plan.versions. Taking that follow-up would close the gap; until then it is a +// real asymmetry and it lands on exactly the long-lived consumer this note is +// about. +// // ⚠️ This did not come for free, and the earlier draft of this change did NOT // have the property. Keying parsed by the request's ver.String() -- the obvious // key -- made the memo unbounded by anything the corpus controls: 20,000 @@ -282,12 +329,19 @@ func (idx *RSFIndex) deps(pkg PackageName) (map[string]pypirsf.VersionDeps, erro // requests -- the exact consumer deps' own note says is out of scope -- would // have grown until it was restarted. // -// The remaining growth, in both caches, is one entry per package and version a -// process has genuinely been asked about, which for a long-lived server against -// the whole corpus is still the corpus. A bound is one policy over the PAIR -// rather than two, since bounding the memo alone would leave deps holding the -// same package set in raw form. It is still not needed here: every caller in -// this module creates an RSFIndex per resolve. +// The remaining growth, in all three caches, is one entry per package and +// version a process has genuinely been asked about, which for a long-lived +// server against the whole corpus is still the corpus. A bound is one policy over +// the SET rather than three, since bounding one would leave the others holding +// the same package set in another form. +// +// ⚠️ It is not needed by any caller IN THIS MODULE -- each creates an RSFIndex +// per resolve -- and that sentence has now been true of a claim that was +// nonetheless wrong for a server twice over (see the ver.String() keying above). +// So, plainly: the parsed-version memo MULTIPLIES the per-package retention, and +// a bound is a prerequisite for embedding this index in a long-lived server +// process, not an optimization to consider later. Whoever does that integration +// owns it before the first resolve, not after the first out-of-memory. // lookupMetadata reads the parsed memo. key is a STORED version key, not a // caller's rendering -- see memoEntry. ok is false when nothing is memoized for @@ -323,26 +377,25 @@ func (idx *RSFIndex) storeMetadata(pkg PackageName, key string, entry memoEntry) // from ver. See the memo notes above for why the copy is made and why it is // shallow. // -// ⚠️ Version is overwritten with the CALLER'S OWN value rather than served from -// the memo, and that is a concurrency requirement, not a tidiness one. A -// version.Version must not be shared between goroutines: Version.Compare pads -// the shorter release segment with append, into spare capacity a by-value copy -// shares, so two goroutines comparing two copies write to the same memory. See -// Versions for the full account. The memo would otherwise hand the FIRST -// caller's Version to every later caller on every goroutine. ver is a value the -// caller already owns, so returning it shares nothing new. -// -// The substitution is not observable, and not because the two render alike -- -// they need not, since the memo is keyed by the STORED key and a caller may have -// spelled the version differently. It is not observable because there is no -// other value on offer: Metadata zeroes Version before storing, so the memo has -// never held one. Returning the caller's own value is the only thing this can -// do, and it is what buildMetadata did before the memo existed. -// -// Nothing else in PackageMetadata carries a version.Version. Requirement and -// Marker store their operands as strings and parse per call, verified against -// go-python-packaging v0.5.0, which is why the parsed requirements CAN be -// shared. +// Version is overwritten with the CALLER'S OWN value rather than served from the +// memo. That WAS a concurrency requirement: under go-python-packaging v0.5.0 and +// earlier a version.Version could not be shared between goroutines at all, so +// handing the first caller's Version to every later caller was a data race. It is +// no longer one -- v0.6.0 pads into a fresh slice, and Versions now memoizes +// parsed versions on the strength of that -- so this substitution survives on its +// SECOND reason alone, which is the one that was always sufficient. +// +// That reason: there is no other value on offer. Metadata zeroes Version before +// storing, so the memo has never held one. Returning the caller's own value is +// the only thing this can do, and it is what buildMetadata did before the memo +// existed. Nor is the substitution observable through a rendering difference -- +// the memo is keyed by the STORED key and a caller may have spelled the version +// differently, so the two need not render alike. +// +// ⚠️ Do not now "simplify" this by storing the first caller's Version and serving +// it: that is safe from a data-race standpoint today, and it would still tell the +// second caller a different version string than it asked about. See unusableErr, +// which declines to memoize a finished message for exactly the same reason. // // # The copy policy, field by field // @@ -418,56 +471,62 @@ func cloneMetadata(m PackageMetadata, ver version.Version) PackageMetadata { // meant is unknowable from the snapshot, and a caller still cannot detect that a // class was collapsed. // -// # Memoized per package -- as KEYS, not as parsed versions -// -// What is memoized is the ORDER: the winning stored key of each equality class, -// already sorted and deduped. Each call still parses those keys into fresh -// version.Version values. -// -// That looks like leaving the obvious win on the table, and it is not. The sort -// is where the time went -- 0.84 s of the 0.91 s this method cost on the -// corpus's app-set entry -- because sorting n versions is O(n log n) PEP 440 -// comparisons while parsing them is n parses. Memoizing the order removes the -// comparisons and keeps the parses. -// -// Metadata reads the same memo, binary-searching it to turn a caller's version -// into the stored key it names. That is not an incidental reuse: it is what -// makes "the version Versions hands out" and "the record Metadata resolves for -// it" the same choice by construction rather than by two call sites agreeing on -// preferKey. See resolveStoredKey. -// -// ⚠️ 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 -// array rather than reallocating. A by-value copy of a Version copies the slice -// HEADER, so two goroutines comparing two copies write to the same memory. -// -// Verified, not inferred: a memo holding parsed versions makes eight concurrent -// resolutions against one shared RSFIndex fail `go test -race`, and the same -// test passes both without the memo and with the memo holding keys. The writes -// happen to store the same value at the same address, so the corruption is -// 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 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 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. 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. +// # Memoized per package -- the order AND the parsed versions +// +// What is memoized is the ORDER (the winning stored key of each equality class, +// already sorted and deduped) and the PARSED version of each of those keys. A +// call after the first neither sorts nor parses; it copies. +// +// Memoizing the order came first and bought the larger share: sorting n versions +// is O(n log n) PEP 440 comparisons while parsing them is n parses, and the sort +// was 0.84 s of the 0.91 s this method cost on the corpus's app-set entry. What +// remained was the n parses, and by 0.7.0 they were most of what was left: +// profiled as a share of resolver.Resolve's own cumulative cost, re-parsing the +// stored keys was 22.7% of a warm app-set resolution and 69.1% of a warm +// wide-versions one, and 58.4% and 94.8% of the objects those resolutions +// allocated. Memoizing them is worth 1.37x and 3.23x end to end. +// +// ⚠️ Those shares are of THIS tree. The same memo measured against 0.6.0 was +// 1.24x and 2.27x, because 0.7.0's pep440set change shrank the denominator rather +// than the parse. A percentage-of-Resolve figure is a fact about one base; see +// the CHANGELOG, which records both and why they differ. +// +// ⚠️ THIS COULD NOT BE DONE BEFORE go-python-packaging v0.6.0, and the reason is +// worth keeping because it is the reason the memo has the shape it has. Under +// v0.5.0 and earlier a version.Version could not be shared between goroutines at +// all: Version.Compare padded the shorter operand's release segment with +// `append`, and cmpkey built that segment by RESLICING away trailing zeros, so +// "3.0.0" carried a Parts of len 1 and cap 3 and padding it back wrote into spare +// capacity in the backing array. A by-value copy of a Version copies the slice +// HEADER, so two goroutines comparing two copies wrote to the same memory. v0.6.0 +// removes that two ways over: a packable version never touches part.Parts at all, +// and the fallback pads by copying. The defect was upstream, in rstudio/go-version +// v0.0.2, and was never fixable here -- key.release is unexported. +// +// So the parsed values are safe to share, and this memo shares them WITHIN the +// index. It does not hand them out: see the copy below. +// +// ⚠️ The returned slice is a COPY of the memo's, and that is not defensive +// tidiness. Before this memo existed every call built a fresh slice, so a caller +// has always been free to sort what it was given, and this module has such a +// caller -- cmd/pyresolve's `versions` subcommand sorts the result in place. +// Returning plan.versions would make that call reorder the cache for every later +// caller, permanently, with nothing at the mutation site to suggest it: a silent +// breaking change dressed as an optimization. Its cost was measured rather than +// waved away -- see the copy note above deps -- and it is inside the figures +// quoted here. +// +// Metadata reads the same memo, binary-searching plan.order to turn a caller's +// version into the stored key it names. That is not an incidental reuse: it is +// what makes "the version Versions hands out" and "the record Metadata resolves +// for it" the same choice by construction rather than by two call sites agreeing +// on preferKey. See resolveStoredKey. +// +// ⚠️ The parsed half is RETENTION, not just churn traded away, and it is retained +// for the life of the index rather than of a resolution. See the retention note +// above deps: for this module's callers, which build an index per resolve, it is +// a rounding error; for a long-lived server it is the thing that needs a bound +// before the integration, not after it. func (idx *RSFIndex) Versions(ctx context.Context, pkg PackageName) ([]version.Version, error) { if err := ctx.Err(); err != nil { return nil, err @@ -476,21 +535,26 @@ func (idx *RSFIndex) Versions(ctx context.Context, pkg PackageName) ([]version.V idx.memoMu.RLock() plan, ok := idx.versionList[pkg] idx.memoMu.RUnlock() - if ok { - return parseKeys(plan.order) - } - - decoded, err := idx.deps(pkg) - if err != nil { - return nil, err + if !ok { + decoded, err := idx.deps(pkg) + if err != nil { + return nil, err + } + plan = computeVersionOrder(decoded) + idx.storePlan(pkg, plan) } - plan, parsed := computeVersionOrder(decoded) - idx.storePlan(pkg, plan) - - // The freshly parsed values, not a re-parse of what was just stored: the - // first call should not pay twice. - return parsed, nil + // Never plan.versions itself. See the copy note above -- and note that the + // FIRST call copies too: it holds the same slice it just stored, so returning + // it directly would leave exactly one caller per package able to corrupt the + // memo, which is the worst of both arrangements to debug. + // + // make, not slices.Clone: a package whose every key is unparseable must come + // back as an empty NON-NIL slice, which is what it returned before any memo + // existed, and slices.Clone propagates nil. + out := make([]version.Version, len(plan.versions)) + copy(out, plan.versions) + return out, nil } // versionPlan is what Versions computed for one package, and what Metadata @@ -502,6 +566,15 @@ type versionPlan struct { // sorted it. order []string + // versions is order, parsed, element for element: versions[i] is + // version.Parse(order[i]). Same length, same order, always. + // + // ⚠️ SHARED, and only inside the index. Versions returns a copy; nothing here + // or in Metadata writes to it. Sharing it at all became legal in + // go-python-packaging v0.6.0 and would have been a data race before -- see + // Versions. + versions []version.Version + // alias maps a class winner's CANONICAL RENDERING to its stored key, for the // classes where the two differ. Nil when every winner is already spelled // canonically, which is most packages. @@ -533,7 +606,7 @@ func (idx *RSFIndex) versionPlanFor(pkg PackageName, decoded map[string]pypirsf. return plan } - plan, _ = computeVersionOrder(decoded) + plan = computeVersionOrder(decoded) idx.storePlan(pkg, plan) return plan } @@ -549,24 +622,33 @@ func (idx *RSFIndex) storePlan(pkg PackageName, plan versionPlan) { idx.memoMu.Lock() defer idx.memoMu.Unlock() - // Clipped to its length. computeVersionOrder sizes order for every candidate - // and appends only the class representatives, so a package with a collapsed - // equality class leaves spare capacity behind -- and a cached slice with - // len < cap is the shape that lets an append by one holder overwrite what - // another holder is reading. Nothing appends to this today; the clip is what - // keeps that from becoming load-bearing. + // Clipped to their length. computeVersionOrder sizes both slices for every + // candidate and appends only the class representatives, so a package with a + // collapsed equality class leaves spare capacity behind -- and a cached slice + // with len < cap is the shape that lets an append by one holder overwrite + // what another holder is reading. Nothing appends to either today; the clip + // is what keeps that from becoming load-bearing. + // + // ⚠️ The clip does NOT reclaim the spare capacity -- the backing array is + // unchanged and only a later append would reallocate. It is not a retention + // measure and should not be read as one; equality classes collapse in 59 + // classes across the whole production snapshot, so there is nothing there to + // reclaim. What it buys is that an append by one holder cannot grow IN PLACE + // into memory another holder is reading. plan.order = plan.order[:len(plan.order):len(plan.order)] + plan.versions = plan.versions[:len(plan.versions):len(plan.versions)] idx.versionList[pkg] = plan } // computeVersionOrder does the parse, sort and dedup behind both Versions and -// versionPlanFor, returning the plan and the parsed versions of its order, in -// the same order. -// -// The two returns are parallel by construction, which is what lets Versions -// serve its first call from the parsed half without re-parsing the keys it just -// stored. -func computeVersionOrder(decoded map[string]pypirsf.VersionDeps) (versionPlan, []version.Version) { +// versionPlanFor. +// +// plan.order and plan.versions are filled in one pass and are parallel by +// construction, which is the invariant everything above relies on: it is what +// lets Versions serve from the parsed half while resolveStoredKey binary-searches +// the key half, without either needing to check that the two still agree. Keep +// the two appends adjacent. +func computeVersionOrder(decoded map[string]pypirsf.VersionDeps) versionPlan { type candidate struct { key string parsed version.Version @@ -605,16 +687,20 @@ func computeVersionOrder(decoded map[string]pypirsf.VersionDeps) (versionPlan, [ candidates[j].key, candidates[j].canonical) }) - out := make([]version.Version, 0, len(candidates)) - plan := versionPlan{order: make([]string, 0, len(candidates))} + // Both non-nil even for a package whose every key is unparseable: Versions has + // always answered that with an empty slice rather than a nil one. + plan := versionPlan{ + order: make([]string, 0, len(candidates)), + versions: make([]version.Version, 0, len(candidates)), + } for i, c := range candidates { if i > 0 && candidates[i-1].parsed.Equal(c.parsed) { // A later member of a class already represented. See the dedup note in // the method doc. continue } - out = append(out, c.parsed) plan.order = append(plan.order, c.key) + plan.versions = append(plan.versions, c.parsed) // Only the non-canonical winners need an alias entry: for a canonical // key the caller's ver.String() IS the key, and decoded resolves it @@ -627,29 +713,7 @@ func computeVersionOrder(decoded map[string]pypirsf.VersionDeps) (versionPlan, [ } } - return plan, out -} - -// parseKeys re-parses a memoized version order. -// -// Every key here parsed successfully when the order was built, so a failure now -// would mean version.Parse is not a function of its input. Treated as a -// programming error rather than skipped, because silently dropping a version -// would make the memoized answer differ from the first one. -// -// Always non-nil, matching what Versions returned before the memo existed: the -// slice was built with make, so a package whose every key is unparseable came -// back as an empty non-nil slice rather than nil. -func parseKeys(order []string) ([]version.Version, error) { - out := make([]version.Version, len(order)) - for i, key := range order { - v, err := version.Parse(key) - if err != nil { - return nil, fmt.Errorf("index: memoized version key %q no longer parses: %w", key, err) - } - out[i] = v - } - return out, nil + return plan } // UnparseableVersionKeys returns the stored version keys for pkg that PEP 440 @@ -775,10 +839,18 @@ func (idx *RSFIndex) resolveStoredKey( // findEqualKey binary-searches a sorted, deduped version order for the key whose // version is PEP 440-equal to ver. // -// Each probe is parsed fresh and discarded, which is the point: the order holds -// strings precisely because a version.Version cannot be shared between -// goroutines (see Versions), so the comparison has to re-parse. Only log2(n) of -// them are parsed. +// Each probe is parsed fresh and discarded. That USED to be forced -- the order +// held strings because a version.Version could not be shared between goroutines +// -- and it no longer is: plan.versions now holds the parsed value of every +// element of plan.order, at the same index, so this search could probe those +// directly and parse nothing. +// +// ⚠️ It deliberately does not, YET. Only log2(n) probes are parsed -- about 14 +// against a package with ten thousand releases -- so this is not where the parse +// cost was, and folding it into the parsed-version memo's change would have +// confounded that memo's measurement with a second effect. It is a real follow-up +// with a real (small) win, not an oversight. Whoever takes it: pass the plan +// rather than the order, and the error return goes away with the parse. // // A parse failure here would mean version.Parse is not a function of its input, // since every key in the order parsed when the order was built. Reported rather diff --git a/index/rsfmemo_test.go b/index/rsfmemo_test.go index e54d6dd..2c8106d 100644 --- a/index/rsfmemo_test.go +++ b/index/rsfmemo_test.go @@ -26,12 +26,31 @@ import ( // suggest it. cmd/pyresolve's `versions` subcommand really does sort the result // of Versions in place, so this is not a hypothetical caller. // -// The Versions half currently cannot fail, because that memo holds keys and -// re-parses them, so what it returns was never in the cache. It is kept as the -// regression test for the DAY the memo starts holding parsed values -- which is -// coming, see the upstream defect described on Versions -- and it is a test of -// slice identity, not of the data race. The race is guarded by -// resolver/concurrency_test.go. +// ⚠️ THE VERSIONS HALF IS NOW LIVE. It was written as a regression test for the +// day the version memo started holding parsed values rather than keys, and could +// not fail until then, because what Versions returned had been re-parsed and was +// never in the cache. That day arrived: the memo holds parsed versions, and +// deleting the copy in Versions makes this fail with "the second call saw the +// first call's mutations". It was the ONLY test that failed on that deletion when +// the memo landed; shared_memo_test.go then added two more that do. +// +// ⚠️ All three are in THIS package. Nothing outside index/ catches the deletion, +// cmd/pyresolve's own tests included -- and cmd/pyresolve is the caller that +// sorts the result of Versions in place, which is the whole reason the copy +// exists. It gets away with it because one invocation calls Versions once. So the +// protection here is not redundant with an end-to-end test somewhere; there is no +// end-to-end test that would notice. +// +// ⚠️ "Three tests" is a count, not three independent guards. The other two catch +// it through a white-box pointer-identity check against an unexported field, and +// in TestSharedMemoizedVersionsAreRaceFree that check is a t.Fatalf PRECONDITION +// -- it aborts before the concurrent phase rather than detecting the defect. This +// test is the only one that observes the damage through the exported API, which +// is why it is the one that must not be folded into the others. +// +// It is a test of slice IDENTITY. The separate question of whether the parsed +// version.Version VALUES are safe to share between goroutines is +// TestSharedMemoizedVersionsAreRaceFree in shared_memo_test.go. func TestVersionsMemoIsNotAliasedByTheCaller(t *testing.T) { idx := openFixtureIndex(t) @@ -452,9 +471,23 @@ func guardTrailingZeroVersions(t *testing.T, pkgs []string) { // nested, so this also exercises the ordering. // // The property under test is that Versions and Metadata hand every goroutine -// its own state. The failure it is shaped to catch is not a torn map -- the -// mutexes handle that -- but a version.Version shared between goroutines, which -// is why the memo holds keys. See the notes at each mutation site. +// its own state, from a COLD memo -- goroutines racing to be the one that builds +// each package's plan. +// +// ⚠️ Cold is what it covers and cold is ALL it covers, which is narrower than it +// reads. Every goroutine calls Versions exactly ONCE, so on a 48-goroutine, +// 6-package run most of them take the first-call path and get a slice nobody else +// holds. With the defensive copy in Versions deleted, this test catches it 8 +// times in 20 -- measured, one fresh process per run -- against 20 in 20 for the +// warm-memo test. It reaches the same hazard under a v0.5.0 dependency pin at the +// same 8-in-20 rate. It is a scheduling lottery for the sharing case, and a +// lottery is not a guard: it is a flake generator that happens to be pointing at +// something real. +// +// So it is kept for what it does cover -- concurrent first calls, the memoMu/mu +// ordering, and Metadata's slice copies -- and the sharing case is covered +// deterministically, by warming the memo first, in +// TestSharedMemoizedVersionsAreRaceFree (shared_memo_test.go). func TestMemoIsSafeUnderConcurrentUse(t *testing.T) { idx := openFixtureIndex(t) ctx := context.Background() @@ -484,11 +517,12 @@ func TestMemoIsSafeUnderConcurrentUse(t *testing.T) { // would sort just as busily and prove nothing. Asserted by // guardTrailingZeroVersions above rather than left to this comment. // - // ⚠️ It does NOT catch versionList itself being handed back, and no - // arrangement of this method could: the memo holds []string and this - // returns []version.Version, so the two cannot be the same object - // until the memo's type changes. That day is the one the aliasing - // test at the top of this file is kept for. + // ⚠️ It does NOT reliably catch versionList's own slice being handed + // back, now that the memo holds parsed versions and the two COULD be + // the same object. See the note above this function: one call per + // goroutine means most of them build their own plan. Slice identity + // is TestVersionsMemoIsNotAliasedByTheCaller's job, and sharing under + // a WARM memo is TestSharedMemoizedVersionsAreRaceFree's. sort.Sort(sort.Reverse(version.SortedVersions(vers))) for _, v := range vers { diff --git a/index/shared_memo_test.go b/index/shared_memo_test.go new file mode 100644 index 0000000..edb4f87 --- /dev/null +++ b/index/shared_memo_test.go @@ -0,0 +1,702 @@ +// SPDX-License-Identifier: Apache-2.0 OR MIT + +package index + +import ( + "context" + "os" + "path/filepath" + "sort" + "strings" + "sync" + "testing" + + rsf "github.com/rstudio/repository-snapshot-format" + + "github.com/posit-dev/go-pyresolver/pypirsf" + "github.com/posit-dev/go-python-packaging/version" +) + +// These cover what the parsed-version memo added: RSFIndex.Versions now hands +// every caller by-value copies of version.Version values held in a SHARED memo, +// so those values are read concurrently by every goroutine that resolves against +// one index. +// +// ⚠️ THIS FILE OVERLAPS index/shared_version_test.go HEAVILY, and the overlap is +// deliberate but not free. That file arrived with #44 and pins the same hazard +// one level down: that a Version parsed DIRECTLY is safe to share, which is gpp's +// guarantee. This one pins that RSFIndex.Versions actually hands that guarantee +// to concurrent callers -- the memo is the new thing, not the parse. +// +// The two fixture tables are the same four (shared, longer, packable) triples, +// chosen for the same reasons, and the two shape guards assert the same three +// properties. That is duplication, in one package, with nothing keeping them in +// step: an upstream packability change corrected in one table leaves the other +// asserting a stale path, and the failure mode is a green two-path test that +// covers one path. If a third file ever wants these fixtures, hoist the table and +// the guard into one place rather than copying them a third time. +// +// # The hazard this is shaped for +// +// Until go-python-packaging v0.6.0, sharing a parsed Version was a data race. +// rstudio/go-version's part.Parts.Normalize resliced the comparison key's release +// segment to drop trailing zeros, leaving len < cap, and part.Parts.Padding then +// appended into that spare capacity IN PLACE. A by-value copy of a Version copies +// the slice HEADER, so two goroutines comparing two copies wrote to the same +// backing array. v0.6.0 removes it twice over: a packable version never touches +// part.Parts at all, and the general path pads by COPYING (padParts). +// +// That fix is the whole licence for this memo, and this module's go.mod pin is +// what makes it true for this module's callers. These tests are what objects if +// that pin is ever walked back. +// +// ⚠️ THE HAZARD IS NARROW AND A CARELESS FIXTURE MISSES IT ENTIRELY. It fires +// only when both of these hold: +// +// 1. The shared operand carries spare capacity, which it acquires only where +// trailing zeros were stripped. part.BigIntSliceToParts allocates exactly +// len segments and Normalize reslices, so cap is the RAW segment count and +// len is the stripped one. "3.11" has no spare capacity and is IMMUNE. +// "3.11.0" strips to two segments with capacity for three and races. +// 2. The shared operand is the SHORTER of the pair, because that is the side +// Padding grows. +// +// The padding DISTANCE does not have to fit in the spare capacity: Padding +// appends one element at a time, so the first append writes in place and only a +// later one reallocates. One stripped trailing zero is enough. +// +// The version keys below are chosen for that shape, and +// TestSharedMemoFixturesReachTheHazard asserts the shape rather than trusting +// this comment. A comment cannot stop someone adding "1.2.3" to the table later, +// and a green race run on an immune fixture says nothing at all. +// +// # What was verified against the old pin, where, and why you cannot just repeat it +// +// The COMPARISON half of every case here reported WARNING: DATA RACE with +// go-python-packaging pinned back to v0.5.0 -- 20 fresh processes per case, 80 +// for 80. +// +// ⚠️ SCOPE, precisely, because "80 for 80" over-reads easily. That run was made +// at go-pyresolver 6c13230, on the version of this file that existed then: four +// fixtures, Compare and sort, and NO ReleaseKey. The ReleaseKey assertions were +// added later, when c006d47 made pep440set a second reader of a shared parsed +// Version, and they are NOT covered by that 80-for-80 -- they cannot be, since +// version.ReleaseKey does not exist before gpp v0.7.0. What backs the ReleaseKey +// half is gpp's own three-index slicing plus this test driving it under -race, +// not a demonstrated detection. +// +// ⚠️ AND THE RUN CANNOT BE REPEATED AS WRITTEN on the current base. Pinning back +// with `go mod edit -require=github.com/posit-dev/go-python-packaging@v0.5.0` +// fails to compile twice over: pep440set/bound.go and pep440set/verpos.go call +// version.ReleaseKey (four sites), and so does THIS FILE. Repeating it means +// reverting pep440set to its pre-c006d47 state AND stripping the ReleaseKey +// assertions below. That is the honest cost -- an instruction that looks +// executable and is not would be worse than saying so. +// +// ⚠️ Each of those runs was a SEPARATE PROCESS, and that is not a detail. Go's +// race detector deduplicates reports by stack within a process, so several +// subtests racing at the same line report once and stay quiet. Measured that way +// the same fixtures look flaky. They are not; the detector is deduplicating. One +// `go test -run .../ -count=1` per case, or the number is meaningless -- +// I published a wrong claim from a single-process run before measuring this +// properly. See the note on what else objects, below. +// +// ⚠️ TestSharedMemoFixturesReachTheHazard FAILS under a v0.5.0 pin too, and for a +// reason unrelated to the race: v0.5.0 has no packed comparison path, so the +// packable case allocates and the guard says so. That is the guard working. Do +// not read it as a second race report. +// +// # Why WARM, and not just "run the existing concurrency test" +// +// index/rsfmemo_test.go's TestMemoIsSafeUnderConcurrentUse looks like it already +// covers this. It does not, and the way it fails to is worse than not covering it +// at all: every goroutine there calls Versions exactly once against a COLD memo, +// so most take the first-call path and build a plan nobody else holds, and +// whether any two goroutines ever share is a scheduling lottery. +// +// Measured on the tree with the defensive copy deleted, one fresh process per +// run: that test catches it 8 times in 20, this one 20 in 20. A test that notices +// a real defect two times in five is not a guard, it is a source of +// unreproducible CI failures. The memo here is warmed before any goroutine +// starts, so the sharing is guaranteed rather than raced for. +// +// # ⚠️ A SECOND reader of a shared parsed Version, as of c006d47 +// +// Everything above is about Version.Compare, because until recently that was the +// only thing this module did with a parsed Version on a hot path. It is not any +// more: pep440set.verPos.init calls version.ReleaseKey on candidate versions, and +// those versions come from this memo, so ReleaseKey reads shared state too. +// +// It is safe -- gpp v0.7.0 clips the release slice it hands back (v.release[:n:n]) +// for exactly this hazard -- but that is a property of the current pin rather +// than something this module gets to assume. The goroutines below therefore call +// ReleaseKey on the shared values as well as comparing them, so the second reader +// is exercised rather than reasoned about. + +// memoShareCase is one fixture package whose two version keys form a +// (shared, longer) pair for the concurrent comparison below. +type memoShareCase struct { + // pkg is the fixture package name, one per case so each pair sits in its + // own memoized plan. + pkg string + + // shared is the SHORTER key, the one padding would grow. It must end in a + // zero release segment or the hazard is unreachable. + shared string + + // longer has strictly more release segments than shared once trailing zeros + // are stripped. + longer string + + // packable records which of v0.6.0's two independent safety arguments this + // case exercises. Asserted by allocation count, not assumed: see + // TestSharedMemoFixturesReachTheHazard. + packable bool + + // why records what this case covers that no other case covers. + why string +} + +var memoShareCases = []memoShareCase{ + { + pkg: "packableshare", + shared: "1.2.0", + longer: "1.2.3.4", + packable: true, + why: "The common case and the overwhelming majority of a real snapshot. " + + "Comparison runs off the packed integer key and never reaches " + + "part.Parts, so there is no slice to alias.", + }, + { + pkg: "localshare", + shared: "1.2.0+shared", + longer: "1.2.3.4+other", + packable: false, + why: "A local version label disqualifies packing outright, so this pair " + + "walks the general path and is safe only because padParts copies. " + + "Roughly a quarter of distinct versions in a production snapshot " + + "take that path, under an entirely separate safety argument, and a " + + "table covering only packable versions would say nothing about it.", + }, + { + pkg: "epochshare", + shared: "1!1.2.0", + longer: "1!1.2.3.4", + packable: false, + why: "A non-zero epoch is a second, independent disqualifier. It is here " + + "because the local-label case would stop covering the fallback if " + + "the packer ever learned to carry a local label, and one fixture " + + "standing for a whole path is one fixture too few.", + }, + { + pkg: "longshare", + shared: "1.2.3.4.5.6.7.0", + longer: "1.2.3.4.5.6.7.8.9", + packable: false, + why: "More than six release segments after stripping, the third " + + "disqualifier, and the only case where the padding distance is " + + "greater than a single segment.", + }, +} + +// openShareFixtureIndex writes an RSF holding one package per memoShareCase and +// returns an index over it. +// +// Separate from openFixtureIndex rather than added to it: those packages are +// asserted on by name and by count across half a dozen files, and a fixture that +// serves two purposes ends up serving neither. Built with the same helpers. +func openShareFixtureIndex(t *testing.T) *RSFIndex { + t.Helper() + + path := filepath.Join(t.TempDir(), "share.rsf") + f, err := os.Create(path) + if err != nil { + t.Fatalf("creating fixture: %v", err) + } + w := rsf.NewWriter(f) + for _, c := range memoShareCases { + rec := pypirsf.PackageRecord{ + CanonicalName: c.pkg, + ProjectName: c.pkg, + Snapshots: []pypirsf.SnapshotRecord{ + {Snapshot: "2026080100", Version: c.shared, ReleaseDate: "\x00\x01", Summary: "x"}, + }, + Deps: buildStoredDepsField([]fixtureVersion{ + {version: c.shared, requiresPython: ">=3.9"}, + {version: c.longer, requiresPython: ">=3.9"}, + }), + Depsdict: buildDepsdictField(), + } + if _, err := w.WriteObject(rec); err != nil { + t.Fatalf("writing %s: %v", c.pkg, err) + } + } + if err := f.Close(); err != nil { + t.Fatalf("closing fixture: %v", err) + } + + file, err := pypirsf.Open(path) + if err != nil { + t.Fatalf("pypirsf.Open: %v", err) + } + t.Cleanup(func() { _ = file.Close() }) + + idx, err := NewRSFIndex(file, "share-rsf") + if err != nil { + t.Fatalf("NewRSFIndex: %v", err) + } + return idx +} + +// strippedReleaseLen returns how many release segments s has once the epoch, the +// pre/post/dev suffix and the local label are removed and trailing zeros are +// stripped -- which is what the comparison key does, and where the spare capacity +// the hazard needs comes from. +// +// It reads the FIXTURE STRING rather than asking the library, deliberately. What +// is being guarded is that the test data still has the shape the hazard needs, +// which is a question about the data and not about the code under test. The +// fixtures are simple enough that this does not have to be a PEP 440 parser. +func strippedReleaseLen(s string) int { + if i := strings.Index(s, "!"); i >= 0 { + s = s[i+1:] + } + if i := strings.Index(s, "+"); i >= 0 { + s = s[:i] + } + if i := strings.IndexFunc(s, func(r rune) bool { + return r != '.' && (r < '0' || r > '9') + }); i >= 0 { + s = strings.TrimSuffix(s[:i], ".") + } + segs := strings.Split(s, ".") + for len(segs) > 1 && segs[len(segs)-1] == "0" { + segs = segs[:len(segs)-1] + } + return len(segs) +} + +// endsInZeroSegment reports whether s's release ends in a "0" segment, which is +// what leaves spare capacity behind when the comparison key strips it. +func endsInZeroSegment(s string) bool { + if i := strings.Index(s, "!"); i >= 0 { + s = s[i+1:] + } + if i := strings.Index(s, "+"); i >= 0 { + s = s[:i] + } + if i := strings.IndexFunc(s, func(r rune) bool { + return r != '.' && (r < '0' || r > '9') + }); i >= 0 { + s = strings.TrimSuffix(s[:i], ".") + } + return strings.HasSuffix(s, ".0") +} + +// TestSharedMemoFixturesReachTheHazard is the anti-vacuity guard for +// TestSharedMemoizedVersionsAreRaceFree. A race test on immune fixtures passes +// while checking nothing, and nothing in the race test itself would say so. +// +// It asserts three things a green -race run cannot: +// +// 1. Every shared key ends in a zero release segment, so it carries the spare +// capacity without which the hazard is unreachable. +// +// 2. Every shared key is strictly the SHORTER operand, so it is the side +// padding would have grown. +// +// 3. Each case takes the comparison path it claims to. The packed path +// allocates NOTHING and the general path allocates, so allocation is a +// public-API discriminator between v0.6.0's two separate safety arguments: +// "the fallback is covered too" is measured rather than recalled from a +// version string and a memory of the rules, and it fails loudly if a fixture +// silently changes paths on an upstream bump, which is how a two-path test +// decays into a one-path test. +// +// ⚠️ The general path's allocations are NOT padParts copying, and an earlier +// draft of this comment said they were. Measured: a general-path pair whose +// release lengths already match, so padParts does nothing, still allocates 17 +// per comparison against the padding pair's 18. They come from key.compare +// building a part.Parts and boxing each field into an interface. The +// discriminator is sound -- zero allocations still means the packed path -- +// but do not read the count as a measure of the copy. +func TestSharedMemoFixturesReachTheHazard(t *testing.T) { + var packable, unpackable int + + for _, c := range memoShareCases { + t.Run(c.pkg, func(t *testing.T) { + if !endsInZeroSegment(c.shared) { + t.Fatalf("shared key %q does not end in a zero release segment, so it carries "+ + "no spare capacity and cannot exercise the padding hazard; see the note "+ + "at the top of this file (%s)", c.shared, c.why) + } + ls, ll := strippedReleaseLen(c.shared), strippedReleaseLen(c.longer) + if ls >= ll { + t.Fatalf("shared key %q strips to %d release segments and %q to %d: shared must "+ + "be strictly SHORTER or it is not the operand that gets padded", + c.shared, ls, c.longer, ll) + } + + short := mustVersion(t, c.shared) + long := mustVersion(t, c.longer) + allocs := testing.AllocsPerRun(100, func() { + _ = short.Compare(long) + }) + + switch { + case c.packable && allocs != 0: + t.Errorf("%q vs %q allocated %.0f per comparison; a packable pair compares off "+ + "the packed integer key and must allocate nothing. This case no longer "+ + "covers the packed path.", c.shared, c.longer, allocs) + packable++ + case !c.packable && allocs == 0: + t.Errorf("%q vs %q allocated nothing per comparison, so it took the PACKED path "+ + "and not the padParts fallback it is here to cover (%s)", + c.shared, c.longer, c.why) + unpackable++ + case c.packable: + packable++ + default: + unpackable++ + } + }) + } + + // Both arguments must actually be exercised. Without this, deleting every + // unpackable case leaves a suite that is green and covers half the fix. + if packable == 0 || unpackable == 0 { + t.Errorf("the table covers %d packable and %d unpackable cases; v0.6.0 makes TWO "+ + "separate safety arguments and both need a case", packable, unpackable) + } +} + +// TestSharedMemoizedVersionsAreRaceFree shares one memoized parse across +// goroutines and compares it, which is exactly what a concurrent resolver does +// through the index now that Versions answers from a parsed memo. +// +// The memo is WARMED first. That is the whole difference between this and +// TestMemoIsSafeUnderConcurrentUse: after the warm-up every Versions call is a +// copy of one stored slice, so every goroutine holds a by-value copy of the SAME +// parsed version.Version and the aliasing is guaranteed rather than raced for. +// +// ⚠️ RUN UNDER -race. Without it this test asserts NOTHING about the hazard: the +// racing writes store the same bytes at the same address, so the answers stay +// right and no other check can see them. Its entire value is in the detector. +// +// CI runs it: .github/workflows/ci.yml has a `Race` step as of #44. That is what +// makes this file worth anything on a pull request, and it is recent -- before +// #44 the workflow ran plain `go test ./...`, under which these tests are a green +// no-op. Measured, on a tree with the defensive copy deleted: +// TestMemoIsSafeUnderConcurrentUse catches it 8 times in 20 under -race and 0 +// times in 20 without it. If that step is ever removed, this file stops being a +// guard and nothing else in the module will say so. +// +// Confirmed to report WARNING: DATA RACE with go-python-packaging pinned back to +// v0.5.0, for all four cases, 20 fresh processes each -- 20 out of 20 every time. +// Without that confirmation this test is a hypothesis. See the note at the top of +// this file for where that was run and why it cannot simply be repeated. +// +// ⚠️ WHAT ELSE OBJECTS, corrected twice. An earlier draft said this was the only +// test in the module that objects to a v0.5.0 pin. That was wrong, and wrong +// because it came from a SINGLE process -- the exact trap the note at the top of +// this file warns other people about. Measured properly at 6c13230, 20 fresh +// processes each, defensive copy intact: +// +// TestSharedMemoizedVersionsAreRaceFree 20/20 +// TestMemoIsSafeUnderConcurrentUse 8/20 +// TestVersionsMemoIsNotAliasedByTheCaller 0/20 +// +// ⚠️ shared_version_test.go OBJECTS TOO, and a second draft of this list omitted +// it. TestSharedParsedVersionIsRaceFree and TestSupportsPythonSharedTargetIsRaceFree +// arrived with #44 for the same hazard one level down -- a Version parsed +// directly rather than served from this memo -- and their own doc records the +// same v0.5.0 result. They are not redundant with this file and this file is not +// redundant with them: they pin gpp's guarantee, this pins that RSFIndex.Versions +// hands that guarantee to concurrent callers. +// +// resolver/concurrency_test.go, provider and candidate pass. ⚠️ pep440set passed +// at 6c13230 and does not COMPILE under a v0.5.0 pin on the current base, which +// is a different statement about a different tree; see mock.go, whose note is +// about the earlier one. +// +// The 8/20 is consistent with, not contrary to, the account of that test above: +// it shares memoized values only when the scheduler happens to let two goroutines +// past the first-call path, so it reaches the hazard sometimes. A test that +// reports a real data race in 8 runs out of 20 is not a second guard, it is a +// flake generator -- which is the argument for this test existing, not against +// it. +// +// ⚠️ Verifying that yourself needs `-timeout` raised: under a v0.5.0 pin the +// provider and candidate suites take about 50 s each under -race, and running +// several packages in one `go test` invocation on a busy machine trips the +// default 10-minute bound. A timeout there is not a race report. +func TestSharedMemoizedVersionsAreRaceFree(t *testing.T) { + const goroutines = 8 + + idx := openShareFixtureIndex(t) + ctx := context.Background() + + for _, c := range memoShareCases { + t.Run(c.pkg, func(t *testing.T) { + pkg := NewPackageName(c.pkg) + + // Warm the memo, and check that it is warm rather than assuming so. + // If the plan were absent the goroutines below would each build + // their own and share nothing, which is the failure mode this test + // exists to avoid in the first place. + warm, err := idx.Versions(ctx, pkg) + if err != nil { + t.Fatalf("Versions(%s): %v", pkg, err) + } + if len(warm) != 2 { + t.Fatalf("fixture %s gave %d versions, want 2 (%v)", pkg, len(warm), renderVersions(warm)) + } + idx.memoMu.RLock() + plan, ok := idx.versionList[pkg] + idx.memoMu.RUnlock() + if !ok { + t.Fatalf("the memo holds no plan for %s after a Versions call, so the "+ + "goroutines below would not share anything", pkg) + } + if len(plan.versions) != len(plan.order) { + t.Fatalf("plan for %s holds %d parsed versions against %d keys; the two are "+ + "parallel by construction", pkg, len(plan.versions), len(plan.order)) + } + // The returned slice must be a copy, or the goroutines below would + // be racing on the slice rather than on the values inside it, and + // this test would be measuring the wrong thing. + if &warm[0] == &plan.versions[0] { + t.Fatalf("Versions returned the memo's own slice for %s; this test needs a "+ + "copy so that what is shared is the parsed VALUES", pkg) + } + + // The answer every goroutine must agree on. Computed once, before + // any concurrency, from values the memo has not yet handed out + // twice. + want := plan.versions[0].Compare(plan.versions[1]) + if want == 0 { + t.Fatalf("fixture %s: %q and %q compare equal, so this case cannot detect a "+ + "corrupted pad", pkg, c.shared, c.longer) + } + // The same answer through the second reader. Not asserted equal to + // want: a ReleaseKey drops the pre/post/dev/local components, so two + // versions that differ only there share a release key. These fixtures + // differ in the release segment, so the two agree -- and that is + // checked here rather than assumed, because a fixture added later + // might not. + wantRel := plan.versions[0].ReleaseKey().Compare(plan.versions[1].ReleaseKey()) + if wantRel == 0 { + t.Fatalf("fixture %s: %q and %q share a release key, so the ReleaseKey check "+ + "below cannot detect a corrupted release segment", pkg, c.shared, c.longer) + } + + var ( + wg sync.WaitGroup + start = make(chan struct{}) + ) + for range goroutines { + wg.Add(1) + go func() { + defer wg.Done() + <-start + + vers, err := idx.Versions(ctx, pkg) + if err != nil { + t.Errorf("Versions(%s): %v", pkg, err) + return + } + + // Repeated because the write the detector is looking for + // happens inside Compare's padding step, and one comparison + // per goroutine gives it a narrow window. + for range 200 { + if got := vers[0].Compare(vers[1]); got != want { + t.Errorf("%s: %q.Compare(%q) = %d, want %d -- a shared release "+ + "segment was padded from under this goroutine", + pkg, c.shared, c.longer, got, want) + return + } + + // The OTHER reader of a shared parsed Version, as of + // c006d47: pep440set.verPos.init takes this of every + // candidate version, and its candidates come from this + // memo. Driven here so that reader is under the detector + // too, rather than resting on the argument that gpp + // clips what it returns. + // + // Compared through ReleaseKey.Compare, which is the only + // exported way to observe one: the two keys must order + // the same way the versions do, every time. + if got := vers[0].ReleaseKey().Compare(vers[1].ReleaseKey()); got != wantRel { + t.Errorf("%s: ReleaseKey(%q).Compare(ReleaseKey(%q)) = %d, want %d "+ + "-- a shared release segment moved under this goroutine", + pkg, c.shared, c.longer, got, wantRel) + return + } + } + + // And sort the returned slice, which is what + // cmd/pyresolve's `versions` subcommand does. Under a + // missing defensive copy every goroutine would be sorting + // one shared array. + sort.Sort(sort.Reverse(version.SortedVersions(vers))) + }() + } + close(start) + wg.Wait() + + // The memo must be untouched by all of that. + after, err := idx.Versions(ctx, pkg) + if err != nil { + t.Fatalf("Versions(%s) after the concurrent phase: %v", pkg, err) + } + if got, want := renderVersions(after), renderVersions(warm); !equalStrings(got, want) { + t.Errorf("the memo changed under concurrent callers:\n got %v\nwant %v", got, want) + } + }) + } +} + +// TestVersionsNeverReturnsTheMemosSlice checks slice identity on BOTH paths. +// +// TestVersionsMemoIsNotAliasedByTheCaller covers the warm path behaviourally, by +// mutating and re-reading. This covers the FIRST call as well, which that test +// cannot reach: the first call holds the very slice it just stored, so returning +// it directly would leave exactly one caller per package able to corrupt the memo +// -- a bug that reproduces once per process and never again. +func TestVersionsNeverReturnsTheMemosSlice(t *testing.T) { + idx := openShareFixtureIndex(t) + ctx := context.Background() + pkg := NewPackageName(memoShareCases[0].pkg) + + first, err := idx.Versions(ctx, pkg) + if err != nil { + t.Fatalf("Versions: %v", err) + } + + idx.memoMu.RLock() + plan := idx.versionList[pkg] + idx.memoMu.RUnlock() + if len(plan.versions) == 0 { + t.Fatal("the memo holds no parsed versions, so there is no identity to check") + } + if &first[0] == &plan.versions[0] { + t.Error("the FIRST call returned the memo's own slice; a caller sorting it corrupts " + + "the cache for every later caller") + } + + second, err := idx.Versions(ctx, pkg) + if err != nil { + t.Fatalf("Versions (second call): %v", err) + } + if &second[0] == &plan.versions[0] { + t.Error("a warm call returned the memo's own slice") + } + if &second[0] == &first[0] { + t.Error("two calls returned the same slice") + } +} + +// TestVersionPlanHalvesStayParallel enforces the invariant everything above rests +// on: plan.versions[i] is version.Parse(plan.order[i]), same length, same order. +// +// ⚠️ Until this existed, that invariant was held up by a comment ("keep the two +// appends adjacent") and by the two appends happening to sit next to each other +// in computeVersionOrder. Nothing checked it. A plan built by any other path -- +// a future partial construction, or a versionPlan{order: ...} literal -- would +// make Versions return an empty NON-NIL slice, which reads as "this package has +// no versions", with no error and nothing red. That is the worst available +// failure mode: a wrong answer that is indistinguishable from a right one. +// +// Element-wise, not just by length, because a length check passes on two lists +// that have drifted out of order -- which is the shape a dedup or sort change +// would produce. +func TestVersionPlanHalvesStayParallel(t *testing.T) { + for _, idx := range []*RSFIndex{openShareFixtureIndex(t), openFixtureIndex(t)} { + for _, name := range idx.file.Packages() { + pkg := NewPackageName(name) + if _, err := idx.Versions(context.Background(), pkg); err != nil { + t.Fatalf("Versions(%s): %v", pkg, err) + } + + idx.memoMu.RLock() + plan := idx.versionList[pkg] + idx.memoMu.RUnlock() + + if len(plan.versions) != len(plan.order) { + t.Fatalf("%s: plan holds %d parsed versions against %d keys", + pkg, len(plan.versions), len(plan.order)) + } + for i, key := range plan.order { + want, err := version.Parse(key) + if err != nil { + t.Fatalf("%s: stored key %q in the order does not parse: %v", pkg, key, err) + } + if !plan.versions[i].Equal(want) { + t.Errorf("%s: plan.versions[%d] is %s but plan.order[%d] is %q; the two halves "+ + "of a versionPlan must be parallel, and Versions serves from the first "+ + "while Metadata searches the second", pkg, i, plan.versions[i], i, key) + } + } + } + } +} + +// TestVersionsIsEmptyNotNilWhenNothingParses pins the shape Versions has always +// answered with, which the memo must not quietly change. +// +// A package whose every stored key PEP 440 rejects comes back as an empty NON-NIL +// slice. It was non-nil because the pre-memo code built it with make; it stays +// non-nil because the copy does too. slices.Clone would propagate nil here and a +// caller branching on nil would start seeing a different answer. +func TestVersionsIsEmptyNotNilWhenNothingParses(t *testing.T) { + path := filepath.Join(t.TempDir(), "unparseable.rsf") + f, err := os.Create(path) + if err != nil { + t.Fatalf("creating fixture: %v", err) + } + w := rsf.NewWriter(f) + rec := pypirsf.PackageRecord{ + CanonicalName: "allbroken", + ProjectName: "AllBroken", + Snapshots: []pypirsf.SnapshotRecord{ + {Snapshot: "2026080100", Version: "not-a-version", ReleaseDate: "\x00\x01", Summary: "x"}, + }, + Deps: buildStoredDepsField([]fixtureVersion{ + {version: "not-a-version"}, + {version: "also-not-a-version"}, + }), + Depsdict: buildDepsdictField(), + } + if _, err := w.WriteObject(rec); err != nil { + t.Fatalf("writing fixture: %v", err) + } + if err := f.Close(); err != nil { + t.Fatalf("closing fixture: %v", err) + } + + file, err := pypirsf.Open(path) + if err != nil { + t.Fatalf("pypirsf.Open: %v", err) + } + t.Cleanup(func() { _ = file.Close() }) + + idx, err := NewRSFIndex(file, "unparseable-rsf") + if err != nil { + t.Fatalf("NewRSFIndex: %v", err) + } + + for _, call := range []string{"first", "memoized"} { + vers, err := idx.Versions(context.Background(), NewPackageName("allbroken")) + if err != nil { + t.Fatalf("Versions (%s call): %v", call, err) + } + if vers == nil { + t.Errorf("the %s call returned a nil slice; Versions has always answered an "+ + "empty NON-NIL slice for a package whose every key is unparseable", call) + } + if len(vers) != 0 { + t.Errorf("the %s call returned %d versions, want 0", call, len(vers)) + } + } +} diff --git a/provider/provider.go b/provider/provider.go index 1a20be9..4558df9 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -297,21 +297,41 @@ func (p *Provider) Candidates(pkg Package, allowed pep440set.Set) (pep440set.Set // This one is keyed by package name and is bounded by the closure of a single // resolution. // -// # ⚠️ Two hazards, and why neither bites here -// -// A version.Version MUST NOT BE SHARED BETWEEN GOROUTINES even for reads -- -// Version.Compare pads a release segment with append into spare capacity that a -// by-value copy still shares, an upstream defect in rstudio/go-version that -// index.RSFIndex documents at length and declines to memoize parsed versions -// because of. That is why this memo is on the PROVIDER and not on the index: a -// Provider serves one resolution and is documented as unsafe for concurrent use, -// so nothing here is read from two goroutines. Concurrent resolutions get one -// Provider each and therefore one memo each. Moving this onto a shared index -// would reintroduce the race in full. -// -// Second, the map is unbounded in principle. In practice it holds one entry per -// package the resolution reaches, which is the same bound the unusable record set -// already lives under, and the Provider is discarded when the resolve ends. +// # ⚠️ Why this memo is on the PROVIDER and not on the index +// +// ⚠️ HISTORICAL as of go-python-packaging v0.6.0. This used to read: a +// version.Version must not be shared between goroutines even for reads, because +// Version.Compare padded a release segment with append into spare capacity that a +// by-value copy still shared -- so a memo of parsed versions had to sit on a +// Provider, which serves one resolution and is documented as unsafe for +// concurrent use, and moving it onto a shared index would reintroduce the race in +// full. +// +// THAT IS NO LONGER TRUE. v0.6.0 pads into a fresh slice, index.RSFIndex now +// memoizes parsed versions on the strength of it, and this memo COULD move onto +// the index, where the ranked list would survive across resolutions rather than +// only within one. It is deliberately not part of the parsed-version memo's +// change, because a second effect would confound that one's measurement. +// +// candidate.Rank is the only work moving the memo up would save, and only ACROSS +// resolutions, since within one this memo already runs it once per package. On +// the tree that took the parsed-version memo, warm wide-versions against the +// production snapshot, rankedVersions is 27.5% of resolver.Resolve's cumulative +// cost and candidate.Rank inside it is 17.4% -- so that is the shape of the +// opportunity and 17.4% is its ceiling. +// +// ⚠️ Treat that as a pointer, not a promise, and RE-PROFILE before acting on it. +// An earlier draft of this comment quoted a residual profile naming a pep440set +// frame that c006d47 then deleted, so it advertised a cost that no longer existed +// in a function that no longer existed. A profile is a fact about one tree. +// +// ⚠️ Whoever takes it owns the retention question, and it is bigger here than for +// the parsed list: a ranked list is per package too, and an index is long-lived +// where a Provider is discarded. +// +// Until then the memo stays here, and being per-resolution it is bounded twice +// over: one entry per package the resolution reaches, and the whole Provider is +// discarded when the resolve ends. // // # ⚠️ Three things this narrows, none of them free // diff --git a/resolver/bench_test.go b/resolver/bench_test.go index 7a2868f..9581698 100644 --- a/resolver/bench_test.go +++ b/resolver/bench_test.go @@ -489,16 +489,25 @@ // still 34% of Resolve, but it is now parsing SPECIFIER OPERANDS inside // FromSpecifiers rather than version keys. // -// ⚠️ THAT NEXT MEMO WOULD RE-OPEN THE HAZARD THIS ONE AVOIDS. A pep440set.Set -// holds bounds, a bound holds a version.Version and a *posKey whose pub is -// another, and Set is copied BY VALUE -- so a memoized Set shares parsed -// versions between every goroutine that reads it, and Set.Singleton() hands -// sp.lo.v straight out. Stressing it under -race today comes back clean, but -// only incidentally: after #33 the sole surviving Compare call site is reached -// only once the release lengths already match, which makes Padding a no-op. -// Reordering cmpBound's discriminators brings the race back. Whoever builds -// that memo owns the question, and "it was clean when I tried it" is not the -// answer. +// ⚠️ HISTORICAL as of go-python-packaging v0.6.0. This paragraph used to warn +// that the projection memo would re-open a data race: a pep440set.Set holds +// bounds, a bound holds a version.Version and a *posKey whose pub is another, and +// Set is copied BY VALUE -- so a memoized Set shares parsed versions with every +// goroutine that reads it, and Set.Singleton() hands sp.lo.v straight out. That +// was safe under v0.5.0 only incidentally, because after #33 the sole surviving +// Compare call site was reached once the release lengths already matched, making +// Padding a no-op; reordering cmpBound's discriminators would have brought it +// back. +// +// Sharing a parsed version.Version is no longer a race at all. v0.6.0 pads into a +// fresh slice, and index.RSFIndex memoizes parsed versions on exactly that basis. +// So the projection memo is free of THIS objection, and cmpBound's discriminators +// can be reordered on their merits. +// +// ⚠️ What has not changed is that the memo would be keyed by (package, version, +// extra) and would retain parsed structure for the life of whatever holds it. See +// index/rsfindex.go's retention note: a bounded key set is not bounded memory, +// and that is the question that memo owns. // // Two upstream costs are now visible that the parse used to hide, and both are // in go-python-packaging's dependency rather than in this module: diff --git a/resolver/peak_heap_test.go b/resolver/peak_heap_test.go index 9de100c..163d115 100644 --- a/resolver/peak_heap_test.go +++ b/resolver/peak_heap_test.go @@ -124,6 +124,117 @@ func TestPeakHeapDuringOneResolve(t *testing.T) { } } +// TestIndexRetainedHeapAfterResolve reports how much heap ONE warmed +// index.RSFIndex holds onto after a resolution has finished, which is the number +// the parsed-version memo moves and the number no benchmark reports. +// +// # Why this is the number that matters, and TestPeakHeapDuringOneResolve is not +// +// The peak test above measures the high-water mark DURING a resolve. This +// measures what survives it. They answer different questions and the parsed +// memo moves them in opposite directions: churn falls sharply (warm allocs/op +// drops 55.4% on app-set and 94.4% on wide-versions, measured at base 11da678) +// while the index's steady-state footprint rises, because parsed versions that +// used to be transient garbage now live in versionList for the life of the index. +// +// For every caller in this module that is a rounding error, because each builds +// an RSFIndex per resolve and drops it. For a long-lived server holding one index +// over a 932,861-package corpus it is the whole question, and index/rsfindex.go +// says plainly that a bound is a prerequisite for that integration. This test is +// what makes that claim a measured one. +// +// # How +// +// Warm the index with a full resolution, drop the result, force GC and read +// HeapAlloc with the index still reachable; then drop the index, force GC and +// read again. The difference is what the index alone was keeping alive. +// +// runtime.KeepAlive is what makes the first reading mean anything: without it the +// compiler is entitled to consider idx dead the moment the last method call +// returns, and both readings would measure the same thing. +// +// ⚠️ It is a LIVE-HEAP difference, not an allocator footprint. It excludes the +// mmap'd snapshot itself, which is not Go heap, and it includes whatever the +// resolution left reachable from the index -- the decoded blob cache and both +// parsed memos, not just the one under test. That is deliberate: the question a +// server operator has is "what does holding this index cost me", not "what does +// this one field cost me". +// +// Two GC cycles are forced per reading, because one is not guaranteed to reclaim +// everything that became unreachable during it -- an object finalized or +// re-queued by the first pass is only freed by the second. That is belt and +// braces rather than a fix for an observed problem, and it is cheap. The readings +// it produces are stable: five interleaved rounds against the production snapshot +// agreed to 0.01 MB on every corpus entry, on both builds. +// +// Skipped unless GPR_RETAIN is set: it is slow, it forces GC, and it asserts +// nothing. It is a measurement, run deliberately and diffed between two builds. +func TestIndexRetainedHeapAfterResolve(t *testing.T) { + if os.Getenv("GPR_RETAIN") == "" { + t.Skip("set GPR_RETAIN=1 to measure retained heap") + } + + file, excerpt := benchSnapshotT(t) + ctx := context.Background() + + // ⚠️ Say WHICH corpus, loudly, before printing a single megabyte figure. + // Without PYPIRSF_TEST_FILE this measures the 139-package committed excerpt + // and produces numbers nothing like the published table -- silently, because + // every other test here treats the excerpt as a legitimate fixture and + // benchSnapshotT returns the flag without insisting anyone read it. A + // retention figure whose corpus is unstated is not a figure. + corpus := "PRODUCTION snapshot from PYPIRSF_TEST_FILE" + if excerpt { + corpus = "COMMITTED EXCERPT -- set PYPIRSF_TEST_FILE for figures comparable to the CHANGELOG's" + } + t.Logf("corpus: %s, %d packages", corpus, file.Len()) + + for _, entry := range benchCorpus { + t.Run(entry.Name, func(t *testing.T) { + reqs := mustRequirements(t, entry.Requirements...) + opts := testOptions(t) + + idx, err := index.NewRSFIndex(file, "production") + if err != nil { + t.Fatalf("NewRSFIndex: %v", err) + } + + res, err := resolver.Resolve(ctx, reqs, idx, opts) + if err != nil && !entry.WantFailure && !excerpt { + t.Fatalf("Resolve: %v", err) + } + pins := 0 + if res != nil { + pins = len(res.Pinned) + } + res = nil //nolint:ineffassign,wastedassign // dropped so only the index is measured + + withIndex := liveHeap() + runtime.KeepAlive(idx) + idx = nil //nolint:ineffassign,wastedassign // the point of the next reading + withoutIndex := liveHeap() + + retained := float64(withIndex) - float64(withoutIndex) + t.Logf("%-16s retained by the index %7.2f MB (live %6.1f -> %6.1f MB, %d pins)", + entry.Name, + retained/(1<<20), + float64(withIndex)/(1<<20), + float64(withoutIndex)/(1<<20), + pins) + }) + } +} + +// liveHeap returns HeapAlloc after two forced collections. See the note above on +// why one is not enough. +func liveHeap() uint64 { + runtime.GC() + runtime.GC() + var m runtime.MemStats + runtime.ReadMemStats(&m) + return m.HeapAlloc +} + // benchSnapshotT is benchSnapshot for a *testing.T rather than a *testing.B. // Same contract, including that a missing file is a failure rather than a skip. func benchSnapshotT(t *testing.T) (*pypirsf.File, bool) {