do not redraw screen, if tabline function calls settabvar

[Christian Brabandt]

Bram,
I am maintaining a eye-candy statusline plugin, that can also create a
nice tabline. I recently made a change, trying to improve the
performance of the plugin by caching certain values
(https://github.com/vim-airline/vim-airline/commit/b2f301f73c168104cf2202ac5f2e2b7c078c7aa1)

The issue can be reproduced with this little vim snippet:
,----
| function! TabTitle(n)
| let title = gettabvar(a:n, 'title')
| if empty(title)
| " call an expensive function to get the title
| " try to avoid the expensive call, by caching the title
| let title='foobar'
| endif
| "if a:n == tabpagenr()
| " let t:title = title
| "endif
| call settabvar(a:n, 'title', title)
| return title
| endfunction
|
| function! DrawTabline()
| let tabline = ''
| for i in range(1, tabpagenr('$'))
| let tabline .= "%(Tab: ".i.' "%{TabTitle('.i.')}"%)'
| endfor
| return tabline
| endfunction
| set tabline=%!DrawTabline()
`----


However, it turns out, that this causes flicker on movement and so makes
Vim redraw a lot more often than actually needed. I looked into the
source, trying to find out, why this happens and I think it happens
because settabvar does internally switch tabpages (e.g. calling
enter_tabpage) which unconditionally set must_redraw=CLEAR.

However this happens on a call to draw_tabline() which is done when we
already updating the screen (e.g. in a callchain coming from
update_scree()). So I think we can prevent this by only setting
must_redraw, if we are not updating the screen currently, which is what
this patch does:

diff --git a/src/window.c b/src/window.c
index 8078e011d..785d240e8 100644
--- a/src/window.c
+++ b/src/window.c
@@ -3925,7 +3925,8 @@ enter_tabpage(

last_status(FALSE); /* status line may appear or disappear */
(void)win_comp_pos(); /* recompute w_winrow for all windows */
- must_redraw = CLEAR; /* need to redraw everything */
+ if (!updating_screen)
+ must_redraw = CLEAR; /* need to redraw everything */
#ifdef FEAT_DIFF
diff_need_scrollbind = TRUE;
#endif

On a second thought, it might be better not to set must_redraw for
settabvar() at all, since after we are finished, we switch back the
tabpage, and thus theoretically a redraw should not be necessary. So
perhaps this patch is preferable:

diff --git a/src/evalfunc.c b/src/evalfunc.c
index 2d796f7e1..4b8f95e50 100644
--- a/src/evalfunc.c
+++ b/src/evalfunc.c
@@ -10385,6 +10385,7 @@ f_settabvar(typval_T *argvars, typval_T *rettv)
#endif
char_u *varname, *tabvarname;
typval_T *varp;
+ int redraw;

rettv->vval.v_number = 0;

@@ -10404,6 +10405,7 @@ f_settabvar(typval_T *argvars, typval_T *rettv)
)
{
#ifdef FEAT_WINDOWS
+ redraw = must_redraw;
save_curtab = curtab;
goto_tabpage_tp(tp, FALSE, FALSE);
#endif
@@ -10421,6 +10423,7 @@ f_settabvar(typval_T *argvars, typval_T *rettv)
/* Restore current tabpage */
if (valid_tabpage(save_curtab))
goto_tabpage_tp(save_curtab, FALSE, FALSE);
+ must_redraw = redraw; /* avoid an unnecessary redraw */
#endif
}
}


And finally, I made another observation, that looks suspicious as well.
When update_screen() is called, it will eventually call this code, if
type==CLEAR:
,----
| if (type == CLEAR) /* first clear screen */
| {
| screenclear(); /* will reset clear_cmdline */
| type = NOT_VALID;
| }
`----

screenclear() will then call screenclear2() which will call win_rest_invalid(firstwin)
which will then call
,----
| redraw_win_later(wp, NOT_VALID);
`----

So although we are in the process of updating the current screen, we
will force another redraw after the next main_loop. I think we can save
that call here, and only call redraw_win_later if updating_screen is not
true.

Best,
Christian
--
Moral ist eine Wichtigtuerei des Menschen vor der Natur.
-- Friedrich Wilhelm Nietzsche

[Nikolay Aleksandrovich Pavlov]

2017-06-28 13:12 GMT+03:00 Christian Brabandt <cbl...@256bit.org>:
> Bram,
> I am maintaining a eye-candy statusline plugin, that can also create a
> nice tabline. I recently made a change, trying to improve the
> performance of the plugin by caching certain values
> (https://github.com/vim-airline/vim-airline/commit/b2f301f73c168104cf2202ac5f2e2b7c078c7aa1)
>
> The issue can be reproduced with this little vim snippet:
> ,----
> | function! TabTitle(n)
> | let title = gettabvar(a:n, 'title')
> | if empty(title)
> | " call an expensive function to get the title
> | " try to avoid the expensive call, by caching the title
> | let title='foobar'
> | endif
> | "if a:n == tabpagenr()
> | " let t:title = title
> | "endif
> | call settabvar(a:n, 'title', title)

As a workaround I would suggest just using

:call extend(gettabvar(a:n, ''), {"title": title})

. Despite the fact that gettabvar() is doing useless job of switching
windows back and forth (using similar code with :python and
vim.Tabpage(n).vars will not perform any switching, AFAIK not even in
Neovim) it should not cause redraws.

Or, given that you already need to query tabpage for variables:

```VimL
let tabscope = gettabvar(a:n, '')
let title = get(tabscope, 'title')
if title is 0
let title = Expensive_function()
let tabscope.title = title
endif
return title
```

I.e. do at most one gettabvar() call per tab and work with scope
dictionary from that point.

> --
> --
> You received this message from the "vim_dev" 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 the Google Groups "vim_dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Bram Moolenaar]

Hmm, switching tabs halfway updating the display is something that
should be avoided.

> However, it turns out, that this causes flicker on movement and so makes
> Vim redraw a lot more often than actually needed. I looked into the
> source, trying to find out, why this happens and I think it happens
> because settabvar does internally switch tabpages (e.g. calling
> enter_tabpage) which unconditionally set must_redraw=CLEAR.
>
> However this happens on a call to draw_tabline() which is done when we
> already updating the screen (e.g. in a callchain coming from
> update_scree()). So I think we can prevent this by only setting
> must_redraw, if we are not updating the screen currently, which is what
> this patch does:

First of all, if we don't actually switch tab pages, then
enter_tabpage() won't be called. If we do change tab pages, I don't
know what the effects are. At least it doesn't trigger autocommands, so
perhaps it's not so bad.

> diff --git a/src/window.c b/src/window.c
> index 8078e011d..785d240e8 100644
> --- a/src/window.c
> +++ b/src/window.c
> @@ -3925,7 +3925,8 @@ enter_tabpage(
>
> last_status(FALSE); /* status line may appear or disappear */
> (void)win_comp_pos(); /* recompute w_winrow for all windows */
> - must_redraw = CLEAR; /* need to redraw everything */
> + if (!updating_screen)
> + must_redraw = CLEAR; /* need to redraw everything */
> #ifdef FEAT_DIFF
> diff_need_scrollbind = TRUE;
> #endif
>
> On a second thought, it might be better not to set must_redraw for
> settabvar() at all, since after we are finished, we switch back the
> tabpage, and thus theoretically a redraw should not be necessary. So
> perhaps this patch is preferable:

Yeah, not setting must_redraw there has unpredictable effects.

> diff --git a/src/evalfunc.c b/src/evalfunc.c
> index 2d796f7e1..4b8f95e50 100644
> --- a/src/evalfunc.c
> +++ b/src/evalfunc.c
> @@ -10385,6 +10385,7 @@ f_settabvar(typval_T *argvars, typval_T *rettv)
> #endif
> char_u *varname, *tabvarname;
> typval_T *varp;
> + int redraw;
>
> rettv->vval.v_number = 0;
>
> @@ -10404,6 +10405,7 @@ f_settabvar(typval_T *argvars, typval_T *rettv)
> )
> {
> #ifdef FEAT_WINDOWS
> + redraw = must_redraw;
> save_curtab = curtab;
> goto_tabpage_tp(tp, FALSE, FALSE);
> #endif
> @@ -10421,6 +10423,7 @@ f_settabvar(typval_T *argvars, typval_T *rettv)
> /* Restore current tabpage */
> if (valid_tabpage(save_curtab))
> goto_tabpage_tp(save_curtab, FALSE, FALSE);
> + must_redraw = redraw; /* avoid an unnecessary redraw */
> #endif
> }
> }

The problem here is that we use a very blunt high-level function just to
be able to access a variable. I don't think there are ever side
effects. The only part that really matters is getting the hash table
for the variables, which happens in find_var_ht().

It seems that refactoring set_var(), splitting off the part that calls
find_var_ht() to use the right tabpage, would avoid a lot of trouble.

> And finally, I made another observation, that looks suspicious as well.
> When update_screen() is called, it will eventually call this code, if
> type==CLEAR:
> ,----
> | if (type == CLEAR) /* first clear screen */
> | {
> | screenclear(); /* will reset clear_cmdline */
> | type = NOT_VALID;
> | }
> `----
>
> screenclear() will then call screenclear2() which will call win_rest_invalid(firstwin)
> which will then call
> ,----
> | redraw_win_later(wp, NOT_VALID);
> `----
>
> So although we are in the process of updating the current screen, we
> will force another redraw after the next main_loop. I think we can save
> that call here, and only call redraw_win_later if updating_screen is not
> true.

Nice catch. Happens when typing CTL-L in any window but the first one.

Resetting must_redraw is probably the best solution. If for some reason
the screen is cleared when halfway redrawing, setting the flag is
correct. The win->w_redr_type flags will be reset after the window has
been redrawn.

--
From "know your smileys":
(X0||) Double hamburger with lettuce and tomato

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

[Christian Brabandt]

That is not what I intended to do (and it is not obvious from the
documentation, that settabvar() would switch tabpages, so it sounded
safe.

> > On a second thought, it might be better not to set must_redraw for
> > settabvar() at all, since after we are finished, we switch back the
> > tabpage, and thus theoretically a redraw should not be necessary. So
> > perhaps this patch is preferable:
>
> Yeah, not setting must_redraw there has unpredictable effects.

Not sure I understand. That means, it should be changed, yes?

> Nice catch. Happens when typing CTL-L in any window but the first one.

Thanks for including.

Best,
Christian
--
Ich glaube, ein Mann will von einer Frau das gleiche wie eine Frau von
einem Mann: Respekt.
-- Clint Eastwood