Cache and re-use type bindings for a completion invocation #2535

snjeza · 2023-03-14T20:39:23Z

A related issue - redhat-developer/vscode-java#2982

Steps to reproduce

$ git clone https://github.com/snjeza/jhipster-lite
$ cd jhipster-lite
$ code .

enable lombok
open AssertTest.java

private void test() {
    D d = new D| // try CA here
  }

See redhat-developer/vscode-java#2982 (comment)

rgrunber · 2023-03-30T01:55:21Z

The improvement here is huge. Goes from ~28s to ~2s on my machine, for completion response. Just 2 main questions for now :

Jinbo has an old patch at https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/191217 . Would that also improve this situation ? Would that allow us potentially undo this patch later ?
I didn't see the code action response taking long. Is the cancellation really needed for calculating each one ? I would assume some code actions might require re-parsing of certain source files, but wasn't seeing performance degradation there.

jdneo · 2023-03-30T02:28:22Z

Lazy resolving is a powerful tool to solve the problem. Just one thing I would like to mention: For now, updating the text edit during resolving is still not recommended in the spec, if we decide to go that way, we are on our own for the correctness.

Personally, I think it's worth trying - considering the huge amount of boost we can get (~28s to ~2s). I believe the time cost can be even faster if we use the approach to other cases. But in case that one day the behavior of VS Code changes, which might break our completion functions. I would suggest that we introduce a switch of the completion behavior. The switch controls whether we will lazily resolve the completions. In that way, at least we have a fall back if the regression happens in the future.

jdneo · 2023-03-30T02:35:55Z

I have same question as @rgrunber has for the code action part. Are all the cancellation checks necessary? How does the perf looks like if we only keep the change for the completion resolving part?

What about separating the change of adding cancellation checks for code actions to another PR? Because they are trying to solve the perf issue from different aspects.

snjeza · 2023-03-30T02:49:27Z

Jinbo has an old patch at https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/191217 . Would that also improve this situation ? Would that allow us potentially undo this patch later ?

I will check this patch, but it isn't related to this patch.

I didn't see the code action response taking long. Is the cancellation really needed for calculating each one ? I would assume some code actions might require re-parsing of certain source files, but wasn't seeing performance degradation there.

What about separating the change of adding cancellation checks for code actions to another PR? Because they are trying to solve the perf issue from different aspects.

I will separate them.

snjeza · 2023-03-30T03:14:57Z

@rgrunber @jdneo @CsCherrYY I have updated the PR. It includes only the completion resolving part.

snjeza · 2023-03-30T03:30:09Z

Lazy resolving is a powerful tool to solve the problem. Just one thing I would like to mention: For now, updating the text edit during resolving is still not recommended in the spec, if we decide to go that way, we are on our own for the correctness.

A related issue - #2453

rgrunber · 2023-03-30T03:36:45Z

Lazy resolving is a powerful tool to solve the problem. Just one thing I would like to mention: For now, updating the text edit during resolving is still not recommended in the spec, if we decide to go that way, we are on our own for the correctness.

A related issue - #2453

#2453 (comment) implies we should be ok to lazily resolve. Either way, if we're doing it in other places, we should be fine to at least do it here also.

Update: Jinbo's patch was about caching the type argument info for a constructor completion proposal. This patch completely avoid computing that until the resolve phase. That's why I thought maybe when we get that patch approved, it could provide some performance improvement without having to resolve lazily.

jdneo · 2023-03-30T05:34:53Z

#2453 (comment) implies we should be ok to lazily resolve. Either way, if we're doing it in other places, we should be fine to at least do it here also.

@rgrunber Well, my comment in that issue was not correct. (After I thought about it twice)😥

My understanding of the Dirk's response is that:

The spec does not have any restrictions on what data cannot be resolved during completion resolve phase.
The choice leaves to the client implementation. To be specific, it's controlled by the resolveSupport field sent from client to server. In the case of VS Code, the supported properties are:

"resolveSupport": {
    "properties": [
        "documentation",
        "detail",
        "additionalTextEdits"
    ]
},

Which means we are violating the spec if we update the textEdit during completion/resolve.

AFAIK, besides this PR and #2453, the generic server side snippet is also lazy resolved. Luckily, they are working fine in most cases. But there is no guarantee they will still work in the future.

That's why I suggest having a setting to control the resolve behavior. With that setting exist, I'm feeling safe to leverage lazy resolve approach to more cases.

snjeza · 2023-03-30T13:54:14Z

That's why I suggest having a setting to control the resolve behavior. With that setting exist, I'm feeling safe to leverage lazy resolve approach to more cases.

@jdneo @rgrunber We should add textEdit to resolveSupport in vscode-java.
See #2453 (comment)

rgrunber · 2023-03-30T14:32:40Z

@snjeza , so could we modify this change to check the resolve support capability contains the textEdit property ? Then clients can enable/disable.

snjeza · 2023-03-30T14:44:11Z

@rgrunber Right. See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionClientCapabilities

/**
		 * Indicates which properties a client can resolve lazily on a
		 * completion item. Before version 3.16.0 only the predefined properties
		 * `documentation` and `detail` could be resolved lazily.
		 *
		 * @since 3.16.0
		 */
		resolveSupport?: {
			/**
			 * The properties that a client can resolve lazily.
			 */
			properties: string[];
		};

rgrunber · 2023-04-03T21:21:14Z

@snjeza your new change looks to save the same amount of time as before 🎉 . It makese sense to cache the known bindings for that particular instance of completion activation. However I'm not seeing any major benefit for publishing diagnostics. I'll try to see how much time is spent in that block of code you've removed. Looking at it, I'm not sure why we went from ICompilationUnit to CompilationUnit back to ICompilationUnit :\ Definitely seems simpler to remove it.

Update There's might be some improvement for publishDiagnostics. I can definitely agree that the block took a few hundred ms to run (from timing it), but comparing before/after shows only around ~100ms improvement.

Old: 604 638 669 699 714 748 786 823 923 961 1042 1255 5345
New: 463 541 560 621 650 656 661 680 728 740 767 1196 4240

snjeza · 2023-04-03T22:38:28Z

@rgrunber Could you try the following

$ rm -rf jhisper-lite
$ git clone https://github.com/snjeza/jhipster-lite
$ cd jhipster-lite
$ code .

enable lombok
open AssertTest.java

private void test() {
    D d = new D| // try CA here
   // type
  Model56 model56 = new Model56();
  model56.getWeight()

  }

check line Validated X. Took XXX ms

You should test it using VS Code Pre-Release and VS Code Java 1.17.3

snjeza · 2023-04-03T22:43:03Z

I'm not sure why we went from ICompilationUnit to CompilationUnit back to ICompilationUnit :\ Definitely seems simpler to remove it.

We used AST before we separated publishDiagnostics and introduced reconcile at https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java#L306

rgrunber · 2023-04-03T23:00:21Z

I'll try again with your suggestions. FWIW I did use the validation line when comparing. Either way, I'll have a closer look, and hopefully we can merge since the change should be more acceptable now.

testforstephen · 2023-04-04T02:41:52Z

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

+		if (bindings.get(keys[0]) instanceof ITypeBinding) {
+			return (ITypeBinding) bindings.get(keys[0]);
+		}


To keep consistent as previous implementation, it can be

if (bindings.size() > 0) { return (ITypeBinding) bindings.get(keys[0]); }

testforstephen · 2023-04-04T02:42:53Z

The publish diagnostics and typebinding cache are two different optimization, you may split them to different PRs. The typebinding cache is pretty straightforward, it's safe to merge first.

snjeza · 2023-04-04T14:48:23Z

The publish diagnostics and typebinding cache are two different optimization, you may split them to different PRs. The typebinding cache is pretty straightforward, it's safe to merge first.

See #2574

snjeza · 2023-04-04T14:50:51Z

@fbricon @rgrunber @testforstephen @jdneo @CsCherrYY I have updated the PR.
You can also review #2574

rgrunber

Looks good to me. Could you change the commit title to :

Cache and re-use type bindings for a completion invocation

Then feel free to merge.

Remove CoreASTProvider.getAST(...) from BaseDocumentLifeCycleHandler.publishDiagnostics(IProgressMonitor) - eclipse-jdtls#2535 Signed-off-by: Snjezana Peco <[email protected]>

snjeza · 2023-04-04T17:59:12Z

test this please

Signed-off-by: Snjezana Peco <[email protected]>

Remove CoreASTProvider.getAST(...) from BaseDocumentLifeCycleHandler.publishDiagnostics(IProgressMonitor) - eclipse-jdtls#2535 Signed-off-by: Snjezana Peco <[email protected]>

Remove CoreASTProvider.getAST(...) from BaseDocumentLifeCycleHandler.publishDiagnostics(IProgressMonitor) - #2535 Signed-off-by: Snjezana Peco <[email protected]>

snjeza added the in progress label Mar 14, 2023

snjeza requested review from gorkem, fbricon, rgrunber, jdneo, testforstephen and CsCherrYY March 14, 2023 20:39

snjeza changed the title ~~[WIP] Performance improvement when lombok is present~~ Performance improvement when lombok is present Mar 15, 2023

rgrunber assigned rgrunber and snjeza Mar 22, 2023

rgrunber added this to the Early April 2023 milestone Mar 30, 2023

snjeza changed the title ~~Performance improvement when lombok is present~~ [WIP] Performance improvement when lombok is present Mar 30, 2023

snjeza force-pushed the issue-2982 branch from 4b44180 to db0e365 Compare March 30, 2023 03:12

snjeza changed the title ~~[WIP] Performance improvement when lombok is present~~ Performance improvement when lombok is present Mar 30, 2023

snjeza removed the in progress label Mar 30, 2023

snjeza force-pushed the issue-2982 branch 2 times, most recently from 9ad00f3 to 4e8ad3a Compare March 30, 2023 03:54

snjeza mentioned this pull request Mar 30, 2023

Define completionItem@resolveSupport redhat-developer/vscode-java#3034

Closed

snjeza force-pushed the issue-2982 branch from 4e8ad3a to f133273 Compare March 30, 2023 21:12

testforstephen reviewed Apr 4, 2023

View reviewed changes

snjeza changed the title ~~Performance improvement:constructor CA and publish diagnostics~~ [WIP] Performance improvement:constructor CA and publish diagnostics Apr 4, 2023

snjeza added the in progress label Apr 4, 2023

snjeza force-pushed the issue-2982 branch 2 times, most recently from fc6db2b to c0fcfe0 Compare April 4, 2023 14:47

snjeza changed the title ~~[WIP] Performance improvement:constructor CA and publish diagnostics~~ Performance improvement:constructor CA and publish diagnostics Apr 4, 2023

snjeza removed the in progress label Apr 4, 2023

snjeza changed the title ~~Performance improvement:constructor CA and publish diagnostics~~ Performance improvement:constructor CA Apr 4, 2023

rgrunber added the performance label Apr 4, 2023

rgrunber removed their assignment Apr 4, 2023

rgrunber approved these changes Apr 4, 2023

View reviewed changes

rgrunber mentioned this pull request Apr 4, 2023

Avoid retrieving AST root during diagnostic publishing. #2574

Merged

snjeza force-pushed the issue-2982 branch from c0fcfe0 to f4cfbda Compare April 4, 2023 17:27

snjeza changed the title ~~Performance improvement:constructor CA~~ Cache and re-use type bindings for a completion invocation Apr 4, 2023

Cache and re-use type bindings for a completion invocation

8d40742

Signed-off-by: Snjezana Peco <[email protected]>

snjeza force-pushed the issue-2982 branch from f4cfbda to 8d40742 Compare April 4, 2023 18:08

snjeza merged commit c73f7a2 into eclipse-jdtls:master Apr 4, 2023

jdneo mentioned this pull request Apr 11, 2023

Use a flag to control whether the textEdit of completion will be calculated in resolve request #2584

Closed

rgrunber mentioned this pull request Apr 19, 2023

Slow builds due to large number of project files redhat-developer/vscode-java#2982

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache and re-use type bindings for a completion invocation #2535

Cache and re-use type bindings for a completion invocation #2535

snjeza commented Mar 14, 2023 •

edited

Loading

rgrunber commented Mar 30, 2023

jdneo commented Mar 30, 2023

jdneo commented Mar 30, 2023

snjeza commented Mar 30, 2023

snjeza commented Mar 30, 2023

snjeza commented Mar 30, 2023

rgrunber commented Mar 30, 2023 •

edited

Loading

jdneo commented Mar 30, 2023

snjeza commented Mar 30, 2023

rgrunber commented Mar 30, 2023

snjeza commented Mar 30, 2023

rgrunber commented Apr 3, 2023 •

edited

Loading

snjeza commented Apr 3, 2023 •

edited

Loading

snjeza commented Apr 3, 2023 •

edited

Loading

rgrunber commented Apr 3, 2023

testforstephen Apr 4, 2023 •

edited

Loading

snjeza Apr 4, 2023

testforstephen commented Apr 4, 2023

snjeza commented Apr 4, 2023

snjeza commented Apr 4, 2023

rgrunber left a comment

snjeza commented Apr 4, 2023

Cache and re-use type bindings for a completion invocation #2535

Cache and re-use type bindings for a completion invocation #2535

Conversation

snjeza commented Mar 14, 2023 • edited Loading

rgrunber commented Mar 30, 2023

jdneo commented Mar 30, 2023

jdneo commented Mar 30, 2023

snjeza commented Mar 30, 2023

snjeza commented Mar 30, 2023

snjeza commented Mar 30, 2023

rgrunber commented Mar 30, 2023 • edited Loading

jdneo commented Mar 30, 2023

snjeza commented Mar 30, 2023

rgrunber commented Mar 30, 2023

snjeza commented Mar 30, 2023

rgrunber commented Apr 3, 2023 • edited Loading

snjeza commented Apr 3, 2023 • edited Loading

snjeza commented Apr 3, 2023 • edited Loading

rgrunber commented Apr 3, 2023

testforstephen Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

snjeza Apr 4, 2023

Choose a reason for hiding this comment

testforstephen commented Apr 4, 2023

snjeza commented Apr 4, 2023

snjeza commented Apr 4, 2023

rgrunber left a comment

Choose a reason for hiding this comment

snjeza commented Apr 4, 2023

snjeza commented Mar 14, 2023 •

edited

Loading

rgrunber commented Mar 30, 2023 •

edited

Loading

rgrunber commented Apr 3, 2023 •

edited

Loading

snjeza commented Apr 3, 2023 •

edited

Loading

snjeza commented Apr 3, 2023 •

edited

Loading

testforstephen Apr 4, 2023 •

edited

Loading