Skip to content

Issue 51702: Creating new workbooks/experiments is slow #6076

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

Merged
labkey-jeckels merged 2 commits into from
Nov 22, 2024
Merged

Issue 51702: Creating new workbooks/experiments is slow #6076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3275ee
Issue 51702: Creating new workbooks/experiments is slow
labkey-jeckels Nov 20, 2024
6fb4bee
Merge branch 'release24.11-SNAPSHOT' into 24.11_fb_51702_workbookCrea…
labkey-jeckels Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions api/src/org/labkey/api/cache/CacheWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.jetbrains.annotations.Nullable;
import org.labkey.api.mbean.CacheMXBean;
import org.labkey.api.util.Filter;
import org.labkey.api.util.HeartBeat;
import org.labkey.api.view.ViewServlet;

import javax.management.DynamicMBean;
Expand All @@ -44,6 +45,11 @@ class CacheWrapper<K, V> implements TrackingCache<K, V>, CacheMXBean
private final @Nullable StackTraceElement[] _stackTrace;
private final V _nullMarker = (V)NULL_MARKER;

// Issue 51702. Calculating the size of large caches can be expensive. It's OK to be a little stale or
// miss a brief spike by only updating the max size stat every so often
private boolean _maxSizeDirty;
private long _maxSizeNextUpdate;

CacheWrapper(@NotNull SimpleCache<K, V> cache, @NotNull String debugName, @Nullable Stats stats, @Nullable StackTraceElement[] stackTrace)
{
_cache = cache;
Expand Down Expand Up @@ -212,6 +218,7 @@ public String getDebugName()
@Override
public Stats getStats()
{
updateMaxSize();
return _stats;
}

Expand Down Expand Up @@ -248,6 +255,7 @@ public String toString()

private V trackGet(V value)
{
updateMaxSizeIfStale();
_stats.gets.incrementAndGet();

if (value == null)
Expand All @@ -263,25 +271,47 @@ private void trackPut(V value)

_stats.puts.incrementAndGet();

_maxSizeDirty = true;
updateMaxSizeIfStale();
}

/** @return current size */
private int updateMaxSize()
{
_maxSizeDirty = false;
// Don't update the max size more than once every five seconds
_maxSizeNextUpdate = HeartBeat.currentTimeMillis() + 5_000;
long maxSize = _stats.max_size.get();
long currentSize = size();
int currentSize = size();
if (currentSize > maxSize)
_stats.max_size.compareAndSet(maxSize, currentSize);
return currentSize;
}

private void updateMaxSizeIfStale()
{
if (_maxSizeDirty && _maxSizeNextUpdate <= HeartBeat.currentTimeMillis())
{
updateMaxSize();
}
}

private void trackRemove()
{
updateMaxSizeIfStale();
_stats.removes.incrementAndGet();
}

private int trackRemoves(int removes)
{
updateMaxSizeIfStale();
_stats.removes.addAndGet(removes);
return removes;
}

private void trackClear()
{
updateMaxSizeIfStale();
_stats.clears.incrementAndGet();
}

Expand All @@ -296,7 +326,8 @@ public SimpleCache<K, V> getWrappedCache()
@Override
public int getSize()
{
return size();
// Since we're being asked for the current size, go ahead and update the max size stat too
return updateMaxSize();
}

@Override
Expand Down
40 changes: 24 additions & 16 deletions api/src/org/labkey/api/data/ContainerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ public static Container createContainer(Container parent, String name, @Nullable
SecurityManager.setAdminOnlyPermissions(c, savePolicyUser);
}

_removeFromCache(c); // seems odd, but it removes c.getProject() which clears other things from the cache
_removeFromCache(c, true); // seems odd, but it removes c.getProject() which clears other things from the cache

// Initialize the list of active modules in the Container
c.getActiveModules(true, true, user);
Expand All @@ -382,7 +382,7 @@ public static Container createContainer(Container parent, String name, @Nullable
// NOTE parent caches some info about children (e.g. hasWorkbookChildren)
// since mutating cached objects is frowned upon, just uncache parent
// CONSIDER: we could perhaps only uncache if the child is a workbook, but I think this reasonable
_removeFromCache(parent);
_removeFromCache(parent, true);

if (null != configureContainer)
configureContainer.accept(c);
Expand Down Expand Up @@ -511,7 +511,7 @@ public static void setFolderType(Container c, FolderType folderType, User 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.
_removeFromCache(c);
_removeFromCache(c, false);
}
else
{
Expand Down Expand Up @@ -810,7 +810,7 @@ public static void updateDescription(Container container, String description, Us
new SqlExecutor(CORE.getSchema()).execute(sql, description, container.getRowId());

String oldValue = container.getDescription();
_removeFromCache(container);
_removeFromCache(container, false);
container = getForRowId(container.getRowId());
ContainerPropertyChangeEvent evt = new ContainerPropertyChangeEvent(container, user, Property.Description, oldValue, description);
firePropertyChangeEvent(evt);
Expand All @@ -825,7 +825,7 @@ public static void updateSearchable(Container container, boolean searchable, Use
sql.append(" SET Searchable=? WHERE RowID=?");
new SqlExecutor(CORE.getSchema()).execute(sql, searchable, container.getRowId());

_removeFromCache(container);
_removeFromCache(container, false);
}

public static void updateLockState(Container container, LockState lockState, @NotNull Runnable auditRunnable)
Expand All @@ -837,7 +837,7 @@ public static void updateLockState(Container container, LockState lockState, @No
sql.append(" SET LockState = ?, ExpirationDate = NULL WHERE RowID = ?");
new SqlExecutor(CORE.getSchema()).execute(sql, lockState, container.getRowId());

_removeFromCache(container);
_removeFromCache(container, false);

auditRunnable.run();
}
Expand Down Expand Up @@ -920,7 +920,7 @@ public static void updateExpirationDate(Container container, LocalDate expiratio
// Note: jTDS doesn't support LocalDate, so convert to java.sql.Date
Copy link
Contributor

@labkey-adam labkey-adam Nov 20, 2024

Choose a reason for hiding this comment

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

Just FYI: SQL Server JDBC supports LocalDate. That's for another day, of course.

new SqlExecutor(CORE.getSchema()).execute(sql, java.sql.Date.valueOf(expirationDate), container.getRowId());

_removeFromCache(container);
_removeFromCache(container, false);

auditRunnable.run();
}
Expand All @@ -934,7 +934,7 @@ public static void updateType(Container container, String newType, User user)
sql.append(" SET Type=? WHERE RowID=?");
new SqlExecutor(CORE.getSchema()).execute(sql, newType, container.getRowId());

_removeFromCache(container);
_removeFromCache(container, false);
}

public static void updateTitle(Container container, String title, User user)
Expand All @@ -949,7 +949,7 @@ public static void updateTitle(Container container, String title, User user)
sql.append(" SET Title=? WHERE RowID=?");
new SqlExecutor(CORE.getSchema()).execute(sql, title, container.getRowId());

_removeFromCache(container);
_removeFromCache(container, false);
String oldValue = container.getTitle();
container = getForRowId(container.getRowId());
ContainerPropertyChangeEvent evt = new ContainerPropertyChangeEvent(container, user, Property.Title, oldValue, title);
Expand All @@ -958,7 +958,7 @@ public static void updateTitle(Container container, String title, User user)

public static void uncache(Container c)
{
_removeFromCache(c);
_removeFromCache(c, true);
}

public static final String SHARED_CONTAINER_PATH = "/Shared";
Expand Down Expand Up @@ -1910,7 +1910,7 @@ private static boolean delete(final Container c, User user, @Nullable String com

if (sel.exists())
{
_removeFromCache(c);
_removeFromCache(c, true);
return false;
}

Expand Down Expand Up @@ -1948,7 +1948,7 @@ private static boolean delete(final Container c, User user, @Nullable String com
DATABASE_QUERY_LOCK.lock();
try
{
_removeFromCache(c);
_removeFromCache(c, true);
}
finally
{
Expand Down Expand Up @@ -2097,15 +2097,23 @@ private static void _clearChildrenFromCache(Container c)
navTreeManageUncache(c);
}

private static void _removeFromCache(Container c)
/** @param hierarchyChange whether the shape of the container tree has changed */
private static void _removeFromCache(Container c, boolean hierarchyChange)
{
CACHE.remove(toString(c));
CACHE.remove(c.getId());

// blow away the children caches
CACHE_CHILDREN.clear();
// Blow away the children caches, which can be outdated even if the hierarchy hasn't changed
// For example, changing the folder type or enabled modules in a parent can impact child workbooks
CACHE_ALL_CHILDREN.clear();

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();
}

navTreeManageUncache(c);
}

Expand Down Expand Up @@ -2146,7 +2154,7 @@ public static void notifyContainerChange(String id, Property prop, @Nullable Use
Container c = getForId(id);
if (null != c)
{
_removeFromCache(c);
_removeFromCache(c, false);
c = getForId(id); // load a fresh container since the original might be stale.
if (null != c)
{
Expand Down