fill additionalTextEdits during completionItem/resolve #1487

Eskibear · 2020-06-18T17:37:07Z

The goal is to improve performance of completion. Previously all text edits (including addtional ones which cost much time for imports) are caclulated in CompletionProposalReplacementProvider#updateReplacement. Now we decouple them,
TextEdits is calculated in "completion" stage, and AdditionalTextEdits in "resolve" stage

~~Currenly it's not ready for review, just to show you how far it would improve.~~

Performance Preview

In the initial commit, I defer the calculation of imports (only for TYPE_REF candidates) to resolving stage. In spring-petclinic project, when I type "S" to complete, it takes:

~110ms for 50 items
~330ms for all 3000+ items

(macOS, 2.9 GHz Dual-Core Intel Core i5, 8G RAM)

References

VS Code has already supported the so-called Relaxed resolveCompletionItem in v1.46.
Proposed spec in LSP microsoft/language-server-protocol#1003

Eskibear · 2020-06-19T07:42:16Z

Previously in CompletionProposalReplacementProvider#updateReplacement, when it computes textEdits for the CompletionItem, an object of ImportRewrite is also updated by the way, and then converted into additonalTextEdits via a private method addImports.

ImportRewrite object was updated in multiple locations (for different type of proposals), and my original thought was to decouple it, and remove every update of the ImportRewrite object during completion stage. I partially figure it out, but also made a mess in the code.

addImports is the most expensive part in completion that we want to defer to resolve stage. (see microsoft/vscode#96779 (comment)). So I tend to be more straightforward as below:

completion
- For all items, call updateReplacement as usual but without addImports.
resolve
- For the single item, call updateReplacement again, and addImports in the end.

Pros: minimal code changes, less risk of regression
Cons: for the single item to resolve, CompletionItem is re-calculated again.

I think pros >>> cons

fbricon · 2020-06-19T15:32:03Z

this is not officially specified yet, so we need to ensure clients explicitly support that new behavior by adding an initialization option (like everything we support that deviates from LSP)

Clients not supporting this feature should receive the regular completion items

Eskibear · 2020-06-19T16:07:12Z

Yes, theoretically we shall wait for the release of LSP v3.16, in order not to block any other clients. I create this PR to let us know how far we can improve, and let you guys review it in advance.

The original behaviour of jdtls was to calculate all textEdits in resolve stage. We moved it to completion stage only because of previous vscode's behaviour change. Other clients seemed not to have the regression issue at that time (not sure about their status now).

As vscode has already fixed that, and the behavior has been proposed in LSP v3.16 (though the paper work has not been done yet). If we do want to merge this PR in advance to fix the perf issue for users now, I think we should at least make sure it doesn't block any other client.

fbricon · 2020-06-19T16:10:55Z

...rc/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalReplacementProvider.java

+							org.eclipse.lsp4j.TextEdit edit = toRequiredTypeEdit(requiredProposal, trigger, proposal.canUseDiamond(context));
 							if (proposal.getKind() == CompletionProposal.CONSTRUCTOR_INVOCATION || proposal.getKind() == CompletionProposal.ANONYMOUS_CLASS_CONSTRUCTOR_INVOCATION
 									|| proposal.getKind() == CompletionProposal.ANONYMOUS_CLASS_DECLARATION) {
-							completionBuffer.append(edit.getNewText());
-							range = edit.getRange();
-						} else {
-							additionalTextEdits.add(edit);
-						}
-						break;
-					default:
-						/*
-						 * In 3.3 we only support the above required proposals, see
-						 * CompletionProposal#getRequiredProposals()
-						 */
-						Assert.isTrue(false);
+								completionBuffer.append(edit.getNewText());
+								range = edit.getRange();
+							} else {
+								additionalTextEdits.add(edit);
+							}


Not sure if we should do something like:

if (proposal.getKind() == CompletionProposal.CONSTRUCTOR_INVOCATION || proposal.getKind() == CompletionProposal.ANONYMOUS_CLASS_CONSTRUCTOR_INVOCATION || proposal.getKind() == CompletionProposal.ANONYMOUS_CLASS_DECLARATION) { org.eclipse.lsp4j.TextEdit edit = toRequiredTypeEdit(requiredProposal, trigger, proposal.canUseDiamond(context)); completionBuffer.append(edit.getNewText()); range = edit.getRange(); } else if (addImports) { org.eclipse.lsp4j.TextEdit edit = toRequiredTypeEdit(requiredProposal, trigger, proposal.canUseDiamond(context)); additionalTextEdits.add(edit); }

are all additional edits imports?

are all additional edits imports?

No. In most cases they are imports. But there is a special case CompletionHandlerTest.java#testCompletion_method_withLSPV3(), besides the imports, there is another additonal edit to replace "HashMap" with "HashMap". It look confusing (redudant), but after playing for a while, I found a case that might better explain it.

public class A { public static void main(String[] args) { HashMap map = new HashMap<>(); map.putA } }

// additional edits for `putAll` "additionalTextEdits": [ { "range": { "start": { "line": 2, "character": 8 }, "end": { "line": 2, "character": 15 } }, "newText": "HashMap<K,V>" //<-- this is the `toRequiredTypeEdit` one }, { "range": { "start": { "line": 0, "character": 0 }, "end": { "line": 0, "character": 0 } }, "newText": "import java.util.HashMap;\n\n" }

No matter whether they are imports or not, the additional edits will be correctly calculated in the resolve stage. The change you proposed is surely harmless, and can also reduce the times of calling toRequiredTypeEdit.

fbricon · 2020-06-19T17:22:45Z

new Type completion is really super slow (with no completion limit).
Given:

List<String> list = new A|

I've added some extra logs after https://github.com/eclipse/eclipse.jdt.ls/blob/c2bdb489d305ece226da4e55f6e349d65ce34383/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandler.java#L150 and after https://github.com/eclipse/eclipse.jdt.ls/blob/c2bdb489d305ece226da4e55f6e349d65ce34383/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandler.java#L154

[Info  - 7:15:25 PM] 19-Jun-2020 7:15:25 PM codeComplete call took 147 ms for 517 proposals, before snippets, javadoc and conversion to CompletionItems
[Info  - 7:15:25 PM] 19-Jun-2020 7:15:25 PM Reconciled 0. Took 2 ms
[Info  - 7:15:26 PM] 19-Jun-2020 7:15:26 PM begin problem for /BuildPropertiesWriter.java
[Info  - 7:15:26 PM] 19-Jun-2020 7:15:26 PM 4 problems reported for /BuildPropertiesWriter.java
[Info  - 7:15:26 PM] 19-Jun-2020 7:15:26 PM Validated 1. Took 31 ms
[Info  - 7:15:32 PM] 19-Jun-2020 7:15:32 PM getCompletionItems() call took 7030 ms to convert 517 proposals to CompletionItems

(tested on the spring-boot codebase)

Eskibear · 2020-06-20T08:05:54Z

new Type completion is really super slow (with no completion limit).

I can reproduce it. It's not related to addImports, thus not coverred in this fix. I'll update my findings after investigation

Eskibear · 2020-06-22T03:17:05Z

New type completion is slow, because computing items for "anonymous class creation" is slow. E.g. given List<String> list = new A|, one of the candidate is for abstract interface Autowired,
computing below content (in textEdits) takes time.

StringBuilder@128 "Autowired(){

	@Override
	public Class<? extends Annotation> annotationType() {
		${0:// TODO Auto-generated method stub
		return null;}
	}

	@Override
	public boolean required() {
		// TODO Auto-generated method stub
		return false;
	}
};"

I'm thinking of whether we can put the body in addtionalTextEdits instead of textEdits. E.g.
complete: textEdits = "AutoWired()"
resolve: additionalTextEdits = "{ /* Overrides... */}"

fbricon · 2020-06-22T05:38:34Z

