From d49598c321b082cb2be45a094ad66f733766f05f Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Thu, 24 Jun 2021 11:54:03 +0100 Subject: [PATCH 01/37] Move monitor implementation to Java nodes. --- lib/mri/monitor.rb | 118 ++---------------- .../truffleruby/builtins/BuiltinsClasses.java | 4 + .../org/truffleruby/core/CoreLibrary.java | 1 + .../core/monitor/TruffleMonitorNodes.java | 112 +++++++++++++++++ .../core/mutex/MutexOperations.java | 14 ++- .../org/truffleruby/core/mutex/RubyMutex.java | 2 +- .../truffleruby/core/thread/RubyThread.java | 4 +- .../core/thread/ThreadManager.java | 9 +- 8 files changed, 149 insertions(+), 115 deletions(-) create mode 100644 src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java diff --git a/lib/mri/monitor.rb b/lib/mri/monitor.rb index 1597ee9c4015..b71eeee20466 100644 --- a/lib/mri/monitor.rb +++ b/lib/mri/monitor.rb @@ -1,3 +1,4 @@ +# truffleruby_primitives: true # frozen_string_literal: false # = monitor.rb # @@ -87,39 +88,15 @@ # MonitorMixin module. # module MonitorMixin - EXCEPTION_NEVER = {Exception => :never}.freeze - EXCEPTION_IMMEDIATE = {Exception => :immediate}.freeze - # # FIXME: This isn't documented in Nutshell. # # Since MonitorMixin.new_cond returns a ConditionVariable, and the example # above calls while_wait and signal, this class should be documented. # - class ConditionVariable + class ConditionVariable < ::ConditionVariable class Timeout < Exception; end - # - # Releases the lock held in the associated monitor and waits; reacquires the lock on wakeup. - # - # If +timeout+ is given, this method returns after +timeout+ seconds passed, - # even if no other thread doesn't signal. - # - def wait(timeout = nil) - Thread.handle_interrupt(EXCEPTION_NEVER) do - @monitor.__send__(:mon_check_owner) - count = @monitor.__send__(:mon_exit_for_cond) - begin - Thread.handle_interrupt(EXCEPTION_IMMEDIATE) do - @cond.wait(@monitor.instance_variable_get(:@mon_mutex), timeout) - end - return true - ensure - @monitor.__send__(:mon_enter_for_cond, count) - end - end - end - # # Calls wait repeatedly while the given block yields a truthy value. # @@ -137,29 +114,6 @@ def wait_until wait end end - - # - # Wakes up the first thread in line waiting for this lock. - # - def signal - @monitor.__send__(:mon_check_owner) - @cond.signal - end - - # - # Wakes up all threads waiting for this lock. - # - def broadcast - @monitor.__send__(:mon_check_owner) - @cond.broadcast - end - - private - - def initialize(monitor) - @monitor = monitor - @cond = Thread::ConditionVariable.new - end end def self.extend_object(obj) @@ -171,15 +125,7 @@ def self.extend_object(obj) # Attempts to enter exclusive section. Returns +false+ if lock fails. # def mon_try_enter - if @mon_owner != Thread.current - unless @mon_mutex.try_lock - return false - end - @mon_owner = Thread.current - @mon_count = 0 - end - @mon_count += 1 - return true + Truffle::MonitorOperations.mon_try_mutex(@mon_mutex) end # For backward compatibility alias try_mon_enter mon_try_enter @@ -188,24 +134,14 @@ def mon_try_enter # Enters exclusive section. # def mon_enter - if @mon_owner != Thread.current - @mon_mutex.lock - @mon_owner = Thread.current - @mon_count = 0 - end - @mon_count += 1 + Truffle::MonitorOperations.mon_mutex(@mon_mutex) end # # Leaves exclusive section. # def mon_exit - mon_check_owner - @mon_count -=1 - if @mon_count == 0 - @mon_owner = nil - @mon_mutex.unlock - end + Truffle::MonitorOperations.mon_exit(@mon_mutex) end # @@ -219,7 +155,7 @@ def mon_locked? # Returns true if this monitor is locked by current thread. # def mon_owned? - @mon_mutex.locked? && @mon_owner == Thread.current + @mon_mutex.owned? end # @@ -227,19 +163,8 @@ def mon_owned? # section automatically when the block exits. See example under # +MonitorMixin+. # - def mon_synchronize - # Prevent interrupt on handling interrupts; for example timeout errors - # it may break locking state. - Thread.handle_interrupt(EXCEPTION_NEVER) do - mon_enter - begin - Thread.handle_interrupt(EXCEPTION_IMMEDIATE) do - yield - end - ensure - mon_exit - end - end + def mon_synchronize(&block) + Truffle::MonitorOperations.synchronize_on_mutex(@mon_mutex, &block) end alias synchronize mon_synchronize @@ -248,11 +173,9 @@ def mon_synchronize # receiver. # def new_cond - return ConditionVariable.new(self) + return Truffle::MonitorOperations.condition_var_for_mutex(ConditionVariable, @mon_mutex) end - private - # Use extend MonitorMixin or include MonitorMixin instead # of this constructor. Have look at the examples above to understand how to # use this module. @@ -264,32 +187,13 @@ def initialize(*args) # Initializes the MonitorMixin after being included in a class or when an # object has been extended with the MonitorMixin def mon_initialize - if defined?(@mon_mutex) && @mon_mutex_owner_object_id == object_id + if defined?(@mon_mutex) && Primitive.object_same_or_equal(@mon_mutex_owner_object, self) raise ThreadError, "already initialized" end @mon_mutex = Thread::Mutex.new - @mon_mutex_owner_object_id = object_id - @mon_owner = nil - @mon_count = 0 - end - - def mon_check_owner - if @mon_owner != Thread.current - raise ThreadError, "current thread not owner" - end + @mon_mutex_owner_object = self end - def mon_enter_for_cond(count) - @mon_owner = Thread.current - @mon_count = count - end - - def mon_exit_for_cond - count = @mon_count - @mon_owner = nil - @mon_count = 0 - return count - end end # Use the Monitor class when you want to have a lock object for blocks with diff --git a/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java b/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java index b7f1cf17ef24..fda8c793db9c 100644 --- a/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java +++ b/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java @@ -76,6 +76,8 @@ import org.truffleruby.core.method.UnboundMethodNodesFactory; import org.truffleruby.core.module.ModuleNodesBuiltins; import org.truffleruby.core.module.ModuleNodesFactory; +import org.truffleruby.core.monitor.TruffleMonitorNodesBuiltins; +import org.truffleruby.core.monitor.TruffleMonitorNodesFactory; import org.truffleruby.core.mutex.ConditionVariableNodesBuiltins; import org.truffleruby.core.mutex.ConditionVariableNodesFactory; import org.truffleruby.core.mutex.MutexNodesBuiltins; @@ -241,6 +243,7 @@ public static void setupBuiltinsLazy(CoreMethodNodeManager coreManager) { TruffleDebugNodesBuiltins.setup(coreManager); TruffleGraalNodesBuiltins.setup(coreManager); TruffleKernelNodesBuiltins.setup(coreManager); + TruffleMonitorNodesBuiltins.setup(coreManager); TrufflePosixNodesBuiltins.setup(coreManager); TruffleRegexpNodesBuiltins.setup(coreManager); TruffleRopesNodesBuiltins.setup(coreManager); @@ -404,6 +407,7 @@ public static List>> getCoreN TruffleDebugNodesFactory.getFactories(), TruffleGraalNodesFactory.getFactories(), TruffleKernelNodesFactory.getFactories(), + TruffleMonitorNodesFactory.getFactories(), TrufflePosixNodesFactory.getFactories(), TruffleRegexpNodesFactory.getFactories(), TruffleRopesNodesFactory.getFactories(), diff --git a/src/main/java/org/truffleruby/core/CoreLibrary.java b/src/main/java/org/truffleruby/core/CoreLibrary.java index ea399956ad5b..2cade05db8ad 100644 --- a/src/main/java/org/truffleruby/core/CoreLibrary.java +++ b/src/main/java/org/truffleruby/core/CoreLibrary.java @@ -503,6 +503,7 @@ public CoreLibrary(RubyContext context, RubyLanguage language) { truffleFeatureLoaderModule = defineModule(truffleModule, "FeatureLoader"); truffleKernelOperationsModule = defineModule(truffleModule, "KernelOperations"); truffleInteropOperationsModule = defineModule(truffleModule, "InteropOperations"); + defineModule(truffleModule, "MonitorOperations"); defineModule(truffleModule, "Binding"); defineModule(truffleModule, "POSIX"); defineModule(truffleModule, "Readline"); diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java new file mode 100644 index 000000000000..8c1f5a7dcc9e --- /dev/null +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -0,0 +1,112 @@ +/* + * 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. + */ +package org.truffleruby.core.monitor; + +import java.util.concurrent.locks.Condition; +import java.util.concurrent.locks.ReentrantLock; + +import com.oracle.truffle.api.dsl.Cached; +import com.oracle.truffle.api.dsl.Specialization; +import com.oracle.truffle.api.object.Shape; + +import org.truffleruby.builtins.CoreMethod; +import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; +import org.truffleruby.builtins.CoreModule; +import org.truffleruby.builtins.YieldingCoreMethodNode; +import org.truffleruby.core.klass.RubyClass; +import org.truffleruby.core.mutex.MutexOperations; +import org.truffleruby.core.mutex.RubyConditionVariable; +import org.truffleruby.core.mutex.MutexOperations; +import org.truffleruby.core.mutex.RubyMutex; +import org.truffleruby.core.proc.RubyProc; +import org.truffleruby.core.thread.GetCurrentRubyThreadNode; +import org.truffleruby.core.thread.RubyThread; +import org.truffleruby.language.objects.AllocationTracing; + +@CoreModule("Truffle::MonitorOperations") +public abstract class TruffleMonitorNodes { + + @CoreMethod(names = "condition_var_for_mutex", onSingleton = true, required = 2) + public abstract static class LinkedConditionVariableNode extends CoreMethodArrayArgumentsNode { + + @Specialization + protected RubyConditionVariable linkedConditionVariable(RubyClass rubyClass, RubyMutex mutex) { + final ReentrantLock condLock = mutex.lock; + final Condition condition = MutexOperations.newCondition(condLock); + final Shape shape = getLanguage().conditionVariableShape; + final RubyConditionVariable instance = new RubyConditionVariable(rubyClass, shape, condLock, condition); + AllocationTracing.trace(instance, this); + return instance; + } + } + + @CoreMethod(names = "synchronize_on_mutex", onSingleton = true, required = 1, needsBlock = true) + public abstract static class SynchronizeNode extends YieldingCoreMethodNode { + + @Specialization(guards = "isRubyProc(maybeBlock)") + protected Object synchronizeOnMutex(RubyMutex mutex, Object maybeBlock, + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { + final RubyThread thread = getCurrentRubyThreadNode.execute(); + MutexOperations.lock(getContext(), mutex.lock, thread, this); + try { + return callBlock((RubyProc) maybeBlock); + } finally { + MutexOperations.unlock(mutex.lock, thread); + } + } + + @Specialization(guards = "!isRubyProc(maybeBlock)") + protected Object synchronizeOnMutexNoBlock(RubyMutex mutex, Object maybeBlock, + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { + final RubyThread thread = getCurrentRubyThreadNode.execute(); + MutexOperations.lock(getContext(), mutex.lock, thread, this); + try { + return nil; + } finally { + MutexOperations.unlock(mutex.lock, thread); + } + } + } + + @CoreMethod(names = "mon_try_enter", onSingleton = true, required = 1) + public static abstract class MonitorTryEnter extends CoreMethodArrayArgumentsNode { + + @Specialization + protected Object tryEnter(RubyMutex mutex, + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { + final RubyThread thread = getCurrentRubyThreadNode.execute(); + return MutexOperations.tryLock(mutex.lock, thread); + } + } + + @CoreMethod(names = "mon_enter", onSingleton = true, required = 1) + public static abstract class MonitorEnter extends CoreMethodArrayArgumentsNode { + + @Specialization + protected Object enter(RubyMutex mutex, + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { + final RubyThread thread = getCurrentRubyThreadNode.execute(); + MutexOperations.lock(getContext(), mutex.lock, thread, this); + return mutex.lock.getHoldCount(); + } + } + + @CoreMethod(names = "mon_exit", onSingleton = true, required = 1) + public static abstract class MonitorExit extends CoreMethodArrayArgumentsNode { + + @Specialization + protected Object exit(RubyMutex mutex, + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { + final RubyThread thread = getCurrentRubyThreadNode.execute(); + MutexOperations.unlock(mutex.lock, thread); + return mutex; + } + } +} diff --git a/src/main/java/org/truffleruby/core/mutex/MutexOperations.java b/src/main/java/org/truffleruby/core/mutex/MutexOperations.java index f1db5c573835..b51a7cb63655 100644 --- a/src/main/java/org/truffleruby/core/mutex/MutexOperations.java +++ b/src/main/java/org/truffleruby/core/mutex/MutexOperations.java @@ -26,11 +26,21 @@ public abstract class MutexOperations { @TruffleBoundary - protected static void lock(RubyContext context, ReentrantLock lock, RubyThread thread, RubyNode currentNode) { + public static void lock(RubyContext context, ReentrantLock lock, RubyThread thread, RubyNode currentNode) { lockInternal(context, lock, currentNode); thread.ownedLocks.add(lock); } + @TruffleBoundary + public static boolean tryLock(ReentrantLock lock, RubyThread thread) { + if (lock.tryLock()) { + thread.ownedLocks.add(lock); + return true; + } else { + return false; + } + } + @TruffleBoundary public static void lockInternal(RubyContext context, ReentrantLock lock, Node currentNode) { if (lock.tryLock()) { @@ -91,7 +101,7 @@ public static void internalLockEvenWithException(RubyContext context, ReentrantL } @TruffleBoundary - protected static void unlock(ReentrantLock lock, RubyThread thread) { + public static void unlock(ReentrantLock lock, RubyThread thread) { unlockInternal(lock); thread.ownedLocks.remove(lock); } diff --git a/src/main/java/org/truffleruby/core/mutex/RubyMutex.java b/src/main/java/org/truffleruby/core/mutex/RubyMutex.java index 86e2b9d6becd..3185db8117e1 100644 --- a/src/main/java/org/truffleruby/core/mutex/RubyMutex.java +++ b/src/main/java/org/truffleruby/core/mutex/RubyMutex.java @@ -18,7 +18,7 @@ public final class RubyMutex extends RubyDynamicObject { - final ReentrantLock lock; + public final ReentrantLock lock; public RubyMutex(RubyClass rubyClass, Shape shape, ReentrantLock lock) { super(rubyClass, shape); diff --git a/src/main/java/org/truffleruby/core/thread/RubyThread.java b/src/main/java/org/truffleruby/core/thread/RubyThread.java index 14fdb1c3ed83..2c88f36e3eb0 100644 --- a/src/main/java/org/truffleruby/core/thread/RubyThread.java +++ b/src/main/java/org/truffleruby/core/thread/RubyThread.java @@ -14,7 +14,7 @@ import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import org.truffleruby.RubyContext; import org.truffleruby.RubyLanguage; @@ -41,7 +41,7 @@ public class RubyThread extends RubyDynamicObject implements ObjectGraphNode { public final ThreadLocalGlobals threadLocalGlobals = new ThreadLocalGlobals(); public InterruptMode interruptMode = InterruptMode.IMMEDIATE; // only accessed by this Ruby Thread and its Fibers public volatile ThreadStatus status = ThreadStatus.RUN; - public final List ownedLocks = new ArrayList<>(); + public final List ownedLocks = new ArrayList<>(); public final FiberManager fiberManager; CountDownLatch finishedLatch = new CountDownLatch(1); final RubyHash threadLocalVariables; diff --git a/src/main/java/org/truffleruby/core/thread/ThreadManager.java b/src/main/java/org/truffleruby/core/thread/ThreadManager.java index 2319d500db8a..0c121ed41d43 100644 --- a/src/main/java/org/truffleruby/core/thread/ThreadManager.java +++ b/src/main/java/org/truffleruby/core/thread/ThreadManager.java @@ -16,7 +16,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Supplier; import com.oracle.truffle.api.CompilerAsserts; @@ -421,8 +421,11 @@ public void cleanupThreadState(RubyThread thread, Thread javaThread) { thread.thread = null; if (Thread.currentThread() == javaThread) { - for (Lock lock : thread.ownedLocks) { - lock.unlock(); + for (ReentrantLock lock : thread.ownedLocks) { + int count = lock.getHoldCount(); + for (int i = 0; i < count; i++) { + lock.unlock(); + } } } else { if (!thread.ownedLocks.isEmpty()) { From 4414a0a959ac6cd0207628bb9d1be9bec36312fd Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Fri, 25 Jun 2021 16:02:48 +0100 Subject: [PATCH 02/37] Move to primitives for monitor operations. --- lib/mri/monitor.rb | 10 +++++----- .../truffleruby/builtins/BuiltinsClasses.java | 1 + .../core/monitor/TruffleMonitorNodes.java | 20 ++++++++++--------- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/lib/mri/monitor.rb b/lib/mri/monitor.rb index b71eeee20466..07bc6be42d71 100644 --- a/lib/mri/monitor.rb +++ b/lib/mri/monitor.rb @@ -125,7 +125,7 @@ def self.extend_object(obj) # Attempts to enter exclusive section. Returns +false+ if lock fails. # def mon_try_enter - Truffle::MonitorOperations.mon_try_mutex(@mon_mutex) + Primitive.monitor_try_enter(@mon_mutex) end # For backward compatibility alias try_mon_enter mon_try_enter @@ -134,14 +134,14 @@ def mon_try_enter # Enters exclusive section. # def mon_enter - Truffle::MonitorOperations.mon_mutex(@mon_mutex) + Primitive.monitor_enter(@mon_mutex) end # # Leaves exclusive section. # def mon_exit - Truffle::MonitorOperations.mon_exit(@mon_mutex) + Primitive.monitor_exit(@mon_mutex) end # @@ -164,7 +164,7 @@ def mon_owned? # +MonitorMixin+. # def mon_synchronize(&block) - Truffle::MonitorOperations.synchronize_on_mutex(@mon_mutex, &block) + Primitive.monitor_synchronize(@mon_mutex, block) end alias synchronize mon_synchronize @@ -173,7 +173,7 @@ def mon_synchronize(&block) # receiver. # def new_cond - return Truffle::MonitorOperations.condition_var_for_mutex(ConditionVariable, @mon_mutex) + return Primitive.mutex_linked_condition_variable(ConditionVariable, @mon_mutex) end # Use extend MonitorMixin or include MonitorMixin instead diff --git a/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java b/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java index fda8c793db9c..4210bd645c21 100644 --- a/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java +++ b/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java @@ -325,6 +325,7 @@ public static void setupBuiltinsLazyPrimitives(PrimitiveManager primitiveManager TruffleDebugNodesBuiltins.setupPrimitives(primitiveManager); TruffleGraalNodesBuiltins.setupPrimitives(primitiveManager); TruffleKernelNodesBuiltins.setupPrimitives(primitiveManager); + TruffleMonitorNodesBuiltins.setupPrimitives(primitiveManager); TrufflePosixNodesBuiltins.setupPrimitives(primitiveManager); TruffleRegexpNodesBuiltins.setupPrimitives(primitiveManager); TruffleRopesNodesBuiltins.setupPrimitives(primitiveManager); diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 8c1f5a7dcc9e..30ee2f16f323 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -16,10 +16,9 @@ import com.oracle.truffle.api.dsl.Specialization; import com.oracle.truffle.api.object.Shape; -import org.truffleruby.builtins.CoreMethod; import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; import org.truffleruby.builtins.CoreModule; -import org.truffleruby.builtins.YieldingCoreMethodNode; +import org.truffleruby.builtins.Primitive; import org.truffleruby.core.klass.RubyClass; import org.truffleruby.core.mutex.MutexOperations; import org.truffleruby.core.mutex.RubyConditionVariable; @@ -29,11 +28,12 @@ import org.truffleruby.core.thread.GetCurrentRubyThreadNode; import org.truffleruby.core.thread.RubyThread; import org.truffleruby.language.objects.AllocationTracing; +import org.truffleruby.language.yield.CallBlockNode; @CoreModule("Truffle::MonitorOperations") public abstract class TruffleMonitorNodes { - @CoreMethod(names = "condition_var_for_mutex", onSingleton = true, required = 2) + @Primitive(name = "mutex_linked_condition_variable") public abstract static class LinkedConditionVariableNode extends CoreMethodArrayArgumentsNode { @Specialization @@ -47,8 +47,10 @@ protected RubyConditionVariable linkedConditionVariable(RubyClass rubyClass, Rub } } - @CoreMethod(names = "synchronize_on_mutex", onSingleton = true, required = 1, needsBlock = true) - public abstract static class SynchronizeNode extends YieldingCoreMethodNode { + @Primitive(name = "monitor_synchronize") + public abstract static class SynchronizeNode extends CoreMethodArrayArgumentsNode { + + @Child private CallBlockNode yieldNode = CallBlockNode.create(); @Specialization(guards = "isRubyProc(maybeBlock)") protected Object synchronizeOnMutex(RubyMutex mutex, Object maybeBlock, @@ -56,7 +58,7 @@ protected Object synchronizeOnMutex(RubyMutex mutex, Object maybeBlock, final RubyThread thread = getCurrentRubyThreadNode.execute(); MutexOperations.lock(getContext(), mutex.lock, thread, this); try { - return callBlock((RubyProc) maybeBlock); + return yieldNode.yield((RubyProc) maybeBlock); } finally { MutexOperations.unlock(mutex.lock, thread); } @@ -75,7 +77,7 @@ protected Object synchronizeOnMutexNoBlock(RubyMutex mutex, Object maybeBlock, } } - @CoreMethod(names = "mon_try_enter", onSingleton = true, required = 1) + @Primitive(name = "monitor_try_enter") public static abstract class MonitorTryEnter extends CoreMethodArrayArgumentsNode { @Specialization @@ -86,7 +88,7 @@ protected Object tryEnter(RubyMutex mutex, } } - @CoreMethod(names = "mon_enter", onSingleton = true, required = 1) + @Primitive(name = "monitor_enter") public static abstract class MonitorEnter extends CoreMethodArrayArgumentsNode { @Specialization @@ -98,7 +100,7 @@ protected Object enter(RubyMutex mutex, } } - @CoreMethod(names = "mon_exit", onSingleton = true, required = 1) + @Primitive(name = "monitor_exit") public static abstract class MonitorExit extends CoreMethodArrayArgumentsNode { @Specialization From efc7a22106407489f3314012bb86b65d7c47d0af Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Fri, 25 Jun 2021 17:26:15 +0100 Subject: [PATCH 03/37] Avoid altering the `ownedLocks` during synhhronize operation. --- .../core/monitor/TruffleMonitorNodes.java | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 30ee2f16f323..6d797532857f 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -53,26 +53,22 @@ public abstract static class SynchronizeNode extends CoreMethodArrayArgumentsNod @Child private CallBlockNode yieldNode = CallBlockNode.create(); @Specialization(guards = "isRubyProc(maybeBlock)") - protected Object synchronizeOnMutex(RubyMutex mutex, Object maybeBlock, - @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { - final RubyThread thread = getCurrentRubyThreadNode.execute(); - MutexOperations.lock(getContext(), mutex.lock, thread, this); + protected Object synchronizeOnMutex(RubyMutex mutex, Object maybeBlock) { + MutexOperations.lockInternal(getContext(), mutex.lock, this); try { return yieldNode.yield((RubyProc) maybeBlock); } finally { - MutexOperations.unlock(mutex.lock, thread); + MutexOperations.unlockInternal(mutex.lock); } } @Specialization(guards = "!isRubyProc(maybeBlock)") - protected Object synchronizeOnMutexNoBlock(RubyMutex mutex, Object maybeBlock, - @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { - final RubyThread thread = getCurrentRubyThreadNode.execute(); - MutexOperations.lock(getContext(), mutex.lock, thread, this); + protected Object synchronizeOnMutexNoBlock(RubyMutex mutex, Object maybeBlock) { + MutexOperations.lockInternal(getContext(), mutex.lock, this); try { return nil; } finally { - MutexOperations.unlock(mutex.lock, thread); + MutexOperations.unlockInternal(mutex.lock); } } } From 79e8a09ac9aae6c6662d7b8c11904fcb4ee42b25 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Fri, 25 Jun 2021 17:29:26 +0100 Subject: [PATCH 04/37] Add a micro benchmark for monitor synchronization. --- bench/micro/monitor/synchronize.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 bench/micro/monitor/synchronize.rb diff --git a/bench/micro/monitor/synchronize.rb b/bench/micro/monitor/synchronize.rb new file mode 100644 index 000000000000..7b39034a5848 --- /dev/null +++ b/bench/micro/monitor/synchronize.rb @@ -0,0 +1,15 @@ +# 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 'monitor' + +benchmark 'core-create-join-thread' do + m = Monitor.new + m.synchronize do + end +end From 342736092f7076db85abbe6d1f294549b6d1953d Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Fri, 25 Jun 2021 17:55:05 +0100 Subject: [PATCH 05/37] Move monitor library code to `lib/truffle`. --- lib/{mri => truffle}/monitor.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename lib/{mri => truffle}/monitor.rb (100%) diff --git a/lib/mri/monitor.rb b/lib/truffle/monitor.rb similarity index 100% rename from lib/mri/monitor.rb rename to lib/truffle/monitor.rb From d022bb622f0981eccd4f68778978e40f3dddc2ea Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Fri, 25 Jun 2021 17:55:35 +0100 Subject: [PATCH 06/37] Update copyright notice. --- lib/truffle/monitor.rb | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/truffle/monitor.rb b/lib/truffle/monitor.rb index 07bc6be42d71..8623500fa9a5 100644 --- a/lib/truffle/monitor.rb +++ b/lib/truffle/monitor.rb @@ -1,14 +1,17 @@ # truffleruby_primitives: true -# frozen_string_literal: false -# = monitor.rb -# -# Copyright (C) 2001 Shugo Maeda -# -# This library is distributed under the terms of the Ruby license. -# You can freely distribute/modify this library. -# +# Copyright (c) 2015, 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. + +# Original version licensed under LICENSE.RUBY as it is derived from +# lib/ruby/stdlib/digest.rb and is Copyright (C) 2001 Shugo Maeda +# + # In concurrent programming, a monitor is an object or module intended to be # used safely by more than one thread. The defining characteristic of a # monitor is that its methods are executed with mutual exclusion. That is, at @@ -94,8 +97,8 @@ module MonitorMixin # Since MonitorMixin.new_cond returns a ConditionVariable, and the example # above calls while_wait and signal, this class should be documented. # + class ConditionVariable < ::ConditionVariable - class Timeout < Exception; end # # Calls wait repeatedly while the given block yields a truthy value. @@ -173,7 +176,7 @@ def mon_synchronize(&block) # receiver. # def new_cond - return Primitive.mutex_linked_condition_variable(ConditionVariable, @mon_mutex) + Primitive.mutex_linked_condition_variable(ConditionVariable, @mon_mutex) end # Use extend MonitorMixin or include MonitorMixin instead From 91807a46d3d54f47063dc1dbd63fc7c54ec8eb73 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Mon, 28 Jun 2021 11:54:44 +0100 Subject: [PATCH 07/37] Fix style after moving to primitives. --- lib/truffle/monitor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/truffle/monitor.rb b/lib/truffle/monitor.rb index 8623500fa9a5..86b8f4f5445c 100644 --- a/lib/truffle/monitor.rb +++ b/lib/truffle/monitor.rb @@ -191,7 +191,7 @@ def initialize(*args) # object has been extended with the MonitorMixin def mon_initialize if defined?(@mon_mutex) && Primitive.object_same_or_equal(@mon_mutex_owner_object, self) - raise ThreadError, "already initialized" + raise ThreadError, 'already initialized' end @mon_mutex = Thread::Mutex.new @mon_mutex_owner_object = self From cce8ad0ef3c82af6951aff815470adc1a0e7fc2b Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Mon, 28 Jun 2021 13:39:03 +0100 Subject: [PATCH 08/37] Add change log entry. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2023f95ce234..381416ce7b1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Compatibility: Performance: +* Implemented `MonitorMixin` primitives in Java (#2375). Changes: From 7c55156c9c1fdf9035098b6dd3516a247d36e679 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Mon, 28 Jun 2021 16:43:25 +0100 Subject: [PATCH 09/37] Fix linter errors. --- .../org/truffleruby/core/monitor/TruffleMonitorNodes.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 6d797532857f..5c07155460cc 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -22,7 +22,6 @@ import org.truffleruby.core.klass.RubyClass; import org.truffleruby.core.mutex.MutexOperations; import org.truffleruby.core.mutex.RubyConditionVariable; -import org.truffleruby.core.mutex.MutexOperations; import org.truffleruby.core.mutex.RubyMutex; import org.truffleruby.core.proc.RubyProc; import org.truffleruby.core.thread.GetCurrentRubyThreadNode; @@ -74,7 +73,7 @@ protected Object synchronizeOnMutexNoBlock(RubyMutex mutex, Object maybeBlock) { } @Primitive(name = "monitor_try_enter") - public static abstract class MonitorTryEnter extends CoreMethodArrayArgumentsNode { + public abstract static class MonitorTryEnter extends CoreMethodArrayArgumentsNode { @Specialization protected Object tryEnter(RubyMutex mutex, @@ -85,7 +84,7 @@ protected Object tryEnter(RubyMutex mutex, } @Primitive(name = "monitor_enter") - public static abstract class MonitorEnter extends CoreMethodArrayArgumentsNode { + public abstract static class MonitorEnter extends CoreMethodArrayArgumentsNode { @Specialization protected Object enter(RubyMutex mutex, @@ -97,7 +96,7 @@ protected Object enter(RubyMutex mutex, } @Primitive(name = "monitor_exit") - public static abstract class MonitorExit extends CoreMethodArrayArgumentsNode { + public abstract static class MonitorExit extends CoreMethodArrayArgumentsNode { @Specialization protected Object exit(RubyMutex mutex, From 7b9c842545dd36d6640fb8bfeb2f2e8abf6ca722 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 29 Jun 2021 11:02:28 +0100 Subject: [PATCH 10/37] Spec for `Monitor#synchronize` with no block, and fix to match MRI behaviour. --- spec/ruby/library/monitor/synchronize_spec.rb | 4 ++++ .../org/truffleruby/core/monitor/TruffleMonitorNodes.java | 8 ++------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/ruby/library/monitor/synchronize_spec.rb b/spec/ruby/library/monitor/synchronize_spec.rb index d851b98d6cca..c0aa81908fc0 100644 --- a/spec/ruby/library/monitor/synchronize_spec.rb +++ b/spec/ruby/library/monitor/synchronize_spec.rb @@ -27,4 +27,8 @@ thread.value.should == :ok end end + + it "raises a LocalJumpError if not passed a block" do + -> {Monitor.new.synchronize }.should raise_error(LocalJumpError) + end end diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 5c07155460cc..7b184af1783e 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -26,6 +26,7 @@ import org.truffleruby.core.proc.RubyProc; import org.truffleruby.core.thread.GetCurrentRubyThreadNode; import org.truffleruby.core.thread.RubyThread; +import org.truffleruby.language.control.RaiseException; import org.truffleruby.language.objects.AllocationTracing; import org.truffleruby.language.yield.CallBlockNode; @@ -63,12 +64,7 @@ protected Object synchronizeOnMutex(RubyMutex mutex, Object maybeBlock) { @Specialization(guards = "!isRubyProc(maybeBlock)") protected Object synchronizeOnMutexNoBlock(RubyMutex mutex, Object maybeBlock) { - MutexOperations.lockInternal(getContext(), mutex.lock, this); - try { - return nil; - } finally { - MutexOperations.unlockInternal(mutex.lock); - } + throw new RaiseException(getContext(), coreExceptions().localJumpError("no block given", this)); } } From 2fd45e3b5d23360b2b6cb7253071ad44b490fe74 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 29 Jun 2021 12:12:01 +0100 Subject: [PATCH 11/37] Maintain the owned locks list on synchronize operations. --- .../core/monitor/TruffleMonitorNodes.java | 22 +++++++++++++------ .../truffleruby/core/mutex/MutexNodes.java | 6 +++++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 7b184af1783e..92064d32fb2f 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -52,18 +52,26 @@ public abstract static class SynchronizeNode extends CoreMethodArrayArgumentsNod @Child private CallBlockNode yieldNode = CallBlockNode.create(); - @Specialization(guards = "isRubyProc(maybeBlock)") - protected Object synchronizeOnMutex(RubyMutex mutex, Object maybeBlock) { - MutexOperations.lockInternal(getContext(), mutex.lock, this); + @Specialization(guards = "isRubyProc(block)") + protected Object synchronizeOnMutex(RubyMutex mutex, Object block, + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { + /* Like Mutex#synchronize we must maintain the owned locks + * list here as the monitor might be exited inside + * synchronize block and then re-entered again before the + * end, and we have to make sure the list of owned locks + * remains consistent. + */ + final RubyThread thread = getCurrentRubyThreadNode.execute(); + MutexOperations.lock(getContext(), mutex.lock, thread, this); try { - return yieldNode.yield((RubyProc) maybeBlock); + return yieldNode.yield((RubyProc) block); } finally { - MutexOperations.unlockInternal(mutex.lock); + MutexOperations.unlock(mutex.lock, thread); } } - @Specialization(guards = "!isRubyProc(maybeBlock)") - protected Object synchronizeOnMutexNoBlock(RubyMutex mutex, Object maybeBlock) { + @Specialization(guards = "!isRubyProc(block)") + protected Object synchronizeOnMutexNoBlock(RubyMutex mutex, Object block) { throw new RaiseException(getContext(), coreExceptions().localJumpError("no block given", this)); } } diff --git a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java index b19105291e7a..ad65f3cf7424 100644 --- a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java @@ -150,6 +150,12 @@ protected Object synchronize(RubyMutex mutex, RubyProc block, throw new RaiseException(getContext(), coreExceptions().threadErrorRecursiveLocking(this)); } + /* This code uses lock/unlock because the list of owned + * locks must be maintained. Use code can unlock a mutex + * inside a synchronize block, and then relock it before + * exiting the block, and we need the owned locks list to + * be in consistent state at the end. + */ MutexOperations.lock(getContext(), lock, thread, this); try { return callBlock(block); From 7ae105b2f2d919853b9ead1be99f827e4ba7740c Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 29 Jun 2021 12:17:16 +0100 Subject: [PATCH 12/37] Minor code clean up and de-duplication. --- .../java/org/truffleruby/core/mutex/MutexNodes.java | 13 +------------ .../org/truffleruby/core/thread/ThreadManager.java | 3 +-- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java index ad65f3cf7424..0e39c7d10917 100644 --- a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java @@ -102,20 +102,9 @@ protected boolean tryLock(RubyMutex mutex, if (heldByCurrentThreadProfile.profile(lock.isHeldByCurrentThread())) { return false; } else { - return doTryLock(thread, lock); + return MutexOperations.tryLock(lock, thread); } } - - @TruffleBoundary - private boolean doTryLock(RubyThread thread, ReentrantLock lock) { - if (lock.tryLock()) { - thread.ownedLocks.add(lock); - return true; - } else { - return false; - } - } - } @CoreMethod(names = "unlock") diff --git a/src/main/java/org/truffleruby/core/thread/ThreadManager.java b/src/main/java/org/truffleruby/core/thread/ThreadManager.java index 0c121ed41d43..c71514bb64c4 100644 --- a/src/main/java/org/truffleruby/core/thread/ThreadManager.java +++ b/src/main/java/org/truffleruby/core/thread/ThreadManager.java @@ -422,8 +422,7 @@ public void cleanupThreadState(RubyThread thread, Thread javaThread) { if (Thread.currentThread() == javaThread) { for (ReentrantLock lock : thread.ownedLocks) { - int count = lock.getHoldCount(); - for (int i = 0; i < count; i++) { + while (lock.isHeldByCurrentThread()) { lock.unlock(); } } From 18184a6c9e81421b28fab3784212571f1b1a92b3 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 29 Jun 2021 12:32:35 +0100 Subject: [PATCH 13/37] Fix benchmark name. --- bench/micro/monitor/synchronize.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bench/micro/monitor/synchronize.rb b/bench/micro/monitor/synchronize.rb index 7b39034a5848..6b1663f42cd1 100644 --- a/bench/micro/monitor/synchronize.rb +++ b/bench/micro/monitor/synchronize.rb @@ -8,7 +8,7 @@ require 'monitor' -benchmark 'core-create-join-thread' do +benchmark 'monitor-synchronize' do m = Monitor.new m.synchronize do end From bdf2093f5ddafe84d642f7c9b3dee4d0eecd6edf Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 29 Jun 2021 15:57:50 +0100 Subject: [PATCH 14/37] Update mri import to copy the monitor implementation into lib/truffle. --- tool/import-mri-files.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tool/import-mri-files.sh b/tool/import-mri-files.sh index 67d4a9ea38c5..cf05ce25dacd 100755 --- a/tool/import-mri-files.sh +++ b/tool/import-mri-files.sh @@ -40,6 +40,10 @@ rm lib/mri/debug.rb find lib/mri -name '*.gemspec' -delete find lib/mri -name '.document' -delete +# We keep our own implementation of monitors in lib/truffle, but we'll +# copy across from MRI so that we notice changes. +rm lib/truffle/monitor.rb + # *.c cp ../ruby/st.c src/main/c/cext/st.c @@ -51,6 +55,7 @@ cp -r ../ruby/ext/digest/sha2/lib/* lib/mri/digest cp -r ../ruby/ext/fiddle/lib/fiddle lib/mri cp -r ../ruby/ext/fiddle/lib/fiddle.rb lib/mri cp ../ruby/ext/nkf/lib/*.rb lib/mri +cp ../ruby/ext/monitor/lib/*.rb lib/truffle cp -r ../ruby/ext/openssl/lib/* lib/mri cp ../ruby/ext/pty/lib/*.rb lib/mri cp ../ruby/ext/psych/lib/psych.rb lib/mri From d505acb4c82d6fa3eb4c1c901a5c062918f6c431 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 29 Jun 2021 16:35:34 +0100 Subject: [PATCH 15/37] Add specs for `Monitor#enter` and `Monitor#try_enter`. --- spec/ruby/library/monitor/enter_exit_spec.rb | 27 +++++++++ spec/ruby/library/monitor/try_enter_spec.rb | 58 ++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 spec/ruby/library/monitor/enter_exit_spec.rb create mode 100644 spec/ruby/library/monitor/try_enter_spec.rb diff --git a/spec/ruby/library/monitor/enter_exit_spec.rb b/spec/ruby/library/monitor/enter_exit_spec.rb new file mode 100644 index 000000000000..44ee2d2b039d --- /dev/null +++ b/spec/ruby/library/monitor/enter_exit_spec.rb @@ -0,0 +1,27 @@ +require_relative '../../spec_helper' +require 'monitor' + +describe "Monitor#enter" do + it "acquires the monitor" do + monitor = Monitor.new + 10.times do + locked = false + + thread = Thread.new do + begin + monitor.enter + locked = true + sleep # wait for wakeup. + ensure + monitor.exit + end + end + + Thread.pass until locked + monitor.mon_locked?.should == true + thread.wakeup + thread.join + monitor.mon_locked?.should == false + end + end +end diff --git a/spec/ruby/library/monitor/try_enter_spec.rb b/spec/ruby/library/monitor/try_enter_spec.rb new file mode 100644 index 000000000000..3339c52c426c --- /dev/null +++ b/spec/ruby/library/monitor/try_enter_spec.rb @@ -0,0 +1,58 @@ +require_relative '../../spec_helper' +require 'monitor' + +describe "Monitor#try_enter" do + it "will acquire a monitor not held by another thread" do + monitor = Monitor.new + 10.times do + + thread = Thread.new do + begin + val = monitor.try_enter + ensure + monitor.exit if val + end + val + end + + thread.join + thread.value.should == true + end + end + + it "will not acquire a monitor already held by another thread" do + monitor = Monitor.new + 10.times do + locked = false + + thread1 = Thread.new do + begin + monitor.enter + locked = true + sleep # wait for wakeup. + ensure + monitor.exit + end + end + + Thread.pass until locked + monitor.mon_locked?.should == true + + thread2 = Thread.new do + begin + val = monitor.try_enter + ensure + monitor.exit if val + end + val + end + + thread2.join + thread2.value.should == false + + thread1.wakeup + thread1.join + monitor.mon_locked?.should == false + end + end +end From 026451ede894f729ca8c212d8e2143a912c681c7 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 29 Jun 2021 17:08:20 +0100 Subject: [PATCH 16/37] Raise error if monitor not owned when leaving synchronization block. --- spec/ruby/library/monitor/synchronize_spec.rb | 5 +++++ .../org/truffleruby/core/monitor/TruffleMonitorNodes.java | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/ruby/library/monitor/synchronize_spec.rb b/spec/ruby/library/monitor/synchronize_spec.rb index c0aa81908fc0..152398011e12 100644 --- a/spec/ruby/library/monitor/synchronize_spec.rb +++ b/spec/ruby/library/monitor/synchronize_spec.rb @@ -31,4 +31,9 @@ it "raises a LocalJumpError if not passed a block" do -> {Monitor.new.synchronize }.should raise_error(LocalJumpError) end + + it "raises a thread error if the monitor is not owned on exiting the block" do + monitor = Monitor.new + -> { monitor.synchronize { monitor.exit } }.should raise_error(ThreadError) + end end diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 92064d32fb2f..44f88f494397 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -15,6 +15,7 @@ import com.oracle.truffle.api.dsl.Cached; import com.oracle.truffle.api.dsl.Specialization; import com.oracle.truffle.api.object.Shape; +import com.oracle.truffle.api.profiles.BranchProfile; import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; import org.truffleruby.builtins.CoreModule; @@ -54,7 +55,8 @@ public abstract static class SynchronizeNode extends CoreMethodArrayArgumentsNod @Specialization(guards = "isRubyProc(block)") protected Object synchronizeOnMutex(RubyMutex mutex, Object block, - @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode, + @Cached BranchProfile errorProfile) { /* Like Mutex#synchronize we must maintain the owned locks * list here as the monitor might be exited inside * synchronize block and then re-entered again before the @@ -66,6 +68,7 @@ protected Object synchronizeOnMutex(RubyMutex mutex, Object block, try { return yieldNode.yield((RubyProc) block); } finally { + MutexOperations.checkOwnedMutex(getContext(), mutex.lock, this, errorProfile); MutexOperations.unlock(mutex.lock, thread); } } From b2dd3a10df11fd26731614aed58c2a52467098eb Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Wed, 30 Jun 2021 11:27:22 +0100 Subject: [PATCH 17/37] Remove unused import. --- src/main/java/org/truffleruby/core/mutex/MutexNodes.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java index 0e39c7d10917..1f43511f34a8 100644 --- a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java @@ -9,7 +9,6 @@ */ package org.truffleruby.core.mutex; -import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary; import com.oracle.truffle.api.dsl.Cached; import com.oracle.truffle.api.dsl.CreateCast; import com.oracle.truffle.api.dsl.NodeChild; From de30c81fd256e9be925205b5faaa7a766907ad7e Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Wed, 30 Jun 2021 11:27:35 +0100 Subject: [PATCH 18/37] Don't return held count when entering monitor. --- .../java/org/truffleruby/core/monitor/TruffleMonitorNodes.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 44f88f494397..255f5654a89c 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -98,7 +98,7 @@ protected Object enter(RubyMutex mutex, @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode) { final RubyThread thread = getCurrentRubyThreadNode.execute(); MutexOperations.lock(getContext(), mutex.lock, thread, this); - return mutex.lock.getHoldCount(); + return nil; } } From 49537e284b8497a5c00bc7567610b32ecd4e721b Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Wed, 30 Jun 2021 11:40:33 +0100 Subject: [PATCH 19/37] Fix formatting. --- .../core/monitor/TruffleMonitorNodes.java | 13 +++++-------- .../java/org/truffleruby/core/mutex/MutexNodes.java | 9 +++------ 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 255f5654a89c..e563a307c2f5 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -53,16 +53,13 @@ public abstract static class SynchronizeNode extends CoreMethodArrayArgumentsNod @Child private CallBlockNode yieldNode = CallBlockNode.create(); - @Specialization(guards = "isRubyProc(block)") - protected Object synchronizeOnMutex(RubyMutex mutex, Object block, + @Specialization + protected Object synchronizeOnMutex(RubyMutex mutex, RubyProc block, @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode, @Cached BranchProfile errorProfile) { - /* Like Mutex#synchronize we must maintain the owned locks - * list here as the monitor might be exited inside - * synchronize block and then re-entered again before the - * end, and we have to make sure the list of owned locks - * remains consistent. - */ + /* Like Mutex#synchronize we must maintain the owned locks list here as the monitor might be exited inside + * synchronize block and then re-entered again before the end, and we have to make sure the list of owned + * locks remains consistent. */ final RubyThread thread = getCurrentRubyThreadNode.execute(); MutexOperations.lock(getContext(), mutex.lock, thread, this); try { diff --git a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java index 1f43511f34a8..380bba9fd64f 100644 --- a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java @@ -138,12 +138,9 @@ protected Object synchronize(RubyMutex mutex, RubyProc block, throw new RaiseException(getContext(), coreExceptions().threadErrorRecursiveLocking(this)); } - /* This code uses lock/unlock because the list of owned - * locks must be maintained. Use code can unlock a mutex - * inside a synchronize block, and then relock it before - * exiting the block, and we need the owned locks list to - * be in consistent state at the end. - */ + /* This code uses lock/unlock because the list of owned locks must be maintained. Use code can unlock a + * mutex inside a synchronize block, and then relock it before exiting the block, and we need the owned + * locks list to be in consistent state at the end. */ MutexOperations.lock(getContext(), lock, thread, this); try { return callBlock(block); From 3954c9ac8460c354969c09cc4b5bd88f21602d3d Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Wed, 30 Jun 2021 12:25:51 +0100 Subject: [PATCH 20/37] Remove cast. --- .../java/org/truffleruby/core/monitor/TruffleMonitorNodes.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index e563a307c2f5..3dc430dcb1db 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -63,7 +63,7 @@ protected Object synchronizeOnMutex(RubyMutex mutex, RubyProc block, final RubyThread thread = getCurrentRubyThreadNode.execute(); MutexOperations.lock(getContext(), mutex.lock, thread, this); try { - return yieldNode.yield((RubyProc) block); + return yieldNode.yield(block); } finally { MutexOperations.checkOwnedMutex(getContext(), mutex.lock, this, errorProfile); MutexOperations.unlock(mutex.lock, thread); From e02156116417801663a921e510c02aaa3601e396 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Thu, 1 Jul 2021 15:26:28 +0100 Subject: [PATCH 21/37] Fix semantics of MonitorMixin::ConditionVariable. --- lib/truffle/monitor.rb | 16 +++++++++++ .../core/mutex/ConditionVariableNodes.java | 28 +++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/lib/truffle/monitor.rb b/lib/truffle/monitor.rb index 86b8f4f5445c..9c2cf8add5fd 100644 --- a/lib/truffle/monitor.rb +++ b/lib/truffle/monitor.rb @@ -100,6 +100,22 @@ module MonitorMixin class ConditionVariable < ::ConditionVariable + # + # Releases the lock held in the associated monitor and waits; reacquires the lock on wakeup. + # + # If +timeout+ is given, this method returns after +timeout+ seconds passed, + # even if no other thread doesn't signal. + # + def wait(timeout = nil) + if timeout + raise ArgumentError, 'Timeout must be positive' if timeout < 0 + timeout = timeout * 1_000_000_000 + timeout = Primitive.rb_num2long(timeout) + end + + Primitive.condition_variable_wait(self, nil, timeout) + end + # # Calls wait repeatedly while the given block yields a truthy value. # diff --git a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java index cb52ad6975ac..bef95d90f266 100644 --- a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java @@ -82,6 +82,26 @@ protected RubyConditionVariable withTimeout(RubyConditionVariable condVar, RubyM return condVar; } + @Specialization + protected RubyConditionVariable noMutexNoTimeout(RubyConditionVariable condVar, Nil mutex, Nil timeout, + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode, + @Cached BranchProfile errorProfile) { + final RubyThread thread = getCurrentRubyThreadNode.execute(); + + waitInternal(condVar, null, thread, -1); + return condVar; + } + + @Specialization + protected RubyConditionVariable noMutexWithTimeout(RubyConditionVariable condVar, Nil mutex, long timeout, + @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode, + @Cached BranchProfile errorProfile) { + final RubyThread thread = getCurrentRubyThreadNode.execute(); + + waitInternal(condVar, null, thread, timeout); + return condVar; + } + @TruffleBoundary private void waitInternal(RubyConditionVariable conditionVariable, ReentrantLock mutexLock, RubyThread thread, long durationInNanos) { @@ -104,7 +124,9 @@ private void waitInternal(RubyConditionVariable conditionVariable, ReentrantLock // must imply being ready to be interrupted by Thread#{run,wakeup}. condLock.lock(); try { - mutexLock.unlock(); + if (mutexLock != null) { + mutexLock.unlock(); + } conditionVariable.waiters++; try { @@ -121,7 +143,9 @@ private void waitInternal(RubyConditionVariable conditionVariable, ReentrantLock } } finally { condLock.unlock(); - MutexOperations.internalLockEvenWithException(getContext(), mutexLock, this); + if (mutexLock != null) { + MutexOperations.internalLockEvenWithException(getContext(), mutexLock, this); + } } } From bd82d62a4a9ab1cc6a287288ddeb71462122a257 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 6 Jul 2021 11:53:23 +0100 Subject: [PATCH 22/37] Simplify `Monitor#try_enter` spec. --- spec/ruby/library/monitor/try_enter_spec.rb | 41 ++++++--------------- 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/spec/ruby/library/monitor/try_enter_spec.rb b/spec/ruby/library/monitor/try_enter_spec.rb index 3339c52c426c..04b878f7209c 100644 --- a/spec/ruby/library/monitor/try_enter_spec.rb +++ b/spec/ruby/library/monitor/try_enter_spec.rb @@ -7,11 +7,8 @@ 10.times do thread = Thread.new do - begin - val = monitor.try_enter - ensure - monitor.exit if val - end + val = monitor.try_enter + monitor.exit if val val end @@ -23,35 +20,19 @@ it "will not acquire a monitor already held by another thread" do monitor = Monitor.new 10.times do - locked = false - - thread1 = Thread.new do - begin - monitor.enter - locked = true - sleep # wait for wakeup. - ensure - monitor.exit - end - end - - Thread.pass until locked - monitor.mon_locked?.should == true - - thread2 = Thread.new do - begin + monitor.enter + begin + thread = Thread.new do val = monitor.try_enter - ensure monitor.exit if val + val end - val - end - - thread2.join - thread2.value.should == false - thread1.wakeup - thread1.join + thread.join + thread.value.should == false + ensure + monitor.exit + end monitor.mon_locked?.should == false end end From 657ebbcb70fc613875eb385407d17ed939d4b057 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 6 Jul 2021 11:55:10 +0100 Subject: [PATCH 23/37] Rename spec. --- spec/ruby/library/monitor/{enter_exit_spec.rb => enter_spec.rb} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename spec/ruby/library/monitor/{enter_exit_spec.rb => enter_spec.rb} (100%) diff --git a/spec/ruby/library/monitor/enter_exit_spec.rb b/spec/ruby/library/monitor/enter_spec.rb similarity index 100% rename from spec/ruby/library/monitor/enter_exit_spec.rb rename to spec/ruby/library/monitor/enter_spec.rb From a4a34d9f7eb26aeb21eae30867b85349f5460e5a Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 6 Jul 2021 11:57:27 +0100 Subject: [PATCH 24/37] Small change to micro benchmark. --- bench/micro/monitor/synchronize.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bench/micro/monitor/synchronize.rb b/bench/micro/monitor/synchronize.rb index 6b1663f42cd1..2e9bd589ab1d 100644 --- a/bench/micro/monitor/synchronize.rb +++ b/bench/micro/monitor/synchronize.rb @@ -8,8 +8,8 @@ require 'monitor' +m = Monitor.new benchmark 'monitor-synchronize' do - m = Monitor.new m.synchronize do end end From caf8d0592bb029c20b0036df45ba38437d8931c4 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 6 Jul 2021 12:00:01 +0100 Subject: [PATCH 25/37] Move `monitor.rb` back to `lib/mri`. --- lib/{truffle => mri}/monitor.rb | 0 tool/import-mri-files.sh | 6 +----- 2 files changed, 1 insertion(+), 5 deletions(-) rename lib/{truffle => mri}/monitor.rb (100%) diff --git a/lib/truffle/monitor.rb b/lib/mri/monitor.rb similarity index 100% rename from lib/truffle/monitor.rb rename to lib/mri/monitor.rb diff --git a/tool/import-mri-files.sh b/tool/import-mri-files.sh index cf05ce25dacd..97068909c03d 100755 --- a/tool/import-mri-files.sh +++ b/tool/import-mri-files.sh @@ -40,10 +40,6 @@ rm lib/mri/debug.rb find lib/mri -name '*.gemspec' -delete find lib/mri -name '.document' -delete -# We keep our own implementation of monitors in lib/truffle, but we'll -# copy across from MRI so that we notice changes. -rm lib/truffle/monitor.rb - # *.c cp ../ruby/st.c src/main/c/cext/st.c @@ -55,7 +51,7 @@ cp -r ../ruby/ext/digest/sha2/lib/* lib/mri/digest cp -r ../ruby/ext/fiddle/lib/fiddle lib/mri cp -r ../ruby/ext/fiddle/lib/fiddle.rb lib/mri cp ../ruby/ext/nkf/lib/*.rb lib/mri -cp ../ruby/ext/monitor/lib/*.rb lib/truffle +cp ../ruby/ext/monitor/lib/*.rb lib/mri cp -r ../ruby/ext/openssl/lib/* lib/mri cp ../ruby/ext/pty/lib/*.rb lib/mri cp ../ruby/ext/psych/lib/psych.rb lib/mri From cd5758c31d003c6f49fa3a78220fed568ef630db Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 6 Jul 2021 18:06:18 +0100 Subject: [PATCH 26/37] Refactor to not use the monitor's lock for condition variables. --- lib/mri/monitor.rb | 16 +++++----- .../core/monitor/TruffleMonitorNodes.java | 21 ------------ .../core/mutex/ConditionVariableNodes.java | 32 ++++++------------- 3 files changed, 17 insertions(+), 52 deletions(-) diff --git a/lib/mri/monitor.rb b/lib/mri/monitor.rb index 9c2cf8add5fd..0aeafd06590c 100644 --- a/lib/mri/monitor.rb +++ b/lib/mri/monitor.rb @@ -107,13 +107,7 @@ class ConditionVariable < ::ConditionVariable # even if no other thread doesn't signal. # def wait(timeout = nil) - if timeout - raise ArgumentError, 'Timeout must be positive' if timeout < 0 - timeout = timeout * 1_000_000_000 - timeout = Primitive.rb_num2long(timeout) - end - - Primitive.condition_variable_wait(self, nil, timeout) + super(@mon_mutex, timeout) end # @@ -133,6 +127,12 @@ def wait_until wait end end + + private + + def initialize(mutex) + @mon_mutex = mutex + end end def self.extend_object(obj) @@ -192,7 +192,7 @@ def mon_synchronize(&block) # receiver. # def new_cond - Primitive.mutex_linked_condition_variable(ConditionVariable, @mon_mutex) + ConditionVariable.new(@mon_mutex) end # Use extend MonitorMixin or include MonitorMixin instead diff --git a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java index 3dc430dcb1db..ffe21230d6d0 100644 --- a/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java +++ b/src/main/java/org/truffleruby/core/monitor/TruffleMonitorNodes.java @@ -9,45 +9,24 @@ */ package org.truffleruby.core.monitor; -import java.util.concurrent.locks.Condition; -import java.util.concurrent.locks.ReentrantLock; - import com.oracle.truffle.api.dsl.Cached; import com.oracle.truffle.api.dsl.Specialization; -import com.oracle.truffle.api.object.Shape; import com.oracle.truffle.api.profiles.BranchProfile; import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; import org.truffleruby.builtins.CoreModule; import org.truffleruby.builtins.Primitive; -import org.truffleruby.core.klass.RubyClass; import org.truffleruby.core.mutex.MutexOperations; -import org.truffleruby.core.mutex.RubyConditionVariable; import org.truffleruby.core.mutex.RubyMutex; import org.truffleruby.core.proc.RubyProc; import org.truffleruby.core.thread.GetCurrentRubyThreadNode; import org.truffleruby.core.thread.RubyThread; import org.truffleruby.language.control.RaiseException; -import org.truffleruby.language.objects.AllocationTracing; import org.truffleruby.language.yield.CallBlockNode; @CoreModule("Truffle::MonitorOperations") public abstract class TruffleMonitorNodes { - @Primitive(name = "mutex_linked_condition_variable") - public abstract static class LinkedConditionVariableNode extends CoreMethodArrayArgumentsNode { - - @Specialization - protected RubyConditionVariable linkedConditionVariable(RubyClass rubyClass, RubyMutex mutex) { - final ReentrantLock condLock = mutex.lock; - final Condition condition = MutexOperations.newCondition(condLock); - final Shape shape = getLanguage().conditionVariableShape; - final RubyConditionVariable instance = new RubyConditionVariable(rubyClass, shape, condLock, condition); - AllocationTracing.trace(instance, this); - return instance; - } - } - @Primitive(name = "monitor_synchronize") public abstract static class SynchronizeNode extends CoreMethodArrayArgumentsNode { diff --git a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java index bef95d90f266..f98a743ecf58 100644 --- a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java @@ -82,26 +82,6 @@ protected RubyConditionVariable withTimeout(RubyConditionVariable condVar, RubyM return condVar; } - @Specialization - protected RubyConditionVariable noMutexNoTimeout(RubyConditionVariable condVar, Nil mutex, Nil timeout, - @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode, - @Cached BranchProfile errorProfile) { - final RubyThread thread = getCurrentRubyThreadNode.execute(); - - waitInternal(condVar, null, thread, -1); - return condVar; - } - - @Specialization - protected RubyConditionVariable noMutexWithTimeout(RubyConditionVariable condVar, Nil mutex, long timeout, - @Cached GetCurrentRubyThreadNode getCurrentRubyThreadNode, - @Cached BranchProfile errorProfile) { - final RubyThread thread = getCurrentRubyThreadNode.execute(); - - waitInternal(condVar, null, thread, timeout); - return condVar; - } - @TruffleBoundary private void waitInternal(RubyConditionVariable conditionVariable, ReentrantLock mutexLock, RubyThread thread, long durationInNanos) { @@ -123,9 +103,11 @@ private void waitInternal(RubyConditionVariable conditionVariable, ReentrantLock // If there is an interrupt, it should be consumed by condition.await() and the Ruby Thread sleep status // must imply being ready to be interrupted by Thread#{run,wakeup}. condLock.lock(); + int holdCount = 0; try { - if (mutexLock != null) { + while (mutexLock.isHeldByCurrentThread()) { mutexLock.unlock(); + holdCount++; } conditionVariable.waiters++; @@ -143,8 +125,12 @@ private void waitInternal(RubyConditionVariable conditionVariable, ReentrantLock } } finally { condLock.unlock(); - if (mutexLock != null) { - MutexOperations.internalLockEvenWithException(getContext(), mutexLock, this); + MutexOperations.internalLockEvenWithException(getContext(), mutexLock, this); + if (holdCount > 1) { + // We know we already hold the lock, so we can skip the rest of the logic at this point. + for (int i = 1; i < holdCount; i++) { + mutexLock.tryLock(); + } } } } From 34a683edbfc660d161e7e59bf07a54ff186cbfcb Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 6 Jul 2021 18:20:10 +0100 Subject: [PATCH 27/37] Add spec for monitor condition variables. --- spec/ruby/library/monitor/new_cond_spec.rb | 88 ++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 spec/ruby/library/monitor/new_cond_spec.rb diff --git a/spec/ruby/library/monitor/new_cond_spec.rb b/spec/ruby/library/monitor/new_cond_spec.rb new file mode 100644 index 000000000000..6196ebcff553 --- /dev/null +++ b/spec/ruby/library/monitor/new_cond_spec.rb @@ -0,0 +1,88 @@ +require_relative '../../spec_helper' +require 'monitor' + +describe "Monitor#new_cond" do + it "creates a MonitorMixin::ConditoinVariable" do + m = Monitor.new + c = m.new_cond + c.class.should == MonitorMixin::ConditionVariable + end + + it 'returns a condition variable which can be waited on by a thread holding the monitor' do + m = Monitor.new + c = m.new_cond + + 10.times do + + locked = false + thread = Thread.new do + m.synchronize do + locked = true + c.wait + end + :done + end + + Thread.pass until locked + Thread.pass until thread.stop? + + m.synchronize { c.signal } + + thread.join + thread.value.should == :done + end + end + + it 'returns a condition variable which can be waited on by a thread holding the monitor inside multiple synchronize blocks' do + m = Monitor.new + c = m.new_cond + + 10.times do + + locked = false + thread = Thread.new do + m.synchronize do + m.synchronize do + locked = true + c.wait + end + end + :done + end + + Thread.pass until locked + Thread.pass until thread.stop? + + m.synchronize { c.signal } + + thread.join + thread.value.should == :done + end + end + + it 'returns a condition variable which can be signalled by a thread holding the monitor inside multiple synchronize blocks' do + m = Monitor.new + c = m.new_cond + + 10.times do + + locked = false + thread = Thread.new do + m.synchronize do + locked = true + c.wait + end + :done + end + + Thread.pass until locked + Thread.pass until thread.stop? + + m.synchronize { m.synchronize { c.signal } } + + thread.join + thread.value.should == :done + end + end + +end From 8fa4400f80b198253f69db02ee3b0a70005b4e63 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 6 Jul 2021 11:36:49 +0000 Subject: [PATCH 28/37] Fix type --- src/main/java/org/truffleruby/core/mutex/MutexNodes.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java index 380bba9fd64f..d688bd0a82af 100644 --- a/src/main/java/org/truffleruby/core/mutex/MutexNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/MutexNodes.java @@ -138,7 +138,7 @@ protected Object synchronize(RubyMutex mutex, RubyProc block, throw new RaiseException(getContext(), coreExceptions().threadErrorRecursiveLocking(this)); } - /* This code uses lock/unlock because the list of owned locks must be maintained. Use code can unlock a + /* This code uses lock/unlock because the list of owned locks must be maintained. User code can unlock a * mutex inside a synchronize block, and then relock it before exiting the block, and we need the owned * locks list to be in consistent state at the end. */ MutexOperations.lock(getContext(), lock, thread, this); From 6398c04451e517fac6127d743c315871a2d20d68 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Thu, 8 Jul 2021 11:12:49 +0100 Subject: [PATCH 29/37] Add a comment about `ConditionVariable#await` and locks. --- .../org/truffleruby/core/mutex/ConditionVariableNodes.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java index f98a743ecf58..2c02b0bfd184 100644 --- a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java @@ -158,6 +158,13 @@ private void awaitSignal(RubyConditionVariable self, RubyThread thread, long dur return BlockingAction.SUCCESS; } + /** Condition#await() can only exit (return or throw) after the associated ReentrantLock is + * re-acquired. Even if it's interrupted, the InterruptedException is "stuck inside" until that + * ReentrantLock is re-acquired. So the ReentrantLock we use here must be a ReentrantLock used + * only for that Condition/RubyConditionVariable and nothing else. Specifically it must not be a + * ReentrantLock exposed to Ruby, e.g., via a Ruby Mutex, as that could be held forever by + * another Ruby thread (which did {code mutex.lock} after the #wait), and we would never be able + * to interrupt both threads at the same time for a synchronous ThreadLocalAction). */ condition.await(endNanoTime - currentTime, TimeUnit.NANOSECONDS); } else { condition.await(); From d77142e61959fbd44411109e3e9db76ea7b86706 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 20 Jul 2021 12:34:54 +0100 Subject: [PATCH 30/37] Use separate object for condition to match MRI class ancestry. --- lib/mri/monitor.rb | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/mri/monitor.rb b/lib/mri/monitor.rb index 0aeafd06590c..4b98983343f5 100644 --- a/lib/mri/monitor.rb +++ b/lib/mri/monitor.rb @@ -98,7 +98,7 @@ module MonitorMixin # above calls while_wait and signal, this class should be documented. # - class ConditionVariable < ::ConditionVariable + class ConditionVariable # # Releases the lock held in the associated monitor and waits; reacquires the lock on wakeup. @@ -107,7 +107,7 @@ class ConditionVariable < ::ConditionVariable # even if no other thread doesn't signal. # def wait(timeout = nil) - super(@mon_mutex, timeout) + @cond.wait(@mon_mutex, timeout) end # @@ -128,9 +128,30 @@ def wait_until end end + # + # Wakes up the first thread in line waiting for this lock. + # + def signal + check_owner + @cond.signal + end + + # + # Wakes up all threads waiting for this lock. + # + def broadcast + check_owner + @cond.broadcast + end + private + def check_owner + raise ThreadError, "current thread not owner" unless @mon_mutex.owned? + end + def initialize(mutex) + @cond = ::ConditionVariable.new @mon_mutex = mutex end end From d33d6870f0580fd006c9d02abfae45d1819d7c1e Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 20 Jul 2021 12:53:41 +0100 Subject: [PATCH 31/37] Use lock instead of tryLock when we should own the lock. --- .../org/truffleruby/core/mutex/ConditionVariableNodes.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java index 2c02b0bfd184..eaf8af44f964 100644 --- a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java @@ -125,11 +125,12 @@ private void waitInternal(RubyConditionVariable conditionVariable, ReentrantLock } } finally { condLock.unlock(); + MutexOperations.internalLockEvenWithException(getContext(), mutexLock, this); if (holdCount > 1) { // We know we already hold the lock, so we can skip the rest of the logic at this point. for (int i = 1; i < holdCount; i++) { - mutexLock.tryLock(); + mutexLock.lock(); } } } From 3d849bc1dae9b68e486bb885c82eb64f72c5cac1 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 20 Jul 2021 11:58:06 +0000 Subject: [PATCH 32/37] Add comment about lock counts and monitors. --- .../java/org/truffleruby/core/mutex/ConditionVariableNodes.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java index eaf8af44f964..d539c386deee 100644 --- a/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java +++ b/src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java @@ -103,7 +103,7 @@ private void waitInternal(RubyConditionVariable conditionVariable, ReentrantLock // If there is an interrupt, it should be consumed by condition.await() and the Ruby Thread sleep status // must imply being ready to be interrupted by Thread#{run,wakeup}. condLock.lock(); - int holdCount = 0; + int holdCount = 0; // can be > 1 for MonitorMixin try { while (mutexLock.isHeldByCurrentThread()) { mutexLock.unlock(); From 4cb6b77176e7838d0233bd69ebb3dbb8bfb728ce Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 20 Jul 2021 11:58:32 +0000 Subject: [PATCH 33/37] Small style fix. --- spec/ruby/library/monitor/synchronize_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/ruby/library/monitor/synchronize_spec.rb b/spec/ruby/library/monitor/synchronize_spec.rb index 152398011e12..5212b673813d 100644 --- a/spec/ruby/library/monitor/synchronize_spec.rb +++ b/spec/ruby/library/monitor/synchronize_spec.rb @@ -29,7 +29,7 @@ end it "raises a LocalJumpError if not passed a block" do - -> {Monitor.new.synchronize }.should raise_error(LocalJumpError) + -> { Monitor.new.synchronize }.should raise_error(LocalJumpError) end it "raises a thread error if the monitor is not owned on exiting the block" do From 237f0bb50b4059f1b675bfedf69f6a67b9ea1977 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 20 Jul 2021 11:59:11 +0000 Subject: [PATCH 34/37] Fix typo --- spec/ruby/library/monitor/new_cond_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/ruby/library/monitor/new_cond_spec.rb b/spec/ruby/library/monitor/new_cond_spec.rb index 6196ebcff553..d3c52acec175 100644 --- a/spec/ruby/library/monitor/new_cond_spec.rb +++ b/spec/ruby/library/monitor/new_cond_spec.rb @@ -2,7 +2,7 @@ require 'monitor' describe "Monitor#new_cond" do - it "creates a MonitorMixin::ConditoinVariable" do + it "creates a MonitorMixin::ConditionVariable" do m = Monitor.new c = m.new_cond c.class.should == MonitorMixin::ConditionVariable From 7b472feb01f5442904de7cf4a5a586ab6aebd496 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 20 Jul 2021 14:35:59 +0100 Subject: [PATCH 35/37] Change primitive used to check for monitor owner during initialize.. --- lib/mri/monitor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mri/monitor.rb b/lib/mri/monitor.rb index 4b98983343f5..6c3828c91e74 100644 --- a/lib/mri/monitor.rb +++ b/lib/mri/monitor.rb @@ -227,7 +227,7 @@ def initialize(*args) # Initializes the MonitorMixin after being included in a class or when an # object has been extended with the MonitorMixin def mon_initialize - if defined?(@mon_mutex) && Primitive.object_same_or_equal(@mon_mutex_owner_object, self) + if defined?(@mon_mutex) && Primitive.object_equal(@mon_mutex_owner_object, self) raise ThreadError, 'already initialized' end @mon_mutex = Thread::Mutex.new From 2f7c99865d106515290212c01dc157f5ff42f17d Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Tue, 20 Jul 2021 13:56:18 +0000 Subject: [PATCH 36/37] Change wording in change log. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 381416ce7b1d..bf23ef30c164 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ Compatibility: Performance: -* Implemented `MonitorMixin` primitives in Java (#2375). +* Moved most of `MonitorMixin` to primitives to deal with interrupts more efficiently (#2375). Changes: From d94f3e990edd0bd5104aa1835c1aaa89c0e46b34 Mon Sep 17 00:00:00 2001 From: Duncan MacGregor Date: Thu, 22 Jul 2021 11:29:52 +0100 Subject: [PATCH 37/37] Rewrite monitor specs to use queues to synchronize threads. --- spec/ruby/library/monitor/enter_spec.rb | 11 +++++---- spec/ruby/library/monitor/new_cond_spec.rb | 24 +++++++++---------- spec/ruby/library/monitor/synchronize_spec.rb | 10 ++++---- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/spec/ruby/library/monitor/enter_spec.rb b/spec/ruby/library/monitor/enter_spec.rb index 44ee2d2b039d..f523c42087f6 100644 --- a/spec/ruby/library/monitor/enter_spec.rb +++ b/spec/ruby/library/monitor/enter_spec.rb @@ -5,21 +5,22 @@ it "acquires the monitor" do monitor = Monitor.new 10.times do - locked = false + wait_q = Queue.new + continue_q = Queue.new thread = Thread.new do begin monitor.enter - locked = true - sleep # wait for wakeup. + wait_q << true + continue_q.pop ensure monitor.exit end end - Thread.pass until locked + wait_q.pop monitor.mon_locked?.should == true - thread.wakeup + continue_q << true thread.join monitor.mon_locked?.should == false end diff --git a/spec/ruby/library/monitor/new_cond_spec.rb b/spec/ruby/library/monitor/new_cond_spec.rb index d3c52acec175..ec25d3f8a221 100644 --- a/spec/ruby/library/monitor/new_cond_spec.rb +++ b/spec/ruby/library/monitor/new_cond_spec.rb @@ -14,18 +14,18 @@ 10.times do - locked = false + wait_q = Queue.new thread = Thread.new do m.synchronize do - locked = true + wait_q << true c.wait end :done end - Thread.pass until locked - Thread.pass until thread.stop? + wait_q.pop + # Synchronize can't happen until the other thread is waiting. m.synchronize { c.signal } thread.join @@ -39,20 +39,20 @@ 10.times do - locked = false + wait_q = Queue.new thread = Thread.new do m.synchronize do m.synchronize do - locked = true + wait_q << true c.wait end end :done end - Thread.pass until locked - Thread.pass until thread.stop? + wait_q.pop + #No need to wait here as we cannot synchronize until the other thread is waiting. m.synchronize { c.signal } thread.join @@ -66,18 +66,18 @@ 10.times do - locked = false + wait_q = Queue.new thread = Thread.new do m.synchronize do - locked = true + wait_q << true c.wait end :done end - Thread.pass until locked - Thread.pass until thread.stop? + wait_q.pop + # Synchronize can't happen until the other thread is waiting. m.synchronize { m.synchronize { c.signal } } thread.join diff --git a/spec/ruby/library/monitor/synchronize_spec.rb b/spec/ruby/library/monitor/synchronize_spec.rb index 5212b673813d..d78393eb3a44 100644 --- a/spec/ruby/library/monitor/synchronize_spec.rb +++ b/spec/ruby/library/monitor/synchronize_spec.rb @@ -7,23 +7,25 @@ monitor = Monitor.new 10.times do - locked = false + wait_q = Queue.new + continue_q = Queue.new thread = Thread.new do begin monitor.synchronize do - locked = true + wait_q << true # Do not wait here, we are trying to interrupt the ensure part of #synchronize end - sleep # wait for exception if it did not happen yet + continue_q.pop rescue exc_class monitor.should_not.mon_locked? :ok end end - Thread.pass until locked + wait_q.pop thread.raise exc_class, "interrupt" + continue_q << true thread.value.should == :ok end end