Reduce per-container memory: cache by GUID instead of Container, share JSON keys

api/src/org/labkey/api/ApiModule.java

@@ -39,6 +39,7 @@
 import org.labkey.api.cache.BlockingCache;
 import org.labkey.api.cache.ContainerKeySimpleCache;
 import org.labkey.api.cache.SimpleKeyMappingCache;
+import org.labkey.api.cache.CacheManager;
 import org.labkey.api.collections.ArrayListMap;
 import org.labkey.api.collections.CaseInsensitiveHashMap;
 import org.labkey.api.collections.CaseInsensitiveHashSet;
@@ -179,6 +180,7 @@
 import org.labkey.api.util.JobRunner;
 import org.labkey.api.util.JsonUtil;
 import org.labkey.api.util.JspTestCase;
+import org.labkey.api.util.KeySharingJSONTokener;
 import org.labkey.api.util.MailHelper;
 import org.labkey.api.util.MemTracker;
 import org.labkey.api.util.MimeMap;
@@ -427,6 +429,7 @@ public void registerServlets(ServletContext servletCtx)
             BaseServerProperties.TestCase.class,
             BooleanFormat.TestCase.class,
             BuilderObjectFactory.TestCase.class,
+            CacheManager.TestCase.class,
             CachingDataIterator.ScrollTestCase.class,
             CaseInsensitiveHashMap.TestCase.class,
             CaseInsensitiveHashSet.TestCase.class,
@@ -470,6 +473,7 @@ public void registerServlets(ServletContext servletCtx)
             JobRunner.TestCase.class,
             JsonTest.class,
             JsonUtil.TestCase.class,
+            KeySharingJSONTokener.TestCase.class,
             LimitedUser.TestCase.class,
             MarkableIterator.TestCase.class,
             MaterializedQueryHelper.TestCase.class,

api/src/org/labkey/api/action/BaseApiAction.java

@@ -38,6 +38,7 @@
 import org.labkey.api.util.ExceptionUtil;
 import org.labkey.api.util.HttpUtil;
 import org.labkey.api.util.JsonUtil;
+import org.labkey.api.util.KeySharingJSONTokener;
 import org.labkey.api.util.MimeMap;
 import org.labkey.api.util.StringUtilsLabKey;
 import org.labkey.api.view.BadRequestException;
@@ -500,7 +501,7 @@ protected long getMaximumJsonInputLength()
 
         try (Reader reader = openRequestReader())
         {
-            JSONTokener tokener = new JSONTokener(reader);
+            JSONTokener tokener = new KeySharingJSONTokener(reader);
             return tokener.more() ? new JSONObject(tokener) : null;
         }
     }

api/src/org/labkey/api/cache/CacheManager.java

@@ -19,6 +19,8 @@
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
+import org.junit.Assert;
+import org.junit.Test;
 import org.labkey.api.cache.ehcache.EhCacheProvider;
 import org.labkey.api.collections.CollectionUtils;
 import org.labkey.api.data.Container;
@@ -309,4 +311,23 @@ public interface Sealable
     {
         boolean isSealed();
     }
+
+    public static class TestCase extends Assert
+    {
+        @Test
+        public void testContainerKeyRejected()
+        {
+            try (Cache<Object, String> cache = getTemporaryCache(10, MINUTE, "Container key test", null))
+            {
+                Container c = new Container(null, "", GUID.makeGUID(), 1, 1, null, 0, false);
+                assertThrows(IllegalArgumentException.class, () -> cache.put(c, "value"));
+                assertThrows(IllegalArgumentException.class, () -> cache.put(c, "value", MINUTE));
+                assertThrows(IllegalArgumentException.class, () -> cache.get(c, null, (key, argument) -> "loaded"));
+                assertTrue(cache.getKeys().isEmpty());
+
+                cache.put(c.getEntityId(), "value");
+                assertEquals("value", cache.get(c.getEntityId()));
+            }
+        }
+    }
 }

api/src/org/labkey/api/cache/ContainerKeySimpleCache.java

