CodeMining: reuse existing inlined annotations when minings are unchanged

[tobiasmelcher]

Add getMinings() accessor methods to CodeMiningLineContentAnnotation and CodeMiningLineHeaderAnnotation to expose their mining lists. Enhance findExistingAnnotation() in InlinedAnnotationSupport to accept an optional
minings parameter and compare existing annotations' minings with new ones.
This allows reusing existing annotation instances when the mining content
hasn't changed, avoiding unnecessary annotation recreation and improving performance.

The change prevents flickering and reduces overhead by preserving annotation
objects when only their position is being updated during reconciliation, but
their actual mining data remains identical.

Fixes #2786 (comment)

[github-actions]

Test Results

 3 018 files  ±0   3 018 suites  ±0   2h 3m 54s ⏱️ - 19m 1s
 8 242 tests ±0   7 994 ✅ ±0  248 💤 ±0  0 ❌ ±0 
23 646 runs  ±0  22 855 ✅ ±0  791 💤 ±0  0 ❌ ±0 

Results for commit fc3d22c. ± Comparison against base commit e15e9bf.

♻️ This comment has been updated with latest results.

[mickaelistria]

Would it be possible and/or better to implement the caching directly in this manager instead of expecting all providers to implement it?
Basically, I imagine the manager could store a Map<ICodeMiningProvider, Map<Position, ICodeMining>> lastResults, and that findExistingAnnotation could take directly the provider as input, and become findExistingAnnotation(pos, provider)

[tobiasmelcher]

Thanks a lot for the very fast response.
I had a similar idea in mind. We need an indication from the ICodeMiningProvider implementation when we should keep the existining annotations and when to replace. The current API does not really provides this ability.
Let's take a look at the current JDT implementation:

