Autocompletion/syntax checking extremely slow/unusable

[sihui62]

Autocompletion and syntax checking is extremely slow, if not unusable.

Issue has been reported on the community forum as well, known hardware/software: openHABian and in my case openHAB snapshot #1272.

https://community.openhab.org/t/vs-code-openhab-extension/30205/244 https://community.openhab.org/t/vs-code-openhab-extension/30205/248 (video) https://community.openhab.org/t/vs-code-openhab-extension/30205/245

[kubawolanin]

I would rather recommend submitting an issue on eclipse/smarthome repo, as this is a LSP related issue.

See https://github.com/eclipse/smarthome/issues/4756

[SamuelBrucksch]

Hi,

its not the LSP that slows autocompletion. Its this part, that slows it down: https://github.com/openhab/openhab-vscode/blob/master/src/ItemsExplorer/ItemsModel.ts#L49 https://github.com/openhab/openhab-vscode/blob/master/src/ItemsExplorer/ItemsCompletion.ts#L37

I just did some tests and i noticed, that the openhab rest api sometimes is terribly slow when requesting the items from http://openhab/rest/items/. This can happen with a lot of items or with items that need additional queries. I noticed amazon echo control binding for example slows this down quite a lot. With 3 items from amazon control binding the /rest/items call took 1.5-1.7s and without it around 0.5s, so much faster. This was also reflected in the autocompletion in vscode.

The problem here is, that this call is done on every change of the document. So every call and every proposal is terribly slow when the rest api is slow.

[Confectrician]

The problem here is, that this call is done on every change of the document. So every call and every proposal is terribly slow when the rest api is slow.

That's indeed a problem. To be honest i have no real clue how we can design this, to keep it on an acceptable level. We want suggestions for auto completion to be made as fast as possible, so limiting it to sace or something else is useless too.

An option, that i would have in mind is to cache the current items on extension startup and then on interval. I am pretty sure we need to update them ond not store them only once on startup. When one uses the editor, there is a hogh chance that items will change because he/she is working on the OH environment itself.

Additionally i don't think that this cache is useful for the item/things tree view. When you are navigating in the tree, you are looking for live data, so we have to accept higher loads for the view usecase.

Any other opinions and suggestions are welcome. Maybe some of you use the editor/extension entirely different and i would prefer a solution that fits into most usage situations.

[MHerbst]

I am very new with VS Code extension development, so please excuse if my ideas are totally wrong :-).

If I understood @SamuelBrucksch correctly the code completion is done by using the REST API to retrieve information about available items. Normally code completion is a task of the LSP and I think it would be much more efficient to implement it this way. The LSP is running within openHAB and probably this component can get information about available items and updates etc. in a more efficient way.

In my opinion, all editor-related task should be handled via the LSP and the REST APIs only used for the things and items views.

[SamuelBrucksch]

From what i understand the LSP only does validation but is not used for proposing completion items. The snippets for "rules" and so on that you can execute to get a rule template are from vscode plugin and not from LSP. As far as i saw in the code, completion items are snippets from vscode + items from REST API. So at the moment LSP is more or less useless for code completion. However i might be wrong as i did not look at all the code yet.

I dont know the relationship between openhab and Eclipse Smarthome LSP Server and how the general structure is. I think openhab is based on Eclipse Smarthome something and as the code is validated by LSP it should have a general understanding of the items and so on available. So it really might make sense to ask the LSP for the items, however as the LSP is managed by someone else it might be hard to get this through...

Maybe LSP already offers something like that but it is just not used yet. Will check that out when i have some time.

[MHerbst]

As far as I remember the first version of this extension did not use the LSP at all. It contained the different use and some limited editor support (like code completion). At a later point, the LSP server was implemented and added to the extension. I am quite sure that it is only used for syntax checking.

So it really might make sense to ask the LSP for the items, however as the LSP is managed by someone else it might be hard to get this through...

This should not be an obstacle :-).

I am now trying to set up a test environment for the VSCode extension. I am trying to figure out why I always get an exception when loading an openHAB definition file. As soon as I have a better understanding I can try to help with other issues.

[Confectrician]

To bring some facts in this discussion:

With Language Servers, you can implement autocomplete, error-checking (diagnostics), jump-to-definition and many other language features supported in VS Code.

Source: https://code.visualstudio.com/docs/extensions/example-language-server

So yes, this is a task for lsp too, but maybe it goes beyond the scope of lsp in our usecase. I think (or at least imagine) that autocomepletion is meant for syntax keywords (switch, rollershutter, etc), which are language related.

So it could be possible that we just move the heavy load from rest to lsp. Or maybe i am wrong an it works like a charm. 😄

It is worth a try anyway.

I think it would be useful to simply remove "rest autocempletion" for now. We could leave the tree view as it is. With that you would still hafe the option to insert items from channels through the view.

wdyt?

[MHerbst]

I think it would be useful to simply remove "rest autocempletion" for now. We could leave the tree view as it is. With that you would still hafe the option to insert items from channels through the view.

I agree with you. We could also make the autocompletion feature optional by adding a configuration option. So every user can decide for himself.

[Confectrician]

Just to clarify, it seems that we already have an option to run completion through lsp:

"openhab.restCompletions": { "type": [ "boolean" ], "default": true, "description": "Takes completions from Language Server instead of REST API" },

Maybe we should investigate the current state first. 😄

Only for the record: I am new to extension development and typescript stuff too. I am just hanging around in this repo a bit longer than others and have added some snippets already, which was mainly json to write. (And i have already read through the extension docs and got a bit knowledge of how they work basically.)

[SamuelBrucksch]

Well, auto completion from LSP should return everything that can be used in the code at a specific position. So if that position can be an identifier or a keyword it should be able to offer both. How you resolve what identifiers for example could be is up to the person who writes the LSP.

I just had a look at the samples from MS on their vscode LSP template again and remember, that really most of the things are done in the server. I did some LSP things with antlr about 1.5 years ago, so its all a bit fuzzy what i remember ;). Here is the Server impl: https://github.com/eclipse/smarthome/tree/master/bundles/model/org.eclipse.smarthome.model.lsp/src/main/java/org/eclipse/smarthome/model/lsp/internal

As far as i can see there is no specific stuff about completion, so i guess its just not implemented. that should be the code we need to touch to get completion items from the LSP.

About disabling it for now... Its only a config option, so instead of disbaling it completely i would just change the defaults. So LSP is enabled, but rest api by default is disabled. not everyone has problems here so maybe they still want to use it and can still enable it.

[SamuelBrucksch]

Just to clarify, it seems that we already have an option to run completion through lsp:

"openhab.restCompletions": { "type": [ "boolean" ], "default": true, "description": "Takes completions from Language Server instead of REST API" },

Maybe we should investigate the current state first.

This is a bit misleading...

        const itemsCompletion = new ItemsCompletion()
        ....
        if (config.restCompletions) {
            disposables.push(languages.registerCompletionItemProvider('openhab', itemsCompletion))
        }

And in itemsCompletion:

    public provideCompletionItems(document: TextDocument, position: Position, token: CancellationToken): Thenable<CompletionItem[]> {
        return new Promise((resolve, reject) => {
            let config = workspace.getConfiguration('openhab')

            if (config.useRestApi) {
                this.model.completions.then(completions => {
                    resolve(completions.map((item: Item) => {
                        let completionItem = _.assign(new CompletionItem(item.name), {
                            kind: CompletionItemKind.Variable,
                            detail: item.type,
                            documentation: this.getDocumentation(item)
                        })

                        return completionItem
                    }))
                })
            } else {
                reject()
            }
        })
    }

And the completions property is this:

  public get completions(): Thenable<Item[]> {
    return this.sendRequest(null, (items: Item[]) => {
      return items;
    });
  }

sendRequest is the Method that gets items from Rest API.

So the description there is wrong. Actually it means use completion from rest if true.

[Confectrician]

So the description there is wrong.

Ok that's the smallest problem, we can change that.

But i am a bit confused now. We have the option to use lsp instead of rest here alread, but from your investigation it looks like we have no server side implementation for completion yet. Seems a bit strange to me. I have deactivated the option and now autocompletion seems to use only the known items from the opened file, or any other subset of available completions.

Anyway it is much faster than using rest. So we should rename the description to something useful and point people with problems to this setting, while finding a long term solution.

[SamuelBrucksch]

I have deactivated the option and now autocompletion seems to use only the known items from the opened file, or any other subset of available completions.

Try this setting: "editor.wordBasedSuggestions": false

[SamuelBrucksch]

BTW this is the actual imple that is used in the lsp: https://github.com/eclipse/xtext-core/blob/master/org.eclipse.xtext.ide/xtend-gen/org/eclipse/xtext/ide/server/LanguageServerImpl.java

(from https://github.com/eclipse/smarthome/blob/master/bundles/model/org.eclipse.smarthome.model.lsp/src/main/java/org/eclipse/smarthome/model/lsp/internal/ModelServer.java#L30)

A completion provider seems to be registered here: https://github.com/eclipse/xtext-core/blob/master/org.eclipse.xtext.ide/xtend-gen/org/eclipse/xtext/ide/server/LanguageServerImpl.java#L271

However i do not quite understand right now what happens there.

Then the impl seems to be used here: https://github.com/eclipse/smarthome/blob/master/bundles/model/org.eclipse.smarthome.model.lsp/src/main/java/org/eclipse/smarthome/model/lsp/internal/ModelServer.java#L115

I'm not sure if i understand this right, but to me it looks like the smarthome impl creates a LSP server from the xtext impl, connects to it as LSP client and then i dont know what. Maybe the smarthome impl only acts as proxy and did not pass a CompletionProvider or so.

If thats the case we could maybe add one in the smarthome impl.

[SamuelBrucksch]

Now that i had a look at the LSP impl. i dont want to work on that anymore. Way too complex as a lot of injection and multiple packages are used.

I also thought about the cache approach in the beginning and tried it out already and it was amazingly fast. However its hard to think of a rule when to update the cache. Here is something from a slow rest api thread on openhab that i saw how eclipse does it in some cases: https://www.eclipse.org/smarthome/documentation/features/frameworkUtilities.html#simple-expiring-and-reloading-cache

So if a value is still valid it just returns it, else it will query it. However this is not really adoptable for us.

I also thought about looking into save and modify events, that could work in case of item and rules files and so on, but not when items are added in paper ui.

It looks like openhab supports server push event subscription: https://github.com/openhab/openhab1-addons/issues/542

With that we could do an initial query and then subscribe to all or single items and only update them when we get a message from the server. The documentation on this is hardly there so i still need to read through that and see how it has to be done. This would be the easiest solution i think without touching the LSP.

We can also listen to events like this: http://demo.openhab.org:8080/rest/events

[triller-telekom]

I did not follow the whole discussion, but since I remember a fix in ESH which was made to the registries, I think this is important here too. It should also speed-up the REST calls!

So while during your tests, do you have this fix included in your installation?

https://github.com/eclipse/smarthome/pull/6438

Should be in the latest nightly of openHAB though...

[SamuelBrucksch]

I did not follow the whole discussion, but since I remember a fix in ESH which was made to the registries, I think this is important here too. It should also speed-up the REST calls!

So while during your tests, do you have this fix included in your installation?

eclipse/smarthome#6438

Should be in the latest nightly of openHAB though...

Thanks for pointing that out, will check later if this improves the code completion as it is right now.

[SamuelBrucksch]

Test from the rest api events url:

data: {"topic":"smarthome/items/FF_Bathroom/removed","payload":"{\"type\":\"Group\",\"name\":\"FF_Bathroom\",\"label\":\"Bathroom\",\"category\":\"bath\",\"tags\":[\"Bathroom\"],\"groupNames\":[\"Home\",\"FF\"]}","type":"ItemRemovedEvent"}

So there we can see if something is added or removed, so we are actually able to modify our cache eventbased in the background.

So a logic could somehow be

1. connect to openhab rest api and get items
2. query events and update cache 2a. on connection loss goto 1

[SamuelBrucksch]

I was wondering if we could also just implement our own LSP only for item completion that runs locally in the vscode extension. LSP does not necessarily have to run on a remote server, it can also run in the IDE itself. It can however be moved somewhere else as well if it makes sense. But as the openhab server normally is a raspberry pi or something similiar in limited computing capacity, it might make sense to also consider to run the items completion LSP within the extension. Normally the computers where vscode runs on are much faster anyways in processing.

LSP would then query the rest items and update them in the background. Of course this would then only be an items completion LSP, the openhab LSP for code validation still can be used.

This approach would also work pretty well when remote coding via myopenhab.com is done, as we can still use the LSP for items completion at least.

Here is an issue in vscode github where it is discussed to use multiple LSP servers: https://github.com/Microsoft/vscode/issues/48227

[Confectrician]

I have thought about that too alread. Not exactly with your approach but in general. If do that only for item completion, this would be a valid alternative.

In general it makes definetely sence to keep most of the logic on the server side. This way, we have always the "correct" lsp for our current openHAB version. But items completion is just a stupid task, which needs power so we could give it a try.

[MHerbst]

What a about creating a locally running LSP that implements all functions needed e.g. for code completion (by using SSE it can always have the latest item and things definitions) and that routes all requests regarding syntax check to the LSP running on the openHAB server. This would look like in this picture:

[SamuelBrucksch]

Hey, this is more or less what i had in mind as well. I'm not sure if a LSP Server can connect to another LSP server. However in the Java LSP they have some kind of proxy to connect to another LSP server so it might be possible to do it.

Anyone wants to work on this? Else i will start on that.

Maybey ou can link the SSE docs if there are any, on quick google search i just found a community post about it, but i did not search seriously so i might find it myself later.

[MHerbst]

Hey, this is more or less what i had in mind as well. I'm not sure if a LSP Server can connect to another LSP server.

I already had the assumption that your ideas were like this. I tried to find some information about implementing something like a "LSP proxy" and stumbled across this article: https://dzone.com/articles/part-2-language-server-protocol-a-language-server. It seems to look very similar to our ideas.

Anyone wants to work on this? Else i will start on that.

I am quite busy at the moment and would not find the time to work on it regularly. But I can try to support you.

Maybey ou can link the SSE docs if there are any

I have found some JavaDocs about the implementation within Eclipse Smarthome: https://www.eclipse.org/smarthome/documentation/javadoc/org/eclipse/smarthome/io/rest/sse/SseResource.html, Maybe you can find some examples in the Basic UI implementation https://github.com/eclipse/smarthome/tree/master/extensions/ui/org.eclipse.smarthome.ui.basic

[Confectrician]

Feel free to start on that topic. I am not too familiar with the lsp stuff.

My suggestion would be to add a WIP PR on an early stage. We can then follow your development and maybe help you with pr's on your repo too. :)

[SamuelBrucksch]

Hey, this is more or less what i had in mind as well. I'm not sure if a LSP Server can connect to another LSP server.

I already had the assumption that your ideas were like this.

My idea was to connect to two LSP server in parallel, but i actually like the idea of enhancing the existing one in some kind of proxy LSP server.

Ok i will start on this, i'm quite busy myself but i should find some time during the evening hours.

[MHerbst]

My idea was to connect to two LSP server in parallel, but i actually like the idea of enhancing the existing one in some kind of proxy LSP server.

With the proxy server we were also able to configure the supported capabilities in a easier way. The LSP is based on the default XTEXT implementation which sets by default too many capabilities to "enabled", see: https://github.com/eclipse/smarthome/issues/6654

[SamuelBrucksch]

Just one quick question regarding coding standards for this project... Is there anything like standard/prettier that is used? I already see some unrelated changes from formatting, as everyone has his own formatting properties...

I would propose something like prettier and a pre commit hook that runs prettier so the code in github always looks the same no matter how the code looks locally.

Also something like standard where you can not really configure much would help to have a common code style in this repo.

Would you be interested that i add something like that to the project?

[Confectrician]

Would you be interested that i add something like that to the project?

We could talk about that later, but for now i would like to move this topic to the future. From my information kuba wants to coma back as soon as possible and i would like to hear his opinion about coding standards.

I think we talked about this topic some time ago already, but i am not sure what the state of discussion was.

[SamuelBrucksch]

Ok thats fine, but then the PRs will show more unrelated changes because of different formatting settings.

BTW we will get nice merge conflicts when PRs are merged until i can merge the LSP as the folder structure changed. Maybe we should think about how to deal with that or at least dont merge too many PRs right now. I think i can get a basic LSP running in a few days, so if we merge the basic LSP then we can continue working on different topics again and enhance the LSP after that.

[Confectrician]

See PR comment. I will merge mine and draft a new release and i can do that today too. The extension seems to be stable enough for a wider audience and after that you can rebase and go on without different historys.