[GR-16164] Fix sharing with the benchmark harness, RubyLauncher and IO buffers.

PullRequest: truffleruby/881

Author: eregon

CHANGELOG.md

@@ -1,3 +1,9 @@
+# 20.0.0 beta 2
+
+Bug fixes:
+
+* Sharing for thread-safety of objects is now triggered later as intended, e.g., when a second `Thread` is started.
+
 # 20.0.0 beta 1
 
 Bug fixes:

bench/benchmark-interface/lib/benchmark-interface/backends/bips.rb

@@ -13,7 +13,7 @@ module Bips
       LONG_ITERATION_THRESHOLD = 0.1 # seconds
 
       def self.run(benchmark_set, names, options)
-        BeckhmarkInterface.require_rubygems
+        BenchmarkInterface.require_rubygems
         benchmark_interface_original_require 'benchmark/ips'
 
         unless options['--no-scale']

bench/benchmark-interface/lib/benchmark-interface/backends/deep.rb

@@ -14,8 +14,8 @@ def self.run(benchmark_set, names, options)
         unless names.size == 1
           abort 'The deep backend only works when you run just one benchmark at a time - specify the name on the command line'
         end
-        
-        BeckhmarkInterface.require_rubygems
+
+        BenchmarkInterface.require_rubygems
         benchmark_interface_original_require 'deep-bench/backend'
 
         benchmark = benchmark_set.benchmark(names.first)

bench/benchmark-interface/lib/benchmark-interface/benchmark-set.rb

@@ -14,25 +14,22 @@ class BenchmarkSet
     def initialize
       @benchmarks = []
       @counter = 0
-      @@current = self
+      Thread.current[:benchmark_interface_benchmark_set] = self
       @iterations = 1
     end
 
     def load_benchmarks(path)
       @path = path
-      @@current = self
       load(path)
     end
 
     def load_inlined_benchmark(code)
       @path = '-e'
-      @@current = self
       TOPLEVEL_BINDING.eval(code)
     end
 
     def load_mri_benchmarks(path, options)
       @path = path
-      @@current = self
       Frontends::MRI.load_mri path, options
     end
 
@@ -84,10 +81,8 @@ def benchmark(name)
       benchmarks([name]).first
     end
 
-    @@current = nil
-
     def self.current
-      @@current
+      Thread.current[:benchmark_interface_benchmark_set]
     end
 
   end

bench/benchmark-interface/lib/benchmark-interface/frontends/benchmark.rb

@@ -34,10 +34,6 @@ def self.realtime(name=nil, &block)
   def self.benchmark(caption='', label_width=nil, format=nil, *labels)
     yield BenchmarkInterface::BenchmarkContext.new
   end
-
-  def self.realtime(name=nil, &block)
-    BenchmarkInterface.benchmark name, &block
-  end
 
   def self.bm(label_width=0, *labels)
     yield BenchmarkInterface::BenchmarkContext.new

bench/benchmark-interface/lib/benchmark-interface/require.rb

@@ -6,7 +6,7 @@
 # GNU General Public License version 2, or
 # GNU Lesser General Public License version 2.1.
 
-module BeckhmarkInterface
+module BenchmarkInterface
 
   def self.require_rubygems
     begin

spec/tags/truffle/thread_safe_objects_tags.txt

@@ -0,0 +1,2 @@
+slow:Sharing is correctly propagated for classes and constants and they are not shared until sharing is started
+slow:Sharing is correctly propagated for thread-local IO buffers which should not trigger sharing

spec/truffle/thread_safe_objects_spec.rb

@@ -255,4 +255,34 @@ def wb; @wb; end
     shared?(new_hash).should == true
   end
 
+  it "Fiber local variables which do not share the value" do
+    thread = Thread.current
+    shared?(thread).should == true
+
+    obj = Object.new
+    thread[:sharing_spec] = obj
+    begin
+      shared?(obj).should == false
+    ensure
+      thread[:sharing_spec] = nil
+    end
+  end
+
+  it "Thread local variables which share the value (probably they should not)" do
+    thread = Thread.current
+    shared?(thread).should == true
+
+    obj = Object.new
+    thread.thread_variable_set(:sharing_spec, obj)
+    shared?(obj).should == true # TODO (eregon, 5 Jun 2019): this is the current non-ideal behavior
+  end
+
+  it "classes and constants and they are not shared until sharing is started" do
+    ruby_exe("p Truffle::Debug.shared?(Object)").should == "false\n"
+  end
+
+  it "thread-local IO buffers which should not trigger sharing" do
+    ruby_exe("File.read(#{__FILE__.inspect}); p Truffle::Debug.shared?(Object)").should == "false\n"
+  end
+
 end

