diff options
author | The Bundler Bot <bot@bundler.io> | 2017-07-19 14:19:42 +0000 |
---|---|---|
committer | The Bundler Bot <bot@bundler.io> | 2017-07-19 14:19:42 +0000 |
commit | 04fd47b1221edabbb46caf34798e5340bc46e1ec (patch) | |
tree | c40c05d377a8d105bf1f7330c949c056a07095b7 | |
parent | 59a99983de7447b043a64b60868a90799841eef0 (diff) | |
parent | 4a8d2be31b5bd84ac1e3b3966172ec4d87e9e77b (diff) | |
download | bundler-04fd47b1221edabbb46caf34798e5340bc46e1ec.tar.gz |
Auto merge of #5844 - bundler:seg-speed-up-no-op-install, r=colby-swandale
Speed up no-op installs
### What was the end-user problem that led to this PR?
The problem was that a "no-op `bundle install`", defined as a user running `bundle install` when `bundle check` would have succeeded was taking roughly 4 times as long as `bundle check`.
Closes #5842.
### What was your diagnosis of the problem?
My diagnosis was that there were a few different bottlenecks:
- loading plugins
- using `Bundler::Worker` when the work done in that workers block would finish nearly instantly
- some unoptimized code in the parallel installer
### What is your fix for the problem, implemented in this PR?
My fix avoids the bottlenecks by checking if the `install` is guaranteed to be a no-op and short-circuiting all but the core `install` method, which in turn will now avoid the `Bundler::Worker` overhead and quickly cache gems when needed.
### Why did you choose this fix out of the possible options?
I chose this fix because it succeeded in speeding up no-op installs by a _huge_ margin.
This _should_ make `install` nearly as fast as a `bundle check`, but at the moment it's still around ~200ms slower. Not an insignificant gap, but a far cry from what it was before the patch. Additionally, since the `install` code path does things such as ensuring everything is cached.
### Time
Run against https://gist.github.com/indirect/6f60f0db1e2f86804ca3ac7c01f2afc4
```
$ time dbundle check
The Gemfile's dependencies are satisfied
BUNDLE_DISABLE_POSTIT=1 ruby ~/Development/OpenSource/bundler/exe/bundle 0.32s user 0.06s system 94% cpu 0.403 total
```
```
$ time bundle check
The Gemfile's dependencies are satisfied
BUNDLE_DISABLE_POSTIT=1 ruby ~/Development/OpenSource/bundler/exe/bundle 0.34s user 0.05s system 96% cpu 0.404 total
```
```
$ time dbundle install --quiet
BUNDLE_DISABLE_POSTIT=1 ruby ~/Development/OpenSource/bundler/exe/bundle 0.48s user 0.07s system 95% cpu 0.582 total
```
```
$ time bundle install --quiet
BUNDLE_DISABLE_POSTIT=1 ruby ~/Development/OpenSource/bundler/exe/bundle 1.68s user 0.30s system 92% cpu 2.146 total
```
-rw-r--r-- | lib/bundler/inline.rb | 7 | ||||
-rw-r--r-- | lib/bundler/installer.rb | 25 | ||||
-rw-r--r-- | lib/bundler/installer/parallel_installer.rb | 119 | ||||
-rw-r--r-- | spec/commands/install_spec.rb | 2 |
4 files changed, 92 insertions, 61 deletions
diff --git a/lib/bundler/inline.rb b/lib/bundler/inline.rb index a78a4c1f01..8462dd9cc1 100644 --- a/lib/bundler/inline.rb +++ b/lib/bundler/inline.rb @@ -51,12 +51,7 @@ def gemfile(install = false, options = {}, &gemfile) definition.validate_runtime! missing_specs = proc do - begin - !definition.missing_specs.empty? - rescue Bundler::GemNotFound, Bundler::GitError - definition.instance_variable_set(:@index, nil) - true - end + definition.missing_specs? end Bundler.ui = ui if install diff --git a/lib/bundler/installer.rb b/lib/bundler/installer.rb index f1a7f0c400..db82660176 100644 --- a/lib/bundler/installer.rb +++ b/lib/bundler/installer.rb @@ -80,9 +80,14 @@ module Bundler return end - resolve_if_needed(options) - ensure_specs_are_compatible! - warn_on_incompatible_bundler_deps + if resolve_if_needed(options) + ensure_specs_are_compatible! + warn_on_incompatible_bundler_deps + load_plugins + options.delete(:jobs) + else + options[:jobs] = 1 # to avoid the overhead of Bundler::Worker + end install(options) lock unless Bundler.frozen? @@ -166,10 +171,14 @@ module Bundler # that said, it's a rare situation (other than rake), and parallel # installation is SO MUCH FASTER. so we let people opt in. def install(options) - load_plugins force = options["force"] - jobs = 1 - jobs = [Bundler.settings[:jobs].to_i - 1, 1].max if can_install_in_parallel? + jobs = options.delete(:jobs) do + if can_install_in_parallel? + [Bundler.settings[:jobs].to_i - 1, 1].max + else + 1 + end + end install_in_parallel jobs, options[:standalone], force end @@ -249,12 +258,14 @@ module Bundler "because a file already exists at that path. Either remove or rename the file so the directory can be created." end + # returns whether or not a re-resolve was needed def resolve_if_needed(options) if !options["update"] && !options["force"] && !Bundler.settings[:inline] && Bundler.default_lockfile.file? - return if @definition.nothing_changed? && !@definition.missing_specs? + return false if @definition.nothing_changed? && !@definition.missing_specs? end options["local"] ? @definition.resolve_with_cache! : @definition.resolve_remotely! + true end def lock(opts = {}) diff --git a/lib/bundler/installer/parallel_installer.rb b/lib/bundler/installer/parallel_installer.rb index c8e755d36e..95d9575c44 100644 --- a/lib/bundler/installer/parallel_installer.rb +++ b/lib/bundler/installer/parallel_installer.rb @@ -78,11 +78,6 @@ module Bundler new(*args).call end - # Returns max number of threads machine can handle with a min of 1 - def self.max_threads - [Bundler.settings[:jobs].to_i - 1, 1].max - end - attr_reader :size def initialize(installer, all_specs, size, standalone, force) @@ -100,54 +95,19 @@ module Bundler require "bundler/gem_remote_fetcher" if RUBY_VERSION < "1.9" check_for_corrupt_lockfile - enqueue_specs - process_specs until @specs.all?(&:installed?) || @specs.any?(&:failed?) + + if @size > 1 + install_with_worker + else + install_serially + end + handle_error if @specs.any?(&:failed?) @specs ensure worker_pool && worker_pool.stop end - def worker_pool - @worker_pool ||= Bundler::Worker.new @size, "Parallel Installer", lambda { |spec_install, worker_num| - gem_installer = Bundler::GemInstaller.new( - spec_install.spec, @installer, @standalone, worker_num, @force - ) - success, message = begin - gem_installer.install_from_spec - rescue => e - raise e, "#{e}\n\n#{require_tree_for_spec(spec_install.spec)}" - end - - if success && !message.nil? - spec_install.post_install_message = message - elsif !success - spec_install.state = :failed - spec_install.error = "#{message}\n\n#{require_tree_for_spec(spec_install.spec)}" - end - spec_install - } - end - - # Dequeue a spec and save its post-install message and then enqueue the - # remaining specs. - # Some specs might've had to wait til this spec was installed to be - # processed so the call to `enqueue_specs` is important after every - # dequeue. - def process_specs - spec = worker_pool.deq - spec.state = :installed unless spec.failed? - enqueue_specs - end - - def handle_error - errors = @specs.select(&:failed?).map(&:error) - if exception = errors.find {|e| e.is_a?(Bundler::BundlerError) } - raise exception - end - raise Bundler::InstallError, errors.map(&:to_s).join("\n\n") - end - def check_for_corrupt_lockfile missing_dependencies = @specs.map do |s| [ @@ -173,6 +133,71 @@ module Bundler Bundler.ui.warn(warning.join("\n")) end + private + + def install_with_worker + enqueue_specs + process_specs until finished_installing? + end + + def install_serially + until finished_installing? + raise "failed to find a spec to enqueue while installing serially" unless spec_install = @specs.find(&:ready_to_enqueue?) + spec_install.state = :enqueued + do_install(spec_install, 0) + end + end + + def worker_pool + @worker_pool ||= Bundler::Worker.new @size, "Parallel Installer", lambda { |spec_install, worker_num| + do_install(spec_install, worker_num) + } + end + + def do_install(spec_install, worker_num) + gem_installer = Bundler::GemInstaller.new( + spec_install.spec, @installer, @standalone, worker_num, @force + ) + success, message = begin + gem_installer.install_from_spec + rescue => e + raise e, "#{e}\n\n#{require_tree_for_spec(spec_install.spec)}" + end + if success + spec_install.state = :installed + spec_install.post_install_message = message unless message.nil? + else + spec_install.state = :failed + spec_install.error = "#{message}\n\n#{require_tree_for_spec(spec_install.spec)}" + end + spec_install + end + + # Dequeue a spec and save its post-install message and then enqueue the + # remaining specs. + # Some specs might've had to wait til this spec was installed to be + # processed so the call to `enqueue_specs` is important after every + # dequeue. + def process_specs + worker_pool.deq + enqueue_specs + end + + def finished_installing? + @specs.all? do |spec| + return true if spec.failed? + spec.installed? + end + end + + def handle_error + errors = @specs.select(&:failed?).map(&:error) + if exception = errors.find {|e| e.is_a?(Bundler::BundlerError) } + raise exception + end + raise Bundler::InstallError, errors.map(&:to_s).join("\n\n") + end + def require_tree_for_spec(spec) tree = @spec_set.what_required(spec) t = String.new("In #{File.basename(SharedHelpers.default_gemfile)}:\n") diff --git a/spec/commands/install_spec.rb b/spec/commands/install_spec.rb index a21b735f8e..3858a45b82 100644 --- a/spec/commands/install_spec.rb +++ b/spec/commands/install_spec.rb @@ -496,7 +496,7 @@ RSpec.describe "bundle install with gem sources" do before do gemfile <<-G source 'https://rubygems.org/' - gem 'bundler' + gem "." G end |