Skip to content

Optimized ObjectEntityCache for Thread Safety and Memory Management #3392

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

Open
khresth wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Optimized ObjectEntityCache for Thread Safety and Memory Management #3392

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 49 additions & 5 deletions elide-core/src/main/java/com/yahoo/elide/core/ObjectEntityCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,23 @@
import java.util.IdentityHashMap;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

/**
* Cache to store object entity.
*/
public class ObjectEntityCache {
private static final int MAX_CACHE_SIZE = 10000;
private final Map<String, Object> resourceCache;
private final Map<Object, String> uuidReverseMap;
private final ReadWriteLock lock = new ReentrantReadWriteLock();

/**
* Constructor.
*/
public ObjectEntityCache() {
resourceCache = new LinkedHashMap<>();
resourceCache = new LinkedHashMap<String, Object>(16, 0.75f, true);
uuidReverseMap = new IdentityHashMap<>();
}

Expand All @@ -33,8 +37,38 @@ public ObjectEntityCache() {
* @return the object
*/
public Object put(String type, String id, Object entity) {
uuidReverseMap.put(entity, id);
return resourceCache.put(getCacheKey(type, id), entity);
lock.writeLock().lock();
try {
uuidReverseMap.put(entity, id);

if (resourceCache.size() >= MAX_CACHE_SIZE) {
evictLRU();
}

return resourceCache.put(getCacheKey(type, id), entity);
Comment on lines +42 to +48
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The reverse mapping is added before checking cache size and evicting. If the entity being added already exists in the cache at a different position, and eviction occurs, the old entry's reverse mapping may not be properly cleaned up. Additionally, if the same entity is re-added with a different ID, this will overwrite the previous UUID mapping. Consider handling updates and eviction order more carefully to maintain mapping consistency.

Copilot uses AI. Check for mistakes.
} finally {
lock.writeLock().unlock();
}
}

private void evictLRU() {
if (resourceCache.isEmpty()) {
return;
}

String oldestKey = resourceCache.keySet().iterator().next();
resourceCache.remove(oldestKey);

uuidReverseMap.entrySet().removeIf(entry -> getCacheKeyFromUUID(entry.getValue()).equals(oldestKey));
Comment on lines +60 to +62
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The getCacheKeyFromUUID method is called for every entry in uuidReverseMap during eviction, resulting in O(n*m) complexity where n is the size of uuidReverseMap and m is the size of resourceCache. This is highly inefficient. Instead, retrieve the evicted object from resourceCache before removal and use it to directly remove from uuidReverseMap: Object evictedEntity = resourceCache.get(oldestKey); resourceCache.remove(oldestKey); uuidReverseMap.remove(evictedEntity);

Suggested change
resourceCache.remove(oldestKey);
uuidReverseMap.entrySet().removeIf(entry -> getCacheKeyFromUUID(entry.getValue()).equals(oldestKey));
Object evictedEntity = resourceCache.get(oldestKey);
resourceCache.remove(oldestKey);
uuidReverseMap.remove(evictedEntity);

Copilot uses AI. Check for mistakes.
}

private String getCacheKeyFromUUID(String uuidValue) {
for (String key : resourceCache.keySet()) {
if (key.endsWith("_" + uuidValue)) {
return key;
}
}
return null;
}
Comment on lines +65 to 72
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

The getCacheKeyFromUUID method assumes cache keys end with _<uuid>, but the actual key format from getCacheKey(type, id) is not visible in this diff. This assumption may be incorrect and could cause the reverse mapping cleanup to fail silently. Additionally, iterating through all cache keys is inefficient. Consider storing the type-id pair in uuidReverseMap alongside the UUID, or maintaining a separate map from entity objects to cache keys.

Copilot uses AI. Check for mistakes.

/**
Expand All @@ -45,7 +79,12 @@ public Object put(String type, String id, Object entity) {
* @return object
*/
public Object get(String type, String id) {
return resourceCache.get(getCacheKey(type, id));
lock.readLock().lock();
try {
return resourceCache.get(getCacheKey(type, id));
} finally {
lock.readLock().unlock();
Comment on lines +82 to +86
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

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

Using a read lock with an access-ordered LinkedHashMap (created with accessOrder=true on line 27) is unsafe. The get() operation on an access-ordered map modifies the internal structure by moving the accessed entry to the end, which is a write operation. This will cause inconsistent state when multiple threads read concurrently. Either use a write lock for get() operations or switch to insertion-ordered LinkedHashMap (set third parameter to false).

Suggested change
lock.readLock().lock();
try {
return resourceCache.get(getCacheKey(type, id));
} finally {
lock.readLock().unlock();
lock.writeLock().lock();
try {
return resourceCache.get(getCacheKey(type, id));
} finally {
lock.writeLock().unlock();

Copilot uses AI. Check for mistakes.
}
}

/**
Expand All @@ -55,7 +94,12 @@ public Object get(String type, String id) {
* @return uUID
*/
public String getUUID(Object obj) {
return uuidReverseMap.get(obj);
lock.readLock().lock();
try {
return uuidReverseMap.get(obj);
} finally {
lock.readLock().unlock();
}
}

/**
Expand Down