[#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. |
Cache resource for URI because each call of
LSPEclipseUtils.findResourceFor(URI) takes ~300 microseconds. And it gets called a lot of times.
fixes #1207