Improve *SetOption hooks performance

Here’s a discussion started by mawww on IRC for people not there who might want to chime in:

mawww
For performance reasons, I need to change the way to generate the .*SetOption hooks
The problem is that dumping options to text to generate the “option_name=option_value” text can get quite slow, for example :git blame can be slow here
(in debug) because the option holding the line flags gets updated a lot and every time, we need to go though the list of line flags, format it to text, then format a a filter string in order to run the .*SetOption hooks

So I see two solutions for that
opt-in hook support per option, (like a -hook-on-change switch to the declare-option command)
or opt-in to put value in filter string (some options just generate a .*SetOption <option_name> while other generate a .*SetOption <option_name>=<option_value>)

occivink
how does the option_name=option_value formatting works with list options?

mawww
or, another alternative which would be the cleanest, just remove the option_value from the filter text, but we need to find an alternative to the most common use case: .*SetOption filetype=…
occivink: you’l get something like option_name=‘value1’ ‘value2’ ‘value with ‘‘quotes’’’

occivink
I think I prefer the last option but yeah we’d need a fast way to filter

mawww
the “set -add is slow” bug is due to that for example
yep, if we can find a nice way to express the same thing, and are willing to break everybody’s config, thats the cleanest solution

occivink
looks like in the rc scripts, only filetype= hooks rely on the option value

mawww
Yep, a possiblity would be to make filetype special, either though its own hook, or by letting it put its value in the filter string, but thats far from satisfying

occivink
I think it’d be ok if the setup hooks could just query %opt{filetype} and abort if it’s not the expected value

mawww
Thats only doable in a non hackish way through a shell expand
so every of those hooks would now fork a shell to check the filetype…

occivink
yeah of course, I’m talking about adding some way to filter via a builtin command or something

Screwtape
A hacky idea: if a hook regex starts with literal text up to the first ‘=’, save a separate copy of that text in the hook record alongside the compiled regex. When executing .*SetOption hooks, if the hook has a literal prefix, compare that first against the option name before doing the serialisation and full regex dance.

mawww
and its far less convenient to add your own filetype hook, hook global WinSetOption filetype=blah <commands> is a pretty sweet syntax

occivink
yeah that’s true, having to wrap it in a ‘try %{ filter… }’ would be verbose

mawww
a variant of the first idea is to have some option type serialized and not other
say any lists will just get ‘option_name=…’ (with ‘…’ literally ‘…’, not a placeholder)
not very satisfying either…

1 Like