Cache formula lookups during dependency expansion#22930
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves dependency expansion performance by avoiding repeated dep.to_formula loads and repeated dep.satisfied? checks during a single expansion pass, addressing the Linux slowdown described in #22896 where formula factory caching is disabled on install/upgrade paths.
Changes:
- Add an optional per-expansion
formula_cachetoDependency.expandand use it for recursive expansion to reuse resolvedFormulaobjects for equal dependency edges. - Add per-expansion caching in
FormulaInstaller#expand_dependencies_for_formulaso equal dependency edges only rundep.satisfied?once per expansion call. - Add unit tests covering both caching behavior and ensuring
UsesFromMacOSDependencybounds remain part of dependency identity (no incorrect cache collapsing).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Library/Homebrew/dependency.rb | Adds optional formula_cache: support to reuse dep.to_formula results during expansion (guarded by cache_key). |
| Library/Homebrew/formula_installer.rb | Introduces per-expansion formula_cache and satisfied_cache to cut redundant work in installer dependency expansion. |
| Library/Homebrew/test/dependency_expansion_spec.rb | Adds tests proving formula caching works for equal dependencies and does not collapse differing uses_from_macos bounds. |
| Library/Homebrew/test/formula_installer_spec.rb | Adds tests ensuring dep.satisfied? is evaluated once per equal edge and keeps uses_from_macos bounds distinct. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Patrick Linnane <patrick@linnane.io>
9103985 to
29adb33
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During a single dependency expansion pass,
Dependency.expandresolved every dependency edge with a freshdep.to_formula, andFormulaInstaller#expand_dependencies_for_formulare-randep.satisfied?for equal edges. Homebrew's factory cache is disabled on theinstall/upgradepath, so eachto_formulareloads the formula, and a shared low-level dependency reached through many edges is loaded once per edge. That is the repeatedFormulary::FromAPILoader: loadingwork reported in #22896, worst on Linux when the implicitbubblewrapdependency is unsatisfied and pulled into many subtrees.This adds two caches scoped to one
expand_dependencies_for_formulacall and discarded afterwards: a formula cache so equal edges reuse one resolvedFormula, and a satisfied cache so equal edges checkdep.satisfied?once. Both key on the full dependency identity viahash/eql?(includingUsesFromMacOSDependencybounds), so subclass state is never collapsed. Behaviour is unchanged: the caches only remove repeated work, andformula_cacheraises unless acache_keyis also given, so it is only usable where expansion results are already memoized.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Claude Code (Opus 4.8) implemented the per-expansion caching and its tests and reviewed the change for correctness. I reviewed the diff, confirmed the new specs fail without the caches and pass with them, and ran
brew lgtmplus targetedbrew tests --only=dependency_expansion/--only=formula_installer.