vim-7.4.389 UI extremely sluggish with custom syntax highlighting plugins

[David Barnett]

It looks like some change between 7.4.316 and 7.4.389 has made vim extremely sluggish when just scrolling around in files if the vim-indent-guides plugin is installed: https://github.com/nathanaelkane/vim-indent-guides/issues/75. This plugin does custom syntax highlighting, which may be a little dirty, but it's surprising that performance got so much worse all the sudden.

Were there any recent changes to vim's syntax highlighting that might account for the lag? Would it be possible to fix the regression, or revert the offending change until it can be implemented in a more performant way?

David

[David Barnett]

Looks like it's patch 7.4.362:

Problem:    When matchaddpos() uses a length smaller than the number of bytes

    in the (last) character the highlight continues until the end of

    the line.

Solution:   Change condition from equal to larger-or-equal.

Files:     src/screen.c

It's a very small change but apparently problematic. Can it be reverted or rethought?

David

--
--
You received this message from the "vim_use" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php

---
You received this message because you are subscribed to a topic in the Google Groups "vim_use" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/vim_use/V6cRWX4c13E/unsubscribe.
To unsubscribe from this group and all its topics, send an email to vim_use+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Bram Moolenaar]

David Barnett wrote:

> Looks like it's patch 7.4.362:
>
> Problem: When matchaddpos() uses a length smaller than the number of
> bytes
> in the (last) character the highlight continues until the end of
> the line.
> Solution: Change condition from equal to larger-or-equal.
> Files: src/screen.c
>
>
> It's a very small change but apparently problematic. Can it be reverted or
> rethought?

I'm glad you could pinpoint it. I assumed that when the condition
evaluates to true the match information would be updated. But perhaps
that doesn't happen in this case and it searches for a match every time.

Needs some debugging to figure out what happens. What matches does this
plugin add when it's slow?

--
"How is your new girlfriend?"
"90-60-90 man!"
"What, pale purple?"

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[David Barnett]

You're not asking for the output of getmatches(), are you? Or are you talking about debugging in gdb? I haven't found any way to query match positions from within vim.

The repro case I'm using now is

vim -c "highlight def link RightMargin Error | exec ('match RightMargin /\%<42v.\%>41v/')" SOMEFILE

So you don't need any plugins to verify.

David

[Bram Moolenaar]

David Barnett wrote:

> You're not asking for the output of getmatches(), are you? Or are you
> talking about debugging in gdb? I haven't found any way to query match
> positions from within vim.
>
> The repro case I'm using now is
>
> vim -c "highlight def link RightMargin Error | exec ('match RightMargin
> /\%<42v.\%>41v/')" SOMEFILE
>
> So you don't need any plugins to verify.

The pattern is what I need to reproduce it easily. It's not obvious from
the plugin what patterns it generates.

I don't see it being slow with this example though. But it would be
enough to single step through the code to find out why the "==" to ">="
change matters for this.

Did you try that example with "vim -u NONE -N", is it still slow then?
If not then something else would matter.

--
hundred-and-one symptoms of being an internet addict:
23. You can't call your mother...she doesn't have a modem.

[Dominique Pellé]

David Barnett wrote:

> Looks like it's patch 7.4.362:
>
> Problem: When matchaddpos() uses a length smaller than the number of
> bytes
> in the (last) character the highlight continues until the end of
> the line.
> Solution: Change condition from equal to larger-or-equal.
> Files: src/screen.c
>
>
> It's a very small change but apparently problematic. Can it be reverted or
> rethought?
>
> David

Hi David

Have a look at ":help :syntime". It's a new Ex command
introduced in Vim-7.4 which is useful to analyze why
syntax highlighting is slow.

Regards
Dominique

[David Barnett]

Yep, it's still slow with "-u NONE -N". It seems to vary quite a bit from file to file. The way I reproduce is to just hold down PgDn for a few seconds. It's fine for unaffected versions, but stutters for about half a second in 362. For some reason most vim files aren't too bad for it. src/options.c hangs for about a second for me a few screens down.

Dominique, for some reason the problematic match doesn't even show up in ":syntime report". Looks like a useful troubleshooting tool in general, though.

David

[Dominique Pellé]

David Barnett wrote:

> Yep, it's still slow with "-u NONE -N". It seems to vary quite a bit from
> file to file. The way I reproduce is to just hold down PgDn for a few
> seconds. It's fine for unaffected versions, but stutters for about half a
> second in 362. For some reason most vim files aren't too bad for it.
> src/options.c hangs for about a second for me a few screens down.
>
> Dominique, for some reason the problematic match doesn't even show up in
> ":syntime report". Looks like a useful troubleshooting tool in general,
> though.
>
> David

Hi David

I can now reproduce the slow rendering you
describe with:

$ cd vim/src
$ vim -u NONE option.c \
-c 'syntax on' +471 \
-c "hi def link RightMargin Error|exec ('match RightMargin /\%<42v.\%>41v/')"

Then I press CTRL-F and CTRL-B to scroll forward
and backward by page around line 471. Rendering
is slow with vim-7.4.400. Rendering is much faster
with vim-7.4. When I find time, I'll compare profiling
results before & after vim-7.4.389 (allegedly the patch
that introduces the slow down).

Regards
Dominique

PS: please bottom post in this mailing list.

[David Barnett]

On Sunday, August 10, 2014 10:27:58 PM UTC-7, Dominique Pelle wrote:
> When I find time, I'll compare profiling
> results before & after vim-7.4.389 (allegedly the patch
> that introduces the slow down).

Slight correction: 389 was the patch where I first noticed the problem, but then I bisected it down to 362 being the actual patch that introduced the slowdown.

David

[Christian Brabandt]

Hi Bram!

On Sa, 09 Aug 2014, Bram Moolenaar wrote:

>
> David Barnett wrote:
>
> > Looks like it's patch 7.4.362:
> >
> > Problem: When matchaddpos() uses a length smaller than the number of
> > bytes
> > in the (last) character the highlight continues until the end of
> > the line.
> > Solution: Change condition from equal to larger-or-equal.
> > Files: src/screen.c
> >
> >
> > It's a very small change but apparently problematic. Can it be reverted or
> > rethought?
>
> I'm glad you could pinpoint it. I assumed that when the condition
> evaluates to true the match information would be updated. But perhaps
> that doesn't happen in this case and it searches for a match every time.
>
> Needs some debugging to figure out what happens. What matches does this
> plugin add when it's slow?

Perhaps it is better to revert that patch and make sure, highlighting
gets only applied, if there is a match at the actual position?