src/main/java/org/truffleruby/RubyContext.java

@@ -262,7 +262,7 @@ public void initialize() {
 
         // Share once everything is loaded
         if (options.SHARED_OBJECTS_ENABLED && options.SHARED_OBJECTS_FORCE) {
-            sharedObjects.startSharing();
+            sharedObjects.startSharing(OptionsCatalog.SHARED_OBJECTS_FORCE.getName() + " being true");
         }
 
         if (isPreInitializing()) {

src/main/java/org/truffleruby/core/FinalizationService.java

@@ -111,7 +111,7 @@ public synchronized FinalizerReference addFinalizer(Object object, FinalizerRefe
 
         finalizerReference.addFinalizer(owner, action, root);
 
-        referenceProcessor.processReferenceQueue();
+        referenceProcessor.processReferenceQueue(owner);
         return finalizerReference;
     }
 

src/main/java/org/truffleruby/core/ReferenceProcessingService.java

@@ -113,7 +113,7 @@ public ReferenceProcessor(RubyContext context) {
             this.context = context;
         }
 
-        protected void processReferenceQueue() {
+        protected void processReferenceQueue(Class<?> owner) {
             if (context.getOptions().SINGLE_THREADED) {
 
                 drainReferenceQueue();
@@ -132,18 +132,19 @@ protected void processReferenceQueue() {
                  */
 
                 if (processingThread == null && !context.isPreInitializing() && context.isInitialized() && !context.isFinalizing()) {
-                    createProcessingThread();
+                    createProcessingThread(owner);
                 }
 
             }
         }
 
-        protected void createProcessingThread() {
+        protected void createProcessingThread(Class<?> owner) {
             final ThreadManager threadManager = context.getThreadManager();
             processingThread = threadManager.createBootThread(threadName());
             context.send(processingThread, "internal_thread_initialize");
+            final String sharingReason = "creating " + threadName() + " thread for " + owner.getSimpleName();
 
-            threadManager.initialize(processingThread, null, threadName(), () -> {
+            threadManager.initialize(processingThread, null, threadName(), sharingReason, () -> {
                 while (true) {
                     final ProcessingReference<?> reference = (ProcessingReference<?>) threadManager.runUntilResult(null, processingQueue::remove);
 

src/main/java/org/truffleruby/core/array/ArrayAppendManyNode.java

@@ -11,6 +11,7 @@
 
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.ImportStatic;
+import com.oracle.truffle.api.dsl.ReportPolymorphism;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.object.DynamicObject;
 import com.oracle.truffle.api.profiles.ConditionProfile;
@@ -23,6 +24,7 @@
 import org.truffleruby.language.objects.shared.PropagateSharingNode;
 
 @ImportStatic(ArrayGuards.class)
+@ReportPolymorphism
 public abstract class ArrayAppendManyNode extends RubyBaseNode {
 
     @Child private PropagateSharingNode propagateSharingNode = PropagateSharingNode.create();

src/main/java/org/truffleruby/core/array/ArrayAppendOneNode.java

@@ -12,6 +12,7 @@
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.ImportStatic;
 import com.oracle.truffle.api.dsl.NodeChild;
+import com.oracle.truffle.api.dsl.ReportPolymorphism;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.object.DynamicObject;
 import com.oracle.truffle.api.profiles.ConditionProfile;
@@ -24,6 +25,7 @@
 @NodeChild("array")
 @NodeChild("value")
 @ImportStatic(ArrayGuards.class)
+@ReportPolymorphism
 public abstract class ArrayAppendOneNode extends RubyNode {
 
     @Child private PropagateSharingNode propagateSharingNode = PropagateSharingNode.create();

src/main/java/org/truffleruby/core/support/IONodes.java

@@ -477,8 +477,6 @@ public int write(int fd, DynamicObject string) {
     @Primitive(name = "io_get_thread_buffer", needsSelf = false)
     public static abstract class GetThreadBufferNode extends PrimitiveArrayArgumentsNode {
 
-        private static final ThreadLocal<Pointer> THREAD_BUFFER_THREAD_LOCAL = ThreadLocal.withInitial(() -> Pointer.NULL);
-
         @Specialization
         public DynamicObject getThreadBuffer(long size,
                 @Cached("create()") AllocateObjectNode allocateObjectNode) {
@@ -487,13 +485,15 @@ public DynamicObject getThreadBuffer(long size,
 
         @TruffleBoundary
         public static Pointer getBuffer(RubyContext context, long size) {
-            final Pointer buffer = THREAD_BUFFER_THREAD_LOCAL.get();
+            final DynamicObject rubyThread = context.getThreadManager().getCurrentThread();
+            final Pointer buffer = Layouts.THREAD.getIoBuffer(rubyThread);
+
             if (buffer.getSize() >= size) {
                 return buffer;
             } else {
+                buffer.freeNoAutorelease();
                 final Pointer newBuffer = Pointer.malloc(Math.max(size * 2, 1024));
-                newBuffer.enableAutorelease(context.getFinalizationService());
-                THREAD_BUFFER_THREAD_LOCAL.set(newBuffer);
+                Layouts.THREAD.setIoBuffer(rubyThread, newBuffer);
                 return newBuffer;
             }
         }

src/main/java/org/truffleruby/core/thread/ThreadLayout.java

@@ -18,6 +18,7 @@
 import org.truffleruby.core.InterruptMode;
 import org.truffleruby.core.basicobject.BasicObjectLayout;
 import org.truffleruby.core.fiber.FiberManager;
+import org.truffleruby.extra.ffi.Pointer;
 import org.truffleruby.language.threadlocal.ThreadLocalGlobals;
 
 import java.util.List;
@@ -45,6 +46,7 @@ Object[] build(
             @Nullable @Volatile Object value,
             AtomicBoolean wakeUp,
             @Volatile int priority,
+            Pointer ioBuffer,
             DynamicObject threadGroup,
             String sourceLocation,
             DynamicObject name);
@@ -85,6 +87,9 @@ Object[] build(
     int getPriority(DynamicObject object);
     void setPriority(DynamicObject object, int value);
 
+    Pointer getIoBuffer(DynamicObject object);
+    void setIoBuffer(DynamicObject object, Pointer value);
+
     DynamicObject getThreadGroup(DynamicObject object);
     void setThreadGroup(DynamicObject object, DynamicObject value);
 

src/main/java/org/truffleruby/core/thread/ThreadManager.java

@@ -183,6 +183,7 @@ private Object[] packThreadFields(DynamicObject currentGroup, String info) {
                 null,
                 new AtomicBoolean(false),
                 Thread.NORM_PRIORITY,
+                Pointer.NULL,
                 currentGroup,
                 info,
                 nil());
@@ -228,8 +229,8 @@ private void setupNativeThreadSupport(TruffleNFIPlatform nfi, NativeConfiguratio
         pthread_kill = nfi.getFunction("pthread_kill", "(" + pthread_t + ",sint32):sint32");
     }
 
-    public void initialize(DynamicObject rubyThread, Node currentNode, String info, Supplier<Object> task) {
-        startSharing(rubyThread);
+    public void initialize(DynamicObject rubyThread, Node currentNode, String info, String sharingReason, Supplier<Object> task) {
+        startSharing(rubyThread, sharingReason);
 
         Layouts.THREAD.setSourceLocation(rubyThread, info);
 
@@ -308,16 +309,16 @@ private static void setException(RubyContext context, DynamicObject thread, Dyna
     }
 
     // Share the Ruby Thread before it can be accessed concurrently, and before it is added to Thread.list
-    public void startSharing(DynamicObject rubyThread) {
+    public void startSharing(DynamicObject rubyThread, String reason) {
         if (context.getOptions().SHARED_OBJECTS_ENABLED) {
             // TODO (eregon, 22 Sept 2017): no need if singleThreaded in isThreadAccessAllowed()
-            context.getSharedObjects().startSharing();
+            context.getSharedObjects().startSharing(reason);
             SharedObjects.writeBarrier(context, rubyThread);
         }
     }
 
     public void startForeignThread(DynamicObject rubyThread, Thread javaThread) {
-        startSharing(rubyThread);
+        startSharing(rubyThread, "creating a foreign thread");
         start(rubyThread, javaThread);
     }
 
@@ -336,6 +337,8 @@ public void cleanup(DynamicObject thread, Thread javaThread) {
         final FiberManager fiberManager = Layouts.THREAD.getFiberManager(thread);
         fiberManager.shutdown(javaThread);
 
+        Layouts.THREAD.getIoBuffer(thread).freeNoAutorelease();
+
         unregisterThread(thread);
         Layouts.THREAD.setThread(thread, null);
 

src/main/java/org/truffleruby/core/thread/ThreadNodes.java

@@ -276,15 +276,17 @@ public abstract static class ThreadInitializeNode extends PrimitiveArrayArgument
         public DynamicObject initialize(DynamicObject thread, DynamicObject arguments, DynamicObject block,
                 @Cached("of(arguments)") ArrayStrategy strategy,
                 @Cached("strategy.boxedCopyNode()") ArrayOperationNodes.ArrayBoxedCopyNode boxedCopyNode) {
+            final SourceSection sourceSection = Layouts.PROC.getSharedMethodInfo(block).getSourceSection();
+            final String info = getContext().fileLine(sourceSection);
+            final Object[] args = boxedCopyNode.execute(Layouts.ARRAY.getStore(arguments), Layouts.ARRAY.getSize(arguments));
+            final String sharingReason = "creating Ruby Thread " + info;
+
             if (getContext().getOptions().SHARED_OBJECTS_ENABLED) {
-                getContext().getThreadManager().startSharing(thread);
+                getContext().getThreadManager().startSharing(thread, sharingReason);
                 SharedObjects.shareDeclarationFrame(getContext(), block);
             }
 
-            final Object[] args = boxedCopyNode.execute(Layouts.ARRAY.getStore(arguments), Layouts.ARRAY.getSize(arguments));
-            final SourceSection sourceSection = Layouts.PROC.getSharedMethodInfo(block).getSourceSection();
-            final String info = getContext().fileLine(sourceSection);
-            getContext().getThreadManager().initialize(thread, this, info,
+            getContext().getThreadManager().initialize(thread, this, info, sharingReason,
                     () -> ProcOperations.rootCall(block, args));
             return nil();
         }

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

@@ -26,7 +26,6 @@
 import org.truffleruby.language.methods.DeclarationContext;
 import org.truffleruby.language.methods.InternalMethod;
 import org.truffleruby.language.methods.SharedMethodInfo;
-import org.truffleruby.language.objects.shared.SharedObjects;
 import org.truffleruby.parser.ParserContext;
 import org.truffleruby.parser.RubySource;
 import org.truffleruby.parser.TranslatorDriver;
@@ -88,14 +87,9 @@ public Object execute(VirtualFrame frame) {
                 RubyArguments.getBlock(frame),
                 new Object[]{});
 
-        final Object value = callNode.call(arguments);
-
-        // The return value will be leaked to Java, share it.
-        if (context.getOptions().SHARED_OBJECTS_ENABLED) {
-            SharedObjects.writeBarrier(context, value);
-        }
-
-        return value;
+        // No need to share the returned value here, InlineParsingRequest is not exposed to the Context API
+        // and is only used by instruments (e.g., the debugger) or the RubyLanguage itself.
+        return callNode.call(arguments);
     }
 
 }

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

@@ -90,8 +90,9 @@ public Object execute(VirtualFrame frame) {
                 frame.getArguments());
         final Object value = callNode.call(arguments);
 
-        // The return value will be leaked to Java, share it.
-        if (context.getOptions().SHARED_OBJECTS_ENABLED) {
+        // The return value will be leaked to Java, so share it if the Context API is used.
+        // We share conditionally on EMBEDDED to avoid sharing return values used in RubyLauncher.
+        if (context.getOptions().SHARED_OBJECTS_ENABLED && context.getOptions().EMBEDDED) {
             SharedObjects.writeBarrier(context, value);
         }