[GR-68165] Make DynamicObjectLibrary#setDynamicType transitions weak, same as putConstant transitions

PullRequest: graal/21674

Author: eregon

truffle/CHANGELOG.md

@@ -6,6 +6,7 @@ This changelog summarizes major changes between Truffle versions relevant to lan
 * GR-65048: Introduced `InternalResource.OS.UNSUPPORTED` and `InternalResource.CPUArchitecture.UNSUPPORTED` to represent unsupported platforms. Execution on unsupported platforms must be explicitly enabled using the system property `-Dpolyglot.engine.allowUnsupportedPlatform=true`. If this property is not set, calls to `OS.getCurrent()` or `CPUArchitecture.getCurrent()` will throw an `IllegalStateException` when running on an unsupported platform. `InternalResource` implementations should handle the unsupported platform and describe possible steps in the error message on how to proceed.
 * GR-66839: Deprecate `Location#isFinal()` as it always returns false.
 * GR-67702: Specialization DSL: For nodes annotated with `@GenerateInline`, inlining warnings emitted for `@Cached SomeNode helper` expressions are now suppressed if the helper node class is explicitly annotated with `@GenerateInline(false)`, or is not a DSL node. This avoids unnecessary warnings if inlining for a node was explicitly disabled, and makes it possible to remove `@Cached(inline = false)` in most cases.
+* GR-68165: `DynamicObjectLibrary#setDynamicType` transitions are now weak, like `DynamicObjectLibrary#putConstant` transitions.
 
 ## Version 25.0
 * GR-31495 Added ability to specify language and instrument specific options using `Source.Builder.option(String, String)`. Languages may describe available source options by implementing `TruffleLanguage.getSourceOptionDescriptors()` and `TruffleInstrument.getSourceOptionDescriptors()` respectively.

truffle/src/com.oracle.truffle.api.object.test/src/com/oracle/truffle/object/basic/test/LeakCheckTest.java