diff --git a/src/screen.c b/src/screen.c
--- a/src/screen.c
+++ b/src/screen.c
@@ -3852,7 +3852,7 @@ win_line(wp, lnum, startrow, endrow, noc
{
shl->attr_cur = shl->attr;
}
- else if (v >= (long)shl->endcol)
+ else if (v == (long)shl->endcol)
{
shl->attr_cur = 0;
next_search_hl(wp, shl, lnum, (colnr_T)v, cur);
@@ -3913,7 +3913,10 @@ win_line(wp, lnum, startrow, endrow, noc
}
else
shl = &cur->hl;
- if (shl->attr_cur != 0)
+ /* make sure, match is actually at the current position */
+ if (shl->attr_cur != 0
+ && v >= (long)shl->startcol
+ && v < (long)shl->endcol)
search_attr = shl->attr_cur;
if (shl != &search_hl && cur != NULL)
cur = cur->next;

Best,
Christian
--
Probleme sind Gelegenheiten zu zeigen, was man kann.
-- Duke Ellington

[Bram Moolenaar]

I haven't tried it, but it looks like this only avoids highlighting
positions after the endcol when "v" and shl->endcol aren't exactly the
same (which can happen for multi-byte characters), but it doesn't update
the state, thus a next match might be missed.

--
hundred-and-one symptoms of being an internet addict:

27. You refer to your age as 3.x.

[Christian Brabandt]

On Mo, 11 Aug 2014, Bram Moolenaar wrote:

> Christian Brabandt wrote:
> > On Sa, 09 Aug 2014, Bram Moolenaar wrote:
> > > David Barnett wrote:
> > >
> > > > Looks like it's patch 7.4.362:
> > > >
> > > > Problem: When matchaddpos() uses a length smaller than the number of
> > > > bytes
> > > > in the (last) character the highlight continues until the end of
> > > > the line.
> > > > Solution: Change condition from equal to larger-or-equal.
> > > > Files: src/screen.c
> > > >
> > > >
> > > > It's a very small change but apparently problematic. Can it be reverted or
> > > > rethought?
> > >
> > > I'm glad you could pinpoint it. I assumed that when the condition
> > > evaluates to true the match information would be updated. But perhaps
> > > that doesn't happen in this case and it searches for a match every time.
> > >
> > > Needs some debugging to figure out what happens. What matches does this
> > > plugin add when it's slow?
> >
> > Perhaps it is better to revert that patch and make sure, highlighting
> > gets only applied, if there is a match at the actual position?
> >
> > diff --git a/src/screen.c b/src/screen.c

[...]

>
> I haven't tried it, but it looks like this only avoids highlighting
> positions after the endcol when "v" and shl->endcol aren't exactly the
> same (which can happen for multi-byte characters), but it doesn't update
> the state, thus a next match might be missed.

Yes, that is what patch 7.3.362 tried to fix, isn't it?

Mit freundlichen Grüßen
Christian
--
Kleiner Lötkolben für Prozessorreparatur gesucht.

[John Little]

On Monday, August 11, 2014 5:27:58 PM UTC+12, Dominique Pelle wrote:
>
> I can now reproduce the slow rendering you
> describe with:
>
> $ cd vim/src
> $ vim -u NONE option.c \
> -c 'syntax on' +471 \
> -c "hi def link RightMargin Error|exec ('match RightMargin /\%<42v.\%>41v/')"
>

> Then I press CTRL-F and CTRL-B ...

Dominique, I see the problem with your example, and it persists with syntax off;
this is consistent with syntime report not showing anything interesting. The hi def link command has to be present, however.

Regards, John Little

[Bram Moolenaar]

So your patch doesn't work. Try this:

highlight MyGroup ctermbg=green guibg=green
let m = matchaddpos("MyGroup", [[4, 24, 2], [4, 30, 2]])

" asdfasdfasdfasdfasdfas℅fasdfasdfasdfasdf

Before pach 362 it would highlight everything from column 24.
After the fix it looks OK, but we get complaints about slowness.
After your fix only the first match shows up.

--
How To Keep A Healthy Level Of Insanity:
6. In the memo field of all your checks, write "for sexual favors".

[Christian Brabandt]

Hi Bram!

I don't see that here (screenshot attached)

But anyway, this raises the question again, why matchaddpos() accepts a
list of positions (but only 8 items), while all other functions usually
only accept 1 position.

Best,
Christian
--
Ein Abend, an dem sich alle Anwesenden völlig einig sind, ist ein
verlorener Abend.
-- Albert Einstein

[Bram Moolenaar]

Did you put a multi-byte character at the end of the first match?

> But anyway, this raises the question again, why matchaddpos() accepts a
> list of positions (but only 8 items), while all other functions usually
> only accept 1 position.

It's used for the matchparen plugin. The idea is that it's useful for
other plugins as well.

--
How To Keep A Healthy Level Of Insanity:

12. Sing along at the opera.

[Christian Brabandt]

Ah, now I can reproduce it.

>> But anyway, this raises the question again, why matchaddpos() accepts
>> a
>> list of positions (but only 8 items), while all other functions
>> usually
>> only accept 1 position.
>
> It's used for the matchparen plugin. The idea is that it's useful for
> other plugins as well.

Yes, but for consistency with the other match() functions and the VimL
API
it might still be a good idea to only accept a single position per
matchaddpos() call. It shouldn't be too hard, to adjust the matchparen
plugin
to call matchaddpos() twice, right?

Best,
Christian

[Bram Moolenaar]

Christian Brabandt wrote:

> >> But anyway, this raises the question again, why matchaddpos()
> >> accepts a list of positions (but only 8 items), while all other
> >> functions usually only accept 1 position.
> >
> > It's used for the matchparen plugin. The idea is that it's useful for
> > other plugins as well.
>
> Yes, but for consistency with the other match() functions and the VimL
> API it might still be a good idea to only accept a single position per
> matchaddpos() call. It shouldn't be too hard, to adjust the matchparen
> plugin to call matchaddpos() twice, right?

It would require another way to deal with IDs. Or allow for adding
another match with the same ID.

--
How To Keep A Healthy Level Of Insanity:

15. Five days in advance, tell your friends you can't attend their
party because you're not in the mood.

[Christian Brabandt]

On Do, 14 Aug 2014, Bram Moolenaar wrote:

> Christian Brabandt wrote:
>
> > >> But anyway, this raises the question again, why matchaddpos()
> > >> accepts a list of positions (but only 8 items), while all other
> > >> functions usually only accept 1 position.
> > >
> > > It's used for the matchparen plugin. The idea is that it's useful for
> > > other plugins as well.
> >
> > Yes, but for consistency with the other match() functions and the VimL
> > API it might still be a good idea to only accept a single position per
> > matchaddpos() call. It shouldn't be too hard, to adjust the matchparen
> > plugin to call matchaddpos() twice, right?
>
> It would require another way to deal with IDs. Or allow for adding
> another match with the same ID.

Okay, here is another patch. This should fix all known problems, that
have been mentioned so far. BTW, perhaps we could use h_east (sorry
don't remember your name) test and include it as performance test?

Best,
Christian
--
Wer in sich recht ernstlich hinabsteigt, wird sich immer nur als
Hälfte finden; er fasse nachher ein Mädchen oder eine Welt, um sich
zum Ganzen zu konstituieren, das ist einerlei.
-- Goethe, Maximen und Reflektionen, Nr. 708

[Bram Moolenaar]

Christian Brabandt wrote:

> On Do, 14 Aug 2014, Bram Moolenaar wrote:
>
> > Christian Brabandt wrote:
> >
> > > >> But anyway, this raises the question again, why matchaddpos()
> > > >> accepts a list of positions (but only 8 items), while all other
> > > >> functions usually only accept 1 position.
> > > >
> > > > It's used for the matchparen plugin. The idea is that it's useful for
> > > > other plugins as well.
> > >
> > > Yes, but for consistency with the other match() functions and the VimL
> > > API it might still be a good idea to only accept a single position per
> > > matchaddpos() call. It shouldn't be too hard, to adjust the matchparen
> > > plugin to call matchaddpos() twice, right?
> >
> > It would require another way to deal with IDs. Or allow for adding
> > another match with the same ID.
>
> Okay, here is another patch. This should fix all known problems, that
> have been mentioned so far. BTW, perhaps we could use h_east (sorry
> don't remember your name) test and include it as performance test?

Thanks. It's a bit strange to use mb_l here, it's hard to see it has the
correct value.

I found another solution: check if shl->lnum is equal to lnum. That's
how the match is disabled by next_search_hl().

--
Apparently, 1 in 5 people in the world are Chinese. And there are 5
people in my family, so it must be one of them. It's either my mum
or my dad. Or my older brother Colin. Or my younger brother
Ho-Cha-Chu. But I think it's Colin.

[David Barnett]

Any updates here, or is it still broken in HEAD? If it's still not fixed, it would be great to have the offending change reverted while waiting for the complete solution.

David

[Christian Brabandt]

Hi David!

On Fr, 29 Aug 2014, David Barnett wrote:

> Any updates here, or is it still broken in HEAD? If it's still not fixed,
> it would be great to have the offending change reverted while waiting for
> the complete solution.

Can you still reproduce the issue with the latest patch level? It should
have been fixed with patch 7.4.405

Best,
Christian
--
Niemand lebt davon, daß er das Leben verneint.
-- André Malraux

[David Barnett]

Nope, hadn't tried, I was just watching for an update on this thread.

Thanks for taking care of it!

David