[vim/vim] refactor matchparen plugin in Vim9 script (#7985)

144 views
Skip to first unread message

lacygoill

unread,
Mar 19, 2021, 7:46:24 AM3/19/21
to vim/vim, Subscribed

This PR refactors the default matchparen plugin in Vim9 script. It also includes fixes for these issues:

A few comments regarding the original version of the plugin.

if exists("g:loaded_matchparen") || &cp || !exists("##CursorMoved")

The last exists() check is useless. CursorMoved already existed in 7.0.001 which is 15 years old.

autocmd! CursorMoved,CursorMovedI,WinEnter * call s:Highlight_Matching_Pair()

CursorMoved is fired very often. It would help to delay the function with a timer, and a short – configurable – waiting time. I suggest g:matchparen_config.highlight_delay.

The idea of using a timer comes from the vim-parenmatch plugin, whose author is credited in the new version.

if exists('##TextChanged')

This check is useless; TextChanged was introduced in 7.3.883 which is almost 8 years old.

" Skip the rest if it was already done.
if exists("*s:Highlight_Matching_Pair")
finish
endif

The only reason why this guard is currently necessary is to let :DoMatchParen re-enable the plugin (by re-installing its autocmds):

runtime plugin/matchparen.vim

without the functions being redefined; otherwise an error would be raised because :func is never followed by a bang.

A better design would be to wrap the autocmds inside a function which could install or clear them on-demand. This way, no need for a guard and no need to partially reload the plugin (which often creates issues).

if pumvisible() || (&t_Co < 8 && !has("gui_running"))

Since s:Highlight_Matching_Pair() is invoked on every motion, the last two checks are inefficient. If – on a given motion – the terminal doesn't support colors, or we're not in the GUI, Vim won't start to support more colors or be in the GUI on the next motion. Actually, it could (via :gui), but this is a corner-case which doesn't warrant checking on every motion. It would be more efficient to move these checks at the start of the plugin.

let matches = matchlist(text, '\(.\)\=\%'.c_col.'c\(.\=\)')

Using a regex to get the character under the cursor (and the one before) is inefficient. The fastest method (that I know of) is:

getline('.')[charcol('.') - 1]

This is only reliable in Vim9 script though; if there is a multibyte character under the cursor, it would not work as expected in a legacy function. In legacy Vim script, the fastest method (that I know of) is:

getline('.')->strpart(col('.') - 1, 1, v:true)

For the character before the cursor:

# Vim9

getline('.')->strpart(0, col('.') - 1)[-1]



" legacy

getline('.')->slice(charcol('.') - 2, charcol('.') - 1)

let plist = split(&matchpairs, '.\zs[:,]')

Since s:Highlight_Matching_Pair() is invoked on every motion, parsing 'matchpairs' here is inefficient. It would be faster to parse the option on these events:

 ┌──────────────────────┬─────────────────────────────────────────────────────────────────┐

 │ WinEnter             │ for when we focus a different window                            │

 ├──────────────────────┼─────────────────────────────────────────────────────────────────┤

 │ BufWinEnter          │ for when we stay in the same window but load a different buffer │

 ├──────────────────────┼─────────────────────────────────────────────────────────────────┤

 │ VimEnter             │ for when we start Vim                                           │

 ├──────────────────────┼─────────────────────────────────────────────────────────────────┤

 │ FileType             │ for when a filetype plugin resets 'matchpairs'                  │

 ├──────────────────────┼─────────────────────────────────────────────────────────────────┤

 │ OptionSet matchpairs │ for when we manually reset 'matchpairs'                         │

 └──────────────────────┴─────────────────────────────────────────────────────────────────┘

That's what the vim-parenmatch plugin does, from which – again – I took the idea.

if c_col > 1 && (mode() == 'i' || mode() == 'R')

if mode() == 'i' || mode() == 'R'

Evaluating mode() on these 2 lines is inefficient. It won't change in-between. It's better to save it in a variable.

let has_getcurpos = exists("*getcurpos")

This check is useless. Vim supports getcurpos() since 7.4.313 which is 7 years old.

if !has("syntax") || !exists("g:syntax_on")

The has("syntax") check is inefficient. Vim can't start supporting syntax highlighting in-between 2 motions. It would be better to get the skip expression from another function whose definition would be different depending on whether Vim was compiled with syntax highlighting. One might argue that the gain is negated by the new extra function call; that might be true, but I think it lets us write a more readable code by moving all the logic responsible for determining which skip expression to use in a separate function.

Also, inspecting the syntax under the cursor might be too costly for some users (it depends on their machine capabilities and the type of files they edit); they should have a chance to disable the check. That's why I suggest a new configuration option: g:matchparen_config.syntax_ignored.

let s_skip = '!empty(filter(map(synstack(line("."), col(".")), ''synIDattr(v:val, "name")''), ' .
\ '''v:val =~? "string\\|character\\|singlequote\\|escape\\|comment"''))'

It would be more readable if it was rewritten using lambdas and method calls:

let S_skip = {-> !synstack('.', col('.'))->map({_, v -> synIDattr(v, 'name')})->filter({_, v -> v =~? 'string\|character\|singlequote\|escape\|comment'})->empty()}

Also, no need of filter() + empty(); match() can do the same thing, is easier to read, and might sometimes be a little faster:

let S_skip = {-> synstack('.', col('.'))->map({_, v -> synIDattr(v, 'name')})->match('\cstring\|character\|singlequote\|escape\|comment') >= 0}

And no need of line('.'); synstack() translates '.' as the current line number automatically, when it's its first argument. BTW, it would be nice if it could do the same for col('.'):

# weird because asymmetrical

synstack('.', col('.'))



# nice because symmetrical

synstack('.', '.')

" If executing the expression determines that the cursor is currently in
" one of the syntax types, then we want searchpairpos() to find the pair
" within those syntax types (i.e., not skip). Otherwise, the cursor is
" outside of the syntax types and s_skip should keep its value so we skip
" any matching pair inside the syntax types.
" Catch if this throws E363: pattern uses more memory than 'maxmempattern'.
try
execute 'if ' . s_skip . ' | let s_skip = "0" | endif'

The comment and the code both contain an error.

we want searchpairpos() to find the pair within those syntax types (i.e., not skip)

Making searchpairpos() looking for a pair within specific syntax types (strings/comments/...) is not equivalent to disabling the skip expression; it's equivalent to inverting the skip expression. So, the code should not be:

execute 'if ' . s_skip . ' | let s_skip = "0" | endif'

But rather:

execute 'if ' . s_skip . ' | let s_skip = "!" . s_skip | endif'

Actually, it still needs to be adapted to the previously mentioned lambdas; the previous line just gives an idea of what should be done. Anyway, the new version fixes this issue.

let [m_lnum, m_col] = searchpairpos(c, '', c2, s_flags, s_skip, stopline, timeout)

Because of synstack() inside the skip expression, searchpair() could raise E363. It should be caught to fix these kind of issues:

catch /E118/
" Can't use the timeout, restrict the stopline a bit more to avoid taking
" a long time on closed folds and long lines.
" The "viewable" variables give a range in which we can scroll while
" keeping the cursor at the same position.
" adjustedScrolloff accounts for very large numbers of scrolloff.
let adjustedScrolloff = min([&scrolloff, (line('w$') - line('w0')) / 2])
let bottom_viewable = min([line('$'), c_lnum + &lines - adjustedScrolloff - 2])
let top_viewable = max([1, c_lnum-&lines+adjustedScrolloff + 2])
" one of these stoplines will be adjusted below, but the current values are
" minimal boundaries within the current window
if i % 2 == 0
if has("byte_offset") && has("syntax_items") && &smc > 0
let stopbyte = min([line2byte("$"), line2byte(".") + col(".") + &smc * 2])
let stopline = min([bottom_viewable, byte2line(stopbyte)])
else
let stopline = min([bottom_viewable, c_lnum + 100])
endif
let stoplinebottom = stopline
else
if has("byte_offset") && has("syntax_items") && &smc > 0
let stopbyte = max([1, line2byte(".") + col(".") - &smc * 2])
let stopline = max([top_viewable, byte2line(stopbyte)])
else
let stopline = max([top_viewable, c_lnum - 100])
endif
let stoplinetop = stopline
endif
let [m_lnum, m_col] = searchpairpos(c, '', c2, s_flags, s_skip, stopline)

All of this code is useless and makes the plugin harder to read/maintain. Its original purpose was to handle the case where searchpairpos() doesn't support {timeout}. It does since 7.1.211 which is more than 13 years old; there's no way E118 can be raised nowadays.

if exists('*matchaddpos')

This check is useless; Vim supports matchaddpos() since 7.4.330 which is 7 years old.

call matchaddpos('MatchParen', [[c_lnum, c_col - before], [m_lnum, m_col]], 10, 3)
else
exe '3match MatchParen /\(\%' . c_lnum . 'l\%' . (c_col - before) .
\ 'c\)\|\(\%' . m_lnum . 'l\%' . m_col . 'c\)/'

Using a match can create weird issues, such as:

Text properties would be better. We need prop_type_add(), prop_add() and prop_remove(); they were added in 8.1.0579 which is more than 2 years old.

command DoMatchParen call s:DoMatchParen()
command NoMatchParen call s:NoMatchParen()

The commands should be defined with the -bar attribute so that they can be used in a bar-separated sequence of commands, which is common in an autocmd:

autocmd SomeEvent * if 'some condition' | NoMatchParen | else | DoMatchParen | endif

Currently, this raises an unexpected error:

E488: Trailing characters: | else | DoMatchParen | endif

Also, it would have been better to use the same prefix for the command names:

# bad

:DoMatchParen

:NoMatchParen



# good

:MatchParenOn

:MatchParenOff

This would reduce irrelevant suggestions when using tab completion on the command-line. Obviously, we can't do that now if we want to be backwards compatible. However, we could introduce a new boolean option (g:matchparen_config.old_commands) which would let the user prevent their installation. In any case, the commands would always be installed under their new names.

And since we provide toggling commands, it means we acknowledge the fact that the user might not want the plugin to be enabled all the time. Thus, it makes sense to also provide an option which lets the user disable the plugin on startup:

g:matchparen_config = {

  ...

  on_startup: false,

  ...

}

Which might be easier to read for some people than this:

au VimEnter * MatchParenOff

noau windo silent! call matchdelete(3)

Even with :noau, :windo has unavoidable side-effects. For example, if an inactive window is squashed to 0 lines (only possible if 'winminheigh' was set to 0), :windo will reset its height to 1 line. Instead, we should iterate over the window IDs, and – on each of them – run the command via win_execute(). The latter has fewer side-effects (if not none). Example:

getwininfo()->map((_, v) => win_execute(v.winid, 'silent! matchdelete(3)'))

However, all of this is actually useless. Since this commit, the matchparen plugin has a WinLeave autocmd which removes matches when leaving a window. The purpose was to fix the issue #1244. Now, there can only be 1 window where matches are active. So, we only need to remove them in the current window, for which :windo is overkill. Extra benefit: without :windo, there's no need to save and restore the active window either.

runtime plugin/matchparen.vim

Already mentioned, but re-sourcing the plugin is awkward. The autocmds should be wrapped inside a function, and this function should be called here.

silent windo doau CursorMoved

Again, I don't think :windo makes sense. The matches should only be highlighted in the current window, because they're tied to the cursor. And only 1 window has the cursor.

Also, this is not very readable ("Why firing CursorMoved here?"), and firing CursorMoved could have undesirable side-effects. In any case, that's not what we really want here. What we really want is to highlight the matches; that's the job of s:Highlight_Matching_Pair(), which should be called here.

The search is limited to avoid a delay when moving the cursor. The limits
are:

Since we might introduce the config option g:matchparen_config.highlight_delay, which already uses the term "delay", it would be useful to use another term to avoid any confusion. How about "lag"? If there's an issue, I think that's how a lot of people would frame the issue in a bug report.

- 100 lines above or below the cursor to avoid a long delay when there are
closed folds.
- 'synmaxcol' times 2 bytes before or after the cursor to avoid a delay
in a long line with syntax highlighting.

That was relevant when the complex block of code after the catch /E118/ statement could sometimes be processed; i.e. when matchaddpos() did not support the optional {timeout} argument. That's no longer the case. This block of code is now dead, and this excerpt is stale.

Test for the issue #1532:

vim -Nu NORC -S <(cat <<'EOF'

    vim9script

    set lines=24

    var lines =<< trim END

        1

        2

        3 wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines wrapped on 15 lines

        4

        5

        6

        7

        8

        9

        10

        11

        12

        13

        14

        15 {

        16

        17

        18

        19

        20

        21

        22

        23

        24

        } 25

        26

        27

        28

        29

        30 wrapped on 2 lines wrapped on 2 lines wrapped on 2 lines wrapped on 2 lines wrapped on 2 lines

        31

        32

        33

        34

        35

        36

        37

        38 wrapped on 2 lines wrapped on 2 lines wrapped on 2 lines wrapped on 2 lines wrapped on 2 lines

    END

    setline(1, lines)

    feedkeys("23\<c-e>\<c-b>", 'nt')

EOF

)

With the current version of the plugin, the line with the text 24 is wrongly duplicated at the very bottom.
With the new version, the line with the text 24 is no longer wrongly duplicated; the last line displays the text 27 which is correct.

Test for the issue #3543:

vim -Nu NORC -S <(cat <<'EOF'

    vim9script

    set nocompatible

    filetype plugin indent on

    syntax enable

    au BufReadPost * cursor(6, 7)



    var lines =<< END

%{

figure(1); clf;

plot(1:npairskeep, 1./err.*log(1./err), ...

    1:npairskeep, 0.5*(1:npairskeep));



figure(2); clf;

plot(1:npairskeep, mean(err));

epsilon = linspace(min(mean(err)),1);

mbound = 7./epsilon .* log(20./epsilon);

plot(1:npairskeep, mean(err)+2*std(err)/sqrt(ntrials), ...

    1:npairskeep, mean(err)-2*std(err)/sqrt(ntrials), ...

    1:npairskeep, err, ...

    mbound, epsilon);

%}

END



    writefile(lines, '/tmp/m.m')

    syn on

    sil e /tmp/m.m

    feedkeys('1G2|jjjjjf)kkkkkjjjjj', 'nt')

EOF

)

With the current version of the plugin, the end of the block is no longer highlighted as a multiline comment.
With the new version, all the block remains properly highlighted as a multiline comment.

Test for the issue #4589:

vim -Nu NORC -S <(cat <<'EOF'

    vim9script

    var lines =<< trim END

        a = 'blablabal'

        #     | put cursor above "{" or "}" in the "c = ..." line

        b =  { bla: 'bla' }   # <- start cursor in this line

        bla

        c = '{}{}'   # <- move cursor down to this line, as it is on "{" showmatch won't trigger

        d.blabala()

        c = '{}{}'

        bla

        d.blabala()

    END

    writefile(lines, '/tmp/py.py')

    syn on

    set showmatch

    sil e /tmp/py.py

    feedkeys('3G6|jj', 'nt')

EOF

)

With the current version of the plugin, the brace under the cursor and its matching pair are not highlighted.
With the new version, the braces are highlighted.

Test for #5638:

curl -Ls https://gist.githubusercontent.com/MRAAGH/11e7ea3dd460b393fc7e70d746b368f1/raw/03deddf9db2d248d5e7c56dedfb0c0d9d717bb42/vim_matchparen_bug >/tmp/file && vim -Nu NORC +'set lines=35 columns=166 scrolloff=0 | exe "norm! 97zb/} 2/e\<cr>" | call feedkeys("\<c-d>\<c-u>", "nt")' /tmp/file

If you can't reproduce the issue, temporarily reset the font to DejaVu Sans Mono with a font size of 12.

With the current version of the plugin, the line at the bottom of the window is wrongly displayed twice:

5 (this should be the last visible line in the window)

5 (this should be the last visible line in the window)

With the new version, the bottom line is only displayed once.

Test for #5880 and #6777:

curl -Ls https://www.genivia.com/files/README.md >/tmp/md.md && vim -Nu NORC /tmp/md.md +'syn on | call feedkeys("2735GoThe char[", "nt")'

With the current version of the plugin, E363 is raised from s:Highlight_Matching_Pair():

    Error detected while processing CursorMovedI Autocommands for "*"..function <SNR>18_Highlight_Matching_Pair[134]..CursorMovedI Autocommands for "*"..function <SNR>18_Highlight_Matching_Pair:

    line  104:

    E363: pattern uses more memory than 'maxmempattern'

With the new version of the plugin, E363 is still raised, but not from s:Highlight_Matching_Pair(); from the default markdown syntax plugin. In particular, notice the absence of a stacktrace.

BTW, #6777 is a duplicate of #5880.

Test for #7054:

vim -Nu NORC +'call feedkeys("ii\r{\r}\r{\r}\<c-\>\<c-n>gg3ddpuu", "nt")'

With the current version of the plugin, the brackets on the last 2 lines are wrongly highlighted.
With the new version of the plugin, the brackets are not highlighted.

Test for #7880:

vim -Nu NORC -S <(cat <<'EOF'

    vim9script

    syn on

    var lines =<< trim END

        print('Hi',

              # ) (

              'Bye')

    END

    writefile(lines, '/tmp/py.py')

    sil e /tmp/py.py

    feedkeys('2Gf)', 'nt')

EOF

)

With the current version of the plugin, the opening parenthesis on the first line is wrongly highlighted.
With the new version of the plugin, it is not highlighted.

You can view, comment on, or merge this pull request online at:

  https://github.com/vim/vim/pull/7985

Commit Summary

  • refactor matchparen plugin in Vim9 script

File Changes

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

lacygoill

unread,
Mar 19, 2021, 7:46:51 AM3/19/21
to vim/vim, Subscribed

I've just finished the refactoring, and there are no tests (that I know of) to check whether the new version introduces regressions. If I find issues in the next few days/weeks, I'll update the PR.

lacygoill

unread,
Mar 19, 2021, 7:47:19 AM3/19/21
to vim/vim, Subscribed

While refactoring the plugin, I've noticed that prop_remove() was much slower than matchdelete(). It was especially noticeable while moving the cursor with hl.

To make it faster, I've tried various things:

# don't use 'all: true'
prop_remove({type: 'matchparen'})

# limit the removal to the displayed lines
prop_remove({type: 'matchparen'}, line('w0'), line('w$'))

# limit the removal to the current line
prop_remove({type: 'matchparen'}, line('.'))

# limit the removal to the current line
prop_remove({type: 'matchparen'}, line('.'))

# clearing all text properties on current line
prop_clear(line('.'))

No matter what I did, it was always much slower.

Note that the current version of the plugin doesn't suffer from this, because it only invokes prop_remove() when it's really necessary.

Anyway, could prop_remove() be optimized internally?

codecov[bot]

unread,
Mar 19, 2021, 7:48:47 AM3/19/21
to vim/vim, Subscribed

Codecov Report

Merging #7985 (3749fc1) into master (5f91e74) will decrease coverage by 86.68%.
The diff coverage is 0.00%.

❗ Current head 3749fc1 differs from pull request most recent head 9393f0c. Consider uploading reports for the commit 9393f0c to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@

##           master    #7985       +/-   ##

===========================================

- Coverage   89.17%    2.49%   -86.69%     

===========================================

  Files         148      146        -2     

  Lines      165299   160182     -5117     

===========================================

- Hits       147399     3990   -143409     

- Misses      17900   156192   +138292
Flag Coverage Δ
huge-clang-none ?
huge-gcc-none ?
huge-gcc-testgui ?
huge-gcc-unittests 2.49% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/gui_beval.c 0.00% <0.00%> (-61.64%) ⬇️
src/gui_gtk_x11.c 0.45% <0.00%> (-62.23%) ⬇️
src/gui_xim.c 0.00% <0.00%> (-23.75%) ⬇️
src/list.c 2.43% <0.00%> (-92.65%) ⬇️
src/version.c 0.86% <ø> (-91.48%) ⬇️
src/vim9compile.c 0.00% <0.00%> (-94.04%) ⬇️
src/vim9script.c 0.00% <0.00%> (-93.57%) ⬇️
src/vim9type.c 0.00% <0.00%> (-95.61%) ⬇️
src/sha256.c 0.00% <0.00%> (-97.96%) ⬇️
src/digraph.c 0.00% <0.00%> (-97.78%) ⬇️
... and 142 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f91e74...9393f0c. Read the comment docs.

lacygoill

unread,
Mar 19, 2021, 8:03:15 AM3/19/21
to vim/vim, Subscribed

I've just realized that Vim could be compiled without the +textprop feature. The plugin should fall back on matchaddpos() and matchdelete() in that case. I'll push a commit later.

Dominique Pellé

unread,
Mar 19, 2021, 8:17:35 AM3/19/21
to vim/vim, Subscribed

@lacygoill wrote:

Anyway, could prop_remove() be optimized internally?

Do you have a simple script that illustrates prop_remove() being slow?

lacygoill

unread,
Mar 19, 2021, 8:19:02 AM3/19/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • e97b09a fall back on matchaddpos() if Vim does not support text properties

—
You are receiving this because you are subscribed to this thread.

View it on GitHub or unsubscribe.

lacygoill

unread,
Mar 19, 2021, 8:26:37 AM3/19/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • febea0c prop_type_add() is not available without the +textprop feature

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 19, 2021, 8:36:38 AM3/19/21
to vim/vim, Subscribed

Do you have a simple script that illustrates prop_remove() being slow?

Sorry, I should have posted one. I'll try to find one later. In the meantime, here is a profile log which I got while working on a previous version of the plugin.

In particular, notice this line in the profiling of <SNR>38_RemoveHighlight():

320              0.151003   prop_remove({type: 'matchparen', all: true})

And the 2nd line in this block:

FUNCTIONS SORTED ON TOTAL TIME
count  total (s)   self (s)  function
  320   0.162068   0.007174  <SNR>38_Highlight()
  320   0.151507             <SNR>38_RemoveHighlight()
    9   0.005616   0.000074  <lambda>69()
    1   0.004328   0.000711  <SNR>38_Toggle()
    2   0.002636             <SNR>38_InStringOrComment()
  312   0.001985             <SNR>38_GetOption()
    1   0.001407   0.000005  <SNR>38_GetSkip()
    1   0.000333             <SNR>38_Autocmds()
    1   0.000075             <SNR>38_ParseMatchpairs()
                             <lambda>68()
                             <lambda>70()
                             <lambda>71()
                             <lambda>72()
                             <lambda>73()

As soon as I find a reproducible example, I'll post it here.

lacygoill

unread,
Mar 19, 2021, 8:40:33 AM3/19/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • de15e30 no need to assign any value in declaration

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 19, 2021, 11:08:03 AM3/19/21
to vim/vim, Push

@lacygoill pushed 2 commits.

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 19, 2021, 12:05:12 PM3/19/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • 5ef26fc we don't use :match anymore

—
You are receiving this because you are subscribed to this thread.

Bram Moolenaar

unread,
Mar 19, 2021, 5:03:38 PM3/19/21
to vim/vim, Subscribed

About using prop_remove() vs matchdelete: Text properties will be faster to display, but adding/removing involves updating the text storage. Adding/removing a match will be fast, but reduce display updates. What works best is hard to predict.

lacygoill

unread,
Mar 20, 2021, 2:20:48 AM3/20/21
to vim/vim, Push

@lacygoill pushed 1 commit.

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 20, 2021, 5:10:03 AM3/20/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • 37d68ff WinLeave is not always enough to remove highlights

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 20, 2021, 5:40:14 AM3/20/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • d25c376 if a match is found it's necessarily visible

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 20, 2021, 5:50:26 AM3/20/21
to vim/vim, Push

@lacygoill pushed 1 commit.

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 20, 2021, 2:34:33 PM3/20/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • 59a5ce3 has_key() is more readable as a method

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 21, 2021, 3:50:06 AM3/21/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • eaf99b2 Removing highlights can fail. Make it more reliable.

—
You are receiving this because you are subscribed to this thread.

lacygoill

unread,
Mar 21, 2021, 6:15:25 AM3/21/21
to vim/vim, Subscribed

Do you have a simple script that illustrates prop_remove() being slow?

Sorry for the delay. Here is one:

vim -Nu NONE -S <(cat <<'EOF'
    vim9script
    prop_type_add('test', {highlight: 'ErrorMsg'})
    au CursorMoved * Func()
    def Func()
        prop_remove({type: 'test', all: true})
    enddef
    repeat(['" ' .. repeat('x', 78)], 20)->setline(1)
    syn on
    set ft=vim
    set ww=h,l
EOF
)

When I start Vim with this shell command (in an xterm terminal, 24 lines x 80 columns), then maintain the l key pressed for some time to scroll horizontally on the commented lines, my cpu load increases signficantly. It goes from between 2% and 3% to between 11% and 12%. In practice, it can cause some lag. That is, sometimes, the cursor is not displayed, which is distracting.

Here is another shell command, which uses matchdelete() instead:

vim -Nu NONE -S <(cat <<'EOF'
    vim9script
    au CursorMoved * Func()
    w:id = matchadd('ErrorMsg', '^" xxx')
    def Func()
        sil! matchdelete(w:id)
    enddef
    repeat(['" ' .. repeat('x', 78)], 20)->setline(1)
    syn on
    set ft=vim
    set ww=h,l
EOF
)

This time, my cpu load remains low and stable (i.e. between 2% and 3%). And I can't observe any noticeable lag.

Text properties will be faster to display [...]


Adding/removing a match will be fast, but reduce display updates.

Ah, so this explains why using text properties fixes a whole bunch of reported issues.

but adding/removing involves updating the text storage.

I see. There is still a way to obtain the desired effect here without the slowness. Remove the text property with prop_type_delete(), then immediately afterward re-install it with prop_type_add(). As soon as the text property is deleted, all the existing properties no longer cause any text to be highlighted. Unfortunately, they still exist (we can see them with prop_list()), so I don't use this method as I don't know if anything bad could happen when they pile up over time.

What works best is hard to predict.

It seems that text properties are just better anyway. We just need to not blindly clear them on every single motion. We must first check that there are such properties in the buffer, which is not too hard; a simple dictionary of boolean values seems enough.

lacygoill

unread,
Mar 21, 2021, 6:27:50 AM3/21/21
to vim/vim, Push

@lacygoill pushed 1 commit.

  • 3b70d46 Optimize removal of text properties.

—
You are receiving this because you are subscribed to this thread.

Bram Moolenaar

unread,
Mar 21, 2021, 10:14:33 AM3/21/21
to vim/vim, Subscribed


> > Do you have a simple script that illustrates prop_remove() being slow?
>
> Sorry for the delay. Here is one:
>
> vim -Nu NONE -S <(cat <<'EOF'
> vim9script
> prop_type_add('test', {highlight: 'ErrorMsg'})
> au CursorMoved * Func()
> def Func()
> prop_remove({type: 'test', all: true})
> enddef
> repeat(['" ' .. repeat('x', 78)], 20)->setline(1)
> syn on
> set ft=vim
> set ww=h,l
> EOF
> )
>
> When I start Vim with this shell command (in an xterm terminal, 24
> lines x 80 columns), then maintain the `l` key pressed for some time
> to scroll horizontally on the commented lines, my cpu load increases
> signficantly. It goes from between 2% and 3% to between 11% and 12%.
> In practice, it can cause some lag. That is, sometimes, the cursor is
> not displayed, which is distracting.

You call prop_remove() without any line number, which means it will go
over all lines in the buffer. If you restrict it to a range of lines it
will be much faster.

--
hundred-and-one symptoms of being an internet addict:
8. You spend half of the plane trip with your laptop on your lap...and your
child in the overhead compartment.

/// 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 ///

lacygoill

unread,
Mar 21, 2021, 10:56:25 AM3/21/21
to vim/vim, Subscribed

You call prop_remove() without any line number, which means it will go
over all lines in the buffer. If you restrict it to a range of lines it
will be much faster.

That's one of the first things I tried, and it didn't help. And it still doesn't help in this short snippet:

vim9script
prop_type_add('test', {highlight: 'ErrorMsg'})
au CursorMoved * Func()
def Func
()
    prop_remove({type: 'test'}, line('.'), line('.'))

enddef
repeat(['" ' .. repeat('x', 78)], 20)->setline(1)
syn on
set ft=vim
set ww=h,l

set lines=24 columns=80
lacygoill
unread,
Mar 21, 2021, 10:59:46 AM3/21/21
to vim/vim, Subscribed




That's one of the first things I tried, and it didn't help. And it still doesn't help in this short snippet:




I'm not saying it doesn't help in the general case.  I know it does.  I've even pushed a recent commit to limit the removal of the text properties to the displayed lines, because I've made some tests showing that indeed it could help significantly.


I'm saying that it doesn't help in the (special?) case I posted.  Maybe it has something to do with the syntax highlighting, I don't know.
lacygoill
unread,
Mar 21, 2021, 11:15:52 AM3/21/21
to vim/vim, Subscribed




Maybe it has something to do with the syntax highlighting, I don't know.




Yes, it's an interaction with the syntax highlighting.  Because the issue disappears when it's disabled with :syn off.  And it re-appears with :syn on.  Maybe it's expected, I don't know.  But there is no such issue with matches removed with matchdelete().
Dominique Pellé
unread,
Mar 21, 2021, 12:45:15 PM3/21/21
to vim/vim, Subscribed



If I profile with gprof this command and press l many times...



vim -Nu NONE -S <(cat <<'EOF'

    vim9script

    prop_type_add('test', {highlight: 'ErrorMsg'})

    au CursorMoved * Func()

    def Func()

        prop_remove({type: 'test', all: true})

    enddef

    repeat(['" ' .. repeat('x', 78)], 20)->setline(1)

    syn on

    set ft=vim

    set ww=h,l

EOF

)





I get:



Flat profile:



Each sample counts as 0.01 seconds.

  %   cumulative   self              self     total           

 time   seconds   seconds    calls   s/call   s/call  name    

 29.29      0.82     0.82 24517280     0.00     0.00  addstate

 22.50      1.45     0.63   312806     0.00     0.00  nfa_regmatch

  6.61      1.64     0.19 59872126     0.00     0.00  utfc_ptr2len

  6.43      1.82     0.18 40841835     0.00     0.00  copy_sub

  3.93      1.93     0.11   575100     0.00     0.00  regmatch

  3.21      2.02     0.09 10089100     0.00     0.00  mb_get_class_buf

  2.86      2.10     0.08 11651103     0.00     0.00  vim_iswordc_buf

  2.50      2.17     0.07 27298532     0.00     0.00  utf_ptr2char

  2.50      2.24     0.07   515159     0.00     0.00  vim_strchr

  2.14      2.30     0.06 12079596     0.00     0.00  regnext

  2.14      2.36     0.06   754305     0.00     0.00  nfa_regexec_both

  1.43      2.40     0.04  4984200     0.00     0.00  reg_prev_class

  1.43      2.44     0.04   575100     0.00     0.00  syn_current_attr

  1.07      2.47     0.03 46855576     0.00     0.00  fast_breakcheck

  1.07      2.50     0.03 18906790     0.00     0.00  line_breakcheck

  0.71      2.52     0.02  5175900     0.00     0.00  reg_save





I also  get something similar with valgrind --tool=callgrind ./vim ….



:syntime on … syntime report gives:



  TOTAL      COUNT  MATCH   SLOWEST     AVERAGE   NAME               PATTERN

  0.749363   6460   0       0.000431    0.000116  vimFunc            \%(\%([sSgGbBwWtTlL]:\|<[sS][iI][dD]>\)\=\%(\w\+\.\)*\I[a-zA-Z0-9_.]*\)\ze\s*(

  0.326026   6460   0       0.000181    0.000050  vimOper            \%#=1\(==\|!=\|>=\|<=\|=\~\|!\~\|>\|<\|=\)[?#]\{0,2}

  0.324723   6460   0       0.000180    0.000050  vimNotation        \(\\\|<lt>\)\=<C-R>[0-9a-z"%#:.\-=]

  0.291503   6460   0       0.000128    0.000045  vimSubst           \(:\+\s*\|^\s*\||\s*\)\<\%(\<s\%[ubstitute]\>\|\<sm\%[agic]\>\|\<sno\%[magic]\>\)[:#[:alpha:]]\@!

  0.265514   6460   6460    0.000143    0.000041  vimLineComment     ^[ \t:]*".*$

  0.263650   6460   0       0.000138    0.000041  vimNotFunc         \<if\>\|\<el\%[seif]\>\|\<return\>\|\<while\>

  0.256317   6460   0       0.000109    0.000040  vimNumber          \%(^\|\A\)\zs#\x\{6}

  0.240585   6460   0       0.000150    0.000037  vimSpecFile        #\d\+\|[#%]<\>

  0.228740   6460   0       0.000077    0.000035  vimOperParen       #\={

  0.221684   6460   0       0.000154    0.000034  vimAutoCmdMod      \(++\)\=\(once\|nested\)

  0.202926   6460   0       0.000107    0.000031  vimRange           [`'][a-zA-Z0-9],[`'][a-zA-Z0-9]

  0.181361   6460   6460    0.000124    0.000028  vimIsCommand       \<\h\w*\>

  0.157176   6460   0       0.000155    0.000024  vimRegister        [^,;[{: \t]\zs"[a-zA-Z0-9.%#:_\-/]\ze[^a-zA-Z_":0-9]

  0.150993   6460   0       0.000116    0.000023  vimOper            ||\|&&\|[-+.!]

  0.141072   6460   0       0.000057    0.000022  vimInsert          ^[: \t]*\(\d\+\(,\d\+\)\=\)\=starti\%[nsert]$

  0.124664   6460   0       0.000067    0.000019  vimSubst           \(:\+\s*\|^\s*\)s\ze#.\{-}#.\{-}#

  0.116548   6460   0       0.000052    0.000018  vimCommentTitle    "\s*\%([sS]:\|\h\w*#\)\=\u\w*\(\s\+\u\w*\)*:

  0.105377   6460   0       0.000116    0.000016  vimString          [^(,]'[^']\{-}\zs'

  0.103306   6460   0       0.000087    0.000016  vimCommentString   \S\s\+"

  0.089008   6460   0       0.000047    0.000014  vimFunction        \<\(fu\%[nction]\|def\)!\=\s\+\%(<[sS][iI][dD]>\|[sSgGbBwWtTlL]:\)\=\%(\i\|[#.]\|{.\{-1,}}\)*\ze\s*(





CPU is about 43% on my machine when I keep pressing l.


Unclear at first sight if there is anything to optimize here.
lacygoill
unread,
Mar 21, 2021, 12:51:45 PM3/21/21
to vim/vim, Subscribed


Thank you for the logs.  I suspect that Vim recomputes the syntax items for all the displayed text whenever a text property is added or removed.  Which makes sense.  What I find weird is:  why doing that even when prop_remove() failed to remove any property?
Dominique Pellé
unread,
Mar 21, 2021, 1:47:40 PM3/21/21
to vim/vim, Subscribed


@lacygoill wrote:




I suspect that Vim recomputes the syntax items for all the

displayed text whenever a text property is added or removed.

Which makes sense. What I find weird is: why doing that

even when prop_remove() failed to remove any property?




Yes, I think you're right.  I don't understand the code of text

properties well, but I think this fixes the performance issue:


Can you try it?


diff --git a/src/textprop.c b/src/textprop.c
index b6cae70a8..e74c13849 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -809,6 +809,7 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
     int                id = -1;
     int                type_id = -1;
     int                both;
+    int                is_removed = FALSE;
 
     rettv->vval.v_number = 0;
     if (argvars[0].v_type != VAR_DICT || argvars[0].vval.v_dict == NULL)
@@ -889,6 +890,7 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
                if (both ? textprop.tp_id == id && textprop.tp_type == type_id
                         : textprop.tp_id == id || textprop.tp_type == type_id)
                {
+                   is_removed = TRUE;
                    if (!(buf->b_ml.ml_flags & ML_LINE_DIRTY))
                    {
                        char_u *newptr = alloc(buf->b_ml.ml_line_len);
@@ -920,7 +922,8 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
            }
        }
     }
-    redraw_buf_later(buf, NOT_VALID);
+    if (is_removed)
+       redraw_buf_later(buf, NOT_VALID);
 }



For me, CPU goes from 43% to 0.7% when pressing l continuously.
lacygoill
unread,
Mar 21, 2021, 1:57:55 PM3/21/21
to vim/vim, Subscribed


Thank you for the patch:


diff --git a/src/textprop.c b/src/textprop.c
index b6cae70a8..e74c13849 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -809,6 +809,7 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
     int		id = -1;
     int		type_id = -1;
     int		both;
+    int		is_removed = FALSE;
 
     rettv->vval.v_number = 0;
     if (argvars[0].v_type != VAR_DICT || argvars[0].vval.v_dict == NULL)
@@ -889,6 +890,7 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
 		if (both ? textprop.tp_id == id && textprop.tp_type == type_id
 			 : textprop.tp_id == id || textprop.tp_type == type_id)
 		{
+		    is_removed = TRUE;
 		    if (!(buf->b_ml.ml_flags & ML_LINE_DIRTY))
 		    {
 			char_u *newptr = alloc(buf->b_ml.ml_line_len);
@@ -920,7 +922,8 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
 	    }
 	}
     }
-    redraw_buf_later(buf, NOT_VALID);
+    if (is_removed)
+	redraw_buf_later(buf, NOT_VALID);
 }
 
 /*


Yes, the difference is huge.  Vim is much more responsive, and the cpu load remains low.
Dominique Pellé
unread,
Mar 21, 2021, 2:23:47 PM3/21/21
to vim/vim, Subscribed


@lacygoill  wrote:




Yes, the difference is huge. Vim is much more responsive, and the cpu load remains low.




Good.


The same optimization can also be done for prop_clear() (not sure how often

it helps in practice). Updated patch:


diff --git a/src/textprop.c b/src/textprop.c
index b6cae70a8..f2b21ca3d 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -535,6 +535,7 @@ f_prop_clear(typval_T *argvars, typval_T *rettv UNUSED)
     linenr_T end = start;
     linenr_T lnum;
     buf_T    *buf = curbuf;
+    int              is_cleared = FALSE;
 
     if (argvars[1].v_type != VAR_UNKNOWN)
     {
@@ -562,6 +563,7 @@ f_prop_clear(typval_T *argvars, typval_T *rettv UNUSED)
        len = STRLEN(text) + 1;
        if ((size_t)buf->b_ml.ml_line_len > len)
        {
+           is_cleared = TRUE;
            if (!(buf->b_ml.ml_flags & ML_LINE_DIRTY))
            {
                char_u *newtext = vim_strsave(text);
@@ -575,7 +577,8 @@ f_prop_clear(typval_T *argvars, typval_T *rettv UNUSED)
            buf->b_ml.ml_line_len = (int)len;
        }
     }
-    redraw_buf_later(buf, NOT_VALID);
+    if (is_cleared)
+       redraw_buf_later(buf, NOT_VALID);
 }
 
 /*
@@ -809,6 +812,7 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
     int                id = -1;
     int                type_id = -1;
     int                both;

+    int                is_removed = FALSE;
 
     rettv->vval.v_number = 0;
     if (argvars[0].v_type != VAR_DICT || argvars[0].vval.v_dict == NULL)

@@ -889,6 +893,7 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
                if (both ? textprop.tp_id == id && textprop.tp_type == type_id
                         : textprop.tp_id == id || textprop.tp_type == type_id)
                {

+                   is_removed = TRUE;
                    if (!(buf->b_ml.ml_flags & ML_LINE_DIRTY))
                    {
                        char_u *newptr = alloc(buf->b_ml.ml_line_len);

@@ -920,7 +925,8 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)
            }
        }
     }

-    redraw_buf_later(buf, NOT_VALID);
+    if (is_removed)
+       redraw_buf_later(buf, NOT_VALID);
 }
 
 /*



—
lacygoill
unread,
Mar 21, 2021, 3:00:45 PM3/21/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • cac6728  no need to combine text property with syntax highlighting
    • 





—
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
unread,
Mar 21, 2021, 5:32:27 PM3/21/21
to vim/vim, Subscribed



Dominique wrote:



> @lacygoill wrote:

> 

> > I suspect that Vim recomputes the syntax items for all the

> > displayed text whenever a text property is added or removed.

> > Which makes sense. What I find weird is: why doing that

> > even when prop_remove() failed to remove any property?

> 

> Yes, I think you're right.  I don't understand the code of text

> properties well, but I think this fixes the performance issue:

> 

> Can you try it?

> ```

> diff --git a/src/textprop.c b/src/textprop.c

> index b6cae70a8..e74c13849 100644

> --- a/src/textprop.c

> +++ b/src/textprop.c

> @@ -809,6 +809,7 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)


>      int                id = -1;

>      int                type_id = -1;

>      int                both;

> +    int                is_removed = FALSE;

>  

>      rettv->vval.v_number = 0;

>      if (argvars[0].v_type != VAR_DICT || argvars[0].vval.v_dict == NULL)

> @@ -889,6 +890,7 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)


>                 if (both ? textprop.tp_id == id && textprop.tp_type == type_id

>                          : textprop.tp_id == id || textprop.tp_type == type_id)

>                 {

> +                   is_removed = TRUE;

>                     if (!(buf->b_ml.ml_flags & ML_LINE_DIRTY))

>                     {

>                         char_u *newptr = alloc(buf->b_ml.ml_line_len);

> @@ -920,7 +922,8 @@ f_prop_remove(typval_T *argvars, typval_T *rettv)


>             }

>         }

>      }

> -    redraw_buf_later(buf, NOT_VALID);

> +    if (is_removed)

> +       redraw_buf_later(buf, NOT_VALID);

>  }

> ```


> 

> For me, CPU goes from 43% to 0.7% when pressing `l` continuously.



I just found the same and made a patch.  We can use the return value

instead of adding a new flag.



-- 

The Feynman problem solving Algorithm:

	1) Write down the problem

	2) Think real hard

	3) Write down the answer




 /// 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    ///




—

You are receiving this because you are subscribed to this thread.
lacygoill
unread,
Mar 21, 2021, 5:50:04 PM3/21/21
to vim/vim, Subscribed




I just found the same and made a patch.  We can use the return value

instead of adding a new flag.




Thank you for the patch.  As pointed out by @dpelle, the issue persists with prop_clear():


diff --git a/src/textprop.c b/src/textprop.c
index d6782342d..6b7106590 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -575,7 +575,8 @@ f_prop_clear(typval_T *argvars, typval_T *rettv UNUSED)
 	    buf->b_ml.ml_line_len = (int)len;
 	}
     }
-    redraw_buf_later(buf, NOT_VALID);
+    if (rettv->vval.v_number > 0)
+	redraw_buf_later(buf, NOT_VALID);
 }
 
 /*
lacygoill
unread,
Mar 21, 2021, 8:04:45 PM3/21/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • d5e3140  Actually, we do want to combine the text property with the syntax.
    • 





—
You are receiving this because you are subscribed to this thread.
lacygoill
unread,
Mar 23, 2021, 5:41:52 AM3/23/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • 5545b66  optimize how character before cursor is obtained
    • 

lacygoill
unread,
Mar 24, 2021, 8:24:42 AM3/24/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • 9755485  fix: clear match after undo
    • 

lacygoill
unread,
Mar 24, 2021, 8:46:30 AM3/24/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • 2b204a4  suppress E16 when line('w$') is 0
    • 

lacygoill
unread,
Mar 27, 2021, 1:26:08 PM3/27/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • a346708  Remove timer to simplify code.
    • 

lacygoill
unread,
Mar 27, 2021, 1:42:29 PM3/27/21
to vim/vim, Push
@lacygoill pushed 1 commit.



lacygoill
unread,
Apr 7, 2021, 3:45:16 PM4/7/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • bf484eb  refactor matchparen plugin in Vim9 script
    • 

Chuan Wei Foo
unread,
Apr 7, 2021, 6:26:38 PM4/7/21
to vim/vim, Subscribed


@cwfoo commented on this pull request.






In runtime/plugin/matchparen.vim:


>      else
-      if has("byte_offset") && has("syntax_items") && &smc > 0
-	let stopbyte = max([1, line2byte(".") + col(".") - &smc * 2])
-	let stopline = max([top_viewable, byte2line(stopbyte)])
+      # If evaluating the expression determines that the cursor is
+      # currently in a text with some specific syntax type (like a string
+      # or a comment), then we want searchpairpos() to find a pair within
+      # a text of similar type; i.e. we want to ignore a pair of different
+      # syntax type.
+      if InStringOrComment()
+        return (): bool => !InStringOrComment()



Shouldn't this be something like:


if InString()
  return (): bool => !InString()
elseif InComment()
  return (): bool => !InComment()
else
  return (): bool => InStringOrComment()
endif



Are there occasions where one would like parentheses in comments to match with parentheses in strings?



—
You are receiving this because you are subscribed to this thread.
Chuan Wei Foo
unread,
Apr 7, 2021, 6:37:43 PM4/7/21
to vim/vim, Subscribed


@cwfoo commented on this pull request.






In runtime/plugin/matchparen.vim:


> -	let stopline = min([bottom_viewable, byte2line(stopbyte)])
-      else
-	let stopline = min([bottom_viewable, c_lnum + 100])
-      endif
-      let stoplinebottom = stopline
+enddef
+
+def InStringOrComment(): bool #{{{2
+  # Expression which detects whether the current cursor position is in certain
+  # syntax types (string, comment, etc.); evaluated inside lambda passed as
+  # skip argument to searchpairpos().
+  return synstack('.', col('.'))
+    ->mapnew((_, v: number): string => synIDattr(v, 'name'))
+    # We match "escape" and "symbol" for special items, such as
+    # lispEscapeSpecial or lispBarSymbol.
+    ->match('\cstring\|character\|singlequote\|escape\|symbol\|comment') >= 0



Doesn't this iterate through the entire list every time, instead of stopping when the first match is found?
lacygoill
unread,
Apr 8, 2021, 3:08:06 AM4/8/21
to vim/vim, Subscribed


@lacygoill commented on this pull request.






In runtime/plugin/matchparen.vim:


>      else
-      if has("byte_offset") && has("syntax_items") && &smc > 0
-	let stopbyte = max([1, line2byte(".") + col(".") - &smc * 2])
-	let stopline = max([top_viewable, byte2line(stopbyte)])
+      # If evaluating the expression determines that the cursor is
+      # currently in a text with some specific syntax type (like a string
+      # or a comment), then we want searchpairpos() to find a pair within
+      # a text of similar type; i.e. we want to ignore a pair of different
+      # syntax type.
+      if InStringOrComment()
+        return (): bool => !InStringOrComment()



Yes, I noticed that too.  The issue is that I don't know what kind of texts the original tokens were supposed to match:


\ '''v:val =~? "string\\|character\\|singlequote\\|escape\\symbol\\|comment"''))'



