Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 45377: Poor performance determining required modules #3339

Merged
merged 4 commits into from May 11, 2022
Merged

Issue 45377: Poor performance determining required modules #3339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b347c41
Issue 45377: Poor performance determining required/active modules in …
labkey-jeckels May 10, 2022
6c10318
Code review suggestions
labkey-jeckels May 10, 2022
66d94a2
Fire change event to clear cache before configuring the container
labkey-jeckels May 11, 2022
f7e2885
Merge branch 'develop' into fb_45377_requiredModulesCache
labkey-jeckels May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
85 changes: 68 additions & 17 deletions api/src/org/labkey/api/data/Container.java
View file
Open in desktop
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
Open in desktop
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't care about the sort any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm misreading the code, this never did anything. We create a copy of the list, sort it, and then throw it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, agreed... I hadn't looked very closely at first

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