Refactor for performance improvements #146

milesziemer · 2024-03-26T14:38:56Z

Re-writes almost everything in the language server to improve performance (see #145), and lay the ground work for further progress and features. Given the scope of these changes, this should be considered a WIP as I may have broken some things.

Overview of performance improvements:

Per-file model updates on change events
Model validation only run on save
Async execution of model building and validation with cancellation
Async execution of some language features like completion and document symbol with cancellation
Incremental file change updates
Reduced file reads from disk and string copies

From the end user's perspective, only one major change has been made what the language server can do. It now uses a smithy-build.json in the root of the workspace as the source of truth for what is part of the project. Previously, the server loaded all Smithy files it found in any subdir of the root. This doesn't scale well with multi-root workspaces, and leads to an issue where Smithy files in the build directory are added to the project, duplicating sources. The new process looks for a smithy-build.json and uses only its sources and imports as files to load into the model (maven deps are still supported, this is just referring to project files). For backward compatibility, the old SmithyBuildExtensions build/smithy-dependencies.json and .smithy.json are still supported. A new file, .smithy-project.json, is being developed which allows projects that are configured outside of a smithy-build.json (such as a Gradle project) to specify their project files and dependencies. Right now these dependencies are local, paths to JARs, but it may make sense to support Maven dependencies in there as well. More to come on how .smithy-project.json works in documentation updates. To support using the language server without a smithy-build.json, a future update is in progress to allow a 'detached' project which loads whatever files you open.

Other updates:

Use smithy-syntax formatter
Report progress to client on load
Add configuration option for the minimum severity of validation events
Update dependencies

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

milesziemer · 2024-03-26T14:43:40Z

CI failing due to a missing Smithy change I had locally: smithy-lang/smithy#2214

kubukoz · 2024-03-27T21:27:29Z

I'm trying this out locally... and it doesn't seem to work very well. Probably something obvious happening at project load time that prevents the rest from moving on.

Here's what I'm trying:

./gradlew publishToMavenLocal
Setting my vscode so that it points to

"smithyLsp.lspCoordinates": "software.amazon.smithy:smithy-language-server",
"smithyLsp.version": "0.3.0"

Using the following Smithy file

$version: "2"

namespace hello

service WeatherService {
    operations: [GetWeather, CreateCity]
}

@http(method: "GET", uri: "/cities/{cityId}/weather")
@readonly
operation GetWeather {
    input := {
        @required
        @httpLabel
        cityId: CityId
    }
    output := {
        @required
        weather: String
        degrees: Integer
    }
}

@http(method: "POST", uri: "/cities", code: 201)
operation CreateCity {
    input := {
        @required
        city: String
        @required
        country: String
    }
    output := {
        @required
        cityId: CityId
    }
}

string CityId

Using the following smithy-build.json, just to be sure

{
  "version": "1.0",
  "languageServer": "software.amazon.smithy:smithy-language-server:0.3.0"
}

Here's my output tab: https://gist.github.com/kubukoz/d2158c18a10c6969f8dd9dae898d58cf

I'm not seeing a .smithy.lsp.log file, any pop-ups or other feedback that the server is running. Using https://github.com/disneystreaming/vscode-smithy/ as the client (which works fine with previous versions, including 0.2.4 from this repo).

milesziemer · 2024-03-27T22:45:29Z

@kubukoz Thanks for trying it out. I think the reason it's seemingly not working is because the smithy-build.json doesn't define any sources or imports. The lack of feedback from the server that this is the case is a really bad experience I see. I think before this PR can be considered ready I need to add support for a detached project which loads whatever you open, and/or have some indication through a diagnostic if a smithy file isn't connected to the project.

The PR description has more info on this:

From the end user's perspective, only one major change has been made what the language server can do. It now uses a smithy-build.json in the root of the workspace as the source of truth for what is part of the project. Previously, the server loaded all Smithy files it found in any subdir of the root. This doesn't scale well with multi-root workspaces, and leads to an issue where Smithy files in the build directory are added to the project, duplicating sources. The new process looks for a smithy-build.json and uses only its sources and imports as files to load into the model (maven deps are still supported, this is just referring to project files).

I'm open to suggestions on alternative ways to figure out which files the language server should load as part of the project. This method works essentially the same as how the smithy-cli works, only going off of smithy-build.json (or paths passed as arguments in the case of the cli). The intent is to make the language server load the project the same way the build tool would. If the build tool doesn't use smithy-build.json at all, or doesn't use it as its source of truth for what files exist in the smithy project, that's where the .smithy-project.json comes in.

kubukoz · 2024-03-28T11:35:48Z

Interesting, it didn't work for me in a project with imports either. I'll try again in the minimal setup but with sources/imports.

kubukoz · 2024-03-28T17:29:03Z

ok, the minimal example works with sources. I have a .smithy.lsp.log now, and features like navigation are working.

I made the following next steps:

renamed my imports to sources in The Big Project (with imports the CLI wasn't having it when calling build, I suppose I'd been using that key incorrectly)
successfully ran build in the smithy CLI 1.45.0 (same one as used here), with no errors

and it's still returning empty sets in all the LSP responses 🤔

any ideas?

milesziemer · 2024-03-28T19:38:15Z

@kubukoz Hmm, could you paste a version of the smithy-build.json where imports didn't work? For The Big Project, is it being opened in the same directory as the smithy-build.json?

milesziemer · 2024-03-28T19:47:48Z

Also, if you update your client to send

progressOnInitialization: true

in the initialize request, you should get some indication of the progress of the server loading the project. If you're able to paste any server trace, that would be helpful too, but understand if you can't.

Re-writes almost everything in the language server to improve performance, and lay the ground work for further progress and features. Given the scope of these changes, this should be considered a WIP as I may have broken some things. Overview of performance improvements: - Per-file model updates on change events - Model validation only run on save - Async execution of model building and validation with cancellation - Async execution of some language features like completion and document symbol with cancellation - Incremental file change updates - Reduced file reads from disk and string copies From the end user's perspective, only one major change has been made what the language server can do. It now uses a smithy-build.json in the root of the workspace as the source of truth for what is part of the project. Previously, the server loaded all Smithy files it found in any subdir of the root. This doesn't scale well with multi-root workspaces, and leads to an issue where Smithy files in the build directory are added to the project, duplicating sources. The new process looks for a smithy-build.json and uses only its `sources` and `imports` as files to load into the model (maven deps are still supported, this is just referring to project files). For backward compatibility, the old SmithyBuildExtensions `build/smithy-dependencies.json` and `.smithy.json` are still supported. A new file, `.smithy-project.json`, is being developed which allows projects that are configured outside of a smithy-build.json (such as a Gradle project) to specify their project files _and_ dependencies. Right now these dependencies are local, paths to JARs, but it may make sense to support Maven dependencies in there as well. More to come on how `.smithy-project.json` works in documentation updates. To support using the language server without a smithy-build.json, a future update is in progress to allow a 'detached' project which loads whatever files you open. Other updates: - Use smithy-syntax formatter - Report progress to client on load - Add configuration option for the minimum severity of validation events - Update dependencies

This makes the language server work on files that aren't connected (or attached) to a smithy-build.json/other build file. This works by loading said files as they are opened in their own, single-file projects with no dependencies, which are removed when the file is closed. A diagnostic was also added to indicate when a file is 'detached' from a project, and appears on the first line of the file. I could have made all detached files part of their own special project, could be more convenient when doing something quick with multiple files without a smithy-build.json. The smithy cli can work this way, although you still have to specify the files to build in the command, so we could change this in the future. The difference is I don't think we'd have a way of opting out of the single project without some config that would end up being more work to set up than a smithy-build.json.

milesziemer · 2024-04-08T13:38:19Z

Update: latest commit adds support for files not attached to a smithy-build.json or other build file: 6fce052

kubukoz · 2024-04-23T21:09:20Z

I've got it working, sources was using "./smithy" rather than "smithy". The former worked in the CLI so I assumed it'd work here 😅

some more thoughts/problems/suggestions:

looks like shape references in traits/metadata don't have navigation anymore. for example, metadata foo = [smithy.api#String] -> that would be nice to fix, but it's absolutely fine to leave it out for another PR. It also doesn't seem to happen in trait nodes, e.g. protocol definitions.
smithy-language-server version 0.3.0 started. is sent as window/logMessage, I suggest turning it into a showMessage as it was before: it's useful to see it as a notification to confirm things are working. "project loaded with N jars" was also very helpful on (re)loads
it's nice to see navigation working in errors: and inline I/O :)

kubukoz · 2024-04-23T21:13:02Z

Seeing this guy too:

[Error - 23:12:15] Request textDocument/documentSymbol failed.
Error: name must not be falsy
    at Function.validate (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:150:17471)
    at new d (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:150:17766)
    at asDocumentSymbol (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/protocolConverter.js:604:22)
    at convertBatch (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/utils/async.js:193:25)
    at Object.map (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/utils/async.js:202:17)
    at Object.asDocumentSymbols (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/protocolConverter.js:601:22)
    at _provideDocumentSymbols (/Users/kubukoz/.vscode/extensions/disneystreaming.smithy-0.0.13/node_modules/vscode-languageclient/lib/common/documentSymbol.js:83:76)
    at d.provideDocumentSymbols (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:152:90504)

Could be something wrong with the client, I don't think we've updated vscode-languageclient in that client in a while 🤔

kubukoz · 2024-04-25T22:37:38Z

Another one: metadata seems to be cached between model reloads.

To reproduce, you can simply create a file with e.g. metadata foo = 2, save, then change the value to anything else and save again.

Fixes an issue where the server can't find project files when smithy-build.json has unnormalized paths.

kubukoz · 2024-04-26T17:22:42Z

Managed to reproduce some more issues... here's a 2-in-one:

Screen.Recording.2024-04-26.at.19.20.05.mov

For some reason, when creating new files and typing things before adding a namespace, sometimes the LSP refuses to see that the file is eventually valid. It doesn't seem to happen every time I "simply" start typing, though, so probably a race condition is involved.
reported as a formatter bug: Formatter is not idempotent, keeps adding comments smithy#2261

kubukoz · 2024-05-16T18:37:02Z

Found a new regression: references aren't found in use clauses. I don't think it's a blocker, but worth noting somewhere.

The performance refactor caused a regression where making changes to a file with metadata would cause that metadata key to be duplicated. Since the server used to reload all files on a change, we never had to worry about this, but since we're trying to now only reload single files, we need to remove any metadata in that file from the model before reloading. This works similarly to how we collect per-file shapes, but is slightly more complex because array metadata with the same key get merged into a single array (as opposed to non-arrays, which cause an error when there are the same keys).

This commit makes updates to how we manage files that are opened, and what happens when you add/remove files. Previously, if you moved a detached file into your project, the server would load that file from disk, rather than using the in-memory Document. More test cases were added around adding/moving detached files. Also made it so that diagnostics are re-reported back to the client for open files after a reload.

Fixes an issue where basically any time you created a file not attached to a project, it wouldn't ever be loaded properly and any updates wouldn't register. This happened because the server wasn't creating a SmithyFile for a detached file if it had no shapes in it. The SmithyFile is created only when the project is initialized, so subsequent changes wouldn't fix it.

Also adds some test cases for moving around detached and/or broken files.

The partial loading strategy, which is meant to reduce the amount of unnecessary re-parsing of unchanged files on every file change, works by removing shapes defined in the file that has changed. But this wasn't taking into account traits applied using `apply` in other files. When a shape is removed, all the traits are removed, and the `apply` isn't persisted. So we need to keep track of all trait applications that aren't in the same file as the shape def. Additionally, list traits have their values merged, so we actually need to either partially remove a trait (i.e. only certain elements), or just reload all the files that contain part of the trait's value. This implementation does the latter, which also requires creating a sort of dependency graph of which files need other files to be loaded with them. There's likely room for optimization here (potentially switching to the first approach), but we will have to guage the performance. This commit also consolidates the project updating logic for adding, removing, and changing files into a single method of Project, and adds a bunch of tests around different situations of `apply`.

Fixes an issue where if you did the following: 1. Created a valid project (ok smithy-build.json, loaded model) 2. Made the smithy-build.json invalid (e.g by adding an invalid dep) 3. Fixed the smithy-build.json The project would not recover, and any open project files would be lost to the server.

This commit makes it so you can manually type out a use statement, and get completions for the absolute shape id. It also adds support for completion/definition/hover for absolute shape ids in general.

etspaceman · 2024-06-12T16:28:50Z

Seems this PR doesn't like the windows paths due to the colons in the beginning. Any thoughts on how we could fix that one @milesziemer ?

etspaceman · 2024-06-12T16:40:09Z

https://stackoverflow.com/questions/52755854/java-nio-file-invalidpathexception-illegal-char-when-using-paths-get

Seems that we may just need to remove that leading / character on windows machines.

Previously, Document used System.lineSeparator() for figuring out where line starts would be (index of linesep + 1). But if the file was created (and, say, packaged in a jar) on another OS, it would have different lineseps. This change makes use of a simple fact I overlooked in the initial implementation, which was that '\n' is still the last character on each line, so we don't need to break on System.lineSeparator(), just on newline (unless there's still some OS using '\r' only line breaks). UriAdapter was updated to handle windows URIs, which would be made into invalid paths with a leading '/' after removing 'file://'. A bunch of test cases were also updated, which essentially all had one or both of the above problems.

milesziemer · 2024-06-13T19:53:37Z

@etspaceman Thanks - I've pushed a few updates that address the windows issues in CI.

@kubukoz I think I've pushed fixes for all the issues and regressions you found (and some other issues I found), except for #146 (comment) - I couldn't reproduce but I added a log that will hopefully give us some more info if it happens again (if not, maybe it was just the client). Thanks for the help.

kubukoz · 2024-06-14T09:52:43Z

Awesome! Looks like Windows support is much better now. I'm looking forward to seeing this merged!

.github/workflows/ci.yml

src/main/java/software/amazon/smithy/lsp/DocumentLifecycleManager.java

src/main/java/software/amazon/smithy/lsp/Main.java

src/main/java/software/amazon/smithy/lsp/SmithyLanguageClient.java

src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java

src/main/java/software/amazon/smithy/lsp/document/Document.java

src/main/java/software/amazon/smithy/lsp/project/ProjectConfigLoader.java

src/main/java/software/amazon/smithy/lsp/DocumentLifecycleManager.java

src/main/java/software/amazon/smithy/lsp/Main.java

src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java

src/main/java/software/amazon/smithy/lsp/diagnostics/DetachedDiagnostics.java

hpmellema · 2024-07-25T16:02:48Z

src/main/java/software/amazon/smithy/lsp/handler/CompletionHandler.java

