From fbcbbb8041a9a246b903ef128d6661010bd09199 Mon Sep 17 00:00:00 2001 From: Myers Carpenter Date: Fri, 2 Oct 2026 19:43:03 -0400 Subject: [PATCH] Keep the scalar sequence decoder's debug frame off the kernel stack At -O0 LLVM gives every inlined temporary its own stack slot, so the scalar monolith `decode_and_execute_sequences_scalar` grows with each force-inlined copy. Own frame of each monomorph, measured from the object file of a debug build for x86_64-unknown-none with `panic = "abort"` and `default-features = false`: main BIT_MASK static + executor not forced UserSliceBackend 147,912 104,648 25,736 FlatBuf 88,648 45,384 23,784 RingBuffer 83,912 40,648 23,784 Two causes, both translated from upstream zstd's own shape: - `cpu_kernel::BIT_MASK` was a `const [u64; 256]`. A `const` is a value copied at each use; at -O0 each inlined `mask_lower_bits` load memcpy'd the 2 KiB table into its own slot (43 KiB of the frame). Upstream's `BIT_mask` is `static const unsigned BIT_mask[]` (lib/common/bitstream.h), one object; it is now a `static`. - `execute_one_sequence_pipelined` was `#[inline(always)]`, and the monolith expands it at the three `execute_one_body!` sites, each pulling in the backend's whole inline exec tree. Upstream marks `ZSTD_execSequence` `HINT_INLINE` and lets the build drop the forced inline (`ZSTD_NO_INLINE`, lib/common/compiler.h). The forced inline now applies only without `debug_assertions`; a debug build calls it (26,376 B for UserSliceBackend). Optimized builds keep the forced inline. No storage moved: the frame was many small slots, not one scratch array. Decoded bytes and error behaviour are unchanged. The `static` does reach release code. Timing with `profile_decode` against main, separately built, min of seven interleaved runs on an aarch64 host: z000033 0.700 s vs 0.700 s, z000091 0.229 s vs 0.229 s, z000006 (small frame) 3.713 s vs 3.704 s, all inside the build-to-build band. The x86_64 scalar tier was not timed. Regression test `scalar_decode_fits_a_bounded_debug_stack` enters the scalar monolith for all three backends on a 96 KiB thread; a frame that does not fit hits the guard page and aborts the test binary. Mutation check on main: restoring the `const` alone, the forced inline alone, or both each aborts it with "has overflowed its stack"; with the fix it passes. --- zstd/src/cpu_kernel.rs | 7 +- zstd/src/decoding/sequence_section_decoder.rs | 10 ++- .../sequence_section_decoder/tests.rs | 76 +++++++++++++++++++ 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/zstd/src/cpu_kernel.rs b/zstd/src/cpu_kernel.rs index b76ebf53d..ea2fe3cc6 100644 --- a/zstd/src/cpu_kernel.rs +++ b/zstd/src/cpu_kernel.rs @@ -502,7 +502,12 @@ pub struct ScalarKernel; /// `cmov` on every field of every sequence. Indexed by a `u8`, and sized for /// every `u8`, so the load carries no bounds check either. The widths in use /// are small, so the hot part is the first few cache lines of it. -pub(crate) const BIT_MASK: [u64; 256] = { +/// +/// A `static`, as upstream zstd's `static const unsigned BIT_mask[]` +/// (`lib/common/bitstream.h`) is one object. A `const` array is a value +/// copied at each use; a debug (`-O0`) build copied all 2 KiB of it onto the +/// stack at every inlined load, 43 KiB of the scalar sequence decoder's frame. +pub(crate) static BIT_MASK: [u64; 256] = { let mut table = [u64::MAX; 256]; let mut i: usize = 0; while i < 64 { diff --git a/zstd/src/decoding/sequence_section_decoder.rs b/zstd/src/decoding/sequence_section_decoder.rs index 807ab7064..79ff2c75b 100644 --- a/zstd/src/decoding/sequence_section_decoder.rs +++ b/zstd/src/decoding/sequence_section_decoder.rs @@ -508,7 +508,15 @@ pub(crate) struct ExecSeq { /// The per-call `buffer.reserve(match_length)` is preserved by that /// variant — required for memory safety against malformed inputs whose /// `match_length` exceeds the upfront block-maximum headroom. -#[inline(always)] +/// +/// Forced inline only in optimized builds. Upstream zstd marks +/// `ZSTD_execSequence` `HINT_INLINE` and lets the build drop the forced +/// inline (`ZSTD_NO_INLINE`, `lib/common/compiler.h`). Here a debug build +/// drops it: at `-O0` every inlined copy keeps its own stack slots, and the +/// scalar monolith expands this executor at three call sites, which made that +/// one frame 147,912 B (`UserSliceBackend`, x86_64). Release code is unchanged. +#[cfg_attr(not(debug_assertions), inline(always))] +#[cfg_attr(debug_assertions, inline)] // Grouping the arguments into a struct would push them off the argument // registers and onto memory loads, which is the cost this per-sequence // boundary exists to avoid. diff --git a/zstd/src/decoding/sequence_section_decoder/tests.rs b/zstd/src/decoding/sequence_section_decoder/tests.rs index f3869d193..72d381211 100644 --- a/zstd/src/decoding/sequence_section_decoder/tests.rs +++ b/zstd/src/decoding/sequence_section_decoder/tests.rs @@ -396,6 +396,82 @@ mod init_sequence_stream_tests { ); } + /// Stack for one scalar sequence decode and everything it calls, in a + /// debug (`-O0`) build. The portable tier is what a + /// `default-features = false` build runs on every target. A 64-page + /// (256 KiB) kernel task stack also holds the caller's path down to the + /// decoder, so the decoder gets well under half of it. After the fix the + /// monolith and the out-of-line executor it calls take about 52 KiB at + /// `-O0` (x86_64, `UserSliceBackend`); this leaves room for the thread's own + /// start-up frames without letting any backend's monolith grow back. + #[cfg(feature = "std")] + const SCALAR_DECODE_STACK: usize = 96 * 1024; + + /// Enter the scalar monolith for one backend on a thread whose whole stack + /// is [`SCALAR_DECODE_STACK`]. The prologue probes every page of the frame, + /// so a frame that does not fit hits the guard page and the process aborts + /// with "has overflowed its stack" rather than returning. + #[cfg(feature = "std")] + fn scalar_decode_on_bounded_stack(make_buffer: F) + where + B: crate::decoding::buffer_backend::BufferBackend, + F: FnOnce() -> DecodeBuffer, + DecodeBuffer: Send + 'static, + { + // Everything the harness owns is built here, on the caller's stack, so + // the bounded thread spends its stack on the decoder alone. The FSE + // tables are ~12 KiB inline; the decoder takes them by reference. + let header = predefined_one_sequence_header(); + let mut fse = alloc::boxed::Box::new(FSEScratch::new()); + let mut buf = alloc::boxed::Box::new(make_buffer()); + std::thread::Builder::new() + .stack_size(SCALAR_DECODE_STACK) + .spawn(move || { + let source = [0xFFu8; 8]; + let lits = [0u8; 32]; + let mut offset_hist = [1u32, 4, 8]; + let _ = crate::decoding::seq_decoder_scalar::decode_and_execute_sequences_scalar( + &header, + &source, + &mut fse, + &mut buf, + &mut offset_hist, + &lits, + lits.len() - crate::WILDCOPY_OVERLENGTH, + None, + ); + }) + .expect("spawn bounded-stack decode thread") + .join() + .expect("bounded-stack decode thread panicked"); + } + + /// Debug-build stack regression for the scalar sequence decoder, every + /// backend it is instantiated for. At `-O0` LLVM gives each inlined + /// temporary its own stack slot, so the frame grew with every + /// force-inlined copy: the 2 KiB `BIT_MASK` table was a `const`, copied + /// onto the stack at each inlined load, and the per-sequence executor was + /// force-inlined at all three of the monolith's call sites. The + /// monolith's own frame reached 147,912 B (`UserSliceBackend`), 88,648 B + /// (`FlatBuf`) and 83,912 B (`RingBuffer`); the first is larger than this + /// whole thread stack on its own. After the fix they are 25,736 B, + /// 23,784 B and 23,784 B (x86_64, `panic = "abort"`, no default + /// features). `cargo test` builds the crate at + /// `-O0`, so it runs the configuration a debug kernel links. + #[cfg(feature = "std")] + #[test] + fn scalar_decode_fits_a_bounded_debug_stack() { + use crate::decoding::flat_buf::FlatBuf; + use crate::decoding::user_slice_buf::UserSliceBackend; + + scalar_decode_on_bounded_stack(|| DecodeBuffer::::new(4 * 1024)); + scalar_decode_on_bounded_stack(|| DecodeBuffer::::new(4 * 1024)); + scalar_decode_on_bounded_stack(|| { + let output: &'static mut [u8] = std::vec![0u8; 4 * 1024].leak(); + DecodeBuffer::from_backend(UserSliceBackend::from_slice(output), 1 << 20) + }); + } + /// Drive the BMI2 entry's preamble directly. The runtime kernel /// selector prefers the avx2 tier on any CPU that has BMI2, so this /// tier never runs through the normal dispatch on CI hardware; call it