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

Remove auto_config_jobs feature flag #3393

Merged
merged 1 commit into from Mar 20, 2020

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Mar 16, 2020

Description:

In rubygems/bundler#5986 several changes were made:

  • Fix bundler incorrectly setting the number of parallel installer jobs to be one unit less that the value specified by the user.
  • Hide the fix under a feature flag.
  • Introduce the feature of auto-configuring the number of parallel installer jobs to be the number of available processors.
  • Hide the feature under the same feature flag.

In my opinion, hiding both the fix and the feature under a feature flag is wrong and they should instead be enabled by default.

The rationale is that:

  • We should not hide fixes under feature flags (that's for breaking features).
  • We should not hide non-breaking features under feature flags (that's for breaking features).

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@deivid-rodriguez
Copy link
Member Author

I updated the description of this PR to be more accurate.

Also, note that the referenced PR also hiddenly introduced an untested change where whenever the rake gem is part of the bundle, it's always installed first.

I'm not reverting that change here because I'd like to dedicate sometime to think about whether that could actually fix or improve anything, but my intention would be to revert that too because it's untested, it makes the code harder to figure out, and.. I'm not sure why we do that.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Mar 19, 2020

See below some experiments installing on a empty local path to force a cold gem cache, and with a pre-resolved lock file to take resolution time out of the picture.

Gemfile with multiple (slow to install) extensions.

source "https://rubygems.org"

gem "nokogiri"
gem "sassc"

Extension installation is parallelized so the bottleneck is the slowest to compile extension.

Before

$ rm -rf .bundle && bundle config set --local path vendor/bundle && rm -rf vendor/bundle && time bundle install
Fetching gem metadata from https://rubygems.org/..............
Fetching gem metadata from https://rubygems.org/.
Resolving dependencies...
Using bundler 2.2.0.dev
Fetching ffi 1.12.2
Installing ffi 1.12.2 with native extensions
Fetching mini_portile2 2.4.0
Installing mini_portile2 2.4.0
Fetching nokogiri 1.10.9
Installing nokogiri 1.10.9 with native extensions
Fetching sassc 2.2.1
Installing sassc 2.2.1 with native extensions
Bundle complete! 2 Gemfile dependencies, 5 gems now installed.
Bundled gems are installed into `./vendor/bundle`

real 2m19,708s
user 2m9,274s
sys  0m11,112s

After

$ rm -rf .bundle && bundle config set --local path vendor/bundle && rm -rf vendor/bundle && time bundle install
Fetching gem metadata from https://rubygems.org/..............
Using bundler 2.2.0.dev
Fetching ffi 1.12.2
Fetching mini_portile2 2.4.0
Installing mini_portile2 2.4.0
Fetching nokogiri 1.10.9
Installing ffi 1.12.2 with native extensions
Installing nokogiri 1.10.9 with native extensions
Fetching sassc 2.2.1
Installing sassc 2.2.1 with native extensions
Bundle complete! 2 Gemfile dependencies, 5 gems now installed.
Bundled gems are installed into `./vendor/bundle`

real 2m2,657s
user 1m54,486s
sys  0m9,562s

Gemfile with many dependencies

source "https://rubygems.org"

gem "aws-sdk-resources"

Installation greatly benefits from parallelization and installation time is cut in half.

Before

$ rm -rf .bundle && bundle config set --local path vendor/bundle && rm -rf vendor/bundle && time bundle install
Fetching gem metadata from https://rubygems.org/.............
Fetching aws-eventstream 1.0.3
Installing aws-eventstream 1.0.3
Fetching aws-partitions 1.285.0
Installing aws-partitions 1.285.0
(... a bunch of other similar stuff)
Fetching aws-sdk-xray 1.23.0
Installing aws-sdk-xray 1.23.0
Fetching aws-sdk-resources 3.70.0
Installing aws-sdk-resources 3.70.0
Using bundler 2.2.0.dev
Bundle complete! 1 Gemfile dependency, 228 gems now installed.
Bundled gems are installed into `./vendor/bundle`

real 0m20,670s
user 0m12,688s
sys  0m1,333s

After

$ rm -rf .bundle && bundle config set --local path vendor/bundle && rm -rf vendor/bundle && time bundle install
Fetching gem metadata from https://rubygems.org/.............
Using bundler 2.2.0.dev
Fetching aws-sigv2 1.0.1
Fetching aws-eventstream 1.0.3
Fetching jmespath 1.4.0
Fetching aws-partitions 1.285.0
(... a bunch of similar stuff ...)
Installing aws-sdk-xray 1.23.0
Installing aws-sdk-s3 1.61.1
Fetching aws-sdk-resources 3.70.0
Installing aws-sdk-resources 3.70.0
Bundle complete! 1 Gemfile dependency, 228 gems now installed.
Bundled gems are installed into `./vendor/bundle`

real 0m11,381s
user 0m11,557s
sys  0m2,794s

@indirect
Copy link
Member

Nice! I remember at the time we’re really worried about breaking setups that depended on the old default, but we also thought we would eventually ship it. Let’s ship it. :)

The change for rake is because some gems that have an extconf silently depend on rake to be able to compile and install—without putting rake in their dependencies. :/

@indirect
Copy link
Member

(I can’t find the approve button on my phone but I am r+)

@deivid-rodriguez
Copy link
Member Author

The change for rake is because some gems that have an extconf silently depend on rake to be able to compile and install—without putting rake in their dependencies. :/

Aha, I think I understand the potential issue better now. The code I specifically think is not necessary is:

next if @rake && !@rake.installed? && spec.name != @rake.name

That code is making sure that if rake is part of the dependencies, it's always installed first. So if some gems depend on rake but don't specify rake as a dependency, the problem will still exist since that code will not apply.

I can see that, even if rake is specified as a dependency, it might still need to be installed first, because the compilation of extensions might need it.

But we already have code (right after the @rake conditions) to handle that kind of thing in general, not only for rake:

if spec.dependencies_installed? @specs
spec.state = :enqueued
worker_pool.enq spec
end

So, I think we can remove the @rake stuff from the parallel installer.

@deivid-rodriguez
Copy link
Member Author

Regarding this PR, I'll merge it in a bit, thanks for having a look! :)

And enable the code behind it by default.

This setting was introduced when fixing a bug where bundler would one
worker less than the one specified by default. This was a bug fix, so it
should not be hidden under a feature flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants