Semantic highlight slows down editing

[thenewvu]

It makes editing medium/big files really painful even when I have no configuration for it. Could we consider to disable it by default or improve its performance?

[thomasfaingnaert]

Would it be possible to use Vim's profiling to identify which functions are the culprit?

[thomasfaingnaert]

#634 speeds it up somewhat, but the performance for big files is still not on point:

FUNCTIONS SORTED ON SELF TIME
count  total (s)   self (s)  function
10004              2.192760  lsp#utils#base64_decode()
10004   2.202340   1.375972  <SNR>87_add_highlight()
10004   3.162274   0.504508  <SNR>87_tokens_to_hl_info()
30024              0.465006  <SNR>87_octets_to_number()
10005   0.351520   0.286610  lsp#capabilities#has_semantic_highlight()
100053              0.251802  <SNR>87_get_textprop_name()
    1   5.561511   0.196170  lsp#ui#vim#semantic#handle_semantic()
10005   0.574658   0.169267  lsp#ui#vim#semantic#get_scopes()
20011              0.118792  lsp#get_server_capabilities()

[thomasfaingnaert]

@prabirshrestha Would running handle_semantic in a timer avoid blocking Vim's UI, or do we need to do something more here? One possibility is caching the semantic tokens, calculating a diff with each semantic response from the server, and only adding/deleting the tokens that changed.

[thomasfaingnaert]

@thenewvu If you're using Vim, could you check if #635 helps? The initial semantic highlight will still be slow, but after that there should be no noticable slowdown of Vim.

[thenewvu]

@thomasfaingnaert I think CPU time shows it all, in the above perf table, I'm not sure that it's on UI thread or not, but 2s is just too much for UI-related things, let's imagine when you doing live substitution or repeat macros:

FUNCTIONS SORTED ON SELF TIME
count  total (s)   self (s)  function
10004              2.192760  lsp#utils#base64_decode()
10004   2.202340   1.375972  <SNR>87_add_highlight()
10004   3.162274   0.504508  <SNR>87_tokens_to_hl_info()
30024              0.465006  <SNR>87_octets_to_number()
10005   0.351520   0.286610  lsp#capabilities#has_semantic_highlight()
100053              0.251802  <SNR>87_get_textprop_name()
    1   5.561511   0.196170  lsp#ui#vim#semantic#handle_semantic()
10005   0.574658   0.169267  lsp#ui#vim#semantic#get_scopes()
20011              0.118792  lsp#get_server_capabilities()

Oh.. btw, I'm using nvim, sorry dude :)

[thenewvu]

Hey, as I understand, your current implementation is listen to textDocument/semanticHighlighting event, and add highlights for every lines in the event data right? It leads to a lot of waste computing time I think, because many lines in those may never be displayed.

There's textDocument/documentHighlight request, with the position param, we can request highlighting for only displaying lines, and we only do so after &updatetime (idle state), debounced and in async.

What do you think?

[thomasfaingnaert]

Hey, as I understand, your current implementation is listen to textDocument/semanticHighlighting event, and add highlights for every lines in the event data right? It leads to a lot of waste computing time I think, because many lines in those may never be displayed.

Yes, and unfortunately the textDocument/semanticHighlighting request always sends the entire buffer, rather than the lines that changed. One way to improve performance is to only change lines that changed from the previous request. This is what #635 aims to do. It is faster, but unfortunately, that still requires iterating over all lines, which is still slow for large files. (Also it doesn't work for Neovim at the moment, because it lacks text props)

There's textDocument/documentHighlight request, with the position param, we can request highlighting for only displaying lines, and we only do so after &updatetime (idle state), debounced and in async.

What do you think?

textDocument/documentHighlight is a separate request, used for things like highlighting all references of the identifier under the cursor. IIRC, there were some performance issues with that as well, which is why we disabled it by default (see e.g. #491 and #492).

Still, only applying semantic highlighting to displayed lines is a good idea. It will complicate the logic a bit to make sure everything stays up to date, especially in cases where the user scrolls quickly through the buffer.

An alternative is combining a caching mechanism similar to #635 with executing the highlight in chunks of ~ 100 lines, each in the background.

[prabirshrestha]

One option I can think of is to completely disable semantic highlight based on config if the file is too big. This is what I do for asyncomplete-buffer by passing max_buffer_size. vim doesn't have background thread so no mater what optimization we do it will be impossible to get good perf here.

What we actually need is a new vimprocess where we can execute arbitrary script that returns back to the main vim so we can do heavy computation in different process or use lua/python threads if available. This is why I have really been pushing for https://groups.google.com/forum/#!searchin/vim_dev/vim$209%7Csort:date/vim_dev/__gARXMigYE/5v7dqcHfAwAJ, vim/vim#5198, vim/vim#2115. Based on Bram he would be working on making vimscript faster which might be helpful a bit here but it will still be in UI thread so very difficult. https://groups.google.com/forum/#!searchin/vim_dev/vim$209$20webassembly%7Csort:date/vim_dev/__gARXMigYE/a_aj5KgQCAAJ.

[prabirshrestha]

For now I have disabled semantic highlighting by default.

[davits]

Yes, and unfortunately the textDocument/semanticHighlighting request always sends the entire buffer, rather than the lines that changed.

Could you please point to the server implementation which returns whole buffer each time ?
Proposal for SH states that only delta should be sent to the client. I've looked at the SH info sent from clangd and it contains only SH for lines which were added/changed.
Maybe this issue should be reported on that server side.

[thomasfaingnaert]

@davits I was using clangd 9, I believe, and it didn't send a delta. Which clangd were you testing on? It might've changed in clangd 10.

As for the spec, it doesn't say that servers should only send deltas, only that they, in theory, could:

The textDocument/semanticHighlighting notification is pushed from the server to the client to inform the client about additional semantic highlighting information that has to be applied on the text document. It is the server's responsibility to decide which lines are included in the highlighting information. In other words, the server is capable of sending only a delta information.

[davits]

I was using clangd 10, so they added it recently. I've checked it one more time, it seems their diff algorithm is not the best. It sends only one line if that particular line was modified, but in case of added/removed line it sends back the whole second half of the buffer.

But the next sentence in the spec explicitly states that server should send delta on changes:

For instance, after opening the text document (DidOpenTextDocumentNotification) the server sends the semantic highlighting information for the entire document, but if the server receives a DidChangeTextDocumentNotification, it pushes the information only about the affected lines in the document. The server never sends delta notifications, if no new semantic highlighting ranges were introduced but the existing onces have been shifted. For instance, when inserting a new line to the very beginning of the text document. The server receives the DidOpenTextDocumentNotification, updates its internal state, so that the client and the server shares the same understanding about the highlighted positions but the server does not send any notifications to the client. In such cases, it is the client's responsibility to track this event and shift all existing markers.