[#412] cache enablement for URI #415

ghentschke · 2025-02-05T07:07:08Z

part of #412
fixes #231

ruspl-afed

LGTM
So, only restart will clear the cache and this is desired, correct?

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

LGTM So, only restart will clear the cache and this is desired, correct?

Yes. But I would like to add a clear mechanism which would be triggered when the preferLspEditor has been modifed. I'll wait until your PR #410 has been merged.

ruspl-afed · 2025-02-05T09:57:43Z

I'll wait until your PR #410 has been merged.

Please fill free to merge both when needed.

jonahgraham

I can see that the PropertyTester's test method is called a lot so speeding that up has great value. Here are some comments.

The title of this PR and commit need cash -> cache
PropertyTester must be stateless, see javadoc - the new cache is kindof stateful, or rather you may end up with multiple caches the way it is done now.
The cache is accessed from multiple threads, so some consideration to concurrency is needed.
Should this be doing LRU style so that items don't stay and this grows forever. CDT has one org.eclipse.cdt.internal.core.LRUCache<K, V>
- As it happens @ramele1907 is looking at some concurrency + LRU cache issues in Synchronize the access to LRUCache cdt#978
- That code in CDT deals with a similar problem you have in LSP4E which is URI -> IReource caches. See eclipse-cdt/cdt@9e7b5be for when that was introduced
I think the discussions in [#1207] cache IResource in EnablementTester eclipse-lsp4e/lsp4e#1208 are relevant to this PR too so I am cross referencing them here
Cache invalidation may be needed, see longer comment below

Cache invalidation

My concern with this change is rather a corner case, but that there should be logic to clear the cache as some of the tests can return different results. For example you are checking content types and caching that result. In that case you need to add a listener org.eclipse.core.runtime.content.IContentTypeManager.addContentTypeChangeListener(IContentTypeChangeListener) so that you find out about content type changes.

A simple manual test:

Create a file called "file.jonah"
Double-click to open, will not be opened in CDT LSP because it isn't a C/C++ file
Add jonah as a C file extension in preferences:
Make sure the file now opens as an LSP editor

Doing a addContentTypeChangeListener will be sufficient to invalidate cache for content type changes, similar mechanism may be needed for other cases.

I mentioned LRU above, but time based caching may also be useful.

ghentschke · 2025-02-05T17:40:10Z

Thank you @jonahgraham for the good input I'll consider it!

ghentschke · 2025-02-06T05:59:44Z

Ok, this is more a reminder to me:
The cache shall be cleared if:

the Prefer LSP Editor setting has been changed (workspace or project level) or
on changes in the c/c++ content types for which we enable LSP4E

A resource shall be removed form the cache if it's getting closed in the editor.

The cache shall be implemented as FIFO buffer with n entries which considers concurrency and stateless aspects

part of eclipse-cdt#412

not needed anymore since we now remove files if they are getting closed.

ghentschke · 2025-02-07T13:55:14Z

So, I think this should work now. Please review.

ghentschke · 2025-02-07T14:00:06Z

Note: org.eclipse.cdt.lsp.internal.editor.EditorPreferredOptions.removePreferenceChangedListener(IPreferenceChangeListener) won't be called since the lifetime of the org.eclipse.cdt.lsp.internal.editor.EditorPreferredOptions.EditorPreferredOptions(EditorMetadata, String, IScopeContext[], LanguageServerEnable) instance is not easy to determine.

when one buffer reaches buffer limit

for example from C/C++ stdlib

ghentschke · 2025-02-07T16:08:57Z

Sorry, I think this is it. I found some issue during tests.

jonahgraham

I have only been able to review on my phone. I can try running it up next week.

jonahgraham · 2025-02-07T20:46:19Z

.../org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/internal/server/CLanguageServerEnableCache.java

+			Optional.ofNullable(LSPEclipseUtils.toUri(editor.getEditorInput())).ifPresent(uri -> {
+				var data = cache.get(uri);
+				if (data != null) {
+					if (--data.counter <= 0) {


I think a small comment that counter field should only be accesed from SWT main thread is appropriate here. AFAICT that is how this is implemented as I assume partOpened and partClosed are only called on SWT main thread.

Yes, the partOpen and partClosed methods are called in the main thread

jonahgraham · 2025-02-07T20:47:19Z

.../org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/internal/server/CLanguageServerEnableCache.java

+					if (--data.counter <= 0) {
+						cache.remove(uri);
+					} else {
+						cache.put(uri, data);


This line seems to be a no-op, just putting back in the same instance as you extracted on line 146

jonahgraham · 2025-02-07T20:47:44Z

.../org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/internal/server/CLanguageServerEnableCache.java

+				if (data != null) {
+					data.enable = true;
+					++data.counter;
+					cache.put(uri, data);


No-op line?

jonahgraham · 2025-02-07T20:50:24Z

This change looks good and I think you have well resolved all my previous issues.

this would lead to descreased performance because the cache is only filled on file open. All opened files would not be cached if the cache is getting cleared.

- all opened files in the LSP based editor are added to cache again if the content is still a C/C++ content. - clean up API of CLanguageServerEnableCache

Use case: Several C/C++ files are opened in LSP based editor -> restart -> Change C/C++ content type -> select file -> partOpened gets called and increases the opened counter ==> Fault: counter will be increased twice (1st time during restore after content type change, 2nd time on selection (file was not yet selected after restart)

bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/editor/EditorMetadata.java

bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/editor/EditorOptions.java

ruspl-afed · 2025-02-08T13:14:39Z

thanks!

because they are distinctable and solves the clear-cache-problem when the same URI is opened in several windows. Otherwise we do not know if the URI is still opened somewhere and the cache shouldn't be removed for this URI.

ghentschke · 2025-02-09T21:39:52Z

I don't know. The unit tests run on my machine successfully but not on the server:
at org.eclipse.cdt.lsp.test.internal.server.CLanguageServerEnableCacheTest.testCacheRemovedAfterFileGetsClosed(CLanguageServerEnableCacheTest.java:76)

jonahgraham · 2025-02-10T15:40:50Z

@ghentschke If I run CLanguageServerEnableCacheTest in my IDE with default plug-ins enabled it passes, but if I try to simulate what Tycho does and only enable the plug-ins in the dependency tree it fails locally for me like it does on the build machine.

See https://github.com/eclipse-cdt/cdt/blob/main/TESTING.md#using-limited-sets-of-plug-ins-in-tests for some guidance on how to setup the test in the IDE. Specifically for this case I did this:

Right-click on CLanguageServerEnableCacheTest -> Run As -> JUnit Plugin Test
The tests pass
Open launch configuration dialog, select plug-ins tab
Change "Launch with" to Plug-ins selected below
Press Deselect All
Check "org.eclipse.cdt.lsp.test"
Click Select Required, I end up with about 212 selected bundles
Run this launch
Tests seem to fail in the same way as on build machine

For 7 above here is a screenshot with "Only show selected" checked

NOTE: CDT mostly avoids using fragments for tests. One of the advantages of this is that the test bundles are full-fledged bundles. The flow in the CDT instructions (highlight link) don't work for CDT LSP because CDT LSP uses a fragment and can't be selected. This is why I gave explicit instructions above.

So what to do about the difference. There is probably some implicit dependency the test has for correct behavior. If so, you need to tell tycho about it or remove that dependency. Here is an example in CDT.

jonahgraham · 2025-02-10T16:17:07Z

NOTE: CDT mostly avoids using fragments for tests. One of the advantages of this is that the test bundles are full-fledged bundles. The flow in the CDT instructions (highlight link) don't work for CDT LSP because CDT LSP uses a fragment and can't be selected. This is why I gave explicit instructions above.

I raised eclipse-pde/eclipse.pde#1607 to add the ability to PDE to create the kind of launch that can include the fragment.

ghentschke · 2025-02-10T19:35:59Z

@jonahgraham Thanks for the hints!!! Now it works.

jonahgraham

@ghentschke I hope my comment here makes sense - the combination of adapter factories, handling and auto-downloading of URIs and EGit registering something globally (that arguably it shouldn't) makes this a bit of a mess.

bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/util/LspUtils.java

jonahgraham · 2025-02-11T03:07:41Z

bundles/org.eclipse.cdt.lsp.test/META-INF/MANIFEST.MF

+ org.eclipse.egit.ui,
+ org.eclipse.egit.core


See other comment - please remove this dependency

…ils.java Co-authored-by: Jonah Graham <[email protected]>

- remove obsolete egit dependency in test fragment.

ghentschke · 2025-02-11T15:56:54Z

@jonahgraham any objections left?

ghentschke · 2025-02-11T20:28:07Z

Thank you @jonahgraham and @ruspl-afed !

ghentschke requested review from jonahgraham and ruspl-afed February 5, 2025 07:07

ruspl-afed approved these changes Feb 5, 2025

View reviewed changes

jonahgraham reviewed Feb 5, 2025

View reviewed changes

ghentschke changed the title ~~[#412] cash enablement for URI~~ [#412] cache enablement for URI Feb 5, 2025

ghentschke and others added 2 commits February 6, 2025 22:11

[eclipse-cdt#412] cash enablement for URI

b5820e4

part of eclipse-cdt#412

add URIEnableCache class

4d560b3

ghentschke force-pushed the cash-enablement branch from e1cb948 to 4d560b3 Compare February 6, 2025 21:12

ghentschke added 5 commits February 6, 2025 22:17

remove content type change listener

5b1310f

rename URIEnableCache to URICache and clear cache on LSP stop

d15d636

rename to CLanguageServerEnableCache and enable on opening

7543ffd

do not clear cache on LS stop

51e69c7

not needed anymore since we now remove files if they are getting closed.

track open and closed files

9871322

ghentschke requested a review from jonahgraham February 7, 2025 13:55

ghentschke added 2 commits February 7, 2025 15:51

combine counter and enable buffer to prevent mismatch

852a92b

when one buffer reaches buffer limit

cache also enable for (header) files from external libraries

2a7e573

for example from C/C++ stdlib

jonahgraham reviewed Feb 7, 2025

View reviewed changes

ghentschke added 5 commits February 7, 2025 22:27

remove no-op lines and update header

828330e

remove typo

ea319b3

do not clean cache on LSP enable change

00fb2d4

this would lead to descreased performance because the cache is only filled on file open. All opened files would not be cached if the cache is getting cleared.

restore cache when content type has been modified

65f334a

- all opened files in the LSP based editor are added to cache again if the content is still a C/C++ content. - clean up API of CLanguageServerEnableCache

add comment

42be18c

ghentschke added 4 commits February 8, 2025 00:26

clean-up code

f7d2f64

reset restored after 1st selection

f6c978c

minor improvements

ff6ea9f

ruspl-afed reviewed Feb 8, 2025

View reviewed changes

bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/editor/EditorMetadata.java Outdated Show resolved Hide resolved

ruspl-afed reviewed Feb 8, 2025

View reviewed changes

bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/editor/EditorOptions.java Outdated Show resolved Hide resolved

remove unused changes

b35c302

ghentschke added 5 commits February 9, 2025 08:52

Use editor part hashes to track opened editors

ddb6457

because they are distinctable and solves the clear-cache-problem when the same URI is opened in several windows. Otherwise we do not know if the URI is still opened somewhere and the cache shouldn't be removed for this URI.

add unit test

f92c7b2

add sleep before check

f2bf5f8

add dependency

20f9d3f

add ui test harness tp target

d4b1e29

ghentschke added 2 commits February 10, 2025 19:38

fix bug in pom.xml

6185633

add missing requirements for unit test

0921988

remove test harness

10ce4b6

jonahgraham reviewed Feb 11, 2025

View reviewed changes

ghentschke and others added 2 commits February 11, 2025 07:14

Update bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/util/LspUt…

3fcf933

…ils.java Co-authored-by: Jonah Graham <[email protected]>

Use Optional as return value for the cache and add unit tests

906692a

- remove obsolete egit dependency in test fragment.

jonahgraham approved these changes Feb 11, 2025

View reviewed changes

ghentschke merged commit a2f4434 into eclipse-cdt:main Feb 11, 2025
3 checks passed

ghentschke deleted the cash-enablement branch February 12, 2025 06:15

jonahgraham added this to the 3.0.0 milestone Mar 7, 2025

[#412] cache enablement for URI #415

[#412] cache enablement for URI #415

Uh oh!

Conversation

ghentschke commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruspl-afed left a comment

Choose a reason for hiding this comment

Uh oh!

ghentschke commented Feb 5, 2025

Uh oh!

ruspl-afed commented Feb 5, 2025

Uh oh!

jonahgraham left a comment

Choose a reason for hiding this comment

Cache invalidation

Uh oh!

ghentschke commented Feb 5, 2025

Uh oh!

ghentschke commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghentschke commented Feb 7, 2025

Uh oh!

ghentschke commented Feb 7, 2025

Uh oh!

ghentschke commented Feb 7, 2025

Uh oh!

jonahgraham left a comment

Choose a reason for hiding this comment

Uh oh!

jonahgraham Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

ghentschke Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

jonahgraham Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

jonahgraham Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

jonahgraham commented Feb 7, 2025

Uh oh!

Uh oh!

Uh oh!

ruspl-afed commented Feb 8, 2025

Uh oh!

ghentschke commented Feb 9, 2025

Uh oh!

jonahgraham commented Feb 10, 2025

Uh oh!

jonahgraham commented Feb 10, 2025

Uh oh!

ghentschke commented Feb 10, 2025

Uh oh!

jonahgraham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahgraham Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

ghentschke commented Feb 11, 2025

Uh oh!

Uh oh!

ghentschke commented Feb 11, 2025

Uh oh!

Uh oh!

ghentschke commented Feb 5, 2025 •

edited

Loading

ghentschke commented Feb 6, 2025 •

edited

Loading