[#1207] cache IResource in EnablementTester #1208

ghentschke · 2025-02-05T07:02:21Z

Cache resource for URI because each call of
LSPEclipseUtils.findResourceFor(URI) takes ~300 microseconds. And it gets called a lot of times.

fixes #1207

rubenporras · 2025-02-05T07:17:32Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/enablement/EnablementTester.java

@@ -69,7 +72,7 @@ public boolean evaluate(@Nullable URI uri) {
 		IResource resource = null;
 		try {
 			IDocument document = null;
-			resource = LSPEclipseUtils.findResourceFor(uri);


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?

rubenporras · 2025-02-05T07:20:16Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/enablement/EnablementTester.java

+			if (resource != null && resource.isAccessible()) {
+				return resource;
+			}
+			resource = LSPEclipseUtils.findResourceFor(uri);


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?

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;

and what if the resource has been moved to a place where it is still accessible? Can that happen?

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.

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?

IMO that's not a problem, if the old URI isn't cached anywhere. I'll check that

@rubenporras Please review my current implementation. I use a IResourceChangeListener now to track modifications and to remove deleted/moved resources from the cache.

martinlippert · 2025-02-05T07:21:28Z

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?

mickaelistria · 2025-02-05T07:55:12Z

LSPEclipseUtils.findResourceFor(URI) takes ~300 microseconds. And it gets called a lot of times.

Shouldn't such a strategy be implemented directly in LSPEclipseUtils.findResourceFor(uri) instead? And isn't there some other possible improvement to find there, or even in Eclipse Platform?

ghentschke · 2025-02-05T09:17:22Z

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?

Yes, I'll add a clear mechanism which could be triggered when the expression.evaluate returns false for the given URI.

ghentschke · 2025-02-05T09:19:05Z

Shouldn't such a strategy be implemented directly in LSPEclipseUtils.findResourceFor(uri) instead? And isn't there some other possible improvement to find there, or even in Eclipse Platform?

Yes. I'll consider it.

sebthom · 2025-02-08T03:45:47Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/enablement/EnablementTester.java

@@ -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<>();


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.

Thanks for the hint. I'll take a closer look at it.

@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?

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

I think the same.

Thanks @sebthom and @rubenporras for your input.

Thanks for your inputs. I implemented a singleton cache for URI-to-IResources with guava cache now. I implemented it as a singleton. It can be used everywhere instead of a direct call to LSPEclipseUtils.findResourceFor(URI).

Shouldn't such a strategy be implemented directly in LSPEclipseUtils.findResourceFor(uri) instead?

@mickaelistria it could. But the current implementation leaves it up to the developer whether to use a cache or not. This should minimize possible side effects of a cache implementation directly in LSPEclipseUtils.findResourceFor(uri)

Please review!

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

which can be used as replacement for LSPEclipseUtils.findResourceFor(URI)

ghentschke · 2025-02-12T10:24:39Z

I don't know why the windows build fails due to this:

org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-clean-plugin:3.4.0:clean (default-clean) on project org.eclipse.lsp4e: Failed to clean project: Failed to delete D:\a\lsp4e\lsp4e\org.eclipse.lsp4e\target\org.eclipse.lsp4e-0.18.18-SNAPSHOT.jar

sebthom · 2025-02-12T17:36:56Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/ResourceForUriCache.java

+	}
+
+	@Nullable
+	public synchronized IResource get(@Nullable URI uri) {


I would use a LoadingCache instead of synchronizing the method

That was my first attempt. It seems that LoadingCache is not really appropriate for this use case, because the load method has to return a non null value:

Returns: the value associated with key; must not be null

But it cannot be guaranteed that we find an IResource object for a given URI (e.g. when the file is outside the workspace)

Ah true, that only works with Caffeine. I personally then would just remove the synchronized keyword and take the risk of a race conditions over synchronizing the method. The worst case is that a URI is looked up twice at the same time. Or alternatively use synchronized(cache) { ... } inside the if (uri != null) block. But I believe that isn't really necessary.

I removed the synchronization from the get method because current usage in EnablementTester.evaluate(URI) runs AFAIK in main thread only.
I removed the IResourceChangeListener as well because it won't worked as expected (see git commit comment) and the guave cache has already a mechanism to remove unused entries as well.

I've to withdraw my statement above: there are concurrent calls to EnablementTester.evaluate(URI)

sebthom · 2025-02-12T21:35:07Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/ResourceForUriCache.java

+		//  use getInstance()
+	}
+
+	public static synchronized ResourceForUriCache getInstance() {


how about instead of adding a synchronized getInstance method simply defining a public static final ResourceForUriCache INSTANCE = new ResourceForUriCache();

because it won't work in case a IResource has been deleted or moved, because the listener will be called not in main thread but the get method will be. This leads to the situation where the listener has removed the IResource from the cache but the get methods add it again, when it gets called with the old URI. (tested with renaming)

mickaelistria · 2025-02-13T20:27:37Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/ResourceForUriCache.java

+			if (resource != null) {
+				return resource;
+			}
+			resource = LSPEclipseUtils.findResourceFor(uri);


Should we move the logic of findResourceFor in this cache too, and then make LSPEclipseUtils.findResourceFor(...) reference ResourceForUriCache.getInstance().get(...) ?

Doing so will make all consumers of findResourceFor(...) save 300ms, not just en enablementTester

I also think that is a good idea

rubenporras · 2025-02-14T07:04:22Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/ResourceForUriCache.java

+			if (resource != null) {
+				return resource;
+			}
+			resource = LSPEclipseUtils.findResourceFor(uri);


I also think that is a good idea

rubenporras · 2025-02-14T07:10:18Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/ResourceForUriCache.java

+ * Therefore entries can be removed before the limit exceeds.
+ */
+public final class ResourceForUriCache {
+	private static final Cache<URI, IResource> cache =  CacheBuilder.newBuilder().maximumSize(100).build();


could we pass a CacheLoader to the build method with a call to LSPEclipseUtils.findResourceFor? I think that would save the double access to the cache (get & set) in case of cache miss.

The load method in CacheLoader does not allow a null to be returned See here:

Returns: the value associated with key; must not be null

But it cannot be guaranteed that we find an IResource object for a given URI (e.g. when the file is outside the workspace)

I should add a comment to the method why we do it this way... (See also comment from @sebthom here)

rubenporras · 2025-02-14T17:05:24Z

looks good to me

sebthom · 2025-02-14T18:33:41Z

org.eclipse.lsp4e/src/org/eclipse/lsp4e/ResourceForUriCache.java

+ *   See git history
+ *******************************************************************************/
+
+package org.eclipse.lsp4e;


This should move it into the internal package. I don't think this should be part of the LSP4E API.

ghentschke · 2025-02-15T07:16:49Z

Thanks for the reviews!

rubenporras reviewed Feb 5, 2025

View reviewed changes

jonahgraham mentioned this pull request Feb 5, 2025

[#412] cache enablement for URI eclipse-cdt/cdt-lsp#415

Merged

sebthom reviewed Feb 8, 2025

View reviewed changes

ghentschke added 2 commits February 12, 2025 07:23

[eclipse-lsp4e#1207] cache IResource in EnablementTester

98085ef

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

add ResourceForUriCache

f0a7b50

which can be used as replacement for LSPEclipseUtils.findResourceFor(URI)

ghentschke force-pushed the fix-long-runner branch from 7868e70 to f0a7b50 Compare February 12, 2025 09:34

increase version

f23cf24

simplify bit mask check

1c5c093

ghentschke requested review from rubenporras and sebthom February 12, 2025 14:08

sebthom reviewed Feb 12, 2025

View reviewed changes

ghentschke requested a review from sebthom February 13, 2025 10:20

sebthom approved these changes Feb 13, 2025 •

edited

Loading

View reviewed changes

sebthom self-requested a review February 13, 2025 16:33

mickaelistria reviewed Feb 13, 2025

View reviewed changes

rubenporras reviewed Feb 14, 2025

View reviewed changes

ghentschke added 2 commits February 14, 2025 13:05

move findResourceFor(URI) from LspUtils to ResourceForUriCache

b4eb10c

remove obsolete null check

582aabe

sebthom requested changes Feb 14, 2025

View reviewed changes

ghentschke added 2 commits February 14, 2025 20:12

move ResourceForUtilsCache to internal package

670c79c

make ResourceForUricache.get(URI) static

90fa5f4

ghentschke requested a review from sebthom February 14, 2025 19:30

sebthom approved these changes Feb 14, 2025

View reviewed changes

ghentschke merged commit 3e78918 into eclipse-lsp4e:main Feb 15, 2025
6 checks passed

ghentschke deleted the fix-long-runner branch February 15, 2025 08:59

[#1207] cache IResource in EnablementTester #1208

[#1207] cache IResource in EnablementTester #1208

Conversation

ghentschke commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghentschke Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghentschke Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinlippert commented Feb 5, 2025

mickaelistria commented Feb 5, 2025

ghentschke commented Feb 5, 2025

ghentschke commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebthom Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghentschke Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

ghentschke commented Feb 12, 2025

Choose a reason for hiding this comment

ghentschke Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenporras commented Feb 14, 2025

Choose a reason for hiding this comment

ghentschke commented Feb 15, 2025

ghentschke Feb 5, 2025 •

edited

Loading

ghentschke Feb 5, 2025 •

edited

Loading

sebthom Feb 10, 2025 •

edited

Loading

ghentschke Feb 12, 2025 •

edited

Loading

ghentschke Feb 12, 2025 •

edited

Loading