New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement caching for dependency expansion #10887
Conversation
Review period will end on 2021-03-22 at 03:31:58 UTC. |
0d4589c
to
dcfa634
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, nice work! The only thing I'd be tempted to do here is make the interface a bit more similar to the existing uses
caching i.e. Formulary.enable_factory_cache!
or something similar/shared which allows the cache keys to be set inside the relevant methods.
The hard bit is that the return value of |
About 12 minutes -> less than 2 minutes |
🤯
Ah, I see. No worries, then. |
dcfa634
to
e49a338
Compare
Review period ended. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This adds opt-in caching to
Dependency.expand
andRequirement.expand
.This massively increases the speed of those functions, if a cache key is passed. I've adjusted
Formula#rescursive_dependencies
andFormula#recursive_requirements
to do this if no block is passed.This massively improves the
brew uses
runtime from ~40s on Linux to about 6s, with most of that remaining time now being the actualFormula.to_a
file loading.Care is needed when choosing a cache key - the block is not re-evaulated on cache hits and so the cache key should be constructed depending on what variables might require rerunning the block.
I tried to be careful to clear the cache where it might be necessary - hopefully I haven't missed an edge case.
If successful, this should also speed up the
test default formula (Linux)
CI step.