[GR-17457] Create less nodes for WriteBarrierNode and profile RubyLibrary#isFrozen() usages

PullRequest: truffleruby/3169

Author: eregon

src/main/java/org/truffleruby/core/hash/FreezeHashKeyIfNeededNode.java

@@ -15,10 +15,8 @@
 import org.truffleruby.core.string.ImmutableRubyString;
 import org.truffleruby.language.RubyBaseNode;
 import org.truffleruby.language.dispatch.DispatchNode;
-import org.truffleruby.language.library.RubyLibrary;
 
 import com.oracle.truffle.api.dsl.Specialization;
-import com.oracle.truffle.api.library.CachedLibrary;
 
 @GenerateUncached
 public abstract class FreezeHashKeyIfNeededNode extends RubyBaseNode {
@@ -30,31 +28,21 @@ protected Object immutable(ImmutableRubyString string, boolean compareByIdentity
         return string;
     }
 
-    @Specialization(
-            guards = "rubyLibrary.isFrozen(string)",
-            limit = "getRubyLibraryCacheLimit()")
-    protected Object alreadyFrozen(RubyString string, boolean compareByIdentity,
-            @CachedLibrary("string") RubyLibrary rubyLibrary) {
+    @Specialization(guards = "string.isFrozen()")
+    protected Object alreadyFrozen(RubyString string, boolean compareByIdentity) {
         return string;
     }
 
-    @Specialization(
-            guards = { "!rubyLibrary.isFrozen(string)", "!compareByIdentity" },
-            limit = "getRubyLibraryCacheLimit()")
+    @Specialization(guards = { "!string.isFrozen()", "!compareByIdentity" })
     protected Object dupAndFreeze(RubyString string, boolean compareByIdentity,
-            @CachedLibrary("string") RubyLibrary rubyLibrary,
-            @CachedLibrary(limit = "getRubyLibraryCacheLimit()") RubyLibrary rubyLibraryObject,
             @Cached DispatchNode dupNode) {
-        final Object object = dupNode.call(string, "dup");
-        rubyLibraryObject.freeze(object);
-        return object;
+        final RubyString copy = (RubyString) dupNode.call(string, "dup");
+        copy.freeze();
+        return copy;
     }
 
-    @Specialization(
-            guards = { "!rubyLibrary.isFrozen(string)", "compareByIdentity" },
-            limit = "getRubyLibraryCacheLimit()")
-    protected Object compareByIdentity(RubyString string, boolean compareByIdentity,
-            @CachedLibrary("string") RubyLibrary rubyLibrary) {
+    @Specialization(guards = { "!string.isFrozen()", "compareByIdentity" })
+    protected Object compareByIdentity(RubyString string, boolean compareByIdentity) {
         return string;
     }
 

src/main/java/org/truffleruby/core/kernel/KernelNodes.java

@@ -567,7 +567,7 @@ protected RubyDynamicObject clone(RubyDynamicObject object, Object freeze,
             }
 
             // Default behavior - is just to copy the frozen state of the original object
-            if (forceFrozen(freeze) || (copyFrozen && rubyLibrary.isFrozen(object))) {
+            if (forceFrozen(freeze) || (copyFrozen && rubyLibrary.isFrozen(object))) { // Profiled through lazy usage of rubyLibraryFreeze
                 rubyLibraryFreeze.freeze(newObject);
             }
 
@@ -857,14 +857,13 @@ protected Object freeze(Object self,
         protected Object freezeDynamicObject(Object self,
                 @CachedLibrary("self") RubyLibrary rubyLibrary,
                 @CachedLibrary(limit = "1") RubyLibrary rubyLibraryMetaClass,
-                @Cached ConditionProfile singletonProfile,
+                @Cached ConditionProfile singletonClassUnfrozenProfile,
                 @Cached MetaClassNode metaClassNode) {
             final RubyClass metaClass = metaClassNode.execute(self);
-            if (singletonProfile.profile(metaClass.isSingleton &&
-                    !(RubyGuards.isRubyClass(self) && ((RubyClass) self).isSingleton))) {
-                if (!rubyLibraryMetaClass.isFrozen(metaClass)) {
-                    rubyLibraryMetaClass.freeze(metaClass);
-                }
+            if (singletonClassUnfrozenProfile.profile(metaClass.isSingleton &&
+                    !(RubyGuards.isRubyClass(self) && ((RubyClass) self).isSingleton) &&
+                    !rubyLibraryMetaClass.isFrozen(metaClass))) {
+                rubyLibraryMetaClass.freeze(metaClass);
             }
             rubyLibrary.freeze(self);
             return self;

src/main/java/org/truffleruby/core/string/RubyString.java

@@ -90,12 +90,12 @@ protected String getJavaString() {
 
     // region RubyLibrary messages
     @ExportMessage
-    protected void freeze() {
+    public void freeze() {
         frozen = true;
     }
 
     @ExportMessage
-    protected boolean isFrozen() {
+    public boolean isFrozen() {
         return frozen;
     }
     // endregion

src/main/java/org/truffleruby/language/library/RubyLibrary.java

@@ -34,6 +34,8 @@ public static RubyLibrary getUncached() {
 
     public abstract void freeze(Object object);
 
+    /** The result is not always a PE constant, specifically for RubyString and RubyRange. For RubyDynamicObject it's
+     * only constant if not frozen. */
     public abstract boolean isFrozen(Object object);
 
 }

src/main/java/org/truffleruby/language/objects/WriteInstanceVariableNode.java

@@ -9,6 +9,7 @@
  */
 package org.truffleruby.language.objects;
 
+import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
 import org.truffleruby.RubyContext;
 import org.truffleruby.RubyLanguage;
 import org.truffleruby.core.array.AssignableNode;
@@ -31,6 +32,8 @@ public class WriteInstanceVariableNode extends RubyContextSourceNode implements
     @Child private RubyNode rhs;
     @Child private WriteObjectFieldNode writeNode;
 
+    @CompilationFinal private boolean frozenProfile;
+
     public WriteInstanceVariableNode(String name, RubyNode receiver, RubyNode rhs) {
         this.name = name;
         this.receiver = receiver;
@@ -53,6 +56,10 @@ public void assign(VirtualFrame frame, Object value) {
 
     private void write(Object object, Object value) {
         if (getRubyLibrary().isFrozen(object)) {
+            if (!frozenProfile) {
+                CompilerDirectives.transferToInterpreterAndInvalidate();
+                frozenProfile = true;
+            }
             throw new RaiseException(getContext(), coreExceptions().frozenError(object, this));
         }
 

src/main/java/org/truffleruby/language/objects/WriteObjectFieldNode.java

@@ -19,7 +19,6 @@
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.object.DynamicObjectLibrary;
-import com.oracle.truffle.api.profiles.BranchProfile;
 
 import java.lang.invoke.VarHandle;
 
@@ -39,11 +38,10 @@ protected void writeLocal(RubyDynamicObject object, Object name, Object value,
         objectLibrary.put(object, name, value);
     }
 
-    @Specialization(guards = "objectLibrary.isShared(object)", limit = "getCacheLimit()")
+    @Specialization(guards = "objectLibrary.isShared(object)")
     protected void writeShared(RubyDynamicObject object, Object name, Object value,
-            @CachedLibrary("object") DynamicObjectLibrary objectLibrary,
-            @Cached WriteBarrierNode writeBarrierNode,
-            @Cached BranchProfile shapeRaceProfile) {
+            @CachedLibrary(limit = "getCacheLimit()") DynamicObjectLibrary objectLibrary,
+            @Cached WriteBarrierNode writeBarrierNode) {
 
         // Share `value` before it becomes reachable through `object`
         writeBarrierNode.executeWriteBarrier(value);
@@ -55,14 +53,6 @@ protected void writeShared(RubyDynamicObject object, Object name, Object value,
         VarHandle.storeStoreFence();
 
         synchronized (object) {
-            // Re-check the shape under the monitor as another thread might have changed it
-            // by adding a field or upgrading an existing field to Object storage
-            // (need to use the new storage)
-            if (!objectLibrary.accepts(object)) {
-                shapeRaceProfile.enter();
-                DynamicObjectLibrary.getUncached().put(object, name, value);
-                return;
-            }
             objectLibrary.put(object, name, value);
         }
     }

src/main/java/org/truffleruby/language/objects/shared/WriteBarrierNode.java

@@ -25,7 +25,6 @@
 @NodeField(name = "depth", type = int.class)
 public abstract class WriteBarrierNode extends RubyBaseNode {
 
-    protected static final int CACHE_LIMIT = 8;
     protected static final int MAX_DEPTH = 3;
 
     protected abstract int getDepth();
@@ -36,39 +35,46 @@ public static WriteBarrierNode create() {
 
     public abstract void executeWriteBarrier(Object value);
 
+    @Specialization(guards = "!isRubyDynamicObject(value)")
+    protected void noWriteBarrier(Object value) {
+    }
+
     @Specialization(
-            guards = { "value.getShape() == cachedShape", "getDepth() < MAX_DEPTH" },
+            guards = { "value.getShape() == cachedShape", "cachedShape.isShared()" },
+            limit = "1") // limit of 1 as the next specialization is cheap
+    protected void alreadySharedCached(RubyDynamicObject value,
+            @Cached("value.getShape()") Shape cachedShape) {
+    }
+
+    @Specialization(guards = "value.getShape().isShared()", replaces = "alreadySharedCached")
+    protected void alreadySharedUncached(RubyDynamicObject value) {
+    }
+
+    @Specialization(
+            guards = { "getDepth() < MAX_DEPTH", "value.getShape() == cachedShape", "!cachedShape.isShared()" },
             assumptions = "cachedShape.getValidAssumption()",
-            limit = "CACHE_LIMIT")
+            // limit of 1 to avoid creating many nodes if the value's Shape is polymorphic.
+            // GR-36904: Not simply using "1" so the cached nodes are cleared when writeBarrierUncached() is activated.
+            limit = "getIdentityCacheLimit()")
     protected void writeBarrierCached(RubyDynamicObject value,
             @Cached("value.getShape()") Shape cachedShape,
-            @Cached("cachedShape.isShared()") boolean alreadyShared,
-            @Cached("createShareObjectNode(alreadyShared)") ShareObjectNode shareObjectNode) {
-        if (!alreadyShared) {
-            shareObjectNode.executeShare(value);
-        }
+            @Cached("createShareObjectNode()") ShareObjectNode shareObjectNode) {
+        shareObjectNode.executeShare(value);
     }
 
     @Specialization(guards = "updateShape(value)")
     protected void updateShapeAndWriteBarrier(RubyDynamicObject value) {
         executeWriteBarrier(value);
     }
 
-    @Specialization(replaces = { "writeBarrierCached", "updateShapeAndWriteBarrier" })
+    @Specialization(guards = "!value.getShape().isShared()",
+            replaces = { "writeBarrierCached", "updateShapeAndWriteBarrier" })
     protected void writeBarrierUncached(RubyDynamicObject value) {
         SharedObjects.writeBarrier(getLanguage(), value);
     }
 
-    @Specialization(guards = "!isRubyDynamicObject(value)")
-    protected void noWriteBarrier(Object value) {
-    }
-
-    protected ShareObjectNode createShareObjectNode(boolean alreadyShared) {
-        if (!alreadyShared) {
-            return ShareObjectNodeGen.create(getDepth() + 1);
-        } else {
-            return null;
-        }
+    protected ShareObjectNode createShareObjectNode() {
+        return ShareObjectNodeGen.create(getDepth() + 1);
     }
 
 }