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

formulae_dependents: apply some optimisations #841

Closed
wants to merge 1 commit into from

Conversation

carlocab
Copy link
Member

  1. Doing brew install --only-dependencies and brew postinstall can
    be quite expensive. Let's defer this to when we're sure we have
    dependents to test.
  2. Calling brew uses is slow, because this requires traversing the
    dependency tree. [*] Let's avoid doing this unless we know we really
    need to. Here, we test for "needing to" by checking if another .rb
    file in the tap that might be a formula contains a depends_on line
    declaring a dependency on the formula being tested.
  3. Restrict the second brew uses call to when we are building
    dependents from source, since that is the only instance where we are
    interested in the build dependents.

While we're here, make sure to call brew postinstall on all
dependencies that were rebuilt, and not just the one being tested
currently. This may address #805.

Locally, this results in the following speed up for a formula with no
dependents:

Before

( brew test-bot --only-formulae-dependents --testing-formulae=hello --dry-run)  28.95s user 6.70s system 76% cpu 46.875 total

After

( brew test-bot --only-formulae-dependents --testing-formulae=hello --dry-run)  0.91s user 1.02s system 51% cpu 3.738 total

This makes testing formulae with dependents slightly slower. However,
the vast majority of formulae in Homebrew/core have no dependents (on
macOS, at least), so this is likely a net win for the average workflow.

[*] Potential future optimisation: calling Dependency.expand directly
might give us better opportunities to exploit caching.

1. Doing `brew install --only-dependencies` and `brew postinstall` can
   be quite expensive. Let's defer this to when we're sure we have
   dependents to test.
2. Calling `brew uses` is slow, because this requires traversing the
   dependency tree. [*] Let's avoid doing this unless we know we really
   need to. Here, we test for "needing to" by checking if another `.rb`
   file in the tap that might be a formula contains a `depends_on` line
   declaring a dependency on the formula being tested.
3. Restrict the second `brew uses` call to when we are building
   dependents from source, since that is the only instance where we are
   interested in the build dependents.

While we're here, make sure to call `brew postinstall` on all
dependencies that were rebuilt, and not just the one being tested
currently. This may address Homebrew#805.

Locally, this results in the following speed up for a formula with no
dependents:

Before

    ( brew test-bot --only-formulae-dependents --testing-formulae=hello --dry-run)  28.95s user 6.70s system 76% cpu 46.875 total

After

    ( brew test-bot --only-formulae-dependents --testing-formulae=hello --dry-run)  0.91s user 1.02s system 51% cpu 3.738 total

This makes testing formulae with dependents slightly slower. However,
the vast majority of formulae in Homebrew/core have no dependents (on
macOS, at least), so this is likely a net win for the average workflow.

[*] Potential future optimisation: calling `Dependency.expand` directly
    might give us better opportunities to exploit caching.
next false unless child.file?
next false unless child.extname == ".rb"

child.read.include? "depends_on \"#{formula_name}\""
Copy link
Member

Choose a reason for hiding this comment

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

I'd really love to avoid something this hacky as this when we have RuboCop AST methods to do this sort of thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we construct the AST without loading the formula? Because that's part of what's slow here.

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, doing this instead:

        has_dependents = formula.tap.formula_names.any? do |name|
          Formula[name].deps.map(&:name).include? formula_name
        end

We get:

( brew test-bot --only-formulae-dependents --testing-formulae=hello --dry-run)  12.84s user 2.96s system 68% cpu 23.128 total

It's faster than just doing brew uses, but also a about 6 times slower than just matching text.

Utils.safe_popen_read("brew", "uses", *uses_args, "--include-build", formula_name)
.split("\n")
# We care about build dependents only if we are building them from source.
if args.build_dependents_from_source?
Copy link
Member

Choose a reason for hiding this comment

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

Good call.

# Install formula dependencies. These will have been uninstalled after building.
test "brew", "install", "--only-dependencies", formula_name,
env: { "HOMEBREW_DEVELOPER" => nil }
return if steps.last.failed?

# Restore etc/var files that may have been nuked in the build stage.
test "brew", "postinstall", formula_name
formula_dependencies = Utils.safe_popen_read("brew", "deps", formula_name).split("\n")
# Some dependencies will need postinstalling too.
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why this is the case, can you explain more?

Copy link
Member Author

Choose a reason for hiding this comment

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

Suppose testing formula includes foo and bar, and both have a common dependent baz.

Whenever we test foo first, we do brew postinstall foo but we skip it for bar, but that's wrong. The reasons why we need to brew postinstall foo apply to needing to brew postinstall bar too.

# Install formula dependencies. These will have been uninstalled after building.
test "brew", "install", "--only-dependencies", formula_name,
env: { "HOMEBREW_DEVELOPER" => nil }
return if steps.last.failed?

# Restore etc/var files that may have been nuked in the build stage.
test "brew", "postinstall", formula_name
formula_dependencies = Utils.safe_popen_read("brew", "deps", formula_name).split("\n")
Copy link
Member

Choose a reason for hiding this comment

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

Are we calling brew deps anywhere else? If so, worth caching?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have a look, but I don't think so it's used elsewhere.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale label Oct 18, 2022
@github-actions github-actions bot closed this Oct 25, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants