[#1207] cache IResource in EnablementTester #1208
Conversation
Cache resource for URI because each call of LSPEclipseUtils.findResourceFor(URI) takes ~300 microseconds. And it gets called a lot of times. fixes eclipse-lsp4e#1207
| @@ -69,7 +72,7 @@ public boolean evaluate(@Nullable URI uri) { | |||
| IResource resource = null; | |||
| try { | |||
| IDocument document = null; | |||
| resource = LSPEclipseUtils.findResourceFor(uri); | |||
There was a problem hiding this comment.
I think there are several places where we map resources to URIs. Should we not benefit by having the cache reused by all consumers of the method? If so, what about building the cache as an static map in LSPEclipseUtils?
| if (resource != null && resource.isAccessible()) { | ||
| return resource; | ||
| } | ||
| resource = LSPEclipseUtils.findResourceFor(uri); |
There was a problem hiding this comment.
could it happen that a resource is moved after being cached? Would that not invalidate the cache? If that could happen, should we not either detect to be able to invalidate the cache or have some eviction policy?
There was a problem hiding this comment.
could it happen that a resource is moved after being cached?
Yes, but shouldn't it be handled by the resource.isAccessible call:
Returns whether this resource is accessible. For files and folders, this is equivalent to existing;
There was a problem hiding this comment.
and what if the resource has been moved to a place where it is still accessible? Can that happen?
There was a problem hiding this comment.
If the resource has been moved, the uri will be changed as well. This leads to an new entry and to an obsolete entry in the cache.
There was a problem hiding this comment.
Yes, you are correct that its URI will change, but the cache will still have an entry for the old URI and will return the resource, will it not?
There was a problem hiding this comment.
IMO that's not a problem, if the old URI isn't cached anywhere. I'll check that
|
One question: is there a mechanism to clean up the cache? It looks like it is an ever growing map that keeps references to all those IResource objects forever. Could that be a problem memory-wise over time? |
Shouldn't such a strategy be implemented directly in |
Yes, I'll add a clear mechanism which could be triggered when the |
Yes. I'll consider it. |
| @@ -40,6 +42,7 @@ public final class EnablementTester { | |||
| private final Expression expression; | |||
| private final String description; | |||
| private final Supplier<@Nullable IEvaluationContext> parent; | |||
| private final Map<URI, IResource> cache = new HashMap<>(); | |||
There was a problem hiding this comment.
How about using Guava Cache instead which supports expiring entries after read access and is thread safe. LSP4E already depends on guava so no new dependency needs to be added.
There was a problem hiding this comment.
Thanks for the hint. I'll take a closer look at it.
There was a problem hiding this comment.
@sebthom The Javadocs for Guava's says to prefer using Caffeine instead.
AFAICT nothing contributes caffeine to SimRel yet, but some projects do use it (e.g. Mylyn, m2e's tests) and it is in orbit aggregator for 2025-03 too
Any thoughts on best way to proceed?
There was a problem hiding this comment.
I personally would be pragmatic and just use guava instead of introducing another dependency. I think it is good enough. Esp. since it does not look like guava will go away from lsp4e
Cache resource for URI because each call of
LSPEclipseUtils.findResourceFor(URI) takes ~300 microseconds. And it gets called a lot of times.
fixes #1207