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.

Sign up for GitHub

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

Jump to bottom

#714: Use the build cache to speed up the LS #1107

Merged
merged 4 commits into from
Jul 18, 2022
Merged

#714: Use the build cache to speed up the LS #1107

merged 4 commits into from
Jul 18, 2022

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jun 23, 2022 •
edited
Loading

Motivation

Avoid reindexing libraries on every single text document change event.

Change description

After a verify (both success and failure), the IDE2 calls the arduino.languageserver.notifyBuildDidComplete command (exposed by the VS Code extension) and pushes the most recent build_path to the LS via the custom ino/buildDidComplete JSON-RPC.

  • Use 0.25.0-rc1 CLI,
  • Use 0.0.2-beta.4 VS Code extension, and
  • Use 0.7.1 Arduino LS

Dev:

  • Updated the build scripts; can build the LS from sources besides downloading it from S3 or GH.

Other information

#714

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos changed the title WIP: Use the build cache to speed up the LS [WIP] #714: Use the build cache to speed up the LS Jun 23, 2022
@kittaakos kittaakos added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project topic: language server Related to the Arduino Language Server labels Jun 23, 2022
@kittaakos kittaakos force-pushed the #714-signed branch 5 times, most recently from a7c85b7 to 8d462f2 Compare July 12, 2022 08:19
@kittaakos kittaakos marked this pull request as ready for review July 12, 2022 08:22
@kittaakos kittaakos changed the title [WIP] #714: Use the build cache to speed up the LS #714: Use the build cache to speed up the LS Jul 12, 2022
per1234
per1234 approved these changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is working exactly as intended in all my tests.

I hope this will resolve the issues some people have experienced resulting from the resource impact of the language server, and also avoid the distress that can be caused by spurious problem detection results.

Thanks Akos and Cristian for your work on this!

fstasi
fstasi suggested changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@fstasi fstasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, only minor concerns on using kittaakos fork for the vscode-arduino-tools

arduino-ide-extension/src/browser/contributions/core-error-handler.ts
Comment on lines +14 to +15
this.errors.length = 0;
this.errors.push(...error.data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this equivalent to

Suggested change
this.errors.length = 0;
this.errors.push(...error.data);
this.errors = error.data;

performance wise is also faster

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields are readonly. Your proposal is a compiler error. Did I overlook something?

electron/build/template-package.json Outdated
@@ -141,7 +141,7 @@
"theiaPluginsDir": "plugins",
"theiaPlugins": {
"vscode-builtin-cpp": "https://open-vsx.org/api/vscode/cpp/1.52.1/file/vscode.cpp-1.52.1.vsix",
"vscode-arduino-tools": "https://downloads.arduino.cc/vscode-arduino-tools/vscode-arduino-tools-0.0.2-beta.2.vsix",
"vscode-arduino-tools": "https://github.com/kittaakos/vscode-arduino-tools/raw/realTimeDiagnostics/build-artifacts/vscode-arduino-tools-0.0.2-beta.3.vsix",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rely on https://github.com/arduino/vscode-arduino-tools ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure sure. I did not want to create an RC before testing it out.

package.json
@@ -75,7 +75,7 @@
"theiaPluginsDir": "plugins",
"theiaPlugins": {
"vscode-builtin-cpp": "https://open-vsx.org/api/vscode/cpp/1.52.1/file/vscode.cpp-1.52.1.vsix",
"vscode-arduino-tools": "https://downloads.arduino.cc/vscode-arduino-tools/vscode-arduino-tools-0.0.2-beta.2.vsix",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as before, I'd rather go with https://github.com/arduino/vscode-arduino-tools if possible

This was referenced Jul 15, 2022
Merged
Merged
Akos Kitta added 2 commits July 15, 2022 14:48
Use published VSX.
7ac2179 
Signed-off-by: Akos Kitta <[email protected]>
Use latest LS.
b4570ff 
Signed-off-by: Akos Kitta <[email protected]>
@kittaakos
Copy link
Contributor Author

The PR pinned the version of the VS Code extension and the LS. @fstasi, please review and merge the PR if you are happy with it. Thank you!

@AlbyIanna AlbyIanna requested a review from fstasi July 15, 2022 16:41
@kittaakos kittaakos merged commit 57841b3 into main Jul 18, 2022
@kittaakos kittaakos deleted the #714-signed branch July 18, 2022 08:19
@per1234 per1234 mentioned this pull request Jul 18, 2022
Closed
3 tasks
@per1234 per1234 mentioned this pull request Jul 19, 2022
Closed
@per1234 per1234 mentioned this pull request Aug 6, 2022
Closed
3 tasks
@per1234 per1234 mentioned this pull request Aug 24, 2022
Closed
@per1234 per1234 mentioned this pull request Nov 27, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@per1234 per1234 per1234 approved these changes

@fstasi fstasi fstasi approved these changes

@ubidefeo ubidefeo Awaiting requested review from ubidefeo

Assignees
No one assigned
Labels
topic: code Related to content of the project itself topic: language server Related to the Arduino Language Server type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kittaakos @fstasi @per1234