I'm thinking of whether we can put the body in addtionalTextEdits instead of textEdits. E.g.
complete: textEdits = "AutoWired()"
resolve: additionalTextEdits = "{ /* Overrides... */}"

Sounds good to me.

Eskibear · 2020-06-23T15:58:25Z

I tried my proposal in vscode, given above textEdit and addtionalTextEdits, it didn't work as expected. And I found two related problems in LSP.

/**
* An optional array of additional text edits that are applied when
* selecting this completion. Edits must not overlap (including the same insert position)
* with the main edit nor with themselves.
*
* Additional text edits should be used to change text unrelated to the current cursor position
* (for example adding an import statement at the top of the file if the completion item will
* insert an unqualified type).
*/
additionalTextEdits?: TextEdit[];

Checking LSP spec above, additionalTextEdits cannot overlap with (or even next to) the textEdit.

/**
* The format of the insert text. The format applies to both the insertText property
* and the newText property of a provided textEdit. If omitted defaults to
* InsertTextFormat.PlainText.
*/
insertTextFormat?: InsertTextFormat;

insertTextFormat applies to textEdit instead of additionalTextEdits, thus placeholders like "${0: // TODO}" won't work because by default it's processed as plain text.

I tried the workaround below, and it did work correctly and fast. However, it is against the LSP spec that textEdit must not change.

complete: textEdits = "AutoWired()"
resolve: textEdits = "AutoWired(){ /* Overrides... */}

Now I'm thinking, guarded by the feature flag it won't break any other clients, so maybe it's worth a try?

Signed-off-by: Yan Zhang <[email protected]>

fbricon · 2020-06-24T08:32:02Z

I tried the workaround below, and it did work correctly and fast. However, it is against the LSP spec that textEdit must not change.

complete: textEdits = "AutoWired()"

resolve: textEdits = "AutoWired(){ /* Overrides... */}

Now I'm thinking, guarded by the feature flag it won't break any other clients, so maybe it's worth a try?

I don't think we should go that route. This is exactly what we were doing before. It violates the spec, we have no guarantee that vscode will keep working that way in the future (cc @jrieken), and it won't work for other clients once lazy loading additional textedits is part of the LSP.

Eskibear · 2020-06-24T08:49:28Z

I've created microsoft/language-server-protocol#1032 in LSP repo.

Eskibear · 2020-06-25T02:30:38Z

A compromising approach is, we don't fill unimplemented methods automatically, leaving the work to code actions/source actions (we already have one named "Add unimplemented methods") . Typescript is doing this way as mentioned in microsoft/language-server-protocol#1032 (comment)

fbricon · 2020-06-25T09:14:32Z

A compromising approach is, we don't fill unimplemented methods automatically, leaving the work to code actions/source actions (we already have one named "Add unimplemented methods") . Typescript is doing this way as mentioned in microsoft/language-server-protocol#1032 (comment)

@Eskibear ok let's see how it works

Signed-off-by: Yan Zhang <[email protected]>

Eskibear · 2020-06-26T15:58:46Z

I simply remove the new body in 8a901a9, and it looks like below (with no completion limit).

fbricon · 2020-06-29T13:11:25Z

...core/src/org/eclipse/jdt/ls/core/internal/contentassist/AnonymousTypeCompletionProposal.java

-		}
+		// Construct empty body for performance concern
+		// See https://github.com/microsoft/language-server-protocol/issues/1032#issuecomment-648748013
+		String newBody = "{\n\t${0}\n}";


only use ${0} if snippets are supported by the client

fbricon · 2020-06-29T13:11:36Z

...core/src/org/eclipse/jdt/ls/core/internal/contentassist/AnonymousTypeCompletionProposal.java

+		// Construct empty body for performance concern
+		// See https://github.com/microsoft/language-server-protocol/issues/1032#issuecomment-648748013
+		String newBody = "{\n\t${0}\n}";
+
 		StringBuffer buf = new StringBuffer("new A()"); //$NON-NLS-1$