What does each of these match:


    

  • character
    • 

  • singlequote
    • 

  • escape
    • 

  • symbol
    • 



Some part of a comment?  Some part of a string?  Something else?


Since I don't know, I've just kept the logic of the original plugin which doesn't handle them differently.  With the exception of the test inversion which was wrong.
lacygoill
unread,
Apr 8, 2021, 3:08:50 AM4/8/21
to vim/vim, Subscribed


@lacygoill commented on this pull request.






In runtime/plugin/matchparen.vim:


> -	let stopline = min([bottom_viewable, byte2line(stopbyte)])
-      else
-	let stopline = min([bottom_viewable, c_lnum + 100])
-      endif
-      let stoplinebottom = stopline
+enddef
+
+def InStringOrComment(): bool #{{{2
+  # Expression which detects whether the current cursor position is in certain
+  # syntax types (string, comment, etc.); evaluated inside lambda passed as
+  # skip argument to searchpairpos().
+  return synstack('.', col('.'))
+    ->mapnew((_, v: number): string => synIDattr(v, 'name'))
+    # We match "escape" and "symbol" for special items, such as
+    # lispEscapeSpecial or lispBarSymbol.
+    ->match('\cstring\|character\|singlequote\|escape\|symbol\|comment') >= 0



match() returns the index of the first item where the pattern matches:




When {expr} is a |List| then this returns the index of the

first item where {pat} matches.  Each item is used as a




To get the index of the first item, it doesn't need to iterate over the entire list (unless the pattern never matches).


Here is a simple test:


vim9script
var size: number = 10
var freq: number = 100'000
var hugelist: list<string> = repeat(['stack'], 100) + ['needle'] + repeat(['stack'], size)
def Test()
    for i in range(freq)
        eval match(hugelist, 'needle')
    endfor
enddef
defcompile
var time = reltime()
Test()
(reltime(time)->reltimestr()->matchstr('.*\..\{,3}') .. ' seconds to run Test()')->setline(1)


size controls the size of the list.

freq controls how frequently the function is invoked.


On my machine, this snippet reports that the function takes about 0.6s, no matter the value of size.

That is, even if I increase size from 10 to 100000, the function still takes 0.6s.  If match() was iterating over the entire list, it would take more time as the list grows in size; but it does not.
lacygoill
unread,
Apr 8, 2021, 3:59:44 AM4/8/21
to vim/vim, Subscribed


@lacygoill commented on this pull request.






In runtime/plugin/matchparen.vim:


>      else
-      if has("byte_offset") && has("syntax_items") && &smc > 0
-	let stopbyte = max([1, line2byte(".") + col(".") - &smc * 2])
-	let stopline = max([top_viewable, byte2line(stopbyte)])
+      # If evaluating the expression determines that the cursor is
+      # currently in a text with some specific syntax type (like a string
+      # or a comment), then we want searchpairpos() to find a pair within
+      # a text of similar type; i.e. we want to ignore a pair of different
+      # syntax type.
+      if InStringOrComment()
+        return (): bool => !InStringOrComment()





Are there occasions where one would want parentheses in comments to match with parentheses in strings?




Also, the suggested code wouldn't always work as expected:


elseif InComment()
  return (): bool => !InComment()



Suppose the cursor is on the opening parenthesis on this comment:


# (aaa "bbb)" ccc



InComment() is true.

searchpairpos() finds the closing parenthesis.

On the closing parenthesis, InComment() is still true, !InComment() is false, the skip expression is false, and searchpairpos() does not discard the match.


The end result is that the plugin highlights the closing parenthesis.  But that's not what you wanted; your comment seems to imply that you wanted to ignore a match in a string if the cursor was on a paren in a comment.


What makes things complex is the possibility for syntax items to be nested; like you can have a string highlighted as a string inside a comment.


I'm not saying the current logic can't be improved.  I haven't thought much about it, as I couldn't give more time to this refactoring.  But if you have a specific patch in mind, let me know.
Chuan Wei Foo
unread,
Apr 8, 2021, 4:47:24 AM4/8/21
to vim/vim, Subscribed


@cwfoo commented on this pull request.






In runtime/plugin/matchparen.vim:


> -	let stopline = min([bottom_viewable, byte2line(stopbyte)])
-      else
-	let stopline = min([bottom_viewable, c_lnum + 100])
-      endif
-      let stoplinebottom = stopline
+enddef
+
+def InStringOrComment(): bool #{{{2
+  # Expression which detects whether the current cursor position is in certain
+  # syntax types (string, comment, etc.); evaluated inside lambda passed as
+  # skip argument to searchpairpos().
+  return synstack('.', col('.'))
+    ->mapnew((_, v: number): string => synIDattr(v, 'name'))
+    # We match "escape" and "symbol" for special items, such as
+    # lispEscapeSpecial or lispBarSymbol.
+    ->match('\cstring\|character\|singlequote\|escape\|symbol\|comment') >= 0