@@ -16,7 +16,8 @@ public class ContainerKeySimpleCache<V> extends SimpleKeyMappingCache<Container,
 {
     public ContainerKeySimpleCache(@NotNull SimpleCache<GUID, V> delegate)
     {
-        super(delegate, Container::getEntityId, ContainerManager::getForId);
+        // Some caches use a null key for cross-container data (e.g., ExternalSchemaDefCache)
+        super(delegate, c -> null == c ? null : c.getEntityId(), ContainerManager::getForId);
     }
 
     public static class TestCase extends Assert

api/src/org/labkey/api/cache/ehcache/EhCacheProvider.java

@@ -97,7 +97,7 @@ public <K, V> SimpleCache<K, V> getSimpleCache(String debugName, int limit, long
         if (temporary)
             MemTracker.getInstance().put(ehCache);
 
-        return new EhSimpleCache<>(ehCache);
+        return new EhSimpleCache<>(ehCache, debugName);
     }
 
     void closeCache(Cache cache)

api/src/org/labkey/api/cache/ehcache/EhSimpleCache.java

@@ -23,6 +23,7 @@
 import org.jetbrains.annotations.Nullable;
 import org.labkey.api.cache.CacheType;
 import org.labkey.api.cache.SimpleCache;
+import org.labkey.api.data.Container;
 import org.labkey.api.util.IntegerUtils;
 
 import java.util.List;
@@ -33,22 +34,33 @@ class EhSimpleCache<K, V> implements SimpleCache<K, V>
     private static final Logger LOG = LogManager.getLogger(EhSimpleCache.class);
 
     private final Cache _cache;
+    private final String _debugName;
 
-    EhSimpleCache(Cache cache)
+    EhSimpleCache(Cache cache, String debugName)
     {
         _cache = cache;
+        _debugName = debugName;
+    }
+
+    private void validateKey(K key)
+    {
+        // Ehcache keeps the first key object stored for an entry, so a Container key pins a stale copy
+        if (key instanceof Container)
+            throw new IllegalArgumentException("Container used as a key in cache \"" + _debugName + "\". Pass Container.class (keyClass) to the Cache factory method to allow this.");
     }
 
     @Override
     public void put(@NotNull K key, V value)
     {
+        validateKey(key);
         Element element = new Element(key, value);
         _cache.put(element);
     }
 
     @Override
     public void put(@NotNull K key, V value, long timeToLive)
     {
+        validateKey(key);
         Element element = new Element(key, value);
         element.setTimeToLive(IntegerUtils.asInteger(timeToLive / 1000)); // Convert from ms to sec
         _cache.put(element);

api/src/org/labkey/api/data/Container.java

@@ -141,45 +141,41 @@ public class Container implements Serializable, Comparable<Container>, Securable
     private Long _fileRootSize = null;
     private LocalDateTime _fileRootLastCrawled = null;
 
-    private final static BlockingCache<GUID, Set<Module>> REQUIRED_MODULES_CACHE = new BlockingCache<>(
-        CacheManager.getCache(
-            Constants.getMaxContainers(),
-            CacheManager.DAY,
-            "Required modules per container"),
-        (key, argument) -> {
-            if (!(argument instanceof Container c))
-            {
-                throw new IllegalStateException("Expected usage pattern is to include the container instance as the argument. Key: " + key);
-            }
+    private final static BlockingCache<Container, Set<Module>> REQUIRED_MODULES_CACHE = DatabaseCache.get(
+        Container.class,
+        CoreSchema.getInstance().getScope(),
+        Constants.getMaxContainers(),
+        CacheManager.DAY,
+        "Required modules per container",
+        (c, _) -> {
             Set<Module> requiredModules = new HashSet<>(c.getRequiredModulesForFolderType(c.getFolderType()));
             requiredModules.add(ModuleLoader.getInstance().getModule("API"));
 
-            for (Container child: c.getChildren())
+            if (c.canHaveChildren())
             {
-                if (child.isWorkbook())
-                {
-                    requiredModules.addAll(c.getRequiredModulesForFolderType(child.getFolderType()));
-                }
+                for (FolderType workbookFolderType : ContainerManager.getWorkbookChildFolderTypes(c))
+                    requiredModules.addAll(c.getRequiredModulesForFolderType(workbookFolderType));
             }
 
             return Collections.unmodifiableSet(requiredModules);
         });
 
     static
     {
-        // Clear the required modules cache on any change to the container tree or container properties
+        // An entry depends on its container's folder type, its workbook children's, and (via module path patterns) its
+        // path. Uncache just the container and its parent, except renames and moves, which change descendants' paths.
         ContainerManager.addContainerListener(new ContainerManager.ContainerListener()
         {
             @Override
             public void containerCreated(Container c, User user)
             {
-                REQUIRED_MODULES_CACHE.clear();
+                uncacheRequiredModules(c, c.getParent());
             }
 
             @Override
             public void containerDeleted(Container c, User user)
             {
-                REQUIRED_MODULES_CACHE.clear();
+                uncacheRequiredModules(c, c.getParent());
             }
 
             @Override
@@ -191,11 +187,24 @@ public void containerMoved(Container c, Container oldParent, User user)
             @Override
             public void propertyChange(PropertyChangeEvent evt)
             {
-                REQUIRED_MODULES_CACHE.clear();
+                if (evt instanceof ContainerManager.ContainerPropertyChangeEvent cpce && null != cpce.container
+                    && cpce.property != ContainerManager.Property.Name && cpce.property != ContainerManager.Property.Parent)
+                    uncacheRequiredModules(cpce.container, cpce.container.getParent());
+                else
+                    REQUIRED_MODULES_CACHE.clear();
             }
         });
     }
 
+    private static void uncacheRequiredModules(Container... containers)
+    {
+        for (Container c : containers)
+        {
+            if (null != c)
+                REQUIRED_MODULES_CACHE.remove(c);
+        }
+    }
+
     // Might add others in the future (e.g., ReadOnly)
     public enum LockState
     {
@@ -1007,7 +1016,7 @@ public void setFolderType(FolderType folderType, User user, BindException errors
 
     public Set<Module> getRequiredModules()
     {
-        return REQUIRED_MODULES_CACHE.get(getEntityId(), this);
+        return REQUIRED_MODULES_CACHE.get(this);
     }
 
     public Set<Module> getRequiredModulesForFolderType(FolderType folderType)

api/src/org/labkey/api/data/ContainerManager.java

@@ -648,22 +648,44 @@ public static String getFolderTypeName(Container c)
     }
 
 
+    /** Left-joins each container (alias "c") to its stored folder type name, exposed as pe.Value; null when none is stored */
+    private static SQLFragment getFolderTypeNameJoin()
+    {
+        return new SQLFragment(" LEFT OUTER JOIN ").append(PropertySchema.getInstance().getTableInfoPropertyEntries(), "pe")
+            .append(" ON pe.ObjectId = c.EntityId AND pe.Category = ? AND pe.Name = ? AND pe.UserId = ?")
+            .add(FOLDER_TYPE_PROPERTY_SET_NAME).add(FOLDER_TYPE_PROPERTY_NAME).add(PropertyManager.SHARED_USER.getUserId());
+    }
+
+    /** Same resolution as getFolderType(), minus its bad-name logging */
+    private static @NotNull FolderType resolveFolderType(@Nullable String name)
+    {
+        FolderType folderType = null == name ? null : FolderTypeManager.get().getFolderType(name);
+        return null == folderType ? FolderType.NONE : folderType;
+    }
+
     @NotNull
-    public static Map<String, Integer> getFolderTypeNameContainerCounts(Container root)
+    public static Map<String, Integer> getFolderTypeNameContainerCounts()
     {
+        SQLFragment sql = new SQLFragment("SELECT pe.Value, COUNT(*) FROM ").append(CORE.getTableInfoContainers(), "c")
+            .append(getFolderTypeNameJoin())
+            .append(" GROUP BY pe.Value");
         Map<String, Integer> nameCounts = new TreeMap<>();
-        for (Container c : getAllChildren(root))
-        {
-            Integer count = nameCounts.get(c.getFolderType().getName());
-            if (null == count)
-            {
-                count = Integer.valueOf(0);
-            }
-            nameCounts.put(c.getFolderType().getName(), ++count);
-        }
+        new SqlSelector(CORE.getSchema(), sql).forEach(rs -> nameCounts.merge(resolveFolderType(rs.getString(1)).getName(), rs.getInt(2), Integer::sum));
         return nameCounts;
     }
 
+    /** Distinct folder types of the parent's workbook children, without loading each workbook's property map */
+    @NotNull
+    public static Set<FolderType> getWorkbookChildFolderTypes(Container parent)
+    {
+        SQLFragment sql = new SQLFragment("SELECT DISTINCT pe.Value FROM ").append(CORE.getTableInfoContainers(), "c")
+            .append(getFolderTypeNameJoin())
+            .append(" WHERE c.Parent = ? AND c.Type = ?").add(parent.getEntityId()).add(WorkbookContainerType.NAME);
+        return new SqlSelector(CORE.getSchema(), sql).stream(String.class)
+            .map(ContainerManager::resolveFolderType)
+            .collect(Collectors.toSet());
+    }
+
     @NotNull
     public static Map<String, Integer> getProductFoldersMetrics(@NotNull FolderType folderType)
     {
@@ -994,7 +1016,7 @@ public static void updateTitle(Container container, String title, User user)
 
     public static void uncache(Container c)
     {
-        _removeFromCache(c, true);
+        _removeFromCache(c, false);
     }
 
     public static final String SHARED_CONTAINER_PATH = "/Shared";
@@ -1122,9 +1144,11 @@ private static Map<String, Container> getChildrenMap(Container parent)
                 new SqlSelector(CORE.getSchema(),
                         "SELECT * FROM " + CORE.getTableInfoContainers() + " WHERE Parent = ? ORDER BY SortOrder, LOWER(Name)",
                         parent.getId()).forEach(rs -> {
-                    Container c = factory.handle(rs);
-                    ids.add(c.getEntityId());
-                    _addToCache(c);
+                    GUID id = new GUID(rs.getString("EntityId"));
+                    // Reuse the cached instance; a fresh copy would replace it while Container-keyed caches keep pinning the old one
+                    Container c = CACHE_ENTITY_ID.get(id);
+                    _addToCache(null != c ? c : factory.handle(rs));
+                    ids.add(id);
                 });
 
                 childIds = Collections.unmodifiableList(ids);
@@ -2150,9 +2174,11 @@ private static void _removeFromCache(Container c, boolean hierarchyChange)
 
         if (hierarchyChange)
         {
-            // This is strictly keeping track of the parent/child relationships themselves so it only needs to be
-            // cleared when the tree changes
-            CACHE_CHILDREN.clear();
+            // Creating or deleting c only changes its own and its parent's child lists; moves and renames use clearCache()
+            CACHE_CHILDREN.remove(c.getEntityId());
+            Container parent = c.getParent();
+            if (null != parent)
+                CACHE_CHILDREN.remove(parent.getEntityId());
         }
 
         navTreeManageUncache(c);
@@ -3069,6 +3095,32 @@ private static void createContainers(MultiValuedMap<String, String> mm, String n
             }
         }
 
+        @Test
+        public void testFolderTypeQueries()
+        {
+            User user = TestContext.get().getUser();
+            Container parent = createContainer(_testRoot, "folderTypes", user);
+            Container untyped = createContainer(parent, null, null, null, WorkbookContainerType.NAME, user);
+            Container typed = createContainer(parent, null, null, null, WorkbookContainerType.NAME, user);
+            FolderType collaboration = FolderTypeManager.get().getFolderType("Collaboration");
+            assertNotNull(collaboration);
+
+            Set<Module> collaborationModules = parent.getRequiredModulesForFolderType(collaboration);
+            assertFalse(collaborationModules.isEmpty());
+            assertFalse(parent.getRequiredModules().containsAll(collaborationModules));
+
+            typed.setFolderType(collaboration, user);
+
+            // A workbook's folder type change must uncache its parent's entry
+            assertTrue(parent.getRequiredModules().containsAll(collaborationModules));
+            assertEquals(Set.of(FolderType.NONE, collaboration), getWorkbookChildFolderTypes(parent));
+            assertEquals(FolderType.NONE, untyped.getFolderType());
+
+            Map<String, Integer> expected = new TreeMap<>();
+            getAllChildren(getRoot()).forEach(c -> expected.merge(c.getFolderType().getName(), 1, Integer::sum));
+            assertEquals(expected, getFolderTypeNameContainerCounts());
+        }
+
         private static void cleanUpChildren(MultiValuedMap<String, String> mm, String name, Container parent)
         {
             Collection<String> nodes = mm.get(name);

api/src/org/labkey/api/data/MvUtil.java

@@ -35,7 +35,7 @@
  */
 public class MvUtil
 {
-    private static final Cache<Container, Map<String, String>> CACHE = CacheManager.getBlockingCache(Constants.getMaxContainers(), CacheManager.YEAR, "Missing value indicators", (c, argument) -> getFromDb(c));
+    private static final Cache<Container, Map<String, String>> CACHE = CacheManager.getBlockingCache(Container.class, Constants.getMaxContainers(), CacheManager.YEAR, "Missing value indicators", (c, argument) -> getFromDb(c));
 
     private MvUtil() {}