Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid retrieving AST root during diagnostic publishing. #2574

Merged
merged 1 commit into from Apr 6, 2023

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Apr 4, 2023

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

Steps to reproduce

$ 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

Test vsix 1.17.3

@rgrunber
Copy link
Contributor

rgrunber commented Apr 5, 2023

I can confirm the the vsix reduces the time to publish diagnostics from a few seconds to a few hundred ms with the vsix. However, this doesn't happn when I build and run against the PR. I even tried with the exact same settings. I think the change is fine to merge, but why the difference ?

Update: Also please rename the commit title to something like Avoid retrieving AST root during diagnostic publishing.

Remove CoreASTProvider.getAST(...) from BaseDocumentLifeCycleHandler.publishDiagnostics(IProgressMonitor)
- eclipse-jdtls#2535

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
@snjeza
Copy link
Contributor Author

snjeza commented Apr 5, 2023

Update: Also please rename the commit title to something like Avoid retrieving AST root during diagnostic publishing.

Fixed

@snjeza snjeza changed the title Performance improvement:publish diagnostics Avoid retrieving AST root during diagnostic publishing. Apr 5, 2023
@snjeza
Copy link
Contributor Author

snjeza commented Apr 5, 2023

However, this doesn't happn when I build and run against the PR. I even tried with the exact same settings.

17.1.3 vsix also includes #2535

@rgrunber
Copy link
Contributor

rgrunber commented Apr 6, 2023

I think I was rebasing your patch to be on top of the latest commit. Even now I'm seeing validation take a few seconds whereas with your vsix it's a few hundred ms.

Update: I'll merge, but it would be good to verify why I'm seeing this.

@rgrunber rgrunber merged commit bc1a26a into eclipse-jdtls:master Apr 6, 2023
6 checks passed
@snjeza
Copy link
Contributor Author

snjeza commented Apr 6, 2023

Update: I'll merge, but it would be good to verify why I'm seeing this.
... with your vsix it's a few hundred ms.

Did you enabled lombok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants