Skip to content

Commit

Permalink
Issue 45377: Poor performance determining required modules (#3339)
Browse files Browse the repository at this point in the history
  • Loading branch information
@labkey-jeckels
labkey-jeckels committed May 11, 2022
1 parent 6760279 commit 6c801ce
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 38 deletions.
85 changes: 68 additions & 17 deletions api/src/org/labkey/api/data/Container.java
View file
Expand Up @@ -25,8 +25,11 @@
import org.jetbrains.annotations.Nullable;
import org.json.JSONArray;
import org.json.JSONObject;
import org.labkey.api.Constants;
import org.labkey.api.action.SpringActionController;
import org.labkey.api.admin.FolderExportContext;
import org.labkey.api.cache.BlockingCache;
import org.labkey.api.cache.CacheManager;
import org.labkey.api.collections.Sets;
import org.labkey.api.data.PropertyManager.PropertyMap;
import org.labkey.api.module.FolderType;
Expand Down Expand Up @@ -70,6 +73,7 @@
import org.labkey.api.webdav.WebdavService;
import org.springframework.validation.BindException;

import java.beans.PropertyChangeEvent;
import java.io.File;
import java.io.Serializable;
import java.lang.ref.WeakReference;
Expand Down Expand Up @@ -127,6 +131,68 @@ public class Container implements Serializable, Comparable<Container>, Securable
private LockState _lockState = null;
private LocalDate _expirationDate = null;

private final static BlockingCache<GUID, Set<Module>> REQUIRED_MODULES_CACHE = new BlockingCache<>(
CacheManager.getCache(
Constants.getMaxContainers(),
CacheManager.DAY,
"Required modules per containers"),
(key, argument) -> {
if (!(argument instanceof Container c))
{
throw new IllegalStateException("Expected usage pattern is to include the container instance as the argument. Key: " + key);
}
Set<Module> requiredModules = new HashSet<>(c.getRequiredModulesForFolderType(c.getFolderType()));
requiredModules.add(ModuleLoader.getInstance().getModule("API"));
requiredModules.add(ModuleLoader.getInstance().getModule("Internal"));

for (Container child: c.getChildren())
{
if (child.isWorkbook())
{
requiredModules.addAll(c.getRequiredModulesForFolderType(child.getFolderType()));
}
}

return Collections.unmodifiableSet(requiredModules);
});

static
{
// Clear the required modules cache on any change to the container tree or container properties
ContainerManager.addContainerListener(new ContainerManager.ContainerListener()
{
@Override
public void containerCreated(Container c, User user)
{
REQUIRED_MODULES_CACHE.clear();
}

@Override
public void containerDeleted(Container c, User user)
{
REQUIRED_MODULES_CACHE.clear();
}

@Override
public void containerMoved(Container c, Container oldParent, User user)
{
REQUIRED_MODULES_CACHE.clear();
}

@Override
public @NotNull Collection<String> canMove(Container c, Container newParent, User user)
{
return Collections.emptyList();
}

@Override
public void propertyChange(PropertyChangeEvent evt)
{
REQUIRED_MODULES_CACHE.clear();
}
});
}

// Might add others in the future (e.g., ReadOnly)
public enum LockState
{
Expand Down Expand Up @@ -951,19 +1017,7 @@ public void setFolderType(FolderType folderType, User user, BindException errors

public Set<Module> getRequiredModules()
{
Set<Module> requiredModules = new HashSet<>(getRequiredModulesForFolderType(getFolderType()));
requiredModules.add(ModuleLoader.getInstance().getModule("API"));
requiredModules.add(ModuleLoader.getInstance().getModule("Internal"));

for (Container child: getChildren())
{
if (child.isWorkbook())
{
requiredModules.addAll(getRequiredModulesForFolderType(child.getFolderType()));
}
}

return requiredModules;
return REQUIRED_MODULES_CACHE.get(getEntityId(), this);
}

public Set<Module> getRequiredModulesForFolderType(FolderType folderType)
Expand Down Expand Up @@ -1572,10 +1626,7 @@ public List<FolderTab> getDeletedTabFolders(String newFolderType)

public boolean hasEnableRestrictedModules(@Nullable User user)
{
boolean userHasEnableRestrictedModules = false;
if (null != user && hasPermission(user, EnableRestrictedModules.class))
userHasEnableRestrictedModules = true;
return userHasEnableRestrictedModules;
return null != user && hasPermission(user, EnableRestrictedModules.class);
}

public boolean hasRestrictedActiveModule(Set<Module> activeModules)
Expand Down
28 changes: 7 additions & 21 deletions api/src/org/labkey/api/data/ContainerManager.java
View file
Expand Up @@ -164,6 +164,7 @@ public enum Property
Policy,
/** The default or active set of modules in the container has changed */
Modules,
FolderType,
WebRoot,
AttachmentDirectory,
PipelineRoot,
Expand Down Expand Up @@ -371,21 +372,11 @@ public static Container createContainerFromTemplate(Container parent, String nam
return c;
}

public static void refreshFolderType(Container c, User user, BindException errors)
{
setFolderType(c, c.getFolderType(), user, errors, true);
}

public static void setFolderType(Container c, FolderType folderType, User user, BindException errors)
{
setFolderType(c, folderType, user, errors, false);
}

private static void setFolderType(Container c, FolderType folderType, User user, BindException errors, boolean forceSet)
{
FolderType oldType = c.getFolderType();

if (!forceSet && folderType.equals(oldType))
if (folderType.equals(oldType))
return;

List<String> errorStrings = new ArrayList<>();
Expand Down Expand Up @@ -437,14 +428,15 @@ private static void setFolderType(Container c, FolderType folderType, User user,

if (c.isContainerTab())
{
Boolean containerTabTypeOverridden = false;
boolean containerTabTypeOverridden = false;
FolderTab tab = c.getParent().getFolderType().findTab(c.getName());
if (null != tab && !folderType.equals(tab.getFolderType()))
containerTabTypeOverridden = true;
props.put(FOLDER_TYPE_PROPERTY_TABTYPE_OVERRIDDEN, containerTabTypeOverridden.toString());
props.put(FOLDER_TYPE_PROPERTY_TABTYPE_OVERRIDDEN, Boolean.toString(containerTabTypeOverridden));
}
props.save();

ContainerManager.notifyContainerChange(c.getId(), Property.FolderType, user);
folderType.configureContainer(c, user); // Configure new only after folder type has been changed

// TODO: Not needed? I don't think we've changed the container's state.
Expand Down Expand Up @@ -1865,7 +1857,6 @@ private static void _clearChildrenFromCache(Container c)

private static void _removeFromCache(Container c)
{
Container project = c.getProject();
Container parent = c.getParent();

CACHE.remove(CONTAINER_PREFIX + toString(c));
Expand Down Expand Up @@ -2001,11 +1992,6 @@ public static MultiValuedMap<Container, Container> getContainerTree(Container ro
MultiValuedMap<Container, Container> mm = new ArrayListValuedHashMap<>();
mm.put(null, root);
addChildren(root, mmIds, mm);
for (Container key : mm.keySet())
{
List<Container> siblings = new ArrayList<>(mm.get(key));
Collections.sort(siblings);
}
return mm;
}

Expand Down Expand Up @@ -2514,7 +2500,7 @@ public void testImproperFolderNamesBlocked()
{
assertTrue(delete(c, TestContext.get().getUser()));
}
catch(Exception e){}
catch(Exception ignored){}
fail("Should have thrown illegal argument when trying to create container with name: " + name);
}
catch(IllegalArgumentException e)
Expand Down Expand Up @@ -2805,7 +2791,7 @@ public ArrayList<Container> handleArrayList(ResultSet rs) throws SQLException
public Container[] handleArray(ResultSet rs) throws SQLException
{
ArrayList<Container> list = handleArrayList(rs);
return list.toArray(new Container[list.size()]);
return list.toArray(new Container[0]);
}
}

Expand Down

0 comments on commit 6c801ce

Please sign in to comment.