+
+public final class CompletionHandler {
+    // TODO: Handle keyword completions
+    private static final List<String> KEYWORDS = Arrays.asList("bigDecimal", "bigInteger", "blob", "boolean", "byte",


Could you provide an example of what you mean by an attached snippet?

src/main/java/software/amazon/smithy/lsp/handler/DefinitionHandler.java

src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java

src/main/java/software/amazon/smithy/lsp/handler/CompletionHandler.java

src/main/java/software/amazon/smithy/lsp/project/SmithyFileDependenciesIndex.java

src/test/java/software/amazon/smithy/lsp/LspMatchers.java

milesziemer requested a review from a team as a code owner March 26, 2024 14:38

milesziemer requested a review from kstich March 26, 2024 14:38

milesziemer added 2 commits April 6, 2024 20:10

milesziemer force-pushed the performance-refactor branch from 56eb604 to 6fce052 Compare April 8, 2024 13:36

Normalize paths in the project loader

53c688e

Fixes an issue where the server can't find project files when smithy-build.json has unnormalized paths.

kubukoz mentioned this pull request May 16, 2024

IOOBE in project loading prevents any analysis #151

Closed

milesziemer added 7 commits June 6, 2024 14:12

Refactor Project to have its ProjectConfig

c9193ad

Also adds some test cases for moving around detached and/or broken files.

Support direct use completions, absolute shape ids

cfe22db

This commit makes it so you can manually type out a use statement, and get completions for the absolute shape id. It also adds support for completion/definition/hover for absolute shape ids in general.

milesziemer force-pushed the performance-refactor branch from 603d209 to 4024cee Compare June 13, 2024 02:00

milesziemer force-pushed the performance-refactor branch 3 times, most recently from e469ee2 to 6472eef Compare June 13, 2024 10:56

milesziemer force-pushed the performance-refactor branch from 2eebc80 to c9c751d Compare June 13, 2024 15:46

milesziemer added 2 commits June 13, 2024 13:20

fix a flaky test

2ae801a

add log for missing name in DocumentSymbol

d467979

milesziemer force-pushed the performance-refactor branch from 66e6279 to d467979 Compare June 13, 2024 19:46

hpmellema requested changes Jun 21, 2024

View reviewed changes

address first round of comments

ec70b28

hpmellema self-requested a review July 23, 2024 18:23

hpmellema reviewed Jul 25, 2024

View reviewed changes

milesziemer commented Jul 25, 2024

View reviewed changes

src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java Outdated Show resolved Hide resolved

milesziemer commented Jul 25, 2024

View reviewed changes

src/main/java/software/amazon/smithy/lsp/handler/CompletionHandler.java Outdated Show resolved Hide resolved

milesziemer commented Jul 25, 2024

View reviewed changes

src/main/java/software/amazon/smithy/lsp/project/SmithyFileDependenciesIndex.java Outdated Show resolved Hide resolved

milesziemer commented Jul 25, 2024

View reviewed changes

src/test/java/software/amazon/smithy/lsp/LspMatchers.java Show resolved Hide resolved

address rest of comments

00f772e

hpmellema approved these changes Jul 26, 2024

View reviewed changes

milesziemer merged commit 517ffe3 into smithy-lang:main Jul 26, 2024
9 checks passed

This was referenced Jul 26, 2024

Use smithy official formatter #139

Closed

Store model text in memory #121

Closed

kubukoz mentioned this pull request Jul 27, 2024

Pull upstream changes disneystreaming/smithy-language-server#53

Merged

milesziemer mentioned this pull request Aug 19, 2024

Performance problems in large codebases #145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor for performance improvements #146

Refactor for performance improvements #146

milesziemer commented Mar 26, 2024 •

edited

Loading

milesziemer commented Mar 26, 2024 •

edited

Loading

kubukoz commented Mar 27, 2024 •

edited

Loading

milesziemer commented Mar 27, 2024

kubukoz commented Mar 28, 2024

kubukoz commented Mar 28, 2024

milesziemer commented Mar 28, 2024 •

edited

Loading

milesziemer commented Mar 28, 2024

milesziemer commented Apr 8, 2024

kubukoz commented Apr 23, 2024

kubukoz commented Apr 23, 2024

kubukoz commented Apr 25, 2024

kubukoz commented Apr 26, 2024 •

edited

Loading

kubukoz commented May 16, 2024

etspaceman commented Jun 12, 2024

etspaceman commented Jun 12, 2024

milesziemer commented Jun 13, 2024

kubukoz commented Jun 14, 2024

hpmellema Jul 25, 2024

Refactor for performance improvements #146

Refactor for performance improvements #146

Conversation

milesziemer commented Mar 26, 2024 • edited Loading

milesziemer commented Mar 26, 2024 • edited Loading

kubukoz commented Mar 27, 2024 • edited Loading

milesziemer commented Mar 27, 2024

kubukoz commented Mar 28, 2024

kubukoz commented Mar 28, 2024

milesziemer commented Mar 28, 2024 • edited Loading

milesziemer commented Mar 28, 2024

milesziemer commented Apr 8, 2024

kubukoz commented Apr 23, 2024

kubukoz commented Apr 23, 2024

kubukoz commented Apr 25, 2024

kubukoz commented Apr 26, 2024 • edited Loading

kubukoz commented May 16, 2024

etspaceman commented Jun 12, 2024

etspaceman commented Jun 12, 2024

milesziemer commented Jun 13, 2024

kubukoz commented Jun 14, 2024

hpmellema Jul 25, 2024

Choose a reason for hiding this comment

milesziemer commented Mar 26, 2024 •

edited

Loading

milesziemer commented Mar 26, 2024 •

edited

Loading

kubukoz commented Mar 27, 2024 •

edited

Loading

milesziemer commented Mar 28, 2024 •

edited

Loading

kubukoz commented Apr 26, 2024 •

edited

Loading