Sorry, I've highlighted the wrong line. I should have highlighted the mapnew() instead of match().


Regarding the mapnew() on line 244:


It seems that synIDattr() is called on every item returned by synstack('.', col('.')). Shouldn't synIDattr() be called as we go through the list instead of calling it on every single item in the list? In other words, shouldn't it be something like this:


function InStringOrComment()
  for synID in synstack(line('.'), col('.'))
    if synIDattr(synID, 'name') =~? '\cstring\|character\|singlequote\|escape\|symbol\|comment'
      return 1
    endif
  endfor
  return 0
endfunction



This way, the number of calls to synIDattr() is minimized.
lacygoill
unread,
Apr 8, 2021, 5:09:59 AM4/8/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • 481df27  minimize synIDattr() invocations
    • 





—
You are receiving this because you are subscribed to this thread.
lacygoill
unread,
Apr 8, 2021, 5:11:31 AM4/8/21
to vim/vim, Subscribed


@lacygoill commented on this pull request.






In runtime/plugin/matchparen.vim:


> -	let stopline = min([bottom_viewable, byte2line(stopbyte)])
-      else
-	let stopline = min([bottom_viewable, c_lnum + 100])
-      endif
-      let stoplinebottom = stopline
+enddef
+
+def InStringOrComment(): bool #{{{2
+  # Expression which detects whether the current cursor position is in certain
+  # syntax types (string, comment, etc.); evaluated inside lambda passed as
+  # skip argument to searchpairpos().
+  return synstack('.', col('.'))
+    ->mapnew((_, v: number): string => synIDattr(v, 'name'))
+    # We match "escape" and "symbol" for special items, such as
+    # lispEscapeSpecial or lispBarSymbol.
+    ->match('\cstring\|character\|singlequote\|escape\|symbol\|comment') >= 0



