Visitar URL original
A cache lookup walks every digest of an output class under the e-graph lock · Issue #15049 · dagger/dagger · GitHub
Skip to content

A cache lookup walks every digest of an output class under the e-graph lock #15049

Open

Description

@yasyf

What happens. One cache lookup can hold the e-graph lock while it walks a whole eq-class, and every other call in the engine queues behind it. On a long-lived cache, a session with a few hundred checks spends minutes with a single core busy and no execs starting.

The path is the fallback in appendTermSetResultsLocked in dagql/cache_egraph.go. When a call's term exists but has no servable result of its own, lookup walks every digest in the term's output eq-class. For each digest it calls appendDigestResultsLocked, which inserts that digest's posted results into a sorted set. All of this happens under egraphMu.

The cost grows with the number of digests in the class, and classes only grow. Every recipe that produces the same content adds a digest, so a file that is read from many directories gains one for each directory.

What I measured. The cache was restored from disk, with about 1.4M digests in 298k classes. The largest class held 116k digests. 44 classes held over 10k. The three largest each held a single result.

  • A 20 s CPU profile during the stall spent 91% of its time in appendDigestResultsLocked, through lookupCacheForRequestLocked.
  • Goroutine samples found about 240 calls inside the cache at once, and caught the walk in 4 of 6 samples.
  • The first session after the restore took 720 s (722 s on a second engine). With the fix in dagql: read a term's equivalent results from the output class index #15050 it took 381 s on an engine restored from the same state, running the same 650 checks. Later sessions were not slowed on either build.

Fix. outputEqClassResults already holds each class's results once. cacheOutputEqClassCandidatesErrorLocked in the tests asserts it matches the union of the digest postings, and canonicalEquivalentSharedResultLocked already reads it. The fallback can read it too. For a class with 116k digests and one result, that is one insert instead of one per digest.

Related, not fixed here. Address.secret is PerCallInput, so a secret setting that a module reads gains a new digest and term each time it is resolved. In my sessions that was once per check, about 650 per session, and it was the largest class in the cache. Making it PerSessionInput, as Address.container is, stopped those resolutions but did not change session times once the lookup was fixed, so I have left it out.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions