Move marking to C call exit to reduce VALUE lifetimes.

Author: aardvark179

lib/truffle/truffle/cext.rb

@@ -18,6 +18,7 @@ module Truffle::CExt
   DATA_TYPE = Primitive.object_hidden_var_create :data_type
   DATA_HOLDER = Primitive.object_hidden_var_create :data_holder
   DATA_MEMSIZER = Primitive.object_hidden_var_create :data_memsizer
+  DATA_MARKER = Primitive.object_hidden_var_create :data_marker
   RB_TYPE = Primitive.object_hidden_var_create :rb_type
   ALLOCATOR_FUNC = Primitive.object_hidden_var_create :allocator_func
   RB_IO_STRUCT = Primitive.object_hidden_var_create :rb_io_struct
@@ -1456,6 +1457,11 @@ def rb_set_end_proc(func, data)
     at_exit { Primitive.call_with_c_mutex_and_frame(func, [data], Primitive.caller_special_variables_if_available, nil) }
   end
 
+  def define_marker(object, marker)
+    Primitive.object_hidden_var_set object, DATA_MARKER, marker
+    Primitive.cext_mark_object_on_call_exit(object) unless Truffle::Interop.null?(marker)
+  end
+
   def rb_data_object_wrap(ruby_class, data, mark, free)
     ruby_class = Object unless ruby_class
     object = ruby_class.__send__(:__layout_allocate__)
@@ -1464,7 +1470,7 @@ def rb_data_object_wrap(ruby_class, data, mark, free)
 
     Primitive.object_space_define_data_finalizer object, free, data_holder unless Truffle::Interop.null?(free)
 
-    define_marker object, data_marker(mark, data_holder) unless Truffle::Interop.null?(mark)
+    define_marker object, mark
 
     object
   end
@@ -1479,19 +1485,21 @@ def rb_data_typed_object_wrap(ruby_class, data, data_type, mark, free, size)
 
     Primitive.object_space_define_data_finalizer object, free, data_holder unless Truffle::Interop.null?(free)
 
-    define_marker object, data_marker(mark, data_holder) unless Truffle::Interop.null?(mark)
+    define_marker object, mark
+
     object
   end
 
-  def data_marker(mark, data_holder)
-    raise unless mark.respond_to?(:call)
-    proc { |obj|
+  def run_marker(obj)
+    mark = Primitive.object_hidden_var_get obj, DATA_MARKER
+    unless Truffle::Interop.null?(mark)
       create_mark_list(obj)
+      data_holder = Primitive.object_hidden_var_get obj, DATA_HOLDER
       data = Primitive.data_holder_get_data(data_holder)
       # This call is done without pushing a new frame as the marking service manages frames itself.
       Primitive.call_with_c_mutex(mark, [data]) unless Truffle::Interop.null?(data)
       set_mark_list_on_object(obj)
-    }
+    end
   end
 
   def data_sizer(sizer, data_holder)

lib/truffle/truffle/cext_structs.rb

@@ -68,6 +68,7 @@ def polyglot_members(internal)
   def polyglot_read_member(name)
     case name
     when 'data'
+      Primitive.cext_mark_object_on_call_exit(@object) unless Primitive.object_hidden_var_get(@object, Truffle::CExt::DATA_MARKER).nil?
       Primitive.data_holder_get_data(@data_holder)
     when 'type'
       type

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

@@ -189,7 +189,7 @@ public RubyContext(RubyLanguage language, TruffleLanguage.Env env) {
         featureLoader = new FeatureLoader(this, language);
         referenceProcessor = new ReferenceProcessor(this);
         finalizationService = new FinalizationService(referenceProcessor);
-        markingService = new MarkingService(referenceProcessor);
+        markingService = new MarkingService();
         dataObjectFinalizationService = new DataObjectFinalizationService(language, referenceProcessor);
 
         // We need to construct this at runtime

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

@@ -108,7 +108,6 @@
 import org.truffleruby.language.objects.MetaClassNode;
 import org.truffleruby.language.objects.WriteObjectFieldNode;
 import org.truffleruby.language.supercall.CallSuperMethodNode;
-import org.truffleruby.language.yield.CallBlockNode;
 import org.truffleruby.parser.IdentifierType;
 
 import com.oracle.truffle.api.CompilerDirectives;
@@ -167,6 +166,8 @@ private static long getNativeStringCapacity(Pointer pointer) {
     public abstract static class CallWithCExtLockAndFrameNode extends PrimitiveArrayArgumentsNode {
 
         @Child protected CallWithCExtLockNode callCextNode = CallWithCExtLockNodeFactory.create(RubyNode.EMPTY_ARRAY);
+        @Child protected MarkingServiceNodes.RunMarkOnExitNode runMarksNode = MarkingServiceNodes.RunMarkOnExitNode
+                .create();
 
         @Specialization
         protected Object callWithCExtLockAndFrame(
@@ -175,7 +176,11 @@ protected Object callWithCExtLockAndFrame(
             final boolean keywordsGiven = RubyArguments.getDescriptor(frame) instanceof KeywordArgumentsDescriptor;
             extensionStack.push(keywordsGiven, specialVariables, block);
             try {
-                return callCextNode.execute(receiver, argsArray);
+                try {
+                    return callCextNode.execute(receiver, argsArray);
+                } finally {
+                    runMarksNode.execute(extensionStack);
+                }
             } finally {
                 extensionStack.pop();
             }
@@ -185,13 +190,17 @@ protected Object callWithCExtLockAndFrame(
     @Primitive(name = "call_with_c_mutex_and_frame_and_unwrap")
     public abstract static class CallWithCExtLockAndFrameAndUnwrapNode extends PrimitiveArrayArgumentsNode {
 
+        @Child protected MarkingServiceNodes.RunMarkOnExitNode runMarksNode = MarkingServiceNodes.RunMarkOnExitNode
+                .create();
+
         @Specialization(limit = "getCacheLimit()")
         protected Object callWithCExtLockAndFrame(
                 VirtualFrame frame, Object receiver, RubyArray argsArray, Object specialVariables, Object block,
                 @CachedLibrary("receiver") InteropLibrary receivers,
                 @Cached ArrayToObjectArrayNode arrayToObjectArrayNode,
                 @Cached TranslateInteropExceptionNode translateInteropExceptionNode,
                 @Cached ConditionProfile ownedProfile,
+                @Cached MarkingServiceNodes.RunMarkOnExitNode runMarksNode,
                 @Cached UnwrapNode unwrapNode) {
             final ExtensionCallStack extensionStack = getLanguage().getCurrentFiber().extensionCallStack;
             final boolean keywordsGiven = RubyArguments.getDescriptor(frame) instanceof KeywordArgumentsDescriptor;
@@ -210,13 +219,18 @@ protected Object callWithCExtLockAndFrame(
                         return unwrapNode.execute(
                                 InteropNodes.execute(receiver, args, receivers, translateInteropExceptionNode));
                     } finally {
+                        runMarksNode.execute(extensionStack);
                         if (!owned) {
                             MutexOperations.unlockInternal(lock);
                         }
                     }
                 } else {
-                    return unwrapNode
-                            .execute(InteropNodes.execute(receiver, args, receivers, translateInteropExceptionNode));
+                    try {
+                        return unwrapNode.execute(
+                                InteropNodes.execute(receiver, args, receivers, translateInteropExceptionNode));
+                    } finally {
+                        runMarksNode.execute(extensionStack);
+                    }
                 }
 
             } finally {
@@ -361,6 +375,18 @@ protected Object publicSendWithoutLock(
         }
     }
 
+    @Primitive(name = "cext_mark_object_on_call_exit")
+    public abstract static class MarkObjectOnCallExit extends PrimitiveArrayArgumentsNode {
+
+        @Specialization
+        protected Object markOnCallExit(Object object,
+                @Cached WrapNode wrapNode,
+                @Cached MarkingServiceNodes.QueueForMarkOnExitNode markOnExitNode) {
+            markOnExitNode.execute(wrapNode.execute(object));
+            return nil;
+        }
+    }
+
     @CoreMethod(names = "cext_start_new_handle_block", onSingleton = true)
     public abstract static class StartNewHandleBlockNode extends CoreMethodArrayArgumentsNode {
         @Specialization
@@ -1677,6 +1703,7 @@ public abstract static class NewMarkerList extends CoreMethodArrayArgumentsNode
         protected Object createNewMarkList(RubyDynamicObject object,
                 @CachedLibrary("object") DynamicObjectLibrary objectLibrary) {
             getContext().getMarkingService().startMarking(
+                    getLanguage().getCurrentThread().getCurrentFiber().extensionCallStack,
                     (Object[]) objectLibrary.getOrDefault(object, Layouts.MARKED_OBJECTS_IDENTIFIER, null));
             return nil;
         }
@@ -1692,7 +1719,8 @@ protected Object addToMarkList(Object markedObject,
             ValueWrapper wrappedValue = toWrapperNode.execute(markedObject);
             if (wrappedValue != null) {
                 noExceptionProfile.enter();
-                getContext().getMarkingService().addMark(wrappedValue);
+                getContext().getMarkingService()
+                        .addMark(getLanguage().getCurrentThread().getCurrentFiber().extensionCallStack, wrappedValue);
             }
             // We do nothing here if the handle cannot be resolved. If we are marking an object
             // which is only reachable via weak refs then the handles of objects it is itself
@@ -1729,29 +1757,12 @@ protected Object setMarkList(RubyDynamicObject structOwner,
             writeMarkedNode.execute(
                     structOwner,
                     Layouts.MARKED_OBJECTS_IDENTIFIER,
-                    getContext().getMarkingService().finishMarking());
+                    getContext().getMarkingService()
+                            .finishMarking(getLanguage().getCurrentThread().getCurrentFiber().extensionCallStack));
             return nil;
         }
     }
 
-    @CoreMethod(names = "define_marker", onSingleton = true, required = 2)
-    public abstract static class CreateMarkerNode extends CoreMethodArrayArgumentsNode {
-
-        @TruffleBoundary
-        @Specialization
-        protected Object createMarker(RubyDynamicObject object, RubyProc marker) {
-            /* The code here has to be a little subtle. The marker must be associated with the object it will act on,
-             * but the lambda must not capture the object (and prevent garbage collection). So the marking function is a
-             * lambda that will take the object as an argument 'o' which will be provided when the marking function is
-             * called by the marking service. */
-            getContext()
-                    .getMarkingService()
-                    .addMarker(object, (o) -> CallBlockNode.getUncached().yield(marker, o));
-            return nil;
-        }
-
-    }
-
     @CoreMethod(names = "rb_thread_check_ints", onSingleton = true, required = 0)
     public abstract static class CheckThreadInterrupt extends CoreMethodArrayArgumentsNode {
 

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

@@ -154,7 +154,6 @@ public void freeAllBlocksInMap(RubyLanguage language) {
     }
 
     public void cleanup(RubyContext context, HandleBlockHolder holder) {
-        context.getMarkingService().queueForMarking(holder.handleBlock);
         holder.handleBlock = null;
     }
 
@@ -214,7 +213,7 @@ public static class HandleBlock {
         private static final Set<HandleBlock> keepAlive = ConcurrentHashMap.newKeySet();
 
         private final long base;
-        @SuppressWarnings("rawtypes") private final ValueWrapper[] wrappers;
+        private final ValueWrapperWeakReference[] wrappers;
         private int count;
 
         @SuppressWarnings("unused") private Cleanable cleanable;
@@ -232,7 +231,7 @@ public HandleBlock(RubyContext context, RubyLanguage language, ValueWrapperManag
                 keepAlive(this);
             }
             this.base = base;
-            this.wrappers = new ValueWrapper[BLOCK_SIZE];
+            this.wrappers = new ValueWrapperWeakReference[BLOCK_SIZE];
             this.count = 0;
             this.cleanable = language.cleaner.register(this, HandleBlock.makeCleaner(manager, base, allocator));
         }
@@ -259,7 +258,7 @@ public int getIndex() {
 
         public ValueWrapper getWrapper(long handle) {
             int offset = (int) (handle & OFFSET_MASK) >> TAG_BITS;
-            return wrappers[offset];
+            return wrappers[offset].get();
         }
 
         public boolean isFull() {
@@ -269,7 +268,7 @@ public boolean isFull() {
         public long setHandleOnWrapper(ValueWrapper wrapper) {
             long handle = getBase() + count * Pointer.SIZE;
             wrapper.setHandle(handle, this);
-            wrappers[count] = wrapper;
+            wrappers[count] = new ValueWrapperWeakReference(wrapper);
             count++;
             return handle;
         }
@@ -285,6 +284,12 @@ public static final class HandleBlockWeakReference extends WeakReference<HandleB
         }
     }
 
+    public static final class ValueWrapperWeakReference extends WeakReference<ValueWrapper> {
+        ValueWrapperWeakReference(ValueWrapper referent) {
+            super(referent);
+        }
+    }
+
     public static class HandleBlockHolder {
         protected HandleBlock handleBlock = null;
         protected HandleBlock sharedHandleBlock = null;
@@ -333,9 +338,6 @@ protected long allocateHandle(ValueWrapper wrapper, RubyContext context, RubyLan
             }
 
             if (block == null || block.isFull()) {
-                if (block != null) {
-                    context.getMarkingService().queueForMarking(block);
-                }
                 if (shared) {
                     block = context.getValueWrapperManager().addToSharedBlockMap(context, language);
                     holder.sharedHandleBlock = block;
@@ -361,9 +363,6 @@ public static HandleBlock allocateNewBlock(RubyContext context, RubyLanguage lan
         HandleBlockHolder holder = getBlockHolder(context, language);
         HandleBlock block = holder.handleBlock;
 
-        if (block != null) {
-            context.getMarkingService().queueForMarking(block);
-        }
         block = context.getValueWrapperManager().addToBlockMap(context, language);
 
         holder.handleBlock = block;

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

@@ -42,7 +42,6 @@ public abstract static class GCStartPrimitiveNode extends PrimitiveArrayArgument
         @TruffleBoundary
         @Specialization
         protected Object vmGCStart() {
-            getContext().getMarkingService().queueMarking();
             System.gc();
             return nil;
         }
@@ -64,8 +63,6 @@ public abstract static class GCForce extends PrimitiveArrayArgumentsNode {
         @TruffleBoundary
         @Specialization
         protected Object force() {
-            getContext().getMarkingService().queueMarking();
-
             // NOTE(norswap, 16 Apr 20): We could have used a WeakReference here, but the hope is that the extra
             // indirection will prevent the compiler to optimize this method away (assuming JIT compilation, and
             // the fact that such an optimizaton is permitted and possible, both of which are unlikely).