Skip to content
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

global: tweak active_support requires. #8300

Merged
merged 1 commit into from Aug 11, 2020

Conversation

MikeMcQuaid
Copy link
Member

I was hoping that these would speed up boot but unfortunately they make no difference. They are a bit more minimal anyway so probably a good idea.

  • 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 tests with your changes locally?

I was hoping that these would speed up boot but unfortunately they make
no difference. They are a bit more minimal anyway so probably a good
idea.
@MikeMcQuaid MikeMcQuaid merged commit cb3b942 into Homebrew:master Aug 11, 2020
@MikeMcQuaid MikeMcQuaid deleted the active_support_core_ext branch August 11, 2020 12:13
@Bo98
Copy link
Member

Bo98 commented Aug 11, 2020

I was hoping that these would speed up boot but unfortunately they make no difference

The problem I suspect is because require is not exactly the quickest thing in the world.

The load path in brew is quite large, and that is what require searches every time. It doesn't just check for a file's existance in a directory and move on however - it does other things like resolving symlinks etc so it's not the cheapest operation.

I wonder how much quicker things would be if we (for other requiring brew files - not gems) used either require_relative or require with an absolute path (can be a helper function). Ruby sort of nudges you this way anyway - brew files wouldn't ordinarily be in the load path but we make it so.

@MikeMcQuaid
Copy link
Member Author

MikeMcQuaid commented Aug 11, 2020

The load path in brew is quite large, and that is what require searches every time. It doesn't just check for a file's existance in a directory and move on however - it does other things like resolving symlinks etc so it's not the cheapest operation.

Yeh, I was thinking that too. Perhaps if we turned these all into require with a path or require_relative then things could be faster (as you've mentioned).

Another option is extracting the actual logic we care about and vendoring that.

@Bo98
Copy link
Member

Bo98 commented Aug 11, 2020

Played around with the require stuff. It is quicker to give an absolute path but the total savings were only about 75ms. If I can read prof properly (I probably can't) I think the changes I did covered about one quarter of the total require calls.

I didn't touch any taps however.

@MikeMcQuaid
Copy link
Member Author

Yeh, that doesn't sound like much. My guess is it may be the actual code that ActiveSupport is running rather than purely the require lookups.

@Bo98
Copy link
Member

Bo98 commented Aug 11, 2020

There's other micro-optimisations like only requiring debrew/irb when it's actually needed (it takes 30ms to load on its own). It's just a case of seeing what is worth it because it'll be a combination of things rather than one specific cause.

@Bo98
Copy link
Member

Bo98 commented Aug 12, 2020

require "formula" is also a good target that I've played around with. I drafted changes to avoid that requiring file unless needed and saw as much as 150ms improvements.

Cask commands are some of the biggest culprits of requiring half of Homebrew (I profiled cask/cmd.rb as the slowest file to load in Homebrew). They're not lazily loaded like regular brew commands, probably purely because of the brew cask output that lists all subcommands. Subcommands in taps are lazily loaded.

@MikeMcQuaid
Copy link
Member Author

I think active_support is the elephant in the room here; I'm not sure it's worth dealing with other things before that.

@Bo98
Copy link
Member

Bo98 commented Aug 12, 2020

Reducing the requires to just active_support/core_ext/object/blank and active_support/core_ext/object/try saves about 70ms.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 18, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants