Refactor type check caching

We move the type check cache out of SemType to avoid unnecessary type resolutions
ballerina-platform · Oct 3, 2024 · 18d2d26 · 18d2d26
1 parent 5a76fb0
commit 18d2d26
Show file tree

Hide file tree

Showing 6 changed files with 92 additions and 43 deletions.
diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/api/types/semtype/Core.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/api/types/semtype/Core.java
@@ -295,13 +295,7 @@ public static boolean isNever(SemType t) {
     }
 
     public static boolean isSubType(Context cx, SemType t1, SemType t2) {
-        SemType.CachedResult cached = t1.cachedSubTypeRelation(t2);
-        if (cached != SemType.CachedResult.NOT_FOUND) {
-            return cached == SemType.CachedResult.TRUE;
-        }
-        boolean result = isEmpty(cx, diff(t1, t2));
-        t1.cacheSubTypeRelation(t2, result);
-        return result;
+        return isEmpty(cx, diff(t1, t2));
     }
 
     public static boolean isSubtypeSimple(SemType t1, SemType t2) {

diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/api/types/semtype/SemType.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/api/types/semtype/SemType.java
@@ -5,15 +5,11 @@
 import io.ballerina.runtime.internal.types.semtype.MutableSemType;
 import io.ballerina.runtime.internal.types.semtype.SemTypeHelper;
 
-import java.util.Map;
-import java.util.WeakHashMap;
-
 public sealed class SemType extends BasicTypeBitSet
         permits io.ballerina.runtime.internal.types.BType, ImmutableSemType {
 
     private int some;
     private SubType[] subTypeData;
-    private volatile Map<SemType, CachedResult> cachedResults;
 
     protected SemType(int all, int some, SubType[] subTypeData) {
         super(all);
@@ -42,33 +38,6 @@ public final SubType[] subTypeData() {
         return subTypeData;
     }
 
-    public final CachedResult cachedSubTypeRelation(SemType other) {
-        if (skipCache()) {
-            return CachedResult.NOT_FOUND;
-        }
-        if (cachedResults == null) {
-            synchronized (this) {
-                if (cachedResults == null) {
-                    cachedResults = new WeakHashMap<>();
-                }
-            }
-            return CachedResult.NOT_FOUND;
-        }
-        return cachedResults.getOrDefault(other, CachedResult.NOT_FOUND);
-    }
-
-    private boolean skipCache() {
-        return this.some() == 0;
-    }
-
-    public final void cacheSubTypeRelation(SemType other, boolean result) {
-        if (skipCache() || other.skipCache()) {
-            return;
-        }
-        // we always check of the result before caching so there will always be a map
-        cachedResults.put(other, result ? CachedResult.TRUE : CachedResult.FALSE);
-    }
-
     public final SubType subTypeByCode(int code) {
         if ((some() & (1 << code)) == 0) {
             return null;

diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/TypeChecker.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/TypeChecker.java
@@ -53,6 +53,7 @@
 import io.ballerina.runtime.internal.types.BType;
 import io.ballerina.runtime.internal.types.BTypeReferenceType;
 import io.ballerina.runtime.internal.types.BUnionType;
+import io.ballerina.runtime.internal.types.CacheableTypeDescriptor;
 import io.ballerina.runtime.internal.types.TypeWithShape;
 import io.ballerina.runtime.internal.values.DecimalValue;
 import io.ballerina.runtime.internal.values.HandleValue;
@@ -68,6 +69,7 @@
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
@@ -600,17 +602,36 @@ private static boolean isSubTypeWithInherentType(Context cx, Object sourceValue,
     }
 
     private static boolean isSubType(Type source, Type target) {
+        if (source instanceof CacheableTypeDescriptor sourceCacheableType &&
+                target instanceof CacheableTypeDescriptor targetCacheableType) {
+            return isSubTypeWithCache(sourceCacheableType, targetCacheableType);
+        }
         // This is really a workaround for Standard libraries that create record types that are not the "same". But
         // with the same name and expect them to be same.
-        if (source.equals(target)) {
-            return true;
-        }
+        return isSubTypeInner(source, target);
+    }
+
+    private static boolean isSubTypeInner(Type source, Type target) {
         Context cx = context();
         SemType sourceSemType = SemType.tryInto(source);
         SemType targetSemType = SemType.tryInto(target);
         return Core.isSubType(cx, sourceSemType, targetSemType);
     }
 
+    private static boolean isSubTypeWithCache(CacheableTypeDescriptor source, CacheableTypeDescriptor target) {
+        if (!source.shouldCache() || !target.shouldCache()) {
+            return isSubTypeInner(source, target);
+        }
+        Optional<Boolean> cachedResult = source.cachedTypeCheckResult(target);
+        if (cachedResult.isPresent()) {
+            assert cachedResult.get() == isSubTypeInner(source, target);
+            return cachedResult.get();
+        }
+        boolean result = isSubTypeInner(source, target);
+        source.cacheTypeCheckResult(target, result);
+        return result;
+    }
+
     private static SemType widenedType(Context cx, Object value) {
         if (value instanceof BValue bValue) {
             return bValue.widenedType();

diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/types/BArrayType.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/types/BArrayType.java
@@ -148,7 +148,7 @@ public int hashCode() {
     @Override
     public boolean equals(Object obj) {
         if (obj instanceof BArrayType other) {
-            if (other.state == ArrayState.CLOSED && this.size != other.size) {
+            if ((other.state == ArrayState.CLOSED || this.state == ArrayState.CLOSED) && this.size != other.size) {
                 return false;
             }
             return this.elementType.equals(other.elementType) && this.readonly == other.readonly;

diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/types/BType.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/types/BType.java
@@ -28,7 +28,12 @@
 import io.ballerina.runtime.internal.TypeChecker;
 import io.ballerina.runtime.internal.types.semtype.MutableSemType;
 
+import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
+import java.util.WeakHashMap;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
  * {@code BType} represents a type in Ballerina.
@@ -40,7 +45,8 @@
  *
  * @since 0.995.0
  */
-public abstract non-sealed class BType extends SemType implements Type, MutableSemType, Cloneable {
+public abstract non-sealed class BType extends SemType
+        implements Type, MutableSemType, Cloneable, CacheableTypeDescriptor {
 
     protected String typeName;
     protected Module pkg;
@@ -50,6 +56,8 @@ public abstract non-sealed class BType extends SemType implements Type, MutableS
     private Type cachedImpliedType = null;
     private volatile SemType cachedSemType = null;
     private TypeCreator.TypeMemoKey lookupKey = null;
+    private volatile Map<CacheableTypeDescriptor, Boolean> cachedResults;
+    private final ReadWriteLock cachedResultsLock = new ReentrantReadWriteLock();
 
     protected BType(String typeName, Module pkg, Class<? extends Object> valueClass) {
         this.typeName = typeName;
@@ -285,4 +293,42 @@ public BType clone() {
     public void setLookupKey(TypeCreator.TypeMemoKey lookupKey) {
         this.lookupKey = lookupKey;
     }
+
+    @Override
+    public boolean shouldCache() {
+        return true;
+    }
+
+    @Override
+    public final Optional<Boolean> cachedTypeCheckResult(CacheableTypeDescriptor other) {
+        if (other.equals(this)) {
+            return Optional.of(true);
+        }
+        if (cachedResults == null) {
+            cachedResultsLock.writeLock().lock();
+            try {
+                if (cachedResults == null) {
+                    cachedResults = new WeakHashMap<>();
+                }
+            } finally {
+                cachedResultsLock.writeLock().unlock();
+            }
+        }
+        cachedResultsLock.readLock().lock();
+        try {
+            return Optional.ofNullable(cachedResults.get(other));
+        } finally {
+            cachedResultsLock.readLock().unlock();
+        }
+    }
+
+    @Override
+    public final void cacheTypeCheckResult(CacheableTypeDescriptor other, boolean result) {
+        cachedResultsLock.writeLock().lock();
+        try {
+            cachedResults.put(other, result);
+        } finally {
+            cachedResultsLock.writeLock().unlock();
+        }
+    }
 }
diff --git a/...na-runtime/src/main/java/io/ballerina/runtime/internal/types/CacheableTypeDescriptor.java b/...na-runtime/src/main/java/io/ballerina/runtime/internal/types/CacheableTypeDescriptor.java
@@ -0,0 +1,19 @@
+package io.ballerina.runtime.internal.types;
+
+import io.ballerina.runtime.api.types.Type;
+
+import java.util.Optional;
+
+/**
+ * Represent TypeDescriptors whose type check results can be cached
+ *
+ * @since 2201.11.0
+ */
+public interface CacheableTypeDescriptor extends Type {
+
+    boolean shouldCache();
+
+    Optional<Boolean> cachedTypeCheckResult(CacheableTypeDescriptor other);
+
+    void cacheTypeCheckResult(CacheableTypeDescriptor other, boolean result);
+}