KEMBAR78
formula: default to cache for `Dependency#expand` usage by cho-m · Pull Request #20827 · Homebrew/brew · GitHub
Skip to content

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Oct 6, 2025

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Performance can be very slow without the cache to prune the duplicate parts of the dependency tree so use the cache by default when running Formula#recursive_dependencies. All other usage of Dependency#expand in Homebrew/brew already uses the cache.


For blocks, I decided to create a temporary cache entry and purge it to avoid unnecessary memory consumption. Used the same naming scheme as FormulaInstaller. Not sure if any chance of collision (if so could add a SecureRandom.uuid or something else)

cache_key = "FormulaInstaller-#{formula.full_name}-#{Time.now.to_f}"

This should help with another slow down seen in:


Even on macOS with smaller dependency tree, there is a noticeable impact:

Benchmark 1: git checkout formula-recursive-dependencies-cache && brew reinstall -sv qt
  Time (mean ± σ):     14.824 s ±  1.606 s    [User: 10.861 s, System: 1.654 s]
  Range (min … max):   13.263 s … 18.121 s    10 runs

Benchmark 2: git checkout main && brew reinstall -sv qt
  Time (mean ± σ):     51.123 s ±  0.774 s    [User: 42.229 s, System: 6.600 s]
  Range (min … max):   49.952 s … 52.388 s    10 runs

Summary
  git checkout formula-recursive-dependencies-cache && brew reinstall -sv qt ran
    3.45 ± 0.38 times faster than git checkout main && brew reinstall -sv qt

@cho-m
Copy link
Member Author

cho-m commented Oct 6, 2025

Wasn't sure if there was any reasonable use case of use_cache: false but added it for now (it is possible for someone to write a block that behaved differently when (dependent, dependency) pair occur in particular parts of tree).


Alternatively, could add cache_key and pass that to all block usage.

dev-cmd/test.rb
60:          missing_test_deps = f.recursive_dependencies do |dependent, dependency|

language/python.rb
196:        formula.recursive_dependencies do |dependent, dep|

test_bot/test_formulae.rb
215:          formula.recursive_dependencies do |_, dep|

build.rb
71:    formula.recursive_dependencies do |dependent, dep|

@cho-m cho-m force-pushed the formula-recursive-dependencies-cache branch from fb17e13 to ebb2b67 Compare October 6, 2025 22:06
@cho-m cho-m changed the title formula: always use cache for recursive_dependencies formula: default to cache for Dependency#expand usage Oct 6, 2025
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine! When is use_cache: false used?

@cho-m
Copy link
Member Author

cho-m commented Oct 7, 2025

Looks fine! When is use_cache: false used?

I don't think it is used in Homebrew/brew. Seeing as it is internal API, may just remove.

Performance can be very slow without the cache to avoid the duplicate
parts of the dependency tree so use the cache by default when running
`recursive_dependencies` and `declared_runtime_dependencies`. All other
usage of `Dependency#expand` in Homebrew/brew already uses the cache.
@cho-m cho-m force-pushed the formula-recursive-dependencies-cache branch from ebb2b67 to cd1defa Compare October 7, 2025 12:37
@cho-m
Copy link
Member Author

cho-m commented Oct 7, 2025

Simplified to just required changes as we can add later on if needed. Will likely merge in an hour or so to finish up Qt PR.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L

@cho-m cho-m added this pull request to the merge queue Oct 7, 2025
Merged via the queue into main with commit 3269773 Oct 7, 2025
38 checks passed
@cho-m cho-m deleted the formula-recursive-dependencies-cache branch October 7, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants