tap: fix performance regression in *_files_by_name #16791

apainintheneck · 2024-03-02T03:32:17Z

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?

We essentially stopped caching these accidentally and they get called every time we try to load a cask or formula from the API. It gets really, really, really slow.

I ran brew deps --casks --eval-all before and after the changes.

Before

I let it run for 3 minutes before killing it. No output had been printed to the screen.

After

It finished printing all output (pages and pages of it) in less than a minute.

This should match the caching behavior we had before the recent changes in these two PRs.

We essentially stopped caching these accidentally and they get called every time we try to load a cask or formula from the API. It gets really, really, really slow. I ran `brew deps --casks --eval-all` before and after the changes. I let it run for 3 minutes before killing it. No output had been printed to the screen. It finished printing all output (pages and pages of it) in less than a minute. --- This should match the caching behavior we had before the recent changes in these two PRs. - #16777 - #16775

apainintheneck · 2024-03-02T04:01:48Z

I found another one. Since it's related to other ones, I just added it to this PR.

Tap.reverse_tap_migrations_renames should be a relatively small hash but it gets recalculated every time we call Cask#oldnames or Formula#oldnames which ends up being a lot if you run certain commands with --eval-all.

For example, when running the following, it took 10% of the total runtime.

brew deps --casks --eval-all

This should be a relatively small hash but it gets recalculated every time we call Cask#oldnames or Formula#oldnames which ends up being a lot if you run certain commands with --eval-all. For example, when running the following, it took 10% of the total runtime. ``` brew deps --casks --eval-all ```

Bo98

Thanks for catching this quickly!

Bo98 · 2024-03-02T05:00:06Z

Library/Homebrew/tap.rb

@@ -1279,7 +1281,7 @@ def cask_tokens
 def cask_files_by_name
 return super if Homebrew::EnvConfig.no_install_from_api?

- Homebrew::API::Cask.all_casks.each_with_object({}) do |item, hash|
+ @cask_files_by_name ||= Homebrew::API::Cask.all_casks.each_with_object({}) do |item, hash|


Hmm, we probably should avoid Pathname here too

reitermarkus · 2024-03-02T13:04:51Z

Library/Homebrew/tap.rb

@@ -820,7 +820,7 @@ def formula_reverse_renames

 sig { returns(T::Hash[String, T::Array[String]]) }
 def self.reverse_tap_migrations_renames
- Tap.each_with_object({}) do |tap, hash|
+ cache[:reverse_tap_migrations_renames] ||= Tap.each_with_object({}) do |tap, hash|


@apainintheneck, I remember specifically not caching this because it will never be cleared, except in tests.

Basically, every instance's #clear_cache would need to clear the global cache[:reverse_tap_migrations_renames], because it depends on all taps.

My understanding is that Tap.reverse_tap_migrations_renames loads each tap with Tap.each and then calls Tap#tap_migrations on each tap to build this list. Do we expect either Tap.each or Tap#tap_migrations to change over the life of the program?

I guess you're worried about the case where a tap gets installed with Tap#ensure_installed! after Tap.reverse_tap_migrations_renames has been built, right? Maybe it'd just be a good idea to clear the class level cache when we do that since Tap#ensure_installed! should only very rarely need to install a new tap.

Or we could just go back to how things are. Either way works for me.

Could just have the cache inside the Tap.each_with_object instead and cache by cache[:reverse_tap_migrations_renames][tap.name], or have an instance method so it's tied to instance cache.

Now that I look at it a bit more the problem might be Tap.each in this case.

This is from running brew prof -- deps --casks --eval-all.

A special cache of installed tap instances that gets reset on Tap#install and Tap#uninstall would make sense to me.

That sounds doable. I'm not sure how that could be implemented in a simple, straightforward way though. This seemed like a no-brainer but now I know it wasn't.

A special cache of installed tap instances that gets reset on Tap#install and Tap#uninstall would make sense to me.

@Bo98 @apainintheneck @reitermarkus I think we should/could just nuke all tap caches on those methods given how infrequently they are called.

apainintheneck added the bug Reproducible Homebrew/brew bug label Mar 2, 2024

apainintheneck marked this pull request as draft March 2, 2024 04:15

apainintheneck force-pushed the fix-perfomance-regression-in-tap-files_by_name branch from 5493649 to 2da1c93 Compare March 2, 2024 04:21

apainintheneck marked this pull request as ready for review March 2, 2024 04:21

Bo98 approved these changes Mar 2, 2024

View reviewed changes

Bo98 merged commit 4e65af0 into master Mar 2, 2024
26 checks passed

Bo98 deleted the fix-perfomance-regression-in-tap-files_by_name branch March 2, 2024 05:00

reitermarkus reviewed Mar 2, 2024

View reviewed changes

This was referenced Mar 2, 2024

tap: revert caching Tap.reverse_tap_migrations_renames #16797

Merged

tap: memoize loading installed taps #16806

Closed

github-actions bot added the outdated PR was locked due to age label Apr 3, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tap: fix performance regression in *_files_by_name #16791

tap: fix performance regression in *_files_by_name #16791

apainintheneck commented Mar 2, 2024 •

edited

apainintheneck commented Mar 2, 2024 •

edited

Bo98 left a comment

Bo98 Mar 2, 2024

apainintheneck Mar 2, 2024

reitermarkus Mar 2, 2024 •

edited

apainintheneck Mar 2, 2024

apainintheneck Mar 2, 2024 •

edited

apainintheneck Mar 2, 2024

Bo98 Mar 2, 2024 •

edited

apainintheneck Mar 2, 2024

Bo98 Mar 2, 2024

apainintheneck Mar 2, 2024

MikeMcQuaid Mar 4, 2024 •

edited

tap: fix performance regression in *_files_by_name #16791

tap: fix performance regression in *_files_by_name #16791

Conversation

apainintheneck commented Mar 2, 2024 • edited

Before

After

apainintheneck commented Mar 2, 2024 • edited

Bo98 left a comment

Choose a reason for hiding this comment

Bo98 Mar 2, 2024

Choose a reason for hiding this comment

apainintheneck Mar 2, 2024

Choose a reason for hiding this comment

reitermarkus Mar 2, 2024 • edited

Choose a reason for hiding this comment

apainintheneck Mar 2, 2024

Choose a reason for hiding this comment

apainintheneck Mar 2, 2024 • edited

Choose a reason for hiding this comment

apainintheneck Mar 2, 2024

Choose a reason for hiding this comment

Bo98 Mar 2, 2024 • edited

Choose a reason for hiding this comment

apainintheneck Mar 2, 2024

Choose a reason for hiding this comment

Bo98 Mar 2, 2024

Choose a reason for hiding this comment

apainintheneck Mar 2, 2024

Choose a reason for hiding this comment

MikeMcQuaid Mar 4, 2024 • edited

Choose a reason for hiding this comment

apainintheneck commented Mar 2, 2024 •

edited

apainintheneck commented Mar 2, 2024 •

edited

reitermarkus Mar 2, 2024 •

edited

apainintheneck Mar 2, 2024 •

edited

Bo98 Mar 2, 2024 •

edited

MikeMcQuaid Mar 4, 2024 •

edited