@Override
	public CompletableFuture<List<? extends ICodeMining>> provideCodeMinings(ITextViewer viewer,
			IProgressMonitor monitor) {
		if (!editorEnabled) {
			return CompletableFuture.completedFuture(Collections.emptyList());
		}
		if (viewer instanceof ISourceViewerExtension5) {
			ISourceViewerExtension5 codeMiningViewer = (ISourceViewerExtension5)viewer;
			if (!JavaCodeMiningReconciler.isReconciled(codeMiningViewer)) {
				// the provider isn't able to return code minings for non-reconciled viewers
				return CompletableFuture.completedFuture(Collections.emptyList()); // <!--------------
			}
		}
		return CompletableFuture.supplyAsync(() -> {
			monitor.isCanceled();
			ITextEditor textEditor= super.getAdapter(ITextEditor.class);
	                ... // here the real calculation logic takes place

The Java implementation returns a CompletableFuture.completedFuture(Collections.emptyList()); when the reconciler is not yet finished and calculation of the code minings cannot be done. The current implementation replaces all existing annotations with the ones returned from the code mining provider. This means, the existing ones are deleted and the flickering appears.
We need to distinguish between "replace existing annotations with the ones I provide you" and "keep the existing annotations because I cannot calculate right now".
We would need to extend the API which could get quite complex.

I locally already started with a second Extension interface which allows to provide this information. But compatibility handling then got so complicated that I gave up. This pull request together with eclipse-jdt/eclipse.jdt.ui#2697 is much less complex and easier to follow. For sure, we move more complexity to the provider implementation.
Here the interface which I tried to introduce and implement:

public interface ICodeMiningProviderExtension {

    /**
     * Sealed result type for code mining calculations.
     * 
     * <p>Exactly one of {@link CodeMiningSuccess} or {@link CodeMiningDeferred} will be returned.
     * Use pattern matching (switch expressions) for exhaustive handling.</p>
     */
    public static sealed interface CodeMiningResult 
            permits CodeMiningSuccess, CodeMiningDeferred {}

    /**
     * <strong>SUCCESS</strong>: Code minings calculated successfully.
     * 
     * <p>The returned {@code minings} <strong>REPLACE</strong> any existing code minings
     * from previous invocations of this provider.</p>
     * 
     * <ul>
     * <li><strong>Empty list:</strong> Clears all code minings from this provider</li>
     * <li><strong>Null:</strong> Also clears all code minings (treated as empty)</li>
     * <li><strong>Non-empty:</strong> Displays the new code minings</li>
     * </ul>
     * 
     * @param minings New code minings to display (replaces existing ones)
     */
    public static final record CodeMiningSuccess(List<? extends ICodeMining> minings) 
            implements CodeMiningResult {}

    /**
     * <strong>DEFERRED</strong>: Code minings cannot be calculated right now.
     * 
     * <p><strong>CRITICAL:</strong> Existing code minings from previous successful calls
     * to this provider are <strong>PRESERVED</strong> and <strong>NOT REMOVED</strong>.</p>
     * 
     * <p>Typical reasons include:</p>
     * <ul>
     * <li>Document not fully reconciled</li>
     * <li>Operation timeout (e.g., &gt;30s)</li>
     * <li>Progress monitor cancelled</li>
     * </ul>
     * 
     * <p>Consumers should <strong>NOT</strong> clear the UI or update displays when receiving
     * this result. The system will retry automatically on the next reconciliation cycle.</p>
     */
    public static final class CodeMiningDeferred implements CodeMiningResult {}

    /**
     * Asynchronously computes code minings for the given viewer.
     * 
     * @param viewer The text viewer requesting code minings
     * @param monitor Progress monitor for cancellation checks
     * @return Future completing with either:
     * <ul>
     * <li>{@link CodeMiningSuccess} - New minings ready (replaces existing)</li>
     * <li>{@link CodeMiningDeferred} - Calculation deferred (keeps existing minings)</li>
     * </ul>
     * @throws CancellationException if the future is cancelled
     */
    CompletableFuture<CodeMiningResult> provideCodeMinings(ITextViewer viewer, 
            IProgressMonitor monitor);
}

A simpler alternative approach could be the introduction of a subclass of ICodeMining with name like KeepExistingCodeMinings which gives the indication to keep the existing ones.

What is your opinion here? In which direction should I go?

[laeubi]

The Java implementation returns a CompletableFuture.completedFuture(Collections.emptyList()); when the reconciler is not yet finished and calculation of the code minings cannot be done. The current implementation replaces all existing annotations with the ones returned from the code mining provider. This means, the existing ones are deleted and the flickering appears.
...
What is your opinion here? In which direction should I go?

As I mentioned in the other ticket already, this approach is fundamentally broken and completely contradicts the usage of CompletableFuture, so I don't think it is anywhere useful to further support that case.

The use case of CompletableFuture actually is to schedule something (maybe long running) in the background and then allow consumers to wait for the result. So returning something as "I'm not ready" is simply not needed, just return an (uncompleted) CompletableFuture and complete it whenever you are ready to do so!

This is then all what's needed to let the caller know "keep whats currently used because I have nothing new (yet) for you"... Also otherwise how should the "manager" no when it is time to ask for something new? That's all what completable future is for!

[tobiasmelcher]

@laeubi could you please take a look at eclipse-jdt/eclipse.jdt.ui@master...tobiasmelcher:eclipse.jdt.ui:wait_for_reconciler ? Are you proposing this approach?

I added a Thread.sleep(3000) by intention to show a sceanrio where reconciliation takes a longer time.

Here a screencast showing the JDT and Copilot code minings providing some results.
The current framework implementation waits until all CompletableFutures are available and is then triggering a redraw. This breaks the Copilot functionality from my point of view because the copilot proposals are shown too late.

java_reconciler_3_seconds_.mp4

What is your opiniion? What would you propose? Should we trigger a redraw for each CompletableFuture separately? I think that we then get complaints that we trigger redraw too much often.

[laeubi]

Are you proposing this approach?

This is not optimal and has some room for improvements but yes this is how it basically is supposed to work and correct.

I added a Thread.sleep(3000) by intention to show a scenario where reconciliation takes a longer time.

If reconcilation takes really that long we would get much more complains, but yes its valid for investigation of course. One still might question if this really happens at all?

The current framework implementation waits until all CompletableFutures are available and is then triggering a redraw.

This is a very simple approach what might can be optimized in several ways I think, e.g. one might wait only a certain time (e.g. 100ms) for providers and then already perform an update with whats there. Then one slow provider will not block all others, a total timeout might be useful as well (e.g. if a provider takes > 10 seconds cancel it), but this also depends on how the providers work, for example if the document constantly changing (during typing) then maybe even the document positions become stale anyways.

This breaks the Copilot functionality from my point of view because the copilot proposals are shown too late.

It all depends a bit on how we define "break". I would assume copilote is showing things based on the document model. If that currently is reconciled it would mean it is not up-to date, so even if we can show the things faster they might be wrong / outdated. So do we want to show something fast, or something more correct?

What is your opinion? What would you propose?

For the matter of this topic, see above. In general it might be useful to reconsider what copilot does here and possibly offering some specialized support for "ghosttext" that do not interfere with code mining. For example we have similar for auto-completion already, where typing more reduces the list of recommendations. e.g. in this particular case the editor can do much of the work already without the need of the provider, so in your example

• // -> provider returns nothing
• // co -> provider returns "comment"
• // com -> we can reuse last return value "comment" and update the editor right now, then ask the provider if it now wants to supply a different thing (what might be slower than the user types) and update it once it is available.

Such an approach, specially crafted to "inline code completion" would not need to retrigger all codemining providers.

[tobiasmelcher]

This is a very simple approach what might can be optimized in several ways I think, e.g. one might wait only a certain time (e.g. 100ms) for providers and then already perform an update with whats there. Then one slow provider will not block all others, a total timeout might be useful as well (e.g. if a provider takes > 10 seconds cancel it), but this also depends on how the providers work, for example if the document constantly changing (during typing) then maybe even the document positions become stale anyways.

Very good proposal, I like it. I can take a look but this takes some time for me. You can also take over here if you want.

[tobiasmelcher]

For the matter of this topic, see above. In general it might be useful to reconsider what copilot does here and possibly offering some specialized support for "ghosttext" that do not interfere with code mining. For example we have similar for auto-completion already, where typing more reduces the list of recommendations. e.g. in this particular case the editor can do much of the work already without the need of the provider, so in your example

Why introducing a new API only for the Copilot plugin? I am not quite sure. You would then need to take over here. I will not do it.

[laeubi]

Do you want to continue here and provide the "optimal" solution? I can help if you provide more feedback.

Not quite yet, I just wanted to get some feedback.

Very good proposal, I like it. I can take a look but this takes some time for me. You can also take over here if you want.

Sure no one want to force you hear, I just wanted to mention it here because the problem itself has different angles and places where things can be improved independently.

Why introducing a new API only for the Copilot plugin? I am not quite sure.

Before you where fine to introduce new API as well... of course it is not for copilot plugin it would be for who ever wants to provide "ghos text" what would not be needing an AI at all (e.g. local variables, fields, whatever...)

You would then need to take over here. I will not do it.

No obligations here, I just wanted to mention it that the actual usage of codemining + copilot approach refreshing everything on every keypress might better be solved with a dedicated API.

So in general I would say: Fix the immediate JDT issue as described, if its getting slow for copilot its a different topic and should not be your primary concern here.

[tobiasmelcher]

@laeubi and @mickaelistria I created a pull request for the JDT project eclipse-jdt/eclipse.jdt.ui#2698 which should solve the flickering issue. Could you please be so kind and review the code? Do you have contacts in the JDT team who could support me with my request?

[mickaelistria]

I think the outcome of this discussion is great and the PR is good too.

[laeubi]

As the discussion shows I don't think anyone needs access to this data structure or should access it! This is even an internal class so no one should ever call any method on this outside the framework itself.