feat: Add Caching for ElementUtil#140
Conversation
|
|
||
| private Map<String, String> globalLookup = new HashMap<>(); | ||
| private Map<String, Map<String, String>> localLookupByFileName = new HashMap<>(); | ||
| private static final int INITIAL_CAPACITY = 10000; |
There was a problem hiding this comment.
Set large initial map capacity to try to reduce the number of copy/ increases needed.
|
|
||
| public abstract class BaseLookup<T extends Element> { | ||
|
|
||
| private static final int INITIAL_CAPACITY = 500000; |
There was a problem hiding this comment.
Set large initial map capacity to try to reduce the number of copy/ increases needed. This is for ClassLookup, ClassItemsLookup, and PackageLookup. Maybe I can allow configuration of this capacity for the different lookups. ClassLookup (mid size), ClassItemsLookup (large), PackageLookup (small)
|
|
||
| public class ElementUtil { | ||
|
|
||
| private static final int INITIAL_CAPACITY = 1000000; |
There was a problem hiding this comment.
Set large initial map capacity to try to reduce the number of copy/ increases needed. There are a lot of lookups for a javax.lang.Element (Package, Class, Method) so caching each Element can help speed this up.
| public List<? extends Element> getEnclosedElements(Element element) { | ||
| elementMap.computeIfAbsent(element, this::getEnclosedElementsInternal); |
|
For my future learning, would you enrich this pull request description with your observations that explain why you focused on optimizing |
|
@suztomo Sure. Changes were initially part this draft PR: #139, which was two parts 1. Add concurrency and 2. Add Element caching. When I created the original PR, I came across a test failure with the concurrency code so I split out the caching part while I debug the other issue. Both these changes were part of the initial debugging with YourKit profiler. The concurrent code calling getEnclosedElements() would need to be synchronized between the different threads. They were making the same call to |
|
@lqiu96 Thanks. Would you paste here the screenshot of YourKit profiler showing getEnclosedElements? |
|
@suztomo I didn't take a screenshot unfortunately. |
Fixes #126
Running the kokoro script with the updated element caching (no concurrency)
Seems the jobs without concurrency takes ~90 minutes. This is without running mvn clean install (which may take ~30 minutes/ ~12 minutes with -T 1C). Since the mvn javadoc:aggregate job needs to run mvn compile, do we need to have the have it
clean install?I don't think we would run into any memory issues. I can try to reduce the initial capacity since we're no longer running this for the entire monorepo (only for each submodule in the monorepo).
Both these changes were part of the initial debugging with YourKit profiler. The concurrent code calling getEnclosedElements() would need to be synchronized between the different threads. They were making the same call to element.getEnclosedElements() (https://docs.oracle.com/javase/7/docs/api/javax/lang/model/element/Element.html#getEnclosedElements()) but the result for this function doesn't change. Adding a map/cache to store the already computed values.