Repository navigation
Track manifest entries instead of the whole manifest; add tectonixMemo - #72
Conversation
…ifest Add builtins.tectonixManifestEntry, tectonixManifestKeys and tectonixManifestIdToPath. Each records a synthetic tracked path (`.meta/manifest.json#<zonePath>`, `#keys` or `#id/<zoneId>`) instead of `.meta/manifest.json`, so a target depends on the part of the manifest it read, not on the whole file. dependencyFingerprintCached fingerprints a synthetic path by reading the manifest from the accessor and hashing the entry's JSON, the sorted key list, or the zone path an id maps to. Recording and validation go through the same function, so an eval-cache row stays valid across commits that change other parts of the manifest (for example, adding an unrelated zone). The parsed manifest is kept per fingerprint-cache generation, so it always belongs to the accessor being fingerprinted.
The base environment and the `builtins` attribute set it mirrors were allocated
with a fixed 140 slots each, written without a bounds check. Every global
constant and primop takes one slot of each. With the new manifest and memo
builtins, `nix __dump-language` (run by the manual build; it registers every
builtin, including those gated behind experimental features) needs 141:
ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 8 ... 0 bytes after 1128-byte region
nix::EvalState::addConstant eval.cc
nix::EvalState::createBaseEnv primops.cc
An unsanitized build does not notice. Give both the same named capacity
(`baseEnvCapacity`, now 160) and make registration throw a clear error instead
of writing past the end.
Test: tests/functional/tecnix/base-env.sh runs `nix __dump-language`. With the
check in place and the old size of 140 it fails with "the base environment has
only 140 slots", so the test catches this in an ordinary build too.
…ked dependencies `builtins.tectonixMemo namespace key f` evaluates `f key` once per EvalState and returns the shared value. Under Tecnix source tracking, the one evaluation runs inside a TrackedSourceDepsScope so its accesses intern into a reusable label stored alongside the value; every later consumer records that label as a child of its own frame (and inherits the value's label through the value-copy hooks), so all consumers share the same tracked dependencies without re-evaluating `f key`. A thread-local in-progress guard turns a self-referential `f` (a zone loading itself transitively) into a catchable AssertionError instead of unbounded recursion through fresh thunks. Re-entrant misses for distinct keys (zone A loading zone B) evaluate outside any concurrent_flat_map bucket lock, so they never deadlock. Functional test: tests/functional/tectonix/memo.sh.
Functional test tests/functional/tecnix/manifest-tracking.sh: what each of tectonixManifestEntry, tectonixManifestKeys and tectonixMemo records, and how eval-cache rows behave across commits. Adding an unrelated zone invalidates only the target that enumerates the keys and one that reads the whole manifest; changing a zone's entry invalidates that zone's target; moving an id invalidates its lookup; changing a file read inside a memoized load invalidates every consumer; an unrelated file invalidates nothing. Unit tests src/libexpr-tests/tecnix-manifest-fingerprint.cc: the synthetic fingerprints follow only the part of the manifest they name, and follow the accessor across fingerprint caches on one thread.
3df5bd1 to
aa7083b
Compare
# Conflicts: # tests/functional/tecnix/meson.build # tests/functional/tectonix/meson.build
|
Merged Conflicts (only two, both additive test-list entries at the same spot):
No source conflicts ( Verified locally: build OK; |
joshheinrichs-shopify
left a comment
joshheinrichs-shopify
left a comment
There was a problem hiding this comment.
generally adding a memo and breaking down //.meta/manifest.json into more granular observations is a great change 👍 couple notes from my bot to yours
- Blocker: the memo gives stale cache hits. If a key is first filled outside tracking, it's stored with no dependencies, and later tracked targets reuse it and record nothing. Reproduced: renumber zone a, get a stale warm hit. Fix: copy the guard from getTecnixModuleValue (primops/tecnix.cc:400-423), which doesn't cache untracked builds when tracking is on and keeps untracked entries in a separate keyspace.
- Follow-up: tec query affected-targets can't see the new keys. It matches changed file paths and their parent directories, so .meta/manifest.json#//zones/a never matches. A manifest change then never marks those targets affected, and CI planning skips them. Fix: strip the #… suffix in the tec matcher, or keep public dependency keys as real paths.
- Memo design: rename it to builtins.tecnixMemo and move it (and its test) to primops/tecnix.cc. Better still, memoize a function value instead of a global namespace and key: g = builtins.tecnixMemoize f, bound once, so g k is always f k. That drops the rule that the key must fully determine the result, and per-system worlds can't hand each other the wrong system's value. If you keep string keys, document that rule (include the system in the key).
- Have the manifest builtins read through the Tecnix repo accessor instead of the legacy loader, so values and fingerprints come from the same place.
…ed calls A result computed outside Tecnix source tracking was stored with no source dependencies, and a later tracked call reused it and recorded nothing, so the target's eval-cache row lacked the manifest entry the result came from: renumbering the zone left a stale warm hit. The reverse direction lost dependencies too: an untracked call could force the lazy parts of a tracked result, leaving them unlabelled for every later tracked consumer. Tracked and untracked calls now use separate tables, as the file-eval and import-resolution caches already do. Untracked results are still memoized, in their own table. The resolver-module cache's other rule (do not cache an untracked build while tracking is on) does not carry over: `nix eval` runs pure with the eval cache on by default, so that condition would turn memoization off for every untracked call, and a recursive loader (zone A loads zone B) would re-walk everything it shares on each one. The new manifest-tracking cases fail before this change (both targets lack .meta/manifest.json#//zones/a, and after renumbering zone a the warm run is a cache hit) and pass after it. They also pin that a tracked memo hit replays the dependencies recorded when its entry was filled, which already held.
`builtins.tectonixMemo namespace key f` shared results through a global string namespace, so the key had to determine the result on its own: callers that used the same namespace and key with different functions (one loader per system, say) got each other's values. `builtins.tecnixMemoize f` instead returns `g`, a memoized `f`: `g k` is always `f k`, keyed by `f` itself (the address of its value, which each entry roots) and the string `k`. It lives in primops/tecnix.cc with the other Tecnix builtins; `tectonixMemo` is removed, as it had no released user. Tracked and untracked calls keep the separate tables of the previous commit, lookups borrow the key instead of copying it, and the primop documentation states what `k` may be, how long results live and how tracking interacts. The memo test moves to tests/functional/tecnix/memoize.sh and counts `f`'s evaluations with builtins.trace, instead of relying on a second `f` being ignored; manifest-tracking.sh uses the new builtin.
… accessor tectonixManifestEntry, tectonixManifestKeys and tectonixManifestIdToPath read .meta/manifest.json through the legacy loader, while the fingerprints of the dependency keys they record read it through the Tecnix repo accessor, like every other tracked read. The two can serve different manifests: from a checkout whose HEAD has moved past the evaluated rev, the legacy loader reads the working tree's file and the accessor serves the rev's, so a target's value and its recorded fingerprints described different manifests. The builtins now read the manifest through the accessor as well, parsed once per evaluation and without recording the whole file (they record their own keys). The new manifest-tracking case evaluates the first commit from a checkout at a later one; before this change the builtins report the checkout's ids and key set while the resolver reads the commit's manifest, after it they agree. The legacy unsafeTectonixInternalManifest builtins keep the legacy loader.
The synthetic keys the manifest builtins record (.meta/manifest.json#<zonePath>, #keys and #id/<zoneId>) are a contract with anything that reads closures, such as a tool that maps changed files to affected targets. Their exact grammar, what each fingerprint tracks, and the rule for consumers (such a key depends on .meta/manifest.json, and a consumer that cannot interpret the fragment must treat any change to that file as affecting it) are now stated in the tectonixManifestEntry documentation and in a new explainer section, 4.2. Only keys that begin with `.meta/manifest.json#` are synthetic: repo paths may contain `#`, so a consumer must not split arbitrary keys at it. One key changes. tectonixManifestEntry given an argument that does not start with `//`, which is never a manifest key, now records the whole file: recorded verbatim, such an argument could spell another form (`keys`, say) whose fingerprint tracks something else. Zone-path arguments record the same keys as before.
|
Thanks. I've addressed all the Tecnix-side points in four commits. The 2. Stale hits (blocker): 9d519dd. I wrote the test first. In
On I kept one difference from the 4. Memo design: 1931407. I went with the "better still" option.
5. Repo accessor: ee40917. The new case in manifest-tracking.sh ("the manifest builtins at an older rev of a checkout") evaluates the first commit from a checkout whose HEAD is later. On the old code the builtins returned the checkout's id 3. Key grammar for the
A key is synthetic only if it starts with One key changes: Tests (local, macOS): build OK,
The libutil, libstore, libfetchers, libexpr and libflake unit suites pass. |
On a miss, `f` was called with the caller's own key value. If the caller computed `k` from a source read, that value carries the read's label, and `f` forcing it inside the memo scope stored the label with the shared entry, so every later caller replayed the first caller's dependencies. Call `f` with a fresh, unlabelled string equal to `k`.
A call that re-entered its own `f k` threw AssertionError, which builtins.tryEval catches. A loader that wraps dependency loads in tryEval then survived a zone cycle with whichever zone the evaluation reached second stored without the other, so results depended on evaluation order, and the eval cache could serve such a result after the cycle was gone, since the order is recorded nowhere. Throw InfiniteRecursionError instead: a value that needs itself fails as it does anywhere else in Nix.
joshheinrichs-shopify
left a comment
joshheinrichs-shopify
left a comment
There was a problem hiding this comment.
i added tests + fixes for a couple minor bugs my bot found. some were pre-existing but just easier to hit with this. if they look reasonable to you feel free to merge!
2 participants
What
Two additions that let a target depend on the part of
.meta/manifest.jsonit read, not on the whole file, plus tests. Four commits:tectonixManifestEntry zonePath,tectonixManifestKeysandtectonixManifestIdToPath zoneId. Each records a synthetic tracked path (.meta/manifest.json#<zonePath>,#keys,#id/<zoneId>) instead of.meta/manifest.json. The eval cache fingerprints a synthetic path by reading the manifest from the accessor and hashing that entry's JSON, the sorted key list, or the zone path an id maps to. Recording and validation use the same function.builtins.tectonixMemo namespace key f. Evaluatesf keyonce per EvalState and returns the shared value. Under Tecnix source tracking the one evaluation runs inside aTrackedSourceDepsScope; its source accesses intern into one label that is stored with the value, and every later consumer records that label, so all consumers share the tracked dependencies without re-evaluatingf key. A same-key re-entry (a zone loading itself) throws a catchableAssertionError; re-entrant misses for distinct keys evaluate outside the map's bucket lock.builtinsattribute set were allocated with a fixed 140 slots each, with no bounds check, and every global constant and primop takes a slot of each. The four new builtins pushnix __dump-language(which the manual build runs, and which registers every builtin including those gated behind experimental features) from 137 to 141, one past the end. CI's sanitizer leg caught it (heap-buffer-overflow,WRITE of size 8, 0 bytes after a 1128-byte region, inEvalState::addConstant). Both now use one named capacity,baseEnvCapacity(160), and registration throws a clear error instead of writing past the end. New testtests/functional/tecnix/base-env.shrunsnix __dump-language; with the check in and the old size of 140 it fails with "the base environment has only 140 slots", so ordinary builds catch this too. I reproduced the overflow locally with an AddressSanitizer build of this branch before the fix (first inaddConstanton the env, then, once that was sized, on thebuiltinsset, which had its own hard-coded 140);nix __dump-languageruns clean under ASan with the fix. This commit sits before the memo commit so every commit after the manifest builtins builds and runs cleanly.Before this, a target that looked up one zone still recorded the whole manifest, so any commit that touched the manifest (for example adding an unrelated zone) invalidated every such target's eval-cache row.
One PR, not two
The two builtins share no code, and I compiled commit 1 alone, so they can be reviewed and even taken separately. I kept them together because the memo only pays off with the per-entry builtins: a memoized zone loader that read the whole manifest or the key set would hand that dependency to every consumer through the shared label. The property this is for (adding an unrelated zone keeps existing targets' cache rows valid) needs both, and both edit the same region of
primops/tectonix.cc.Gate and default
Both are new builtins, so nothing changes unless an expression calls them. The existing manifest builtins (
unsafeTectonixInternalManifestand friends) are untouched and still record the whole file. The only change to existing code is a new branch in the fingerprint function for paths that start with.meta/manifest.json#, which nothing recorded before.A fix included in commit 1
The per-thread fingerprint cache kept the parsed manifest across fingerprint-cache generations, so a second evaluation on the same thread (another
EvalState, for example in an embedding program) fingerprinted its synthetic paths against the first one's manifest. The parsed manifest is now reset when the generation changes. Six of the eight new unit tests, includingTecnixManifestFingerprint.FollowsTheAccessorAcrossCaches, failed before the fix (only the first test in a process saw a fresh thread cache). ThenixCLI has oneEvalStateper process, so I could only reproduce this with the unit tests, not from the command line.What the cache does now
Dependency sets from the new functional test (trimmed): each target records only what it read.
Effect across commits, as asserted by the test (
hit= eval-cache row reused):Measured on a large monorepo with a resolver built on these builtins: across a zone-adding commit, 272 of 272 sampled existing targets stayed eval-cache hits (only the new zone's targets missed), and drvPaths did not change. That resolver lives in the consuming repository and is not part of this PR.
Tests
Built with
nix develop -c meson compile -C build; formatter clean (nix develop -c ./maintainers/format.sh, run on a clean tree, which re-wrapped some cherry-picked lines and needed oneshellcheck disable=SC2016for the Nix expressions inmemo.sh).tests/functional/tecnix/manifest-tracking.sh(scenarios in the table above, all through the eval cache with--pure-eval), and the memo'stests/functional/tectonix/memo.sh(8 checks: correctness, memoization within one evaluation, distinct keys and namespaces, sharing, attrset sharing, cycle detection, re-entrant distinct-key miss).src/libexpr-tests/tecnix-manifest-fingerprint.cc(8): an entry fingerprint ignores other entries and follows its own; an absent entry has its own fingerprint; the key fingerprint tracks the key set only and ignores key order; an id fingerprint tracks where the id points; fingerprints follow the accessor across caches; no manifest means no fingerprint.tests/functional/tecnix/base-env.sh(see the capacity fix above).meson test -C build --suite tecnix --suite tectonix: 9/9 OK (tecnix:base-env,builtins,gc,manifest-tracking; tectonix:basic,errors,deduplication,dirty-zones,memo).meson test -C build --suite libexpr-tests --suite libfetchers-tests --suite libstore-tests --suite libutil-tests --suite libflake-tests: all OK.nix __dump-languageunder an AddressSanitizer build (-Db_sanitize=address): heap-buffer-overflow before the fix, exit 0 after.tectonixMemo(thetry_emplace_and_cvisitrace branch); tracked evaluation never spawns parallel work, so the functional tests cannot reach it.