cut monitor entry / exits in half on "happy path" requires

[tenderlove]

Before this change RUBYGEMS_ACTIVATION_MONITOR would enter and exit
twice for every call to require. This commit pushes the "exit" call
outside of the ensure block to the rescue and unlocks in the
appropriate place. Eliminating the effectively "global" unlock in then
ensure block allows us to lock and unlock once during happy path
requires, and only lock / unlock twice when gem_original_require fails
and we need to attempt to activate the gem.

Here is a test to demonstrate the problem:

require 'rubygems'
require 'tempfile'

moncalls = []
trace = TracePoint.new(:c_call, :call) do |tp|
  if tp.method_id == :mon_enter
    moncalls << [tp.defined_class, tp.method_id, tp.path]
  end
end

f = Tempfile.open(['foo', '.rb'])
f.write "true"
f.close

$: << File.dirname(f.path)
file = File.basename f.path, '.rb'

trace.enable
require file
trace.disable

p moncalls.length

Before this change moncalls.length will return 2, after this commit,
it returns 1.

[drbrain]

This also blocks concurrent require, see @16fc8e8b90830644cf5eed6b71c7ec2dac4ec5fc For references to other tickets.

Is there another way to reduce monitor acquisition without blocking the original Kernel#require?

[tenderlove]

How does it block concurrent require? It should be the same number of entry / exit during the gem activation path. IOW, this should be exactly equivalent to the previous implementation but fewer locks during the "happy path".

In the current implementation, when this block executes, the monitor exits. If the gem_original_require is successful, the ensure block is executed anyway even though we're supposed to be leaving this method. The final ensure block exits the monitor even though there was no reason to enter it in the first place.

IOW, when this line executes and is successful, we should be exiting the function which means that this line and this line are protecting nothing.

/cc @headius

[tenderlove]

I've added a test to prove that require can be entered from two threads at the same time in 681de4c. It passes on master as well as my branch.

The test ensures that two files are required simultaneously. It creates two latches. The main thread waits on the FILE_ENTERED_LATCH to be released. The FILE_ENTERED_LATCH ensures that both files have been required. Then the child threads wait on the FILE_EXIT_LATCH. The main thread releases the FILE_EXIT_LATCH only after the FILE_ENTERED_LATCH has been released twice.

IOW, the main thread is blocked until both child threads start to evaluate their files, then the child threads are blocked until the main thread releases.

This test doesn't test every branch in require, but it does demo that require is not blocked.

[headius]

A "proof of concept" diff for moving the unlocked bits outside of the locked area, rather than explicitly juggling lock status... The patches by @tenderlove should work fine, but I worry about future early-exits where people forget to add the lock release.

This diff is against JRuby 1.7.x head (jruby-1_7 branch) so it may differ from RG master a bit.

diff --git a/lib/ruby/shared/rubygems/core_ext/kernel_require.rb b/lib/ruby/shared/rubygems/core_ext/kernel_require.rb
index 84bb03f..00bc956 100644
--- a/lib/ruby/shared/rubygems/core_ext/kernel_require.rb
+++ b/lib/ruby/shared/rubygems/core_ext/kernel_require.rb
@@ -36,8 +36,7 @@ module Kernel
   # that file has already been loaded is preserved.

   def require path
-    RUBYGEMS_ACTIVATION_MONITOR.enter
-
+    require_path = RUBYGEMS_ACTIVATION_MONITOR.synchronize do
       path = path.to_path if path.respond_to? :to_path

       spec = Gem.find_unresolved_default_spec(path)
@@ -50,12 +49,7 @@ module Kernel
       # normal require handle loading a gem from the rescue below.

       if Gem::Specification.unresolved_deps.empty? then
-      begin
-        RUBYGEMS_ACTIVATION_MONITOR.exit
-        return gem_original_require(path)
-      ensure
-        RUBYGEMS_ACTIVATION_MONITOR.enter
-      end
+        next path
       end

       # If +path+ is for a gem that has already been loaded, don't
@@ -68,12 +62,7 @@ module Kernel
         s.activated? and s.contains_requirable_file? path
       }

-    begin
-      RUBYGEMS_ACTIVATION_MONITOR.exit
-      return gem_original_require(path)
-    ensure
-      RUBYGEMS_ACTIVATION_MONITOR.enter
-    end if spec
+      next path if spec

       # Attempt to find +path+ in any unresolved gems...

@@ -121,26 +110,21 @@ module Kernel
         valid.activate
       end

-    begin
-      RUBYGEMS_ACTIVATION_MONITOR.exit
-      return gem_original_require(path)
-    ensure
-      RUBYGEMS_ACTIVATION_MONITOR.enter
+      next path
     end
+
+    return gem_original_require(require_path)
   rescue LoadError => load_error
+    require_path = RUBYGEMS_ACTIVATION_MONITOR.synchronize do
       if load_error.message.start_with?("Could not find") or
           (load_error.message.end_with?(path) and Gem.try_activate(path)) then
-      begin
-        RUBYGEMS_ACTIVATION_MONITOR.exit
-        return gem_original_require(path)
-      ensure
-        RUBYGEMS_ACTIVATION_MONITOR.enter
-      end
+        next path
       end

       raise load_error
-  ensure
-    RUBYGEMS_ACTIVATION_MONITOR.exit
+    end
+
+    return gem_original_require(require_path)
   end

   private :require

[tenderlove]

@headius thanks for the feedback! After I can get this fix merged, I'll refactor it to look more like yours. I am also worried about early exits, and I think your way will end up with more clear code.