[vim/vim] copy_tv() and clear_tv() can be improved for scalar values (PR #20787)

4 views

Skip to first unread message

thinca

unread,

Jul 19, 2026, 2:01:20 AM (3 days ago) Jul 19

to vim/vim, Subscribed

Summary

Every value in Vim is a tagged typval_T, and moving values around — onto and off the Vim9 execution stack, into and out of lists/dicts, through for loops, on function return — goes through copy_tv()/clear_tv(), each running a type switch. For the common scalar types (number/bool/special/float) the real work is a trivial value copy or a no-op, so the function call plus the switch dominate the cost.

This makes copy_tv()/clear_tv() thin static inline wrappers (new src/inline_funcs.h) that handle the scalar types at the call site and fall back to the out-of-line copy_tv_inner()/clear_tv_inner() for every refcounted or allocating type. The public names are unchanged, so every caller benefits with no call-site churn; observable behaviour is identical.

Two design notes:

Why a new header. The wrappers reference typval_T (from structs.h) and the *_inner() prototypes (from proto.h), so they must be included after proto.h; macros.h is included too early. A small dedicated header keeps vim.h short, but these two definitions could just as well sit inline in vim.h after proto.h — happy to inline them instead if you prefer.
Why inline rather than an early return. An in-place early return in copy_tv()/clear_tv() would drop the switch but keep the call; inlining at the call site drops both — which is why the wall-clock gain below exceeds the instruction-count gain.

The build-dependency bookkeeping for the new header is split into a second commit, since it is mechanical and is normally regenerated by make depend.

Benchmark

scalar (best case): −10% instructions, −23% wall-clock. container (worst case): unchanged (+1% Ir). Binary: +0.6%.

Base — master at commit 6f02e5cd7c27fc098d0b8dfec99542c7663a807e — vs patched, measured on GitHub Actions. callgrind instruction-reads (Ir) are deterministic and runner-independent; wall-clock is the minimum of 3 native runs (informational — noisy on shared runners).

workload	`Ir` base → patched	`Ir` Δ	time base → patched	time Δ
scalar — Sieve of Eratosthenes (best case)	30,549,307,131 → 27,468,457,830	−10.08%	5.181s → 3.987s	−23.05%
container — deepcopy/slice/remove (worst case)	2,004,554,303 → 2,023,750,673	+0.96%	1.884s → 1.903s	+1.01%

Both workloads produce bit-identical results on the base and patched builds, confirming the change is semantics-preserving. copy_tv()/clear_tv() are exercised throughout the existing test suite, so no new test is added.

For the scalar workload the wall-clock gain (−23%) is larger than the Ir gain (−10%): removing the call and the type switch also helps branch prediction and the instruction cache, which Ir alone does not capture. The container workload is effectively unchanged — its sub-1% Ir increase is the expected cost of the extra scalar-type check before the non-scalar fallback. Its wall-clock delta is within run-to-run noise on the shared runner, where the deterministic Ir is the reliable signal.

This is a general, low-risk constant-factor win: scalars are a common case whose copy or clear is trivial, so paying a call plus a type switch for them is avoidable overhead. It does not touch the interpreter dispatch loop, so it helps all value-churn-heavy Vim9 code rather than being a redesign.

Binary size

Inlining the scalar fast-path at every call site grows the binary a little (Ubuntu 24.04 CI runner, x86-64, gcc -O2 without -g, base → patched):

`vim` binary	base	patched	Δ
as built	4,530,304	4,555,040	+0.55% (+24,736 B)
stripped	4,062,816	4,087,392	+0.60% (+24,576 B)

Benchmark workloads (vim9script)

Ir is measured with valgrind --tool=callgrind --dump-instr=no --branch-sim=no on an -O2 -g build; binary size and wall-clock use an -O2 (no -g) build. Driven by ci/bench/run_typval_bench.sh <base-sha> <patched-sha>.

scalarbench.vim — scalar / best case:

vim9script
# Scalar-heavy workload (the fast-path's best case): a Sieve of Eratosthenes
# over a reused list<bool>.  Each pass resets, sieves and counts purely through
# list-index reads and writes of scalar typvals plus arithmetic and
# comparisons; the list is allocated once (outside the loop) and no builtin or
# string work is in the hot path, so the inline copy_tv()/clear_tv() win is
# what moves the number.
const OUT = exists('g:BENCH_OUT') ? g:BENCH_OUT : '/tmp/scalarbench.txt'
def CountPrimes(sieve: list<bool>): number
  var n = len(sieve)
  var k = 0
  while k < n
    sieve[k] = true			# reset: scalar store (clear_tv + copy_tv)
    k += 1
  endwhile
  var i = 2
  while i * i < n
    if sieve[i]
      var j = i * i
      while j < n
        sieve[j] = false		# mark composite: scalar store
        j += i
      endwhile
    endif
    i += 1
  endwhile
  var count = 0
  i = 2
  while i < n
    if sieve[i]				# read scalar onto the stack
      count += 1
    endif
    i += 1
  endwhile
  return count
enddef
def Main()
  var iter = 200000
  if exists('g:ITER')
    iter = g:ITER
  endif
  var sieve: list<bool> = []
  for _ in range(2000)
    sieve->add(true)			# allocate once, outside the timed loop
  endfor
  var t = reltime()
  var acc = 0
  for _ in range(iter)
    acc += CountPrimes(sieve)		# pi(2000) = 303, so check = iter * 303
  endfor
  writefile([printf('ITER=%d elapsed=%.3fs check=%d',
    iter, reltimefloat(reltime(t)), acc)], OUT)
enddef
try
  Main()
catch
  writefile(['EXC: ' .. v:exception .. ' @ ' .. v:throwpoint], OUT)
endtry
qall!

containerbench.vim — container / worst case:

vim9script
# Non-scalar-heavy counterpart to scalarbench.vim: hammers copy_tv()/clear_tv()
# on strings, lists and dicts (deepcopy / slice / extend / remove / scope exit),
# so any regression from the scalar fast-path's extra branch on non-scalar
# values would show here.
const OUT = exists('g:BENCH_OUT') ? g:BENCH_OUT : '/tmp/containerbench.txt'
def Build(n: number): list<dict<any>>
  var out: list<dict<any>> = []
  for i in range(n)
    out->add({
      name: 'item_' .. i,
      tag: 'tag_' .. (i % 7),
      vals: [string(i), string(i * 2), string(i * 3)],
      meta: {a: 'x' .. i, b: 'y' .. i},
    })
  endfor
  return out
enddef
def Churn(base: list<dict<any>>): number
  var total = 0
  var c = deepcopy(base)		# copy_tv over every nested string/list/dict
  var s = c[0 : len(c) / 2]		# list slice: copy_tv of dict refs
  c->extend(s)
  for d in c
    total += len(d.name) + len(d.vals)
    for v in d.vals
      total += str2nr(v)		# fold the copied value content, not just its length
    endfor
    if has_key(d, 'meta')
      total += len(d.meta) + len(d.meta.a) + len(d.meta.b)
    endif
  endfor
  while len(c) > 0
    remove(c, 0)			# clear_tv of non-scalar items
  endwhile
  return total
enddef
def Main()
  var iter = 20000
  if exists('g:ITER')
    iter = g:ITER
  endif
  var base = Build(32)
  var t = reltime()
  var acc = 0
  for _ in range(iter)
    acc += Churn(base)
  endfor
  writefile([printf('ITER=%d elapsed=%.3fs check=%d',
    iter, reltimefloat(reltime(t)), acc)], OUT)
enddef
try
  Main()
catch
  writefile(['EXC: ' .. v:exception .. ' @ ' .. v:throwpoint], OUT)
endtry
qall!

AI-assisted; the change is disclosed via the Co-Authored-By trailer on the commit, per CONTRIBUTING "Using AI".

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

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

Commit Summary

94550f5 copy_tv() and clear_tv() can be improved for scalar values
98e68d0 Makefile: register inline_funcs.h in the build dependency lists

File Changes

(9 files)

Patch Links:

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.

h_east

unread,

12:40 PM (10 hours ago) 12:40 PM

to vim/vim, Subscribed

h-east left a comment (vim/vim#20787)

I had Clause Code review this PR. I have excluded any points that differ from my own opinion.

Correctness

The scalar path is equivalent to the current implementation. In copy_tv()
the assignments to->v_type = from->v_type; and to->v_lock = 0; that used to
happen before the switch are now done inside the scalar arm, and in
clear_tv() the varp->v_lock = 0; that used to happen after the switch is
done inside the scalar arms. For the four scalar types there is no observable
difference. The union assignment to->vval = from->vval; is equivalent to the
member assignment, since the union is the size of v_number. Neither function
is used as a function pointer anywhere, so making them inline does not break
any caller.

The scalar arms in the *_inner() functions become dead code

clear_tv_inner() and copy_tv_inner() are only reached through the default:
label of the inline wrappers, so the case VAR_NUMBER: / VAR_BOOL: /
VAR_SPECIAL: / VAR_FLOAT: arms that remain in typval.c are unreachable.
The comment added to typval.c ("must stay in sync with this for them") is an
acknowledgement of that duplication.

Having the same rule in two places, where one copy can never be executed, is a
trap for whoever next changes how v_lock or the scalar types are handled.
Please pick one: either drop the unreachable scalar arms from the *_inner()
functions and state in their comment that they only handle the non-scalar
types, or keep a single definition and accept that the switch is not
duplicated. Keeping the duplication and promising in a comment to keep it in
sync is the option that costs the most later.

Reading the benchmark

Ir is deterministic, so the -10% on the scalar workload and the +1% on the
container workload are credible. The -23% wall-clock figure is from a shared
GitHub Actions runner, and the explanation for the gap to the -10% Ir figure
(branch prediction and instruction cache) is a hypothesis that has not been
verified. If that number is to be part of the case for the change, it needs an
alternating A/B measurement on a machine that is not shared.

The scalar workload is a loop of sieve[k] = true, that is, scalar stores and
loads only. That is an upper bound for this optimization, not an estimate of
what a real script gains. It would help to see the numbers for a workload that
is not written to isolate the fast path.

Details

The line length of the comment added to vim.h exceeds 80 columns.

The name inline_funcs.h is generic enough that it invites unrelated inline
functions to be added later. Since the content is specific to typval, a name
like typval_inline.h would make the placement decision obvious for the next
addition.

Using static inline is not a problem in itself: it is already used in mbyte.c
and screen.c, and macros.h already defines inline functions in a header.