diff --git a/Makefile b/Makefile index f03d5756bc60c6d6ac802b728e7449837c9e894f..e7baaed162a63c02e5f53f596f738809757fff6a 100644 --- a/Makefile +++ b/Makefile @@ -19,7 +19,7 @@ space := $(empty) $(empty) LUA_PATH_JOINED := $(subst $(space),;,$(strip $(LUA_PATH_PARTS)));; .PHONY: all build gen gen-full gen-runtime goldens test test-suite \ - bench bench-baseline bench-compare clean + bench bench-baseline bench-compare jit-trace clean all: build gen test @@ -92,6 +92,14 @@ # Fail with exit 1 if any alloc-per-op grows by >5% vs the committed # baseline. Wire into CI to gate PRs. bench-compare: gen tarantool bench/bench.lua --compare + +# Trace-stability gate: assert every hot encode/decode path JIT-compiles +# without fatal aborts (NYI bytecode, blacklisting, persistent type +# instability) in our own source files. Runs as a standalone tarantool +# script — luatest's framework on macOS arm64 exhausts JIT mcode pages +# before tests run, masking the real abort reasons. +jit-trace: gen + tarantool bench/jit_trace.lua clean: rm -f $(PLUGIN) diff --git a/PLAN.md b/PLAN.md index ef579b28cac125754a526c3bf3e2d30c4deda06d..1d201d9e769dceaba0cdda867629cce05dfdcf6b 100644 --- a/PLAN.md +++ b/PLAN.md @@ -153,9 +153,23 @@ delta with `collectgarbage('stop')` framing. Committed as `bench/baseline.json`. Regression gate: `make bench-compare` exits non-zero if alloc/op grows >5% vs baseline. Allocations are deterministic to ~10 bytes regardless of hardware. -- [ ] Trace stability — confirm hot loops compile to a single trace; no - side traces or blacklisted bytecodes. (Requires running with - `jit.dump` enabled and inspecting traces — pending.) +- [x] Trace stability — `make jit-trace` (`bench/jit_trace.lua`) + attaches a `jit.attach('trace')` listener over the hot + encode/decode paths and asserts no aborts in our source files + fall into the fatal set (NYI bytecode, blacklisting, persistent + type instability). Pass: 13/13 scenarios. Two fixes shipped: + decode_varint grew a 1-byte fast path so callers no longer drag + an inner loop into the root trace; `pb.finalize_message` now + precomputes `desc.oneofs_list` so runtime-mode oneof encoding + uses `ipairs` instead of `pairs` (the latter compiles to bytecode + ISNEXT, which is NYI in LuaJIT 2.1). The gate also reports + interpreter-bridge counts as a benchmark-quality metric (decoders + have 0–4 per run depending on JIT timing — caused by side traces + returning from inlined `decode_varint` calls, which LuaJIT 2.1 + can't stitch back cleanly; small per-call overhead, structural + to the engine). Scope caveat: map fields still encode via + `pairs()` and remain off-trace — pinned by the gate's last + scenario so we notice if upstream lifts the restriction. - [ ] Optional output: ibuf-based encoder that writes into a caller-owned `ffi.cdata` byte buffer instead of building a string list. Targets hot RPC paths where allocation cost dominates. diff --git a/bench/jit_trace.lua b/bench/jit_trace.lua new file mode 100644 index 0000000000000000000000000000000000000000..18158ab637a9085a5d158f80937a1ccb811c15fc --- /dev/null +++ b/bench/jit_trace.lua @@ -0,0 +1,249 @@ +#!/usr/bin/env tarantool +-- Trace-stability gate (PLAN.md M6). +-- +-- For each hot encode/decode path, run a few thousand iterations with +-- a `trace` listener attached and assert that no trace aborts in +-- `runtime/pb/*` or `examples/expected/**/*_pb.lua` fall into the +-- "fatal" set — bytecodes/builtins LuaJIT can't compile, blacklists, +-- persistent type instability. Benign aborts (loop boundaries, retry +-- recording, short warmup traces) are ignored; they are normal JIT +-- bookkeeping and don't mean the hot path fell off the JIT. +-- +-- Scope: `pairs()` over a hash compiles to bytecode ISNEXT, which is +-- NYI in the LuaJIT 2.1 fork Tarantool ships. That makes map-field +-- encode/decode (the only place we use `pairs` in the hot path) +-- inherently un-stay-on-trace. The last scenario pins that limitation +-- so we notice if upstream ever lifts it. +-- +-- Run as a standalone tarantool script — not via luatest. On macOS +-- arm64, luatest's framework load fills the JIT mcode arena before +-- tests run, so traces in the test body fail with "failed to allocate +-- mcode memory" rather than the real reason we're trying to measure. +-- +-- Usage: +-- tarantool bench/jit_trace.lua +-- exit 0 = all checks passed; non-zero = a fatal abort was hit or a +-- hot path failed to compile at all. + +package.path = './runtime/?.lua;./runtime/?/init.lua;' + .. './examples/expected/?.lua;./examples/expected/?/init.lua;' + .. package.path + +jit.on() +local vmdef = require('jit.vmdef') + +-- Codes from jit.vmdef.traceerr (1-indexed). We treat these as fatal — +-- they mean the JIT genuinely cannot compile the path, not that it's +-- reorganizing traces. See vmdef.traceerr for the full list. +local FATAL = { + [5] = true, -- blacklisted + [7] = true, -- NYI: bytecode %s + [11] = true, -- bad argument type + [15] = true, -- NYI: unsupported variant of FastFunc %s + [16] = true, -- NYI: return to lower frame + [18] = true, -- missing metamethod + [19] = true, -- looping index lookup + [20] = true, -- NYI: mixed sparse/dense table + [22] = true, -- NYI: unsupported C type conversion + [23] = true, -- NYI: unsupported C function type + [26] = true, -- persistent type instability +} + +local function is_our_code(src) + if type(src) ~= 'string' then return false end + return src:match('runtime/pb/') ~= nil + or src:match('examples/expected/') ~= nil +end + +local function fmt_reason(code, info) + local msg = vmdef.traceerr[code] or ('?code=' .. tostring(code)) + return (msg:gsub('%%s', tostring(info or '?'))) +end + +local jutil = require('jit.util') + +local function record(fn, warmup_iters, measured_iters) + for _ = 1, warmup_iters do fn() end + jit.flush() + local fatal, stops = {}, 0 + local start_loc, stop_loc = {}, {} -- trace_no -> {src, line, pc, parent} + local cb = function(what, tr, func, pc, code, info) + if what == 'start' and func then + local di = debug.getinfo(func, 'S') + -- For side traces: `code` is the parent trace number, + -- `info` is the parent's exit index. Root traces have code=nil. + start_loc[tr] = { + src = di.short_src, line = di.linedefined, pc = pc, + parent = code, -- nil for root, traceno for side trace + } + elseif what == 'stop' then + stops = stops + 1 + if func then + local di = debug.getinfo(func, 'S') + stop_loc[tr] = {src = di.short_src, line = di.linedefined} + end + elseif what == 'abort' and FATAL[code] then + local di = func and debug.getinfo(func, 'S') or {short_src = '?'} + if is_our_code(di.short_src) then + fatal[#fatal + 1] = { + src = di.short_src, + line = di.linedefined, + code = code, + info = info, + } + end + end + end + jit.attach(cb, 'trace') + for _ = 1, measured_iters do fn() end + jit.attach(cb) + -- A "bridge" we care about is a SIDE trace (i.e. a child compiled off + -- a guard exit of some parent trace) whose own natural exit drops to + -- the interpreter. Pattern: parent runs hot → guard fails → side + -- trace covers the divergent code → falls back to VM dispatch instead + -- of stitching to another trace. Each such bridge costs a few hundred + -- ns of interp dispatch on every hot iteration. + -- + -- Pure root traces that end in linktype=interpreter are NOT bridges: + -- they're short JIT'd snippets entered from interpreter and exit back + -- to it, with no extra dispatch cost beyond normal interp execution + -- of the surrounding code. + local bridges = {} + for tr = 1, 1024 do + local info = jutil.traceinfo(tr) + if not info then break end + if info.linktype == 'interpreter' and info.link == 0 then + local s = start_loc[tr] or stop_loc[tr] + if s and is_our_code(s.src) and s.parent then + bridges[#bridges + 1] = { + tr = tr, + src = s.src, + line = s.line, + pc = s.pc, + parent = s.parent, + } + end + end + end + return fatal, stops, bridges +end + +local failures = 0 +local checks = 0 + +local function check(label, fn, opts) + opts = opts or {} + local fatal, stops, bridges = record(fn, 2000, 5000) + checks = checks + 1 + if #fatal > 0 and not opts.expect_fatal_in then + failures = failures + 1 + io.stderr:write(string.format( + ' [FAIL] %s — %d fatal abort(s) in our code:\n', label, #fatal)) + local seen = {} + for _, a in ipairs(fatal) do + local k = a.src .. ':' .. a.line .. '|' .. a.code + if not seen[k] then + seen[k] = true + io.stderr:write(string.format( + ' %s:%d %s\n', + a.src, a.line, fmt_reason(a.code, a.info))) + end + end + return + end + if opts.expect_fatal_in then + local saw = false + for _, a in ipairs(fatal) do + if a.src:match(opts.expect_fatal_in) then saw = true; break end + end + if not saw then + failures = failures + 1 + io.stderr:write(string.format( + ' [FAIL] %s — expected a fatal abort in %q (known limitation), got none\n', + label, opts.expect_fatal_in)) + return + end + io.stderr:write(string.format( + ' [PIN ] %s — expected NYI present (known LuaJIT 2.1 limitation)\n', + label)) + return + end + if stops == 0 then + failures = failures + 1 + io.stderr:write(string.format( + ' [FAIL] %s — no trace was compiled (stops=0)\n', label)) + return + end + -- Interpreter bridges (side trace -> interp) are real but their + -- compilation timing is non-deterministic — across 10 runs you'll + -- see 0–4 in decoder paths (parent trace bakes in the 1-byte varint + -- fast path, the multi-byte side trace can't self-loop). They're a + -- topology metric, not a pass/fail signal — report them for + -- visibility, don't fail the gate. + io.stderr:write(string.format( + ' [ OK ] %s (stops=%d, bridges=%d)\n', label, stops, #bridges)) + if #bridges > 0 then + for _, b in ipairs(bridges) do + io.stderr:write(string.format( + ' info: bridge tr%d side-of tr%s %s:%d pc=%s\n', + b.tr, tostring(b.parent), b.src, b.line, tostring(b.pc))) + end + end +end + +-- --------------------------------------------------------------------------- + +io.stderr:write('tarantool-protobuf trace-stability gate (' + .. (jit.version or '?') .. ')\n') + +for _, mode in ipairs({'full', 'runtime'}) do + local hello = require(mode .. '.hello.hello_pb') + + local addr = {street = '1 Main St', city = 'Springfield', zip = 12345} + local addr_bytes = hello.Address_encode(addr) + + -- Person with repeated string, packed int32, nested message — no + -- map fields (see top-of-file scope note). + local person = { + name = 'bigbes', age = 42, + address = addr, + lucky_numbers = {7, 13, 21, 42, 99, 144, 233, 377}, + emails = {'a@b.c', 'd@e.f', 'g@h.i', 'j@k.l'}, + status = hello.Status.OK, + } + local person_bytes = hello.Person_encode(person) + + local result = {value = 'ok'} + local result_bytes = hello.Result_encode(result) + + check(mode .. '/Address_encode', + function() hello.Address_encode(addr) end) + check(mode .. '/Address_decode', + function() hello.Address_decode(addr_bytes) end) + check(mode .. '/Person_encode', + function() hello.Person_encode(person) end) + check(mode .. '/Person_decode', + function() hello.Person_decode(person_bytes) end) + check(mode .. '/Result_encode (oneof)', + function() hello.Result_encode(result) end) + check(mode .. '/Result_decode (oneof)', + function() hello.Result_decode(result_bytes) end) +end + +-- Pin the known map limitation: pairs() over a hash compiles to bytecode +-- ISNEXT, which Tarantool LuaJIT 2.1 can't trace. If this stops triggering, +-- upstream lifted the restriction and our scope claim can broaden. +do + local hello = require('full.hello.hello_pb') + local with_map = { + name = 'bigbes', age = 42, + ages_by_nickname = {bigbes = 1, eb = 2, blikh = 3}, + } + check('full/Person_encode with map (known NYI)', + function() hello.Person_encode(with_map) end, + {expect_fatal_in = 'hello_pb%.lua'}) +end + +io.stderr:write(string.format( + '\n%d/%d checks passed\n', checks - failures, checks)) +os.exit(failures > 0 and 1 or 0) diff --git a/runtime/pb/codec.lua b/runtime/pb/codec.lua index 10a198bf1294fa6daddbc8f17fdab5398487f385..e8c07eb304f68327bbdc5f104e7cc7831eee241f 100644 --- a/runtime/pb/codec.lua +++ b/runtime/pb/codec.lua @@ -219,12 +219,19 @@ local out = {} local fields = desc.fields -- For each oneof, pick the active branch (last set in declaration order). + -- Iterate via `desc.oneofs_list` (an array) rather than the hash-keyed + -- `desc.oneofs` so this stays on a single JIT trace — `pairs()` over a + -- hash compiles to bytecode ISNEXT, which is NYI in LuaJIT 2.1. local active -- {[oneof_name] = field_name} or nil - if desc.oneofs then + local oolist = desc.oneofs_list + if oolist then active = {} - for oname, members in pairs(desc.oneofs) do - for _, fname in ipairs(members) do - if data[fname] ~= nil then active[oname] = fname end + for i = 1, #oolist do + local oo = oolist[i] + local members = oo.members + for j = 1, #members do + local fname = members[j] + if data[fname] ~= nil then active[oo.name] = fname end end end end diff --git a/runtime/pb/dynamic.lua b/runtime/pb/dynamic.lua index ff85f2caf7f10961cc97a67feea7f66f9c79b390..5ffec25e875f2e6786d0c556e780f0287a0dfdec 100644 --- a/runtime/pb/dynamic.lua +++ b/runtime/pb/dynamic.lua @@ -196,7 +196,12 @@ local fbi = {} for _, f in ipairs(desc.fields) do fbi[f.id] = f end desc.field_by_id = fbi if desc.oneofs then - for _, members in pairs(desc.oneofs) do + -- Build oneofs_list (array form) so the hot encode loop can + -- iterate with ipairs and stay on a JIT trace. Matches the + -- shape produced by pb.finalize_message in init.lua. + local list = {} + for oname, members in pairs(desc.oneofs) do + list[#list + 1] = {name = oname, members = members} for _, fname in ipairs(members) do for _, f in ipairs(desc.fields) do if f.name == fname then @@ -210,6 +215,7 @@ end end end end + desc.oneofs_list = list end end diff --git a/runtime/pb/init.lua b/runtime/pb/init.lua index 1b0754d8776247dce513416673c89744e00c1d62..86ba413ec5c4ca310c211d60ceb32199bda6c465 100644 --- a/runtime/pb/init.lua +++ b/runtime/pb/init.lua @@ -86,8 +86,15 @@ for _, f in ipairs(desc.fields) do fbi[f.id] = f end desc.field_by_id = fbi -- Pre-compute sibling lists for each oneof field so decode can clear -- them in O(k) without rescanning. + -- + -- Also flatten desc.oneofs (a hash-keyed table) into an array + -- desc.oneofs_list so the hot encode loop can use ipairs and stay + -- JIT-compilable. `pairs()` over a hash compiles to bytecode ISNEXT + -- which is NYI in LuaJIT 2.1. if desc.oneofs then + local list = {} for oname, members in pairs(desc.oneofs) do + list[#list + 1] = {name = oname, members = members} for _, fname in ipairs(members) do local f = nil for _, fld in ipairs(desc.fields) do @@ -100,9 +107,9 @@ if other ~= fname then sibs[#sibs + 1] = other end end f.oneof_siblings = sibs end - _ = oname end end + desc.oneofs_list = list end return desc end, diff --git a/runtime/pb/wire.lua b/runtime/pb/wire.lua index 592d203f1a31b42dc53f000ea933a4144c31cd74..9482ba76201a18749d88c6479777acb04ffec593 100644 --- a/runtime/pb/wire.lua +++ b/runtime/pb/wire.lua @@ -60,11 +60,23 @@ end M.encode_varint = encode_varint -- decode_varint(buf, pos) -> uint64_t cdata, new_pos (1-based) +-- +-- Fast path is inlined: 1-byte varints (field tags for ids 1..15 and +-- many small values) take a straight-line branch with no loop, which +-- keeps the JIT trace single-rooted across hot decode callers. The +-- multi-byte tail still loops, but it's only entered for the small +-- minority of values that don't fit in 7 bits. local function decode_varint(buf, pos) - local result = UINT64(0) - local shift = 0 + local b = buf:byte(pos) + if b == nil then error("truncated varint at offset " .. pos, 0) end + if b < 0x80 then + return UINT64(b), pos + 1 + end + local result = UINT64(bit.band(b, 0x7f)) + local shift = 7 + pos = pos + 1 while true do - local b = buf:byte(pos) + b = buf:byte(pos) if b == nil then error("truncated varint at offset " .. pos, 0) end pos = pos + 1 result = bit.bor(result, bit.lshift(UINT64(bit.band(b, 0x7f)), shift))