Merge pull request #9625 from ruby/claude/silly-joliot-7993f2

Implement a make jobserver (continuation of #9210)

Author: hsbt

bundler/lib/bundler/installer/parallel_installer.rb

@@ -110,10 +110,31 @@ def failed_specs
     end
 
     def install_with_worker
-      installed_specs = {}
-      enqueue_specs(installed_specs)
+      with_jobserver do
+        installed_specs = {}
+        enqueue_specs(installed_specs)
 
-      process_specs(installed_specs) until finished_installing?
+        process_specs(installed_specs) until finished_installing?
+      end
+    end
+
+    def with_jobserver
+      r, w = IO.pipe
+      r.close_on_exec = false
+      w.close_on_exec = false
+      w.write("*" * @size)
+
+      old_makeflags = ENV["MAKEFLAGS"]
+      ENV["MAKEFLAGS"] = [old_makeflags, "--jobserver-auth=#{r.fileno},#{w.fileno}"].compact.join(" ")
+
+      yield
+    ensure
+      # Restore MAKEFLAGS before closing the pipe so a close failure can't
+      # leave the process with descriptors that point at a closed pipe.
+      old_makeflags ? ENV["MAKEFLAGS"] = old_makeflags : ENV.delete("MAKEFLAGS")
+
+      r&.close
+      w&.close
     end
 
     def install_serially

bundler/lib/bundler/rubygems_gem_installer.rb

@@ -4,6 +4,11 @@
 
 module Bundler
   class RubyGemsGemInstaller < Gem::Installer
+    # Cap how many jobserver slots a single gem's `make` may grab so that one
+    # gem with many recipes doesn't starve the others sharing the pool. Beyond
+    # a handful of jobs the extra parallelism rarely pays off in practice.
+    MAX_JOBS_PER_GEM = 3
+
     def check_executable_overwrite(filename)
       # Bundler needs to install gems regardless of binstub overwriting
     end
@@ -124,10 +129,18 @@ def generate_bin_script(filename, bindir)
     end
 
     def build_jobs
-      Bundler.settings[:jobs] || super
+      @jobserver_read_io&.read_nonblock(MAX_JOBS_PER_GEM, @jobserver_tokens)
+      acquired_jobs = @jobserver_tokens.empty? ? nil : @jobserver_tokens.size
+
+      acquired_jobs || Bundler.settings[:jobs] || super
+    rescue IO::WaitReadable, EOFError
+      1
     end
 
     def build_extensions
+      @jobserver_tokens = +""
+      @jobserver_read_io, @jobserver_write_io = connect_to_jobserver
+
       extension_cache_path = options[:bundler_extension_cache_path]
       extension_dir = spec.extension_dir
       unless extension_cache_path && extension_dir
@@ -151,6 +164,11 @@ def build_extensions
           FileUtils.cp_r extension_dir, extension_cache_path
         end
       end
+    ensure
+      unless @jobserver_tokens.empty?
+        @jobserver_write_io.write(@jobserver_tokens)
+        @jobserver_write_io.flush
+      end
     end
 
     def spec
@@ -167,6 +185,21 @@ def gem_checksum
 
     private
 
+    def connect_to_jobserver
+      return unless ENV["MAKEFLAGS"]
+      # We append our own --jobserver-auth, so read the last one. Otherwise a
+      # parent jobserver's descriptors (e.g. `bundle install` run under
+      # `make -j`) would be picked up instead of the pool ParallelInstaller created.
+      read_fd, write_fd = ENV["MAKEFLAGS"].scan(/--jobserver-auth=(\d+),(\d+)/).last
+
+      return unless read_fd && write_fd
+
+      # Pass explicit modes. On POSIX, IO.new detects the descriptor's access
+      # mode, but on Windows it can't, so the write end would default to read
+      # mode and raise "IOError: not opened for writing" when releasing slots.
+      [IO.new(read_fd.to_i, "r", autoclose: false), IO.new(write_fd.to_i, "w", autoclose: false)]
+    end
+
     def prepare_extension_build(extension_dir)
       SharedHelpers.filesystem_access(extension_dir, :create) do
         FileUtils.mkdir_p extension_dir

lib/rubygems/ext/builder.rb

@@ -41,8 +41,9 @@ def self.make(dest_path, results, make_dir = Dir.pwd, sitedir = nil, targets = [
     # nmake doesn't support parallel build
     unless is_nmake
       have_make_arguments = make_program.size > 1
+      n_jobs ||= 0
 
-      if !have_make_arguments && !ENV["MAKEFLAGS"] && n_jobs
+      if !have_make_arguments && n_jobs > 1 && !ENV["MAKEFLAGS"]&.match(/-j\d*(\s|\Z)/)
         make_program << "-j#{n_jobs}"
       end
     end

spec/bundler/installer/parallel_installer_spec.rb

@@ -76,4 +76,138 @@
       parallel_installer.call
     end
   end
+
+  describe "connect to make jobserver" do
+    before do
+      unless Gem::Installer.private_method_defined?(:build_jobs)
+        skip "This example is runnable when RubyGems::Installer implements `build_jobs`"
+      end
+
+      require "support/artifice/compact_index"
+
+      @previous_client = Gem::Request::ConnectionPools.client
+      Gem::Request::ConnectionPools.client = Gem::Net::HTTP
+      Gem::RemoteFetcher.fetcher.close_all
+
+      build_repo2 do
+        build_gem "one", &:add_c_extension
+        build_gem "two", &:add_c_extension
+      end
+
+      gemfile <<~G
+        source "https://gem.repo2"
+
+        gem "one"
+        gem "two"
+      G
+      lockfile <<~L
+        GEM
+          remote: https://gem.repo2/
+          specs:
+            one (1.0)
+            two (1.0)
+
+        DEPENDENCIES
+          one
+          two
+      L
+
+      @old_ui = Bundler.ui
+      Bundler.ui = Bundler::UI::Silent.new
+    end
+
+    after do
+      Bundler.ui = @old_ui
+      Gem::Request::ConnectionPools.client = @previous_client
+      Artifice.deactivate
+    end
+
+    let(:definition) do
+      allow(Bundler).to receive(:root) { bundled_app }
+
+      definition = Bundler::Definition.build(bundled_app.join("Gemfile"), bundled_app.join("Gemfile.lock"), false)
+      definition.tap(&:setup_domain!)
+    end
+    let(:installer) { Bundler::Installer.new(bundled_app, definition) }
+    let(:gem_one) { definition.specs.find {|spec| spec.name == "one" } }
+    let(:gem_two) { definition.specs.find {|spec| spec.name == "two" } }
+
+    it "takes all available slots" do
+      redefine_build_jobs do
+        Bundler::ParallelInstaller.call(installer, definition.specs, 5, false, true)
+      end
+
+      # Take 3 slots out of the 5 available.
+      expect(File.read(File.join(gem_one.extension_dir, "gem_make.out"))).to include("make -j3")
+      # Take the remaining 2 slots.
+      expect(File.read(File.join(gem_two.extension_dir, "gem_make.out"))).to include("make -j2")
+    end
+
+    it "fallback to non parallel when no slots are available" do
+      redefine_build_jobs do
+        Bundler::ParallelInstaller.call(installer, definition.specs, 3, false, true)
+      end
+
+      # Take 3 slots out of the 3 available.
+      expect(File.read(File.join(gem_one.extension_dir, "gem_make.out"))).to include("make -j3")
+      # Fallback to one slot (non parallel).
+      expect(File.read(File.join(gem_two.extension_dir, "gem_make.out"))).to_not include("make -j")
+    end
+
+    it "uses one jobs when installing serially" do
+      Bundler.settings.temporary(jobs: 1) do
+        Bundler::ParallelInstaller.call(installer, definition.specs, 1, false, true)
+      end
+
+      expect(File.read(File.join(gem_one.extension_dir, "gem_make.out"))).to_not include("make -j")
+      expect(File.read(File.join(gem_two.extension_dir, "gem_make.out"))).to_not include("make -j")
+    end
+
+    it "release the job slots" do
+      build_repo2 do
+        build_gem "one", &:add_c_extension
+        build_gem "two" do |spec|
+          spec.add_c_extension
+          spec.add_dependency(:one) # ParallelInstaller will wait for `one` to be fully installed.
+        end
+      end
+
+      Bundler::ParallelInstaller.call(installer, definition.specs, 3, false, true)
+
+      # Take 3 slots out of the 3 available.
+      expect(File.read(File.join(gem_one.extension_dir, "gem_make.out"))).to include("make -j3")
+      # Take 3 slots that were released.
+      expect(File.read(File.join(gem_two.extension_dir, "gem_make.out"))).to include("make -j3")
+    end
+
+    def redefine_build_jobs
+      old_method = Bundler::RubyGemsGemInstaller.instance_method(:build_jobs)
+      Bundler::RubyGemsGemInstaller.remove_method(:build_jobs)
+
+      # Rendezvous so that "one" grabs its slots first and keeps holding them
+      # until "two" has grabbed the rest. Blocking on a queue avoids the
+      # busy-wait and makes the ordering deterministic.
+      one_acquired = Thread::Queue.new
+      two_acquired = Thread::Queue.new
+
+      Bundler::RubyGemsGemInstaller.define_method(:build_jobs) do
+        if spec.name == "one"
+          value = old_method.bind(self).call
+          one_acquired << true
+          two_acquired.pop
+        elsif spec.name == "two"
+          one_acquired.pop
+          value = old_method.bind(self).call
+          two_acquired << true
+        end
+
+        value
+      end
+
+      yield
+    ensure
+      Bundler::RubyGemsGemInstaller.remove_method(:build_jobs)
+      Bundler::RubyGemsGemInstaller.define_method(:build_jobs, old_method)
+    end
+  end
 end

spec/commands/install_spec.rb

@@ -1389,7 +1389,7 @@ def run
       expect(gem_make_out).not_to include("make -j8")
     end
 
-    it "pass down the BUNDLE_JOBS to RubyGems when running the compilation of an extension" do
+    it "uses 3 slots from the available pool when running the compilation of an extension" do
       ENV.delete("MAKEFLAGS")
 
       install_gemfile(<<~G, env: { "BUNDLE_JOBS" => "8" })
@@ -1399,10 +1399,10 @@ def run
 
       gem_make_out = File.read(File.join(@gemspec.extension_dir, "gem_make.out"))
 
-      expect(gem_make_out).to include("make -j8")
+      expect(gem_make_out).to include("make -j3")
     end
 
-    it "uses nprocessors by default" do
+    it "consumes 3 slots from the pool when BUNDLE_JOBS isn't set" do
       ENV.delete("MAKEFLAGS")
 
       install_gemfile(<<~G)
@@ -1412,7 +1412,7 @@ def run
 
       gem_make_out = File.read(File.join(@gemspec.extension_dir, "gem_make.out"))
 
-      expect(gem_make_out).to include("make -j#{Etc.nprocessors + 1}")
+      expect(gem_make_out).to include("make -j3")
     end
   end