@@ -93,7 +93,7 @@ public void leakCheck() {
     }
 
     /**
-     * Make sure constant properties do not leak.
+     * Make sure constant property transitions do not leak.
      */
     @Test
     public void constantPropertyLeakCheck() {
@@ -127,7 +127,41 @@ public void constantPropertyLeakCheck() {
     }
 
     /**
-     * Make sure constant properties do not leak.
+     * Make sure dynamic type transitions do not leak.
+     */
+    @Test
+    public void dynamicTypeLeakCheck() {
+        Shape emptyShape = newEmptyShape();
+        List<WeakReference<Shape>> weakShapeRefs = new ArrayList<>();
+        List<Shape> strongShapeRefs = new ArrayList<>();
+
+        for (int i = 0; i < 100000; i++) {
+            DynamicObject obj = newInstance(emptyShape);
+            Leak value = new Leak();
+            LIBRARY.setDynamicType(obj, value);
+
+            Shape shape = obj.getShape();
+            value.shape = shape;
+            weakShapeRefs.add(new WeakReference<>(shape));
+            strongShapeRefs.add(shape);
+        }
+
+        strongShapeRefs.clear();
+        System.gc();
+
+        for (WeakReference<Shape> fullShapeRef : weakShapeRefs) {
+            assertNull("Shape should have been garbage-collected", fullShapeRef.get());
+        }
+
+        // trigger transition map cleanup
+        DynamicObject obj = newInstance(emptyShape);
+        LIBRARY.setDynamicType(obj, new Leak());
+
+        Reference.reachabilityFence(emptyShape);
+    }
+
+    /**
+     * Make sure constant property transitions do not leak.
      */
     @Test
     public void constantPropertyLeakCheckSingleTransition() {
@@ -164,6 +198,47 @@ public void constantPropertyLeakCheckSingleTransition() {
         Reference.reachabilityFence(shapesToKeepAlive);
     }
 
+    /**
+     * Make sure dynamic type transitions do not leak.
+     */
+    @Test
+    public void dynamicTypeLeakCheckSingleTransition() {
+        List<Shape> shapesToKeepAlive = new ArrayList<>();
+        List<WeakReference<Shape>> weakShapeRefs = new ArrayList<>();
+        List<Shape> strongShapeRefs = new ArrayList<>();
+
+        for (int i = 0; i < 100000; i++) {
+            Shape emptyShape = newEmptyShape();
+            shapesToKeepAlive.add(emptyShape);
+            DynamicObject obj = newInstance(emptyShape);
+
+            Leak leak1 = new Leak();
+            LIBRARY.setDynamicType(obj, leak1);
+            leak1.shape = obj.getShape();
+
+            Leak leak2 = new Leak();
+            LIBRARY.setDynamicType(obj, leak2);
+            leak2.shape = obj.getShape();
+
+            Leak leak3 = new Leak();
+            LIBRARY.setDynamicType(obj, leak3);
+            leak3.shape = obj.getShape();
+
+            Shape shape = obj.getShape();
+            weakShapeRefs.add(new WeakReference<>(shape));
+            strongShapeRefs.add(shape);
+        }
+
+        strongShapeRefs.clear();
+        System.gc();
+
+        for (WeakReference<Shape> fullShapeRef : weakShapeRefs) {
+            assertNull("Shape should have been garbage-collected", fullShapeRef.get());
+        }
+
+        Reference.reachabilityFence(shapesToKeepAlive);
+    }
+
     private static final class Leak {
         @SuppressWarnings("unused") Shape shape;
         @SuppressWarnings("unused") byte[] data = new byte[100];

truffle/src/com.oracle.truffle.api.object/src/com/oracle/truffle/api/object/DynamicObjectLibrary.java

@@ -317,9 +317,10 @@ public void putLong(DynamicObject object, Object key, long value) {
      * be used sparingly (with at most one constant value per property) since it could cause an
      * excessive amount of shapes to be created.
      * <p>
-     * Note: the value will be strongly referenced from the shape and should be a value type or
-     * light-weight object without any references to guest language objects in order to prevent
-     * potential memory leaks.
+     * Note: the value is strongly referenced from the shape property map. It should ideally be a
+     * value type or light-weight object without any references to guest language objects in order
+     * to prevent potential memory leaks from holding onto the Shape in inline caches. The Shape
+     * transition itself is weak, so the previous shapes will not hold strongly on the value.
      *
      * <h3>Usage example:</h3>
      *
@@ -352,9 +353,11 @@ public void putLong(DynamicObject object, Object key, long value) {
      * Sets the object's dynamic type identifier. What this type represents is completely up to the
      * language. For example, it could be a guest-language class.
      *
-     * The type object is strongly referenced from the shape. It is important that this be a
-     * singleton or light-weight object without any references to guest language objects in order to
-     * keep the memory footprint low and prevent potential memory leaks.
+     * The type object is strongly referenced from the shape. It should ideally be a singleton or
+     * light-weight object without any references to guest language objects in order to keep the
+     * memory footprint low and prevent potential memory leaks from holding onto the Shape in inline
+     * caches. The Shape transition itself is weak, so the previous shapes will not hold strongly on
+     * the type object.
      *
      * Type objects are always compared by object identity, never {@code equals}.
      *

truffle/src/com.oracle.truffle.api.object/src/com/oracle/truffle/api/object/ShapeImpl.java

@@ -384,7 +384,7 @@ private static Object newTransitionMap(Transition firstTransition, ShapeImpl fir
     }
 
     private static ShapeImpl addToTransitionMap(Transition transition, ShapeImpl successor, TransitionMap<Transition, ShapeImpl> map) {
-        if (transition.hasConstantLocation()) {
+        if (transition.isWeak()) {
             return map.putWeakKeyIfAbsent(transition, successor);
         } else {
             return map.putIfAbsent(transition, successor);
@@ -416,7 +416,7 @@ private static boolean isTransitionMap(Object trans) {
     private static Object newSingleEntry(Transition transition, ShapeImpl successor) {
         transitionSingleEntriesCreated.inc();
         Object key = transition;
-        if (transition.hasConstantLocation()) {
+        if (transition.isWeak()) {
             key = new WeakKey<>(transition);
         }
         return new StrongKeyWeakValueEntry<>(key, successor);

truffle/src/com.oracle.truffle.api.object/src/com/oracle/truffle/api/object/Transition.java

@@ -69,7 +69,7 @@ public boolean equals(Object obj) {
 
     public abstract boolean isDirect();
 
-    protected boolean hasConstantLocation() {
+    protected boolean isWeak() {
         return false;
     }
 
@@ -127,7 +127,7 @@ public int getPropertyFlags() {
         }
 
         @Override
-        protected boolean hasConstantLocation() {
+        protected boolean isWeak() {
             return getProperty().getLocation().isConstant();
         }
     }
@@ -244,6 +244,11 @@ public boolean isDirect() {
             return true;
         }
 
+        @Override
+        protected boolean isWeak() {
+            return true;
+        }
+
         @Override
         public String toString() {
             return String.format("objectType(%s)", getObjectType());
@@ -296,7 +301,7 @@ public String toString() {
         }
 
         @Override
-        protected boolean hasConstantLocation() {
+        protected boolean isWeak() {
             return getPropertyBefore().getLocation().isConstant() || getPropertyAfter().getLocation().isConstant();
         }
     }