Repository navigation
Design for review: typed-module materialization across runs (v1 → v2 relief) - #11779
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cedb0d43a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| At the seam the cross-worker store already uses — `index_insert_typed` writes through, a miss | ||
| decodes from the store — so there is no second cache layer and no new representation. The existing | ||
| transport is reused as-is; only its backing changes from an in-process map to a host directory of | ||
| content-addressed entries. |
There was a problem hiding this comment.
In the target scenario the nominal fold runs with one thread, but the existing store this design proposes to reuse is armed only when the adaptive worker width exceeds one; dag/gunbc/floor/floor_materialization.dag explicitly says width 1 retains the private per-index Rc cache, and index_get_typed uses that private cache whenever cross_worker_store is absent. Merely changing the shared map's backing therefore leaves the measured 21-minute fold unable to read or populate the persistent store, so the claimed warm-run relief cannot occur unless the design explicitly adds a width-1 persistent-store route and accounts for its serialization costs.
Useful? React with 👍 / 👎.
| A second, cheaper check belongs in CI itself: the fold already prints a subject digest | ||
| (`digest=96e8b36e00b61c7a`). A warm run and a cold run of the same commit must print the same | ||
| digest. |
There was a problem hiding this comment.
This warm/cold check cannot detect a stale typed-module hit: PreparedSubject.subject_digest is computed before strict resolution by subject_digest_for_closure, which hashes the closure source content and compiler transform content, not the resulting TypecheckModuleResult. Warm and cold runs of the same commit will therefore print the same subject digest even if the cache returns a wrong typed result; the CI falsifier needs a digest of the typed outputs or an equivalent verdict/result comparison.
Useful? React with 👍 / 👎.
| ``` | ||
| data floor_runner_host_frame: Frame = Frame { | ||
| name: "runner-host", | ||
| kind: SharedStateFrame |
There was a problem hiding this comment.
SharedStateFrame models rewireable plurality within one shared-state workspace/process, whereas std.materialization_ladder explicitly assigns recurring CI invocations to UnboundedSiblingsFrame. With the proposed kind, successive processes are classified as authored duplication rather than expected-emergent cross-run demand, so the ladder does not derive the persistent-cache obligation this row is intended to discharge; use an UnboundedSiblingsFrame in the host/process frame path.
Useful? React with 👍 / 👎.
| id: "host_persisted_typecheck_store", | ||
| scope: [floor_runner_host_frame], | ||
| coverage: CoversIdentities { identities: [floor_typecheck_store_identity] }, | ||
| tier: CasTier { keying: ContentKeyed }, |
There was a problem hiding this comment.
The proposed implementation is a per-host directory, but std.materialization_ladder.tier_axes defines CasTier as RemoteNetwork and ArtifactTier as LocalFilesystem. Declaring this row as CasTier therefore gives the modeled provider a placement it will not implement and contradicts the stated no-cross-host boundary; this host-local content-addressed directory should be an ArtifactTier provider.
Useful? React with 👍 / 👎.
Hand-authored design for review, not a projection: no generated_artifact row produces this file,
and on acceptance its declarations move into gunbc.floor_materialization and this file gives way to
that module's projection.
The subject is the measurement. Required CI spends 21 minutes in the fold and 6.3 in the drift
gate; inside the fold, 8.3 minutes is strict preparation resolving a 1,733-module closure while the
400 claims it then executes take 53 seconds. CPU equals wall in every phase, and the floor's
teardown names the three caches it drops at exit, so the corpus is re-derived once per process.
The design adds one ladder rung rather than a cache: a runner-host frame, and a second provider
over the SAME identity the in-process share already covers, with CasTier{ContentKeyed} and
persistent_retention. The ladder's admission rules then force what a disk cache would have skipped,
since ReleasedNever refuses CapacityUnbounded and CapacityUnobserved: bounded capacity and observed
occupancy. Typed-module resolution is PureComputation, which is what makes content keying sound;
the key is spelled out with what breaks if each component is omitted, compiler binary digest
included.
It reuses what exists: shared_typecheck_store.rs already serializes typed modules by authored name
so another process can decode them, which is cross-run with a longer gap, and its counters become
the observation the ladder demands. Threading the resolver (13,313 Rc against 40 Arc) and
re-sharding the fold are named non-goals.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… corrections Four findings, all four fair, two of them fatal to the design as written. THE SEAM DOES NOT REACH THE FOLD. The shared typed store is armed only when the adaptive worker width exceeds one, and floor_materialization says width 1 keeps the private per-index Rc cache. index_get_typed takes that fallback -- with a counter for it -- so a persistent backing behind the shared store would never have been read or populated by the nominal fold, which runs at width 1. No warm relief could have occurred. The design now arms the persistent provider independently of width, and does it as TWO LEVELS rather than one: the store's own rule is that when armed it becomes the sole typed-cache authority and reads decode bytes, which at width 1 would replace a cheap Rc clone with a decode on thousands of reads and could make a warm run slower than a cold one. L1 stays the in-process map; L2 answers the first read of a module a previous run resolved. The added encode cost is named as a first-class unknown with a measurement that can fail the design. THE FALSIFIER COULD NOT FAIL. PreparedSubject.subject_digest is computed before strict resolution over closure source and compiler transform content -- it hashes the inputs, so warm and cold runs of one commit agree even if the store returns a wrong typed result. Comparing it was spec-without-execution. The check now compares outputs: a digest over the typed results, plus the floor's existing disposition, claim-cost and cross-claim-demand receipts, which must be byte-identical warm against cold. THE FRAME KIND DECIDED WHETHER THE LADDER DERIVES THE CACHE OR FORBIDS IT. SharedStateFrame puts recurring runs in plurality state (1), authored duplication, whose answer is 'rewire/Share, never a cache'. Recurring CI invocations are state (3) under UnboundedSiblingsFrame, which is what obligates a cross-run cache up front. THE TIER NAMED A PLACEMENT NOTHING IMPLEMENTS. tier_axes grounds ArtifactTier at LocalFilesystem and CasTier at RemoteNetwork; a per-host directory is the former. CasTier also contradicted this row's own no-cross-host boundary. A fleet-wide store would be a different provider with a coherence question this one does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2500238 to
df88404
Compare
1 participant
Design only — one markdown file, no code, no generated artifact touched. For review before anything is built.
The measurement it answers
Required CI per push, after the consolidation: fold 21 min, drift gate 6.3 min. Inside the fold:
CPU equals wall in every phase, and the floor's teardown drops
process_resolve_index,process_resolve_storeand the emit memo at exit. The corpus is re-derived once per process, and the drift gate is a second process over the same corpus.The shape
One ladder rung, not a cache:
runner-hostframe (SharedStateFrame), broader than the executor process and narrower than the fleet;CacheProviderover the same identity the in-process share covers —union-typecheck-store(dag,src/v2)— withCasTier { keying: ContentKeyed }andpersistent_retention.Modelling it is what forces the properties a disk cache would have skipped:
retention_admissionrefusesReleasedNeverwithCapacityUnboundedorCapacityUnobserved, so bounded capacity and observed occupancy are obligations, not options.Soundness comes from
PureComputation: same text, same dependency closure, same compiler ⇒ same typed result. The key is spelled out component by component with what breaks if each is omitted — the compiler binary digest included, which is why asrc/v1Rust change gets no relief and a.dagcorpus change gets nearly all of it.It reuses what's already there
shared_typecheck_store.rs("S2a increment C") already serialises typed modules by authored name so a different process can decode them against its own intern table. Cross-run is that same problem with a longer gap; only the backing changes. Its counters become the ladder's required observation.Named non-goals: threading the resolver (13,313
Rc<vs 40Arc<insrc/v1; evenparse_sourcereturnsRc<ParseResult>), re-sharding the fold (forbidden by the fold's own authority), and hosted runners for the fold (20.2 GiB peak vs 16 GB).Expected relief, on the measured basis
Four decisions for you (§8)
main, or PRs warming it themselves.Rollout in §9 is staged so steps 1–2 land dark (
GUNBC_TYPED_STORE_PERSIST, default off), step 3 is the measurement, step 4 arms it, and step 5 widensrequired_gate_prefixesback as preparation gets cheaper.🤖 Generated with Claude Code