fix(lsp): request only changed portions of the buffer in changetracker

[dmitmel]

Closes #11867

This PR is an optimization which fixes the issue I have mentioned in the #neovim-dev room. In short: some plugins, most notably vim-commentary and tcomment.vim, generate a lot of on_lines events when applied to a long range. In my particular case, if I comment out a range with either of the plugins I have mentioned, they both generate one on_lines per each commented line. The issue here is that changetracking.prepare requests the entire buffer contents with nvim_buf_get_lines(bufnr, 0, -1, true), which, while in itself is pretty fast, adds up quickly on hundreds of on_lines events.

I fixed the problem by only requesting the changed parts of the buffer since, obviously, we do get the change ranges. The unchanged lines are taken from the table with previous lines. A function such as Array#splice from JavaScript would've helped here for modifying the lines table in-place, but alas, the best we get in Lua is table.insert/table.remove, which can result in easily result in hidden execution time complexity (case in point: hrsh7th/cmp-buffer#18), and a proper implementation of this splice function can get pretty long (see the cmp-buffer PR, or an ECMAScript-compliant implementation here), so I decided against in-place modification.

Not all is lost, however, as I have found that double-buffering the lines tables also helps. This improves performance by reducing the load on the garbage collector because very long tables are not allocated on every on_lines. The way it is done is described in a comment in the code, but basically, when the incremental_changes function is done with curr_lines and prev_lines, curr_lines is saved as it normally would be, but prev_lines is cleared and also saved because clearing the contents of the table doesn't actually de-allocate the internal storage of the table, the keys with nil values are deleted only on the next garbage collection cycle. As such, a table with a fully-allocated internal storage is kept, which will be re-used on the next on_lines event as curr_lines.

Does the logic in the loops which construct curr_lines need commenting? Perhaps this code should go into a separate function, or a method on changetracking?

[mjlbach]

Perhaps this code should go into a separate function, or a method on changetracking?

We can elevate this into utils

[mjlbach]

LGTM pending our discussion on discussion on matrix/the couple comments I left.

• include the benchmark for pre/post 'nil-ing' the prev_lines table
• include the benchmark for incrementally update curr_lines
• check to what extent this mitigates lsp: synchronization bottleneck due to buf_get_lines (caused by line-by-line changes) #11867 and Saving large files is extremely slow when using nvim-lsp and rustfmt #14205. In my short tests it doesn't (which is ok/not a blocker, just want to make sure we are documenting things correctly)

Thanks for the PR!

[dmitmel]

• include the benchmark for pre/post 'nil-ing' the prev_lines table

• include the benchmark for incrementally update curr_lines

Done

[mjlbach]

This code measures the time taken to comment-out contents of the whole
buffer and then undo the change. tpope's vim-commentary is a pretty good
candidate for this test because it is very popular and suffers from the
problem of generating a text change for every commented-out line. All
text changes will be recorded within a single undo point, and so both
the comment-out and undo commands will emit a ton of on_lines events.
Granted, most likely the need for commenting out thousands of lines
doesn't occur every day, but I believe this is a good substitute for an
action generating a lot of text editing events.

I will be performing tests on files in the neovim repository at commit
2ecf0a4 and neovim itself built with
CMAKE_BUILD_TYPE=Release. As the execution time improvements seem to be
exponential, I am also going to include results for files with line
count of different orders of magnitude. The baseline is the
aforementioned commit 2ecf0a4, the
value after comment= is the AVERAGE number of seconds it took to
comment-out the entire file, and after undo= tells how long, on AVERAGE,
undoing that took.

Is a bit colloquial (first person) and long, can you make it a bit more dry/short with bullet points? Anyways otherwise LGTM, thanks for the changes. Tagging mathias for review as a formality

[mjlbach]

Also you didn't end up including

• handle buffer line endings in buf_get_full_text, continuing the work in fix(lsp): rewrite incremental sync #16252

AFAIK

[dmitmel]

Also you didn't end up including

I moved it to #16286, remember? Should I move that fix back here?

Anyway, I've decided that it fits here better

[mjlbach]

Anyway, I've decided that it fits here better

No, I meant that your commit message is not accurate :)

[dmitmel]

No, I meant that your commit message is not accurate :)

Crap. Well, I have already moved the fix back. Do you mind it?

[mjlbach]

Can you just unmove it back? It doesn't make sense to group the commits this way

[dmitmel]

check to what extent this mitigates #11867 and #14205

Gonna try to check this

[mjlbach]

I didn't notice a difference with my repro

[mjlbach]

The following code measures the time required to perform a buffer edit to
that operates individually on each line, common to plugins such as vim-commentary.

<insert code here>

All text changes will be recorded within a single undo operations. Both the 
comment operation itself and the undo operation will generate an on_line event
for each changed line.

Using the neovim codebase as an example (commit 2ecf04) with  neovim itself built 
at 2ecf04 with CMAKE_BUILD_TYPE=Release shows the following
performance improvement for double buffering <insert explanation here>

This performance improvements is due to avoiding expensive lua string reallocations on each operation, and instead
<insert explanation here>

[dmitmel]

Can confirm that this fixes #11867 (~26s -> ~0.4s). setline seems to cause a lot of on_lines events, even the following code to trigger it is enough:

call setline(1, getline(1, '$'))

Judging by the nature of that issue (an on_lines being fired for every line in the buffer), I believe that in the end my tests are realistic.

[mjlbach]

Cool, when you update the commit message and mathias rubber stamps (or requests changes) we will merge.

[dmitmel]

Please wait, checking the other issue.

[dmitmel]

Fixes #14205 too due to rust.vim's usage of setline().

[dmitmel]

Edited the commit message

[mjlbach]

I'm going to give the fixed incremental sync implementation a few days ~= week for people to report bugs @mfussenegger

Otherwise this PR is failing lint.

[dmitmel]

Otherwise this PR is failing lint.

The failure is in unrelated C code, does this happen to other PRs too?

[mjlbach]

Mathias will merge this PR when he has reviewed. It will likely be in 0.6

[dmitmel]

Nice. Thank you.

[dmitmel]

I should also note that I have found that this PR may break on_reload logic. I'll investigate this.

[dmitmel]

False alarm, this was a bug in a specific Language Server, I've fixed it: LuaLS/lua-language-server#821

[dmitmel]

I have discovered a different bug related to on_reload. In short, the on_reload handler must flush forget all debounced didChange messages before sending didClose and didOpen. I'll fix this in a separate PR. Also, I'm going to rebase this one because it has conflicts with the master branch.

[dmitmel]

Is there anything left to be done with regards to this PR?

[mjlbach]

No, I passed review/merging on a section of my core responsibilities over to @mfussenegger

[mjlbach]

@dmitmel When I tried benchmarking this I didn't get the speedup I expected, using your reproduction from #11867 (comment)

I outlined the situation (which you are probably already familiar with here): #11867 (comment).

I think an alternative to this PR would involve bypassing get_lines completely and caching firstline, lastline, newlastline, etc. on each on_lines call, and only calling buf_get_lines at the beginning and end of the debounce window (without any of the intermediate caching as implemented in this PR).

The alternative, which I am also ok with, is to tell users to switch to plugins which apply better atomic buffer changes (which IMO is what they should be doing anyways).

What do you think?

[dmitmel]

I think an alternative to this PR would involve bypassing get_lines completely and caching firstline, lastline, newlastline, etc. on each on_lines call, and only calling buf_get_lines at the beginning and end of the debounce window (without any of the intermediate caching as implemented in this PR).

Honestly, at that point you might as well stop using firstline/lastline and find the modified range using the procedure I have implemented in #16437. As we have previously established in the Matrix chat, comparing even tens of thousands of strings is very very cheap, and that would considerably simplify the code by bypassing the logic necessary for combining all of the intermediate firstline/lastline ranges into a single one.

[mjlbach]

considerably simplify the code by bypassing the logic necessary for combining all of the intermediate firstline/lastline ranges into a single one

I'm pretty sure that's like 3-4 lines of code...

Anyways, if you have time can you verify this doesn't actually address the performance issues or point out how I can verify that it does? I used basically config from #11867 (comment)

[dmitmel]

No idea. For me it does work, and the improvement is significant (22s -> 0.5s). All I did was compile Nvim on this branch and run the command I specified. Can you record a screencast or something like that?

[mjlbach]

@dmitmel if you want the actual authorship of the commit rather than coauthor you can just apply the changes from https://github.com/neovim/neovim/pull/17118/files directly to this PR which updates it to address the rebase conflicts.

[mjlbach]

I went ahead and merged this in #17118.

Thanks for your contribution and for addressing all of my questions :)