StringBuilder

Signed-off-by: Yan Zhang <[email protected]>

fbricon · 2020-06-30T08:55:14Z

Thanks @Eskibear, completion is great again!

kiennq · 2020-11-23T09:29:53Z

It seems that this has been superset with new capability resolveSupport?, do you have plan to support that?

Eskibear force-pushed the additionalEdit branch from 9e8d692 to 25f2439 Compare June 19, 2020 08:07

Eskibear changed the title ~~[WIP] fill additionalTextEdits during completionItem/resolve~~ fill additionalTextEdits during completionItem/resolve Jun 19, 2020

fbricon reviewed Jun 19, 2020

View reviewed changes

support resolve additionalTextEdits

d1b10d6

Signed-off-by: Yan Zhang <[email protected]>

Eskibear force-pushed the additionalEdit branch from 25f2439 to d1b10d6 Compare June 24, 2020 08:25

Eskibear mentioned this pull request Jun 26, 2020

support to resolve additional text edits redhat-developer/vscode-java#1503

Merged

don't fill unimplemented methods in new type constructor

8a901a9

Signed-off-by: Yan Zhang <[email protected]>

fbricon requested changes Jun 29, 2020

View reviewed changes

use placeholder only when client supports snippet

157ca61

Signed-off-by: Yan Zhang <[email protected]>

fbricon merged commit b963827 into eclipse-jdtls:master Jun 30, 2020

rrudakov mentioned this pull request Jun 30, 2020

Add resolveAdditionalTextEditsSupport capability to speed up completion emacs-lsp/lsp-java#242

Merged

fbricon added content-assist enhancement performance labels Jul 1, 2020

fbricon added this to the End June 2020 milestone Jul 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fill additionalTextEdits during completionItem/resolve #1487

fill additionalTextEdits during completionItem/resolve #1487

Eskibear commented Jun 18, 2020 •

edited

Loading

Eskibear commented Jun 19, 2020

fbricon commented Jun 19, 2020

Eskibear commented Jun 19, 2020

fbricon Jun 19, 2020

Eskibear Jun 20, 2020

fbricon commented Jun 19, 2020 •

edited

Loading

Eskibear commented Jun 20, 2020

Eskibear commented Jun 22, 2020

fbricon commented Jun 22, 2020

Eskibear commented Jun 23, 2020

fbricon commented Jun 24, 2020

Eskibear commented Jun 24, 2020

Eskibear commented Jun 25, 2020

fbricon commented Jun 25, 2020

Eskibear commented Jun 26, 2020

fbricon Jun 29, 2020

fbricon Jun 29, 2020

fbricon commented Jun 30, 2020

kiennq commented Nov 23, 2020

fill additionalTextEdits during completionItem/resolve #1487

fill additionalTextEdits during completionItem/resolve #1487

Conversation

Eskibear commented Jun 18, 2020 • edited Loading

Performance Preview

References

Eskibear commented Jun 19, 2020

fbricon commented Jun 19, 2020

Eskibear commented Jun 19, 2020

fbricon Jun 19, 2020

Choose a reason for hiding this comment

Eskibear Jun 20, 2020

Choose a reason for hiding this comment

fbricon commented Jun 19, 2020 • edited Loading

Eskibear commented Jun 20, 2020

Eskibear commented Jun 22, 2020

fbricon commented Jun 22, 2020

Eskibear commented Jun 23, 2020

fbricon commented Jun 24, 2020

Eskibear commented Jun 24, 2020

Eskibear commented Jun 25, 2020

fbricon commented Jun 25, 2020

Eskibear commented Jun 26, 2020

fbricon Jun 29, 2020

Choose a reason for hiding this comment

fbricon Jun 29, 2020

Choose a reason for hiding this comment

fbricon commented Jun 30, 2020

kiennq commented Nov 23, 2020

Eskibear commented Jun 18, 2020 •

edited

Loading

fbricon commented Jun 19, 2020 •

edited

Loading