Reuse jdt.ls workspace #2970

svor · 2018-09-25T14:22:36Z

Signed-off-by: Valeriy Svydenko vsvydenk@redhat.com

The PR suggests to reuse jdt.ls' workspace multiple times.
The jdt.ls workspace has schema: _ws_<opened-theia-workspace-sha1>_<0..n>

If we have 2 opened instances of Theia with same workspace, we'll have 2 jdt.ls' workspaces:
/tmp/_ws_sha1ValueOfTheiaWs_0
/tmp/_ws_sha1ValueOfTheiaWs_1

Related issue: eclipse-che/che#10325

svor · 2018-09-25T14:23:20Z

@tsmaeder @akosyakov Take a look please

svenefftinge · 2018-09-26T06:10:53Z

This should be solved in the language server itself. The initialize sends the workspace uri and the jdt.ls should internally do the meta data reuse based on the path name. At least the parameter for the meta data should be part of an extended initialize method and not be a program argument as it is closely related to the workspaceroot(s).

Doesn't that same issue exist with jdt.ls when used from VS Code? CC @fbricon

svenefftinge · 2018-09-26T06:11:40Z

packages/java/src/node/java-contribution.ts

        const jarPath = path.resolve(serverPath, jarPaths[0]);
-        const workspacePath = path.resolve(os.tmpdir(), '_ws_' + new Date().getTime());
+
+        const uri = await this.workspaceService.getMostRecentlyUsedWorkspace();


This is only by luck returning the expected root. I.e. if another window connected on different workspace in the meantime it will not be what you expect.

I guess if we're reusing, somewhere in user home would be appropriate?

@svenefftinge It works as I expect when Theia already started and I just open another window on different workspace. But yes, if i have two windows on different workspaces and Theia start initializing - I got the some value for both instances.
Does Theia already have some way to get current workspace's name or URI?

@tsmaeder as an option we can save it in os.homedir() + .theia/

Does Theia already have some way to get current workspace's name or URI?

Yes, however each frontend application is the one aware of what its own current workspace is - the backend doesn't know or keep track of this. This way, there can be multiple frontends, each with its own independently-set workspace (it's permitted to re-use a WS between clients), using the same stateless backend.

One way where jdt.ls may differ from other LS we use is that it probably does not permit sharing a workspace between two or more clients. So for that case, we might still need to have different jdt.ls workspaces. However when a given client is reloaded, it would make sense for it to re-use the same jdt.ls workspace as before.

if we want to eventually share language server instances

Interesting. Would a single jdt.ls instance be able to simultaneously serve two or more LS clients, for the same workspace?

No, it would not: you can't send the initialize message twice, for example. Also, there is no provision for opening multiple communication channels (socket or otherwise). Such multiplexing has to happen outside of the language server.

One way where jdt.ls may differ from other LS we use is that it probably does not permit sharing a workspace between two or more clients.

Actually, I would expect this to be the case for many non-trivial languages servers: there would a some metadata stored and cached by the language server (think symbol index), and if the language server derives from some IDE tool (as jdt.ls), it probably assumes that it's working alone on that metadata.

No, it would not: you can't send the initialize message twice, for example. Also, there is no provision for opening multiple communication channels (socket or otherwise). Such multiplexing has to happen outside of the language server.

One could start one language server process that accepts many connections and then the language server internally could do all kinds of sharing and caching.

One could start one language server process that accepts many connections and then the language server internally could do all kinds of sharing and caching.
Yes, one could, but currently, language servers are not written that way.

tsmaeder · 2018-09-26T07:59:01Z

@svenefftinge I am not sure about jdt.ls doing this itself: for one, it does not know about other instances of jdt.ls running. Also, the concurrency needs depend on the environment: Theia and VSCode have a model where you open an arbitrary folder, whereas Eclipse IDE or Che have a fixed workspace root.

fbricon · 2018-09-26T13:03:26Z

jdt.ls being an Eclipse application, AFAIK, the only way to define the workspace location is as a command line argument. I don't think you can start the app and then change the workspace location. But I'd be happy to be proven wrong.

tsmaeder · 2018-09-29T08:39:08Z

Having thought about this a bit more, I had a couple of thoughts I think might be of interest:

