From 5de1cb1411e9bffba3d59590c11fbb6787226f45 Mon Sep 17 00:00:00 2001 From: dg1sbg Date: Mon, 3 Aug 2026 09:05:35 +0200 Subject: [PATCH] Stop allocating a scratch vector for every bytecode &key call parse-key-args built a SimpleVector to collect keyword arguments, then immediately pushed its contents onto the VM stack and dropped it. The vector never outlived the opcode, and it was allocated unconditionally -- before the lcc_nargs > more_start guard -- so a call passing no keywords at all still paid for it. The cost was 24 + 8n bytes for a callee with n &key parameters, charged per call: (defun k1 (a &key x) ...) called (k1 1) 32 B (defun k2 (a &key x y) ...) called (k2 1) 40 B (defun k3 (a &key x y z) ...) called (k3 1) 48 B (defun k6 (a &key p q r s u v)) called (k6 1) 72 B (string= "abc" "abd") 56 B All of those are 0 B now. The parameter slots are the destination anyway, so push them first and write each argument directly into its slot. The index identity is exact rather than assumed: the loop being deleted pushed key_id descending, so key_id 0 ended on top, and stackref is documented in-tree as "0 is most recently pushed" -- hence slot n is key n. Ordering: the odd-argument-count check is hoisted above the pushes so that error path cannot see them. The unrecognized-keyword throw still can, which is safe -- the VM stack is a registered GC root (allocateRootsAndZero, gctools/threadlocal.cc), sp is a plain C local, and both ~VMFrameDynEnv_O and VMFrameDynEnv_O::proceed restore _stackPointer on unwind. Slots are initialized to unbound before anything can collect. Peak stack use is unchanged: exactly key_count slots either way, earlier. The long-operand form gets the same treatment for consistency. It takes more than 127 &key parameters to reach, since key_count_info is (key_count << 1) | aokp. Adds regression tests for slot identity across argument permutations, leftmost-wins on duplicate keywords, &allow-other-keys, and 16 parameter slots live across a collection. A reversed or off-by-one slot index binds the wrong parameter with no error at all, so these pin the identity rather than the allocation. Gates: boehm 1984, boehmprecise 1986, both with no unexpected failures; ANSI 21936 tests with no unexpected failures. Rebased onto 78c1d1dcc, which made the ordering argument above incomplete. That commit writes vm._stackPointer = sp before allocation sites because the bytecode-stack scanner walks only as far as vm._stackPointer (threadlocal.h:363) and push mutates a local sp that is never written back. Upstream put one such sync in parse_key_args, in front of the scratch vector this commit deletes. Both belong here: the first sync stays where upstream put it, now covering throwOddKeywordsError, and a second is added once the slots are pushed. It is needed precisely because the values now live on the VM stack instead of in a heap vector, so without it a collection triggered by the unknown-keys Cons_O::create, or by either error throw, would stop below them and miss live roots. The gate numbers above predate this base. This exact bytecode.cc content was re-run on 0aa5b71ac in an integration branch carrying it: 2013 successes, 5 expected failures, no unexpected failures, macOS arm64 boehmprecise native. --- src/core/bytecode.cc | 44 ++++++++++++++--------------- src/lisp/regression-tests/misc.lisp | 43 ++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/src/core/bytecode.cc b/src/core/bytecode.cc index 2b4cc0649d..20029f0324 100644 --- a/src/core/bytecode.cc +++ b/src/core/bytecode.cc @@ -512,12 +512,17 @@ bytecode_vm(VirtualMachine& vm, T_O** literals, T_O** closed, Closure_O* closure bool aokp = false; T_sp unknown_keys = nil(); vm._stackPointer = sp; - SimpleVector_sp argstemp = SimpleVector_O::make(key_count, unbound()); + if ((lcc_nargs > more_start) && (((lcc_nargs - more_start) % 2) != 0)) { + T_sp tclosure((gctools::Tagged)gctools::tag_general(closure)); + throwOddKeywordsError(tclosure); + } + // The parameter slots are themselves the destination, so no scratch vector + // is needed. stackref 0 is the most recently pushed, so slot n is key n. + for (size_t i = 0; i < key_count; ++i) + vm.push(sp, unbound().raw_()); + // The slots are live roots now, so the GC scanner must see them. + vm._stackPointer = sp; if (lcc_nargs > more_start) { - if (((lcc_nargs - more_start) % 2) != 0) { - T_sp tclosure((gctools::Tagged)gctools::tag_general(closure)); - throwOddKeywordsError(tclosure); - } // We grab keyword arguments from the end to the beginning. // This means that earlier arguments are put in their variables // last, matching the CL semantics. @@ -536,8 +541,7 @@ bytecode_vm(VirtualMachine& vm, T_O** literals, T_O** closed, Closure_O* closure T_O* ckey = literals[key_id + key_literal_start]; if (key == ckey) { valid_key_p = true; - T_sp value((gctools::Tagged)(lcc_args[arg_index])); - (*argstemp)[key_id] = value; + *vm.stackref(sp, key_id) = lcc_args[arg_index]; break; } } @@ -551,13 +555,6 @@ bytecode_vm(VirtualMachine& vm, T_O** literals, T_O** closed, Closure_O* closure T_sp tclosure((gctools::Tagged)gctools::tag_general(closure)); throwUnrecognizedKeywordArgumentError(tclosure, unknown_keys); } - // Finally, push keys to the stack. - for (size_t i = 0; i < key_count; ++i) { - size_t key_id = key_count - i - 1; - T_sp key((gctools::Tagged)literals[key_id + key_literal_start]); - T_sp value = (*argstemp)[key_id]; - vm.push(sp, value.raw_()); - } pc++; break; } @@ -1299,12 +1296,16 @@ static unsigned char* long_dispatch(VirtualMachine& vm, unsigned char* pc, Multi bool aokp = false; T_sp unknown_keys = nil(); vm._stackPointer = sp; - SimpleVector_sp argstemp = SimpleVector_O::make(key_count, unbound()); + if ((lcc_nargs > more_start) && (((lcc_nargs - more_start) % 2) != 0)) { + T_sp tclosure((gctools::Tagged)gctools::tag_general(closure)); + throwOddKeywordsError(tclosure); + } + // See the short-operand form above; the parameter slots are the destination. + for (size_t i = 0; i < key_count; ++i) + vm.push(sp, unbound().raw_()); + // The slots are live roots now, so the GC scanner must see them. + vm._stackPointer = sp; if (lcc_nargs > more_start) { - if (((lcc_nargs - more_start) % 2) != 0) { - T_sp tclosure((gctools::Tagged)gctools::tag_general(closure)); - throwOddKeywordsError(tclosure); - } // KLUDGE: We use a signed type so that if more_start is zero we don't // wrap arg_index around. There's probably a cleverer solution. ptrdiff_t arg_index; @@ -1320,8 +1321,7 @@ static unsigned char* long_dispatch(VirtualMachine& vm, unsigned char* pc, Multi T_O* ckey = literals[key_id + key_literal_start]; if (key == ckey) { valid_key_p = true; - T_sp value((gctools::Tagged)(lcc_args[arg_index])); - (*argstemp)[key_id] = value; + *vm.stackref(sp, key_id) = lcc_args[arg_index]; break; } } @@ -1335,8 +1335,6 @@ static unsigned char* long_dispatch(VirtualMachine& vm, unsigned char* pc, Multi T_sp tclosure((gctools::Tagged)gctools::tag_general(closure)); throwUnrecognizedKeywordArgumentError(tclosure, unknown_keys); } - for (size_t i = 0; i < key_count; ++i) - vm.push(sp, (*argstemp)[key_count - i - 1].raw_()); pc += 7; break; } diff --git a/src/lisp/regression-tests/misc.lisp b/src/lisp/regression-tests/misc.lisp index 118a1dc0e9..dc65e962a6 100644 --- a/src/lisp/regression-tests/misc.lisp +++ b/src/lisp/regression-tests/misc.lisp @@ -321,3 +321,46 @@ (test single-value-catch (let ((c (catch 'foo 4))) c) (4)) + +;;; parse-key-args in the bytecode VM writes each keyword argument straight into +;;; the callee's parameter slot. An off-by-one or reversed slot index there binds +;;; the wrong parameter with no error at all, so pin the identity for every shape. +(defun kwtest-3 (a &key x y z) (list a x y z)) + +(test-true keyword-parsing-slot-identity + (and (equal (kwtest-3 0) '(0 nil nil nil)) + (equal (kwtest-3 0 :x 1) '(0 1 nil nil)) + (equal (kwtest-3 0 :y 2) '(0 nil 2 nil)) + (equal (kwtest-3 0 :z 3) '(0 nil nil 3)) + (equal (kwtest-3 0 :x 1 :y 2 :z 3) '(0 1 2 3)) + (equal (kwtest-3 0 :z 3 :y 2 :x 1) '(0 1 2 3)) + (equal (kwtest-3 0 :y 2 :z 3 :x 1) '(0 1 2 3)) + (equal (kwtest-3 0 :x 1 :z 3) '(0 1 nil 3)))) + +;;; CLHS 3.4.1.4: when a keyword is repeated the leftmost pair wins. The VM gets +;;; this by scanning arguments right to left and overwriting. +(test-true keyword-parsing-duplicate-keywords + (and (equal (kwtest-3 0 :x 'first :x 'second) '(0 first nil nil)) + (equal (kwtest-3 0 :x 'a :x 'b :x 'c) '(0 a nil nil)) + (equal (kwtest-3 0 :x 'a :y 'p :x 'b) '(0 a p nil)))) + +(defun kwtest-aok (&key x &allow-other-keys) x) + +(test-true keyword-parsing-allow-other-keys + (and (eql 1 (kwtest-aok :x 1 :bogus 2)) + (equal (kwtest-3 0 :x 1 :allow-other-keys t :bogus 9) '(0 1 nil nil)) + (eq :errored (handler-case (progn (kwtest-3 0 :bogus 1) :no-error) + (error () :errored))) + (eq :errored (handler-case (progn (apply #'kwtest-3 0 '(:x)) :no-error) + (error () :errored))))) + +(defun kwtest-16 (&key a b c d e f g h i j k l m n o p) + (list a b c d e f g h i j k l m n o p)) + +;;; Many parameter slots, live across a collection. +(test-true keyword-parsing-many-keys-across-gc + (let ((expected '(1 nil nil nil nil nil nil nil + nil nil nil nil nil nil nil 16))) + (and (equal (kwtest-16 :a 1 :p 16) expected) + (progn (gctools:garbage-collect) + (equal (kwtest-16 :a 1 :p 16) expected)))))