refactor(treesitter): rely more on ts correctness

[vigoux]

Fixes #18108

@bfredl, this removes an old part of the code that was added a long
time ago.

Now comes the question of actually using ephemeral extmarks, and
rather use persistent extmarks and rely on tree updates.

I just leave that there...

[bfredl]

Why remove the state between lines? That seems like a pure regression from a performance perspective (also it just is wrong, we redraw a line range, not a line at a time).

[vigoux]

Why remove the state between lines? That seems like a pure regression from a performance perspective (also it just is wrong, we redraw a line range, not a line at a time).

Because, as most of my fellow developer, I don't have a terrifically deep understanding of the drawing loop.

Why not sending the end line in the callback to the redrawing ?

[vigoux]

And what about using static extmarks instead of a complex highlight state ? That will make a huge part of the code simpler to understand.

[bfredl]

Because, as most of my fellow developer, I don't have a terrifically deep understanding of the drawing loop.

Any perf related change must be based on "understanding", other wise it is just a random hit or miss. Profiling can help to get empirical understanding (I have used this to get rid of bottlenecks in unexpected places, such as strequal() ).

Why not sending the end line in the callback to the redrawing ?

That's on_win, and could be alternative, but then the hlstate array might get much longer (at the point we could be starting to see O(n^2) behavior there, but likely only for real big windows).

[vigoux]

Any perf related change must be based on "understanding", other wise it is just a random hit or miss. Profiling can help to get empirical data.

Running the test showed a slight improvement in performances.

If you don't want the changes, just close that PR, I don't care, I just wanted to raise a point (I did twice) about static extmarks, and you plain ignored it twice.

[bfredl]

If you don't want the changes, just close that PR, I don't care, I just wanted to raise a point (I did twice) about static extmarks, and you plain ignored it twice.

simply because I needed to think a bit more about that? you posted it 37 minutes ago, and I don't see the hurry, sorry. it could be simpler, though I like the current code that it doesn't duplicate state already in the tree in a second static copy, so I think that is a point for simplicity as well.

[vigoux]

I think that is a point for simplicity as well.

That is a valid point. Although I think that this is an instance where this simplicity might lead to a bit too much work on the drawing side.

I think we'd need real data in order to compare this in real life. I remember I did a similar change a while back, and that lead to significant performance improvements.

[bfredl]

That is a valid point. Although I think that this is an instance where this simplicity might lead to a bit too much work on the drawing side.

Sure but OTOH if you pre-render whenever the tree is changed, you do more work upfront instead (which might not end up being used). I suspect the trade-off varies greatly how much text-editing vs scrolling around the user does.

[vigoux]

Good point, maybe we need something that redraws the static extmarks in on_win ?

[vigoux]

@bfredl coming back at this, I am able, with this version of the implementation to limit how far we go on the number of columns.

An even better way of doing this (that I don't know though), would be to pass the very specific byte offsets to the on_line callback, so that we only draw the very portion of the line that the user sees.

[vigoux]

Thus this version should also fix #14852

[vigoux]

Any update on this PR ? I think it is already a step forward in fixing bugs...

[vigoux]

I have added the doubling of the match limit value, which should give more room for queries to run.
I think that the PR is ready this way. @bfredl if you want to give a final opinion, I'd be glad to have it.

[vigoux]

For the record, this fixes:
nvim-treesitter/nvim-treesitter#1788
nvim-treesitter/nvim-treesitter#1506

[bfredl]

Now that we have untangled the correctness from perf aspect, perhaps we could break out the former to a separate PR? I e more or less the changes to treesitter.c (including the free list, as that is more or less obvious)

[clason]

That is a good idea, if only because the fix should definitely be backported to 0.7.

[vigoux]

I am in favor of this, what parts do you want to separate:

• free list + synmaxcol
• iterator reset + doubling?

This way we have the fix on one hand a'd the feature on the other?

[clason]

I'd say put the kväck and the doubling in a separate fix(treesitter) PR.

[lewis6991]

For reference, the testcase from lewis6991/gitsigns.nvim#575 is pretty simple:

1. nvim +'set foldmethod=manual' +'1000,5000fold' +'999' ./src/nvim/eval.c
2. Move cursor up and down

[vigoux]

Reproducible, I am going to attempt a little benchmark somehow.

[vigoux]

I am somehow not able to reproduce this using nvim --clean and that sounds weird to me.

[lewis6991]

Try making the fold as large as possible.

[vigoux]

I finally got a benchmark, as follows:

VIMRUNTIME=$(pwd)/runtime/ time nvim --clean -u init.lua ./src/nvim/eval.c

-- init.lua
local parser = vim.treesitter.get_parser(0, "c", {})
local highlighter = vim.treesitter.highlighter.new(parser)

local function keys(keys)
  vim.api.nvim_feedkeys(keys, 't', true)
end

vim.opt.foldmethod = "manual"
vim.opt.lazyredraw = false

vim.cmd [[1000,7000fold]]
vim.cmd [[999]]

local function mk_keys(n)
  local acc = ""
  for i = 1, n do
    acc = acc .. "j"
  end
  for i = 1, n do
    acc = acc .. "k"
  end

  return "qq" .. acc .. "q"
end

keys(mk_keys(10))

for i = 1, 100 do
  keys "@q"
  vim.cmd[[redraw!]]
end

vim.cmd [[quit!]]

With this PR : 0:01.10
Current master: 0:03.11

That's a 3x improvement in performance.

[bfredl]

I suppose the presence of a (large) fold could be detected by comparing difference of line param and previous value with some threshold?

[vigoux]

What's a "large enough" fold ? I think that the data above is a clear indicator of a perf improvement.
I am working on more benchmarks, in order to bring more proofs of the perf improvement.

[bfredl]

idk, as a simple starting point we can just do it for any fold (so whenever delta > 1)

[lewis6991]

Here's my results using the benchmark above.

	Time
master	5.168
This PR	1.528
#18760	0.247

So 3x improvement with this PR, but 20x with #18760!!!

So there's some evidence that re-using the iterator is worth doing.

[clason]

We really need a dedicated benchmark test suite (make performancetest), not necessarily for CI, but both for such performance PRs and for regression testing with git bisect.

A PR or dedicated issue for discussing this welcome!

[lewis6991]

Ding #18989