Each copy of jdt.ls running needs to have it's own data folder. However, if the projects in that eclipse workspace are not the ones it has seen on previous startups, it will delete and add projects as needed. This means that reusing the same workspace between runs of the ide is a performance issue, not a correctness issue.
When I open a second Theia front end with the same back end, I can start with a different workspace and open different folders. If you open the same file in both copies, would you expect the unsaved changes in one editor to be reflected in the other IDE? Or is a browser window always a completely separate IDE with no shared state except the files in the workspace? What happens with user preferences, what about saving IDE state (for the browser refresh case)?
The "one back end, multiple front ends" case does not apply to Che. Our users would expect that opening a second tab is just accessing the same back end with shared state.

marcdumais-work · 2018-09-29T12:37:52Z

When I open a second Theia front end with the same back end, I can start with a different workspace and open different folders. If you open the same file in both copies, would you expect the unsaved changes in one editor to be reflected in the other IDE?

Yes. This should work today already. Both clients will be watching the file. The second client will re-read the file when notified by the watcher. This works also if the file is modified outside of a Theia editor.

Or is a browser window always a completely separate IDE with no shared state except the files in the workspace? What happens with user preferences, what about saving IDE state (for the browser refresh case)?

I see them as separate IDEs sharing the filesystem, with the option to share state / coordinate when it makes sense.

Saving the IDE state: the saved state is totally independent for each client, and is saved in the browser's local storage. This is "best effort" since changes in widgets or layout save/restore scheme can make it impossible to restore an arbitrary old saved state. AFAIK, when we detect that the browser window is being closed, local browser storage is the only place we can save the state reliably.

Preferences: the intelligence is in the client, the storage in the B-E. Each client's Preferences service watches the preferences files it currently uses. If some preference file is modified, that is shared between clients (same user prefs and/or same workspace prefs), all clients using it will re-read the content and apply the changes, if any. So if two clients use the same workspace, their preferences will stay in sync.

svenefftinge · 2018-10-01T12:07:18Z

As I already pointed out in the inline comment, using WorkspaceServer#getMostRecentlyUsedWorkspace doesn't work. You could instead wait with starting the server until the initialize-request message is sent. It contains the workspaceRoot.

svenefftinge · 2018-10-01T12:09:19Z

jdt.ls being an Eclipse application, AFAIK, the only way to define the workspace location is as a command line argument. I don't think you can start the app and then change the workspace location. But I'd be happy to be proven wrong.

Ok. I just wonder why we should add such an optimization for Theia, only. It seems like the same would be useful for other LSP clients using jdt.ls?

fbricon · 2018-10-01T12:15:26Z

@svenefftinge that's how vscode-java works, as well as all other clients, if they follow the instructions (item 7)

tsmaeder · 2018-10-01T13:16:03Z

Actually, Che (6) does not, since the workspace is always in a fixed location in the container. You cannot different locations in a Che workspace.

tsmaeder · 2018-10-01T14:13:47Z

As I already pointed out in the inline comment, using WorkspaceServer#getMostRecentlyUsedWorkspace doesn't work. You could instead wait with starting the server until the initialize-request message is sent. It contains the workspaceRoot.

Would it make sense to send the workspace root with the start request for the language server? At this point, the workspace root should be known, no?

svenefftinge · 2018-10-02T11:19:58Z

Would it make sense to send the workspace root with the start request for the language server? At this point, the workspace root should be known, no?

Yes, that would be fine, as well.

marcdumais-work · 2018-10-02T15:25:53Z

I tested this PR a bit and it does seem to help the performance of restarting a Java workspace. With the example project I used, the Java LS became responsive to my actions after ~20s instead of ~30s (when a jdt.ls data folder is re-used). That's a nice improvement.

fbricon · 2018-10-02T15:36:33Z

when reusing a workspace, you should expect the server to be responsive within 5sec (tested with https://github.com/spring-projects/spring-boot (and "java.import.gradle.enabled": false) in vscode.

marcdumais-work · 2018-10-02T22:01:38Z

@fbricon The project I used to test generates a lot of errors, so the results are not meant to be representative. I just tried with spring-boot and it took under 5s to become responsive, even with the ~700 errors due to the fact I did not setup the project.

svor · 2018-10-12T08:30:20Z

Would it make sense to send the workspace root with the start request for the language server? At this point, the workspace root should be known, no?

Yes, that would be fine, as well.

It is not possible to send the workspace uri with the start request, because the LS is started as soon as the jsonrpc connection is established on the server

simark · 2018-10-12T19:20:32Z

It is not possible to send the workspace uri with the start request, because the LS is started as soon as the jsonrpc connection is established on the server

@AlexTugarev, this seems to be the same pattern as what we discussed about the Python language server needing to know the virtual env path at the moment we launch it. The frontend wants to influence how the language server process is started, but there is no way for it to pass parameters to the backend code that launches the process.

I think we should settle on how parameters are sent from the frontend to the backend when starting a language server. I see three possibilities:

As query string in the service path. Instead of opening /services/languages/mylang, you open services/languages/mylang?param=value.
In the penMessage structure, send when requesting the opening of a websocket/jsonrpc channel, we add an "any" properties where the thing that requests the opening of the channel can stuff arbitrary data which will be available to the thing started on the other side.
You add some custom RPC calls that the client can use to set some properties in the backend (in your case, in the JavaContribution object). When starting the language server, you go read these values.

AlexTugarev · 2018-10-12T19:30:32Z

@simark, I think we should look into 2nd option first. See:
https://github.com/svor/theia/pull/1#issuecomment-429281204

@akosyakov, WDYT?

simark · 2018-10-12T19:51:59Z

@simark, I think we should look into 2nd option first. See:
svor#1 (comment)

I would be happy with this. After discussing it with you last time it seemed like you didn't really like that idea, but if you do now I don't mind :)

marcdumais-work

One little detail to iron-out before merge: the folder under which we save the jdt.ls data folders.

packages/java/src/node/java-contribution.ts

svor · 2018-10-31T14:33:07Z

@marcdumais-work @AlexTugarev @akosyakov @svenefftinge @tsmaeder
I've added an optional parameter into OpenMessage in WebSocketChannel to provide the workspace uri from frontend to backend when jdt.ls is starting.
Can you please take a look at my last changes.

packages/java/src/node/java-contribution.ts

My previous "blocking" comment is resolved, dismissing previous review

packages/java/src/node/java-contribution.ts

packages/core/src/node/messaging/messaging-contribution.ts

svor · 2018-11-14T08:43:49Z

@akosyakov

Changing the languages protocol is fine with me if it does not break APIs and behavior for existing extensions. We have many already outside of the theia repo.

To avoid changing existing extensions I've sent start parameters before starting language server. And each implementation of ls contribution has an ability to provide such params if it is needed.
Can you please review it one more again?

akosyakov · 2018-11-18T17:48:12Z

@svor good, can you extract changes to the languages extension to a new PR? that we can merge it and use for other languages

packages/languages/src/common/language-server-service.ts

packages/languages/src/browser/language-client-contribution.ts

svor · 2018-11-28T08:11:26Z

This PR related to #3538. Will rework it after merging #3538.

svor · 2018-12-10T13:40:17Z

@akosyakov The PR is updated, take a look please.

akosyakov · 2018-12-11T07:39:21Z

@marcdumais-work @AlexTugarev @svenefftinge @tsmaeder please, can you help finish the review?

packages/java/src/common/index.ts

packages/java/src/node/java-contribution.ts

AlexTugarev

That works nicely! Thanks!

Warmup time for the Java support is noticeable shorter if an Eclipse workspace directory can be reused.

svenefftinge · 2018-12-13T14:43:46Z

packages/java/src/node/java-contribution.ts

@@ -125,6 +143,17 @@ export class JavaContribution extends BaseLanguageServerContribution {
        });
    }

+    private generateDataFolderSuffix(workspaceUri: string): string {


maybe you could make this method protected, so extenders can change it.

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

akosyakov requested review from tsmaeder and svenefftinge September 26, 2018 05:57

svenefftinge reviewed Sep 26, 2018

View reviewed changes

tsmaeder mentioned this pull request Oct 2, 2018

Languages Sprint 156 (K-1) eclipse-che/che#11438

Closed

15 tasks

marcdumais-work previously requested changes Oct 15, 2018

View reviewed changes

packages/java/src/node/java-contribution.ts Outdated Show resolved Hide resolved

tsmaeder mentioned this pull request Oct 25, 2018

Languages Sprint 157 (K-2) eclipse-che/che#11733

Closed

13 tasks

marcdumais-work reviewed Oct 31, 2018

View reviewed changes

packages/java/src/node/java-contribution.ts Outdated Show resolved Hide resolved

marcdumais-work reviewed Oct 31, 2018

View reviewed changes

packages/java/src/node/java-contribution.ts Outdated Show resolved Hide resolved

marcdumais-work reviewed Oct 31, 2018

View reviewed changes

packages/java/src/node/java-contribution.ts Outdated Show resolved Hide resolved

marcdumais-work reviewed Oct 31, 2018

View reviewed changes

packages/java/src/node/java-contribution.ts Outdated Show resolved Hide resolved

AlexTugarev reviewed Nov 5, 2018

View reviewed changes

packages/java/src/node/java-contribution.ts Outdated Show resolved Hide resolved

akosyakov reviewed Nov 5, 2018

View reviewed changes

packages/core/src/node/messaging/messaging-contribution.ts Outdated Show resolved Hide resolved

tsmaeder mentioned this pull request Nov 12, 2018

Languages Sprint 158 (K-3) eclipse-che/che#11903

Closed

15 tasks

simark mentioned this pull request Nov 16, 2018

Ability to specify path to Clangd in preferences #3517

Closed

akosyakov reviewed Nov 18, 2018

View reviewed changes

packages/languages/src/common/language-server-service.ts Outdated Show resolved Hide resolved

akosyakov reviewed Nov 18, 2018

View reviewed changes

packages/languages/src/browser/language-client-contribution.ts Outdated Show resolved Hide resolved

tsmaeder mentioned this pull request Dec 6, 2018

Languages Sprint 159 (L-1) eclipse-che/che#12135

Closed

19 tasks

AlexTugarev reviewed Dec 11, 2018

View reviewed changes

packages/java/src/common/index.ts Outdated Show resolved Hide resolved

packages/java/src/node/java-contribution.ts Outdated Show resolved Hide resolved

AlexTugarev reviewed Dec 11, 2018

View reviewed changes

packages/java/src/node/java-contribution.ts Outdated Show resolved Hide resolved

AlexTugarev approved these changes Dec 11, 2018

View reviewed changes

svenefftinge reviewed Dec 13, 2018

View reviewed changes

[languages][java]Reuse jdt.ls workspace

e270704

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

svor merged commit cb9fc53 into eclipse-theia:master Dec 14, 2018

AlexTugarev mentioned this pull request Jan 30, 2019

Consider reusing the same jdt.ls workspace on restart (for a given opened folder) #1745

Closed

Reuse jdt.ls workspace #2970

Reuse jdt.ls workspace #2970

Conversation

svor commented Sep 25, 2018 • edited Loading

svor commented Sep 25, 2018

svenefftinge commented Sep 26, 2018

svenefftinge Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

tsmaeder Sep 26, 2018

Choose a reason for hiding this comment

svor Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

svor Sep 26, 2018

Choose a reason for hiding this comment

marcdumais-work Sep 26, 2018

Choose a reason for hiding this comment

marcdumais-work Sep 26, 2018

Choose a reason for hiding this comment

tsmaeder Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

tsmaeder Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

svenefftinge Oct 1, 2018

Choose a reason for hiding this comment

tsmaeder Oct 1, 2018

Choose a reason for hiding this comment

tsmaeder commented Sep 26, 2018

fbricon commented Sep 26, 2018

tsmaeder commented Sep 29, 2018

marcdumais-work commented Sep 29, 2018

svenefftinge commented Oct 1, 2018

svenefftinge commented Oct 1, 2018 • edited Loading

fbricon commented Oct 1, 2018 • edited Loading

tsmaeder commented Oct 1, 2018

tsmaeder commented Oct 1, 2018

svenefftinge commented Oct 2, 2018

marcdumais-work commented Oct 2, 2018

fbricon commented Oct 2, 2018 • edited Loading

marcdumais-work commented Oct 2, 2018

svor commented Oct 12, 2018

simark commented Oct 12, 2018

AlexTugarev commented Oct 12, 2018

simark commented Oct 12, 2018

marcdumais-work left a comment

Choose a reason for hiding this comment

svor commented Oct 31, 2018 • edited Loading

svor commented Nov 14, 2018

akosyakov commented Nov 18, 2018

svor commented Nov 28, 2018

svor commented Dec 10, 2018

akosyakov commented Dec 11, 2018

AlexTugarev left a comment • edited Loading

Choose a reason for hiding this comment

svenefftinge Dec 13, 2018

Choose a reason for hiding this comment

svor commented Sep 25, 2018 •

edited

Loading

svenefftinge Sep 26, 2018 •

edited

Loading

svor Sep 26, 2018 •

edited

Loading

tsmaeder Sep 26, 2018 •

edited

Loading

tsmaeder Sep 27, 2018 •

edited

Loading

svenefftinge commented Oct 1, 2018 •

edited

Loading

fbricon commented Oct 1, 2018 •

edited

Loading

fbricon commented Oct 2, 2018 •

edited

Loading

svor commented Oct 31, 2018 •

edited

Loading

AlexTugarev left a comment •

edited

Loading