I didn't notice a speed increase during a simple quick test, but I guess you're right, so I've pushed a commit which refactors the mapnew() into a :for loop.



—
You are receiving this because you are subscribed to this thread.
lacygoill
unread,
May 1, 2021, 7:39:42 AM5/1/21
to vim/vim, Push
@lacygoill pushed 1 commit.






—
You are receiving this because you are subscribed to this thread.
lacygoill
unread,
May 15, 2021, 1:44:01 PM5/15/21
to vim/vim, Subscribed


The current version is still slower than the original one in some cases.


For example, if I copy the script in a temporary file, then write this line in the middle:


return max([5, min([winheight(0), winnr('#')->winheight() / 2])])



That is, right above this line (there are 2 of them; the first one):


if before > 0



Then, if I enter insert mode right before the last paren:


return max([5, min([winheight(0), winnr('#')->winheight() / 2])])
                                                                ^



And keep pressing i, there is some noticeable lag, which doesn't occur when using the original plugin version.


I know that the lag comes from 2 things:


    

  • prop_add() which is slower than matchadd()
    • 

  • the Skip expression which is slower than a legacy eval string
    • 



It mainly comes from the latter.  But Skip is a reference to a compiled function.  Why is a compiled function still slower than an eval string in legacy Vim script?



—
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
unread,
May 15, 2021, 5:22:39 PM5/15/21
to vim/vim, Subscribed



> The current version is still slower than the original one in some cases.

> 

> For example, if I copy the script in a temporary file, then write this line in the middle:

> 

>     return max([5, min([winheight(0), winnr('#')->winheight() / 2])])

> 

> That is, right above this line (there are 2 of them; the first one):

> 

>     if before > 0

> 

> Then, if I enter insert mode right before the last paren:

> 

>     return max([5, min([winheight(0), winnr('#')->winheight() / 2])])

>                                                                     ^

> 

> And keep pressing `i`, there is some noticeable lag, which doesn't occur when using the original plugin version.


> 

> I know that the lag comes from 2 things:

> 

>    - `prop_add()` which is slower than `matchadd()`

>    - the `Skip` expression which is slower than a legacy eval string


> 

> It mainly comes from the latter.  But `Skip` is a reference to a

> compiled function.  Why is a compiled function still slower than an

> eval string in legacy Vim script?



How exactly do you use the skip expression?



Most likely string evaluation is faster than calling a function, because

of the overhead involved in that.



Note that a recent change made it possible to compile the skip

expression, if it is used in a :def function and the expression is a

string constant.  Instead of this:



	searchpair(..., Skip(), ...)



You could try:



	searchpair(..., 'Skip()', ...)



Then, instead of making a function call, it would go straight to

executing the instructions, which is a call to Skip().  I expect that to

be faster.



-- 

   They now pass three KNIGHTS impaled to a tree.  With their feet off the

   ground,  with one lance through the lot of them, they are skewered up

   like a barbecue.

                 "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD




 /// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net   \\\

///                                                                      \\\

\\\        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///


 \\\            help me help AIDS victims -- http://ICCF-Holland.org    ///




—
lacygoill
unread,
May 16, 2021, 12:01:56 PM5/16/21
to vim/vim, Subscribed


Sorry, I made a mistake.  I think the skip expression is fine.  It was not when I found out the issue a few months ago.  But at the time, I was using a binary without optimizations (to debug a crash), and I didn't have these patches:


    

  • 8.2.2842  Vim9: skip argument to searchpair() is not compiled
    • 

  • 8.2.2843  Vim9: skip argument to searchpairpos() is not compiled
    • 

  • 8.2.2844  Vim9: memory leak when using searchpair()
    • 



But there is still some lag when inserting characters in front of a parenthesis.  I think the issue is due to prop_add(), because the lag disappears when I comment it out.


I've done some profiling with :prof.  prop_add() by itself is actually faster than matchaddpos().  I think the issue is that prop_add() makes Vim redraw the screen immediately, causing the syntax to be recomputed for all the displayed text.  This is confirmed by the fact that the lag also disappears when the syntax is temporarily disabled with :syn off.


I don't think this can be fixed in Vim.  The fact that Vim redraws more eagerly with prop_add() than with matchaddpos() is a good thing; it fixes a bunch of issues where the highlighting is currently wrong.


The only workaround I can think of, is a timer.  That is: wait a short amount of time before updating the highlighting again.  Originally, I did implement such a mechanism, but I reverted the feature; it made the code more complex, and I didn't like how the highlighting seemed to blink when inserting characters in front of a parenthesis.
Bram Moolenaar
unread,
May 16, 2021, 2:13:56 PM5/16/21
to vim/vim, Subscribed



> Sorry, I made a mistake.  I think the skip expression is fine.  It was not when I found out the issue a few months ago.  But at the time, I was using a binary without optimizations (to debug a crash), and I didn't have these patches:

> 

>    - [8.2.2842](https://github.com/vim/vim/releases/tag/v8.2.2842)  Vim9: skip argument to searchpair() is not compiled

>    - [8.2.2843](https://github.com/vim/vim/releases/tag/v8.2.2843)  Vim9: skip argument to searchpairpos() is not compiled

>    - [8.2.2844](https://github.com/vim/vim/releases/tag/v8.2.2844)  Vim9: memory leak when using searchpair()

> 

> But there is still some lag when inserting characters in front of a parenthesis.  I think the issue is due to `prop_add()`, because the lag disappears when I comment it out.

> 

> I've done some profiling with `:prof`.  `prop_add()` by itself is actually faster than `matchaddpos()`.  I think the issue is that `prop_add()` makes Vim redraw the screen immediately, causing the syntax to be recomputed for all the displayed text.  This is confirmed by the fact that the lag also disappears when the syntax is temporarily disabled with `:syn off`.



Currently it is simply marking the whole buffer to be redrawn.  That is

indeed inefficient.  Since we know the start and end line number, and

no text is changed, it should be possible to restrict the redraw only to

these lines.  Let me give that a try.  This might cause some highlight

errors, please watch out for those.




> I don't think this can be fixed in Vim.  The fact that Vim redraws more eagerly with `prop_add()` than with `matchaddpos()` is a good thing; it fixes a bunch of issues where the highlighting is currently wrong.

> 

> The only workaround I can think of, is a timer.  That is: wait a short amount of time before updating the highlighting again.  Originally, I did implement such a mechanism, but I reverted the feature; it made the code more complex, and I didn't like how the highlighting seemed to blink when inserting characters in front of a parenthesis.



Your prop_add for matchparen should be triggered when a key is typed.

Then when Vim returns to the main loop, it will check for what to

redraw.  Thus a timer should not be needed, unless you want to not add

the text property when the user is typing fast.



-- 

Facepalm statement #9: "Did you see, there is now even a hobbit book"




 /// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net   \\\

///                                                                      \\\

\\\        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///

 \\\            help me help AIDS victims -- http://ICCF-Holland.org    ///




—
lacygoill
unread,
May 16, 2021, 3:12:40 PM5/16/21
to vim/vim, Subscribed



For some reason, the lag is still there, and I know it's caused by an interaction between prop_add() and the syntax highlighting.  According to :syntime, the overall time spent on syntax is about the same.  However, the number of times a syntax rule is used has been cut almost in half.  I guess it means that – in this particular case – the most costly syntax computations are on the redrawn line, where we insert text in front of a mix of parentheses...



Also, out of the 7 "tests" included in the OP, 2 are now broken by the patch 8.2.2860.  The one checking the fix for the issue #1532, and the one checking the fix for the issue #5638.



I don't think it can be avoided, and it's not that important if it's the price to pay to have better performance.



I'll see if a timer can help further.
lacygoill
unread,
May 16, 2021, 3:38:59 PM5/16/21
to vim/vim, Subscribed




For some reason, the lag is still there, and I know it's caused by an interaction between prop_add() and the syntax highlighting.




Actually, I think the issue comes from an interaction between prop_remove() and the syntax highlighting.  Could the optimization implemented in 8.2.2860 be applied to prop_remove() too?
lacygoill
unread,
May 16, 2021, 3:46:54 PM5/16/21
to vim/vim, Subscribed




Could the optimization implemented in 8.2.2860 be applied to prop_remove() too?




Never mind.  That would not help, since we need to remove the properties across all the lines displayed in the window.
Bram Moolenaar
unread,
May 16, 2021, 6:18:02 PM5/16/21
to vim/vim, Subscribed



> > For some reason, the lag is still there, and I know it's caused by

> > an interaction between prop_add() and the syntax highlighting.

> 

> Actually, I think the issue comes from an interaction between

> `prop_remove()` and the syntax highlighting.  Could the optimization


> implemented in 8.2.2860 be applied to `prop_remove()` too?



Yes, prop_remove() triggers a redraw of the whole window as well.

I'll also restrict it to the specified line numbers.  This does assume

line numbers are specified.  A more advanced solution would remember the

first and last line that were actually changed.



-- 

       When danger reared its ugly head,

       He bravely turned his tail and fled

       Yes, Brave Sir Robin turned about

       And gallantly he chickened out

       Bravely taking to his feet

       He beat a very brave retreat

       Bravest of the brave Sir Robin

       Petrified of being dead

       Soiled his pants then brave Sir Robin

       Turned away and fled.


                 "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD



 /// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net   \\\

///                                                                      \\\

\\\        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///

 \\\            help me help AIDS victims -- http://ICCF-Holland.org    ///




—
Bram Moolenaar
unread,
May 16, 2021, 6:26:07 PM5/16/21
to vim...@googlegroups.com, lacygoill


> > Could the optimization implemented in 8.2.2860 be applied to

> > prop_remove() too?

> 

> Never mind.  That would not help, since we need to remove the properties across all the lines displayed in the window.



I now made it only redraw the range of lines within which a text

property was actually found and removed.  Let's see what the effect of

that is.



-- 

Linux is just like a wigwam: no Windows, no Gates and an Apache inside.

lacygoill
unread,
May 17, 2021, 4:49:04 AM5/17/21
to vim/vim, Subscribed



The last patches (8.2.2862, 8.2.2863) – probably combined with 8.2.2860 – have totally removed the lag; thank you very much.
lacygoill
unread,
May 20, 2021, 9:54:29 AM5/20/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • 8ee0fbd  make highlighting persist after reload
    • 





—
You are receiving this because you are subscribed to this thread.
lacygoill
unread,
Jun 8, 2021, 8:21:20 PM6/8/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • 5379877  optimization: inspecting the syntax is useless in a closed fold
    • 

lacygoill
unread,
Jul 19, 2021, 11:39:41 PM7/19/21
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • f87fd39  Only install one command.
    • 

lacygoill
unread,
Jul 19, 2021, 11:41:22 PM7/19/21
to vim/vim, Push
@lacygoill pushed 1 commit.



lacygoill
unread,
Feb 22, 2022, 9:34:28 AM2/22/22
to vim/vim, Push
@lacygoill pushed 1 commit.



    
  
  • 50c22ae  fix corner case: moving on a paren from the line above after pressing `$`
    • 





—
View it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/7985/push/9171397255@github.com>



Reply all
Reply to author
Forward
0 new messages