Issue 45377: Poor performance determining required modules #3339
Conversation
…a container with many workbooks
| CacheManager.DAY, | ||
| "Required modules per containers")); | ||
|
|
||
| private final static CacheLoader<GUID, Set<Module>> REQUIRED_MODULE_LOADER = |
There was a problem hiding this comment.
You could simply use the Container as the key
| CacheManager.getCache( | ||
| Constants.getMaxContainers(), | ||
| CacheManager.DAY, | ||
| "Required modules per containers")); |
There was a problem hiding this comment.
You could call CacheManager.getBlockingCache() and set the CacheLoader as part of the factory method. And then call get(key, argument).
There was a problem hiding this comment.
I tried doing that originally, but there's no get(key, argument) version. Only get(key) and get(key, argument, loader) But if I switch to using the Container as the key, I can consolidate a bit.
There was a problem hiding this comment.
Cache doesn't have that variant, but BlockingCache does. Declare REQUIRED_MODULES_CACHE as a BlockingCache and voila!
| { | ||
| List<Container> siblings = new ArrayList<>(mm.get(key)); | ||
| Collections.sort(siblings); | ||
| } |
There was a problem hiding this comment.
I guess we don't care about the sort any more?
There was a problem hiding this comment.
Unless I'm misreading the code, this never did anything. We create a copy of the list, sort it, and then throw it out.
There was a problem hiding this comment.
Yep, agreed... I hadn't looked very closely at first
Rationale
When a container has 25k+ child workbooks, calculating its required modules can be time consuming. And we do it multiple times on a single page render. Caching will provide a big speedup.
Changes