Release the C extension lock on rb_funcall()

* See socketry/io-event#2

Author: eregon

CHANGELOG.md

@@ -11,6 +11,7 @@ Bug fixes:
 * Fix the `--backtraces-raise` and `--backtraces-rescue` options in JVM mode (#2335).
 * Fix `File.{atime, mtime, ctime}` to include nanoseconds (#2337).
 * Fix `Array#[a, b] = "frozen string literal".freeze` (#2355).
+* `rb_funcall()` now releases the C-extension lock (similar to MRI).
 
 Compatibility:
 

lib/truffle/truffle/cext.rb

@@ -866,11 +866,11 @@ def rb_cmpint(val, a, b)
   end
 
   def rb_funcall_with_block(recv, meth, args, block)
-    recv.public_send(meth, *args, &block)
+    Primitive.public_send_without_cext_lock(recv, meth, args, block)
   end
 
   def rb_funcallv_public(recv, meth, args)
-    recv.public_send(meth, *args)
+    Primitive.public_send_without_cext_lock(recv, meth, args, nil)
   end
 
   def rb_funcallv(recv, meth, args)
@@ -881,13 +881,15 @@ def rb_funcall(recv, meth, n, *args)
     # see #call_with_thread_locally_stored_block
     thread_local_block = Thread.current[:__C_BLOCK__]
     Thread.current[:__C_BLOCK__] = nil
-    recv.__send__(meth, *args, &thread_local_block)
-  ensure
-    Thread.current[:__C_BLOCK__] = thread_local_block
+    begin
+      Primitive.send_without_cext_lock(recv, meth, args, thread_local_block)
+    ensure
+      Thread.current[:__C_BLOCK__] = thread_local_block
+    end
   end
 
   def rb_apply(recv, meth, args)
-    recv.__send__(meth, *args)
+    Primitive.send_without_cext_lock(recv, meth, args, nil)
   end
 
   def rb_define_attr(klass, name, read, write)
@@ -1645,9 +1647,11 @@ def rb_thread_call_with_gvl(function, data)
   end
 
   def rb_thread_call_without_gvl(function, data1, unblock, data2)
-    Primitive.call_without_c_mutex(-> {
-      Primitive.call_with_unblocking_function(Thread.current, function, data1, unblock, data2)
-    }, [])
+    Primitive.send_without_cext_lock(self, :rb_thread_call_without_gvl_inner, [function, data1, unblock, data2], nil)
+  end
+
+  private def rb_thread_call_without_gvl_inner(function, data1, unblock, data2)
+    Primitive.call_with_unblocking_function(Thread.current, function, data1, unblock, data2)
   end
 
   def rb_iterate(iteration, iterated_object, callback, callback_arg)
@@ -1672,14 +1676,14 @@ def rb_iterate(iteration, iterated_object, callback, callback_arg)
   def rb_thread_wait_fd(fd)
     io = IO.for_fd(fd)
     io.autoclose = false
-    Primitive.call_without_c_mutex(IO.method(:select), [[io]])
+    Primitive.send_without_cext_lock(IO, :select, [[io]], nil)
     nil
   end
 
   def rb_thread_fd_writable(fd)
     io = IO.for_fd(fd)
     io.autoclose = false
-    _r, w, _e = Primitive.call_without_c_mutex(IO.method(:select), [nil, [io]])
+    _r, w, _e = Primitive.send_without_cext_lock(IO, :select, [nil, [io]], nil)
     w.size
   end
 
@@ -1698,7 +1702,7 @@ def rb_wait_for_single_fd(fd, events, tv_secs, tv_usecs)
     if tv_secs >= 0 || tv_usecs >= 0
       timeout = tv_secs + tv_usecs/1.0e6
     end
-    r, w, e = Primitive.call_without_c_mutex(IO.method(:select), [read, write, error, *timeout])
+    r, w, e = Primitive.send_without_cext_lock(IO, :select, [read, write, error, *timeout], nil)
     if r.nil? # timeout
       0
     else

spec/truffle/capi/cext_lock_spec.rb

@@ -0,0 +1,35 @@
+# Copyright (c) 2021 Oracle and/or its affiliates. All rights reserved. This
+# code is released under a tri EPL/GPL/LGPL license. You can use it,
+# redistribute it and/or modify it under the terms of the:
+#
+# Eclipse Public License version 2.0, or
+# GNU General Public License version 2, or
+# GNU Lesser General Public License version 2.1.
+
+require_relative '../../ruby/optional/capi/spec_helper'
+
+load_extension("truffleruby_lock")
+
+describe "TruffleRuby C-ext lock" do
+  before :each do
+    @t = CApiTruffleRubyLockSpecs.new
+  end
+
+  it "is not acquired in Ruby code" do
+    Truffle::CExt.cext_lock_owned?.should == false
+  end
+
+  guard -> { Truffle::Boot.get_option 'cexts-lock' } do
+    it "is acquired in C ext code" do
+      @t.has_lock?.should == true
+    end
+  end
+
+  it "is released inside rb_thread_call_without_gvl" do
+    @t.has_lock_in_call_without_gvl?.should == false
+  end
+
+  it "is released inside rb_funcall" do
+    @t.has_lock_in_rb_funcall?(Truffle::CExt).should == false
+  end
+end

spec/truffle/capi/ext/truffleruby_lock_spec.c

@@ -0,0 +1,43 @@
+/*
+ * Copyright (c) 2020 Oracle and/or its affiliates. All rights reserved. This
+ * code is released under a tri EPL/GPL/LGPL license. You can use it,
+ * redistribute it and/or modify it under the terms of the:
+ *
+ * Eclipse Public License version 2.0, or
+ * GNU General Public License version 2, or
+ * GNU Lesser General Public License version 2.1.
+ */
+#include "ruby.h"
+#include "ruby/thread.h"
+#include "rubyspec.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+static VALUE has_lock(VALUE self) {
+  return RUBY_CEXT_FUNCALL("cext_lock_owned?");
+}
+
+static void* called_without_gvl(void* data) {
+  return RUBY_CEXT_FUNCALL("cext_lock_owned?");
+}
+
+static VALUE has_lock_in_call_without_gvl(VALUE self) {
+  return rb_thread_call_without_gvl(called_without_gvl, 0, RUBY_UBF_IO, 0);
+}
+
+static VALUE has_lock_in_rb_funcall(VALUE self, VALUE truffleCExt) {
+  return rb_funcall(truffleCExt, rb_intern("cext_lock_owned?"), 0);
+}
+
+void Init_truffleruby_lock_spec(void) {
+  VALUE cls = rb_define_class("CApiTruffleRubyLockSpecs", rb_cObject);
+  rb_define_method(cls, "has_lock?", has_lock, 0);
+  rb_define_method(cls, "has_lock_in_call_without_gvl?", has_lock_in_call_without_gvl, 0);
+  rb_define_method(cls, "has_lock_in_rb_funcall?", has_lock_in_rb_funcall, 1);
+}
+
+#ifdef __cplusplus
+}
+#endif

spec/truffle/capi/rbasic_spec.rb

@@ -7,6 +7,7 @@
 # GNU Lesser General Public License version 2.1.
 #
 require_relative '../../ruby/optional/capi/spec_helper'
+
 load_extension("truffleruby_rbasic")
 
 describe "RBasic support" do

src/main/java/org/truffleruby/cext/CExtNodes.java

@@ -25,7 +25,7 @@
 import org.truffleruby.builtins.Primitive;
 import org.truffleruby.builtins.PrimitiveArrayArgumentsNode;
 import org.truffleruby.builtins.YieldingCoreMethodNode;
-import org.truffleruby.cext.CExtNodesFactory.CallCWithMutexNodeFactory;
+import org.truffleruby.cext.CExtNodesFactory.CallWithCExtLockNodeFactory;
 import org.truffleruby.cext.CExtNodesFactory.StringToNativeNodeGen;
 import org.truffleruby.collections.ConcurrentOperations;
 import org.truffleruby.core.CoreLibrary;
@@ -118,16 +118,15 @@
 public class CExtNodes {
 
     @Primitive(name = "call_with_c_mutex_and_frame")
-    public abstract static class CallCWithMuteAndFramexNode extends PrimitiveArrayArgumentsNode {
+    public abstract static class CallWithCExtLockAndFrameNode extends PrimitiveArrayArgumentsNode {
 
-        @Child protected CallCWithMutexNode callCextNode = CallCWithMutexNodeFactory.create(RubyNode.EMPTY_ARRAY);
+        @Child protected CallWithCExtLockNode callCextNode = CallWithCExtLockNodeFactory.create(RubyNode.EMPTY_ARRAY);
 
         @Specialization
-        protected Object callCWithMutex(Object receiver, RubyArray argsArray, Object block,
+        protected Object callWithCExtLockAndFrame(Object receiver, RubyArray argsArray, Object block,
                 @Cached MarkingServiceNodes.GetMarkerThreadLocalDataNode getDataNode) {
-            ExtensionCallStack extensionStack = getDataNode.execute().getExtensionCallStack();
+            final ExtensionCallStack extensionStack = getDataNode.execute().getExtensionCallStack();
             extensionStack.push(block);
-
             try {
                 return callCextNode.execute(receiver, argsArray);
             } finally {
@@ -137,12 +136,12 @@ protected Object callCWithMutex(Object receiver, RubyArray argsArray, Object blo
     }
 
     @Primitive(name = "call_with_c_mutex")
-    public abstract static class CallCWithMutexNode extends PrimitiveArrayArgumentsNode {
+    public abstract static class CallWithCExtLockNode extends PrimitiveArrayArgumentsNode {
 
         public abstract Object execute(Object receiver, RubyArray argsArray);
 
         @Specialization(limit = "getCacheLimit()")
-        protected Object callCWithMutex(Object receiver, RubyArray argsArray,
+        protected Object callWithCExtLock(Object receiver, RubyArray argsArray,
                 @CachedLibrary("receiver") InteropLibrary receivers,
                 @Cached ArrayToObjectArrayNode arrayToObjectArrayNode,
                 @Cached TranslateInteropExceptionNode translateInteropExceptionNode,
@@ -174,14 +173,12 @@ protected int getCacheLimit() {
         }
     }
 
-    @Primitive(name = "call_without_c_mutex")
-    public abstract static class CallCWithoutMutexNode extends PrimitiveArrayArgumentsNode {
-
-        @Specialization(limit = "getCacheLimit()")
-        protected Object callCWithoutMutex(Object receiver, RubyArray argsArray,
+    @Primitive(name = "send_without_cext_lock")
+    public abstract static class SendWithoutCExtLockNode extends PrimitiveArrayArgumentsNode {
+        @Specialization
+        protected Object sendWithoutCExtLock(Object receiver, RubySymbol method, RubyArray argsArray, Object block,
                 @Cached ArrayToObjectArrayNode arrayToObjectArrayNode,
-                @CachedLibrary("receiver") InteropLibrary receivers,
-                @Cached TranslateInteropExceptionNode translateInteropExceptionNode,
+                @Cached DispatchNode dispatchNode,
                 @Cached ConditionProfile ownedProfile) {
             final Object[] args = arrayToObjectArrayNode.executeToObjectArray(argsArray);
 
@@ -193,22 +190,54 @@ protected Object callCWithoutMutex(Object receiver, RubyArray argsArray,
                     MutexOperations.unlockInternal(lock);
                 }
                 try {
-                    return InteropNodes.execute(receiver, args, receivers, translateInteropExceptionNode);
+                    return dispatchNode.callWithBlock(receiver, method.getString(), block, args);
                 } finally {
                     if (owned) {
                         MutexOperations.internalLockEvenWithException(getContext(), lock, this);
                     }
                 }
             } else {
-                return InteropNodes.execute(receiver, args, receivers, translateInteropExceptionNode);
+                return dispatchNode.callWithBlock(receiver, method.getString(), block, args);
             }
-
         }
+    }
 
-        protected int getCacheLimit() {
-            return getLanguage().options.DISPATCH_CACHE;
+    @Primitive(name = "public_send_without_cext_lock")
+    public abstract static class PublicSendWithoutCExtLockNode extends PrimitiveArrayArgumentsNode {
+        @Specialization
+        protected Object publicSendWithoutLock(Object receiver, RubySymbol method, RubyArray argsArray, Object block,
+                @Cached ArrayToObjectArrayNode arrayToObjectArrayNode,
+                @Cached(parameters = "PUBLIC") DispatchNode dispatchNode,
+                @Cached ConditionProfile ownedProfile) {
+            final Object[] args = arrayToObjectArrayNode.executeToObjectArray(argsArray);
+
+            if (getContext().getOptions().CEXT_LOCK) {
+                final ReentrantLock lock = getContext().getCExtensionsLock();
+                boolean owned = ownedProfile.profile(lock.isHeldByCurrentThread());
+
+                if (owned) {
+                    MutexOperations.unlockInternal(lock);
+                }
+                try {
+                    return dispatchNode.callWithBlock(receiver, method.getString(), block, args);
+                } finally {
+                    if (owned) {
+                        MutexOperations.internalLockEvenWithException(getContext(), lock, this);
+                    }
+                }
+            } else {
+                return dispatchNode.callWithBlock(receiver, method.getString(), block, args);
+            }
         }
+    }
 
+    @CoreMethod(names = "cext_lock_owned?", onSingleton = true)
+    public abstract static class IsCExtLockOwnedNode extends CoreMethodArrayArgumentsNode {
+        @Specialization
+        protected boolean isCExtLockOwned() {
+            final ReentrantLock lock = getContext().getCExtensionsLock();
+            return lock.isHeldByCurrentThread();
+        }
     }
 
     @CoreMethod(names = "rb_ulong2num", onSingleton = true, required = 1)

src/main/java/org/truffleruby/language/ReadOwnFrameNode.java

@@ -14,7 +14,7 @@
 public class ReadOwnFrameNode extends RubyBaseNode implements FrameOrVariablesReadingNode {
 
     public Object execute(Frame frame) {
-        return frame.materialize();
+        return frame != null ? frame.materialize() : null;
     }
 
     @Override

test/truffle/cexts/backtraces/bin/backtraces

@@ -20,7 +20,7 @@ def callback
 end
 
 def ruby_call
-  RB_FUNCALLV.call(Truffle::CExt.test_cext_wrap(self), Truffle::CExt.test_cext_wrap('callback'), 0, nil)
+  RB_FUNCALLV.call(Truffle::CExt.test_cext_wrap(self), Truffle::CExt.test_cext_wrap(:callback), 0, nil)
 end
 
 def top