summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorThe Bundler Bot <bot@bundler.io>2017-07-19 14:19:42 +0000
committerThe Bundler Bot <bot@bundler.io>2017-07-19 14:19:42 +0000
commit04fd47b1221edabbb46caf34798e5340bc46e1ec (patch)
treec40c05d377a8d105bf1f7330c949c056a07095b7
parent59a99983de7447b043a64b60868a90799841eef0 (diff)
parent4a8d2be31b5bd84ac1e3b3966172ec4d87e9e77b (diff)
downloadbundler-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.rb7
-rw-r--r--lib/bundler/installer.rb25
-rw-r--r--lib/bundler/installer/parallel_installer.rb119
-rw-r--r--spec/commands/install_spec.rb2
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