From 5ad37a5a8656c438c68dabd31c9d36467390e046 Mon Sep 17 00:00:00 2001 From: Alexander Medvednikov Date: Tue, 23 Jun 2026 16:39:40 +0300 Subject: [PATCH] builtin, cgen, checker: fix spawn-in-if/match-expr, zero-alloc error sentinel, cheaper slice marking, scalable GC (#27544) --- .../thirdparty-freebsd-amd64_bdwgc.sh | 1 + .../thirdparty-linux-amd64_bdwgc.sh | 1 + .../thirdparty-linux-armv7_bdwgc.sh | 1 + .../thirdparty-macos-arm64_bdwgc.sh | 1 + vlib/builtin/allocation.c.v | 9 ++++ vlib/builtin/array.v | 10 ++++- vlib/builtin/builtin_d_gcboehm.c.v | 16 +++++++ vlib/builtin/chan_option_result.v | 11 +++++ vlib/builtin/js/builtin.v | 11 +++++ vlib/v/checker/if.v | 8 ++++ vlib/v/checker/match.v | 8 ++++ vlib/v/gen/c/if.v | 7 +++ vlib/v/tests/array_slice_buffer_mark_test.v | 44 +++++++++++++++++++ vlib/v/tests/autofree_error_sentinel_test.v | 34 ++++++++++++++ .../concurrency/spawn_in_if_match_expr_test.v | 39 ++++++++++++++++ vlib/v/tests/error_sentinel_test.v | 39 ++++++++++++++++ 16 files changed, 238 insertions(+), 2 deletions(-) create mode 100644 vlib/v/tests/array_slice_buffer_mark_test.v create mode 100644 vlib/v/tests/autofree_error_sentinel_test.v create mode 100644 vlib/v/tests/concurrency/spawn_in_if_match_expr_test.v create mode 100644 vlib/v/tests/error_sentinel_test.v diff --git a/thirdparty/build_scripts/thirdparty-freebsd-amd64_bdwgc.sh b/thirdparty/build_scripts/thirdparty-freebsd-amd64_bdwgc.sh index 7bf8880a3..ce35df32b 100755 --- a/thirdparty/build_scripts/thirdparty-freebsd-amd64_bdwgc.sh +++ b/thirdparty/build_scripts/thirdparty-freebsd-amd64_bdwgc.sh @@ -41,6 +41,7 @@ CC=$CC CFLAGS='-Os -mtune=generic -fPIC' LDFLAGS='-Os -fPIC' ./configure \ --enable-parallel-mark \ --enable-single-obj-compilation \ --enable-gc-debug \ + --enable-thread-local-alloc \ --with-libatomic-ops=yes \ --enable-sigrt-signals \ --enable-mmap diff --git a/thirdparty/build_scripts/thirdparty-linux-amd64_bdwgc.sh b/thirdparty/build_scripts/thirdparty-linux-amd64_bdwgc.sh index fcb219e44..35afeb4a2 100755 --- a/thirdparty/build_scripts/thirdparty-linux-amd64_bdwgc.sh +++ b/thirdparty/build_scripts/thirdparty-linux-amd64_bdwgc.sh @@ -41,6 +41,7 @@ CC=$CC CFLAGS='-Os -mtune=generic -fPIC' LDFLAGS='-Os -fPIC' ./configure \ --enable-parallel-mark \ --enable-single-obj-compilation \ --enable-gc-debug \ + --enable-thread-local-alloc \ --with-libatomic-ops=yes \ --enable-sigrt-signals diff --git a/thirdparty/build_scripts/thirdparty-linux-armv7_bdwgc.sh b/thirdparty/build_scripts/thirdparty-linux-armv7_bdwgc.sh index 1253a90bf..29ffc3592 100755 --- a/thirdparty/build_scripts/thirdparty-linux-armv7_bdwgc.sh +++ b/thirdparty/build_scripts/thirdparty-linux-armv7_bdwgc.sh @@ -44,6 +44,7 @@ CC=$CC CFLAGS='-fPIC' LDFLAGS='-fPIC' ./configure \ --enable-parallel-mark \ --enable-single-obj-compilation \ --enable-gc-debug \ + --enable-thread-local-alloc \ --with-libatomic-ops=yes \ --enable-sigrt-signals diff --git a/thirdparty/build_scripts/thirdparty-macos-arm64_bdwgc.sh b/thirdparty/build_scripts/thirdparty-macos-arm64_bdwgc.sh index 95babc042..71efd3ebc 100755 --- a/thirdparty/build_scripts/thirdparty-macos-arm64_bdwgc.sh +++ b/thirdparty/build_scripts/thirdparty-macos-arm64_bdwgc.sh @@ -42,6 +42,7 @@ CC=$CC CFLAGS="-Os -mtune=generic -fPIC" LDFLAGS="-Os -fPIC" ./configure \ --enable-shared=yes \ --enable-single-obj-compilation \ --enable-gc-debug \ + --enable-thread-local-alloc \ --enable-large-config \ --enable-cplusplus \ --with-libatomic-ops=check \ diff --git a/vlib/builtin/allocation.c.v b/vlib/builtin/allocation.c.v index 1b06c61a3..396a24bb6 100644 --- a/vlib/builtin/allocation.c.v +++ b/vlib/builtin/allocation.c.v @@ -447,6 +447,15 @@ pub fn free(ptr voidptr) { if ptr == none_err._object { return } + // `error_sentinel` is likewise a cached, non-owning singleton IError (see + // chan_option_result.v). Under `-autofree`, const cleanup frees every IError + // const's boxed object, and the sentinel can also be copied/re-exported + // (`const my_err = error_sentinel`), so guard its shared object here to avoid + // a double free / dangling singleton, exactly as for `none__` above. + sentinel_err := &C.IError(&error_sentinel) + if ptr == sentinel_err._object { + return + } $if prealloc { return } $else $if vgc ? { diff --git a/vlib/builtin/array.v b/vlib/builtin/array.v index d0574b148..1899a172f 100644 --- a/vlib/builtin/array.v +++ b/vlib/builtin/array.v @@ -118,9 +118,15 @@ fn (mut a array) mark_buffer_has_slices() { if !a.flags.has(.managed) || a.data == unsafe { nil } { return } + // Compute the data-header pointer inline. `data_header()` would redundantly + // re-check `.managed`/`nil` (already guaranteed above), so skip the call and + // the now-dead `header != nil` test. Only store when the flag is not already + // set, to avoid dirtying the buffer's header cache line on every transient + // slice of the same buffer in a hot loop. See issue #27507. unsafe { - header := a.data_header() - if header != nil { + base_data := &u8(a.data) - u64(a.offset) + header := &ArrayDataHeader(base_data - array_data_header_size()) + if !header.has_slices { header.has_slices = true } } diff --git a/vlib/builtin/builtin_d_gcboehm.c.v b/vlib/builtin/builtin_d_gcboehm.c.v index c434d2777..40c2ad17b 100644 --- a/vlib/builtin/builtin_d_gcboehm.c.v +++ b/vlib/builtin/builtin_d_gcboehm.c.v @@ -2,6 +2,22 @@ module builtin $if !no_gc_threads ? { #flag -DGC_THREADS=1 + // Enable Boehm's thread-local allocation: each registered thread gets its own + // small-object free lists, so concurrent `GC_malloc` on the fast path no longer + // serializes on the global allocator lock. Without it, allocation does not scale + // across cores (16 cores ≈ 1 core aggregate). V's spawned threads are registered + // via the `pthread_create` -> `GC_pthread_create` redirect, so their free lists + // are set up automatically. TLA requires GC_THREADS (defined just above). + // + // This flag only matters for the bundled `gc.c` that V compiles from source (the + // `-prod`/no-prebuilt-archive branches below): its amalgamation was generated with + // `--enable-thread-local-alloc=no` (see thirdparty/libgc/amalgamation.txt), so the + // flag turns TLA back on. The prebuilt `thirdparty/tcc/lib/libgc.a`/`.dylib` used by + // the default fast path is already built with TLA (bdwgc enables it by default with + // `--enable-threads=pthreads`; see thirdparty/build_scripts/*_bdwgc.sh), and a system + // libgc that V merely links against is unaffected. So both bundled GC paths end up + // thread-local-alloc enabled. See issues #27486 and #27488. + #flag -DTHREAD_LOCAL_ALLOC=1 } $if use_bundled_libgc ? { diff --git a/vlib/builtin/chan_option_result.v b/vlib/builtin/chan_option_result.v index 7661aec05..81d7293c0 100644 --- a/vlib/builtin/chan_option_result.v +++ b/vlib/builtin/chan_option_result.v @@ -144,6 +144,17 @@ pub fn error_with_code(message string, code int) IError { } } +// error_sentinel is a reusable, allocation-free `IError` for hot "not found"/stateless +// error paths. Unlike `error('...')`, which heap-allocates a fresh `MessageError` on every +// call, `error_sentinel` is a single cached const (mirroring how `none` works for `?T`), +// so `return error_sentinel` from a `!T` function avoids the per-call allocation. +// Use it when the caller discards the error (e.g. `x() or { ... }`) and the specific +// message is not needed; its `.msg()` is just `'error'`. +// Example: fn find(x int) !int { if x < 0 { return error_sentinel } return x } +pub const error_sentinel = IError(&MessageError{ + msg: 'error' +}) + // Option is the base of V's internal option return system. struct Option { state u8 // 0 - ok; 2 - none; 1 - ? diff --git a/vlib/builtin/js/builtin.v b/vlib/builtin/js/builtin.v index ba2c05407..2cd46660f 100644 --- a/vlib/builtin/js/builtin.v +++ b/vlib/builtin/js/builtin.v @@ -149,6 +149,17 @@ pub fn error_with_code(message string, code int) IError { } } +// error_sentinel is a reusable, allocation-free `IError` for hot "not found"/stateless +// error paths. Unlike `error('...')`, which allocates a fresh `MessageError` on every +// call, `error_sentinel` is a single cached const (mirroring how `none` works for `?T`), +// so `return error_sentinel` from a `!T` function avoids the per-call allocation. +// Use it when the caller discards the error (e.g. `x() or { ... }`) and the specific +// message is not needed; its `.msg()` is just `'error'`. +// Example: fn find(x int) !int { if x < 0 { return error_sentinel } return x } +pub const error_sentinel = IError(&MessageError{ + msg: 'error' +}) + // free allows for manually freeing memory allocated at the address `ptr`. // However, this is a no-op on JS backend @[unsafe] diff --git a/vlib/v/checker/if.v b/vlib/v/checker/if.v index 204428bc7..e3ad0bd4e 100644 --- a/vlib/v/checker/if.v +++ b/vlib/v/checker/if.v @@ -395,6 +395,14 @@ fn (mut c Checker) if_expr(mut node ast.IfExpr) ast.Type { if branch.stmts.len > 0 { mut stmt := branch.stmts.last() if mut stmt is ast.ExprStmt { + // A trailing `spawn`/`go` is the value of this if-expression branch, + // so it must produce a usable thread handle (waiter, no detach), + // like the assignment form `t := spawn f()`. See issue #27485. + if mut stmt.expr is ast.SpawnExpr { + stmt.expr.is_expr = true + } else if mut stmt.expr is ast.GoExpr { + stmt.expr.is_expr = true + } if mut stmt.expr is ast.ConcatExpr { for mut val in stmt.expr.vals { c.check_expr_option_or_result_call(val, c.expr(mut val)) diff --git a/vlib/v/checker/match.v b/vlib/v/checker/match.v index 26da2ab03..e40f37ec4 100644 --- a/vlib/v/checker/match.v +++ b/vlib/v/checker/match.v @@ -317,6 +317,14 @@ fn (mut c Checker) match_expr(mut node ast.MatchExpr) ast.Type { && (!node.is_comptime || (node.is_comptime && comptime_match_branch_result)) { mut stmt := branch.stmts.last() if mut stmt is ast.ExprStmt { + // A trailing `spawn`/`go` is the value of this match-expression branch, + // so it must produce a usable thread handle (waiter, no detach), + // like the assignment form `t := spawn f()`. See issue #27485. + if mut stmt.expr is ast.SpawnExpr { + stmt.expr.is_expr = true + } else if mut stmt.expr is ast.GoExpr { + stmt.expr.is_expr = true + } c.expected_type = if c.expected_expr_type != ast.void_type { c.expected_expr_type } else { diff --git a/vlib/v/gen/c/if.v b/vlib/v/gen/c/if.v index 8e8debc60..07eeb47d4 100644 --- a/vlib/v/gen/c/if.v +++ b/vlib/v/gen/c/if.v @@ -261,6 +261,13 @@ fn (mut g Gen) need_tmp_var_in_expr(expr ast.Expr) bool { ast.SqlExpr { return true } + ast.SpawnExpr, ast.GoExpr { + // `spawn`/`go` lower to statement-level setup (arg struct alloc, + // pthread_create, ...) that cannot live inside a C ternary operand, + // so an if/match expression branch that is a spawn must use the + // tmp-var form instead of the ternary form. See issue #27485. + return true + } else {} } diff --git a/vlib/v/tests/array_slice_buffer_mark_test.v b/vlib/v/tests/array_slice_buffer_mark_test.v new file mode 100644 index 000000000..1059edcef --- /dev/null +++ b/vlib/v/tests/array_slice_buffer_mark_test.v @@ -0,0 +1,44 @@ +// Regression test for https://github.com/vlang/v/issues/27507 +// `array.slice()` marks the source buffer's data header (`has_slices`) so a later +// grow/delete of the parent does copy-on-grow instead of mutating a buffer that a +// live slice still references. The marking was made cheaper (inlined header lookup, +// store only when not already set); this test guards that the behavior is unchanged. + +fn test_live_slice_survives_parent_grow() { + mut a := [1, 2, 3, 4, 5] + s := a[1..4] + assert s == [2, 3, 4] + // growing the parent must copy, leaving the slice intact + for i in 0 .. 200 { + a << i + } + a[2] = 999 + assert s == [2, 3, 4] +} + +fn test_repeated_transient_slices_of_same_buffer() { + mut buf := []u8{len: 0, cap: 256} + for i in 0 .. 256 { + buf << u8(i) + } + mut acc := u64(0) + for _ in 0 .. 10000 { + w := buf[8..buf.len] // read-only window, dropped immediately + for b in w { + acc += b + } + } + // 10000 iterations * sum(8..256) + mut one := u64(0) + for j := 8; j < 256; j++ { + one += u64(j) + } + assert acc == one * 10000 +} + +fn test_slice_of_slice() { + a := [10, 20, 30, 40, 50] + s := a[1..5] // [20,30,40,50] + b := s[1..3] // [30,40] + assert b == [30, 40] +} diff --git a/vlib/v/tests/autofree_error_sentinel_test.v b/vlib/v/tests/autofree_error_sentinel_test.v new file mode 100644 index 000000000..61f725d99 --- /dev/null +++ b/vlib/v/tests/autofree_error_sentinel_test.v @@ -0,0 +1,34 @@ +// vtest vflags: -autofree -gc none +// Regression test for the `-autofree` double-free of the `error_sentinel` singleton +// (review on https://github.com/vlang/v/pull/27544). Under `-autofree`, const cleanup +// frees every `IError` const's boxed object; the sentinel can also be copied/re-exported, +// so its shared `MessageError` object must be exempted from `free()` like `none__`. +// `-gc none` is required so `free()` actually returns memory (a double free would abort +// at program cleanup) instead of being a GC no-op. + +const my_err = error_sentinel // re-export: shares the same boxed object + +fn find(x int) !int { + if x < 0 { + return error_sentinel + } + return x +} + +fn alias_find(x int) !int { + if x < 0 { + return my_err + } + return x +} + +fn test_error_sentinel_autofree_no_double_free() { + mut misses := 0 + for i := -50; i < 50; i++ { + find(i) or { misses++ } + alias_find(i) or { misses++ } + } + assert misses == 100 + assert my_err.msg() == 'error' + assert error_sentinel.msg() == 'error' +} diff --git a/vlib/v/tests/concurrency/spawn_in_if_match_expr_test.v b/vlib/v/tests/concurrency/spawn_in_if_match_expr_test.v new file mode 100644 index 000000000..838c3fb8a --- /dev/null +++ b/vlib/v/tests/concurrency/spawn_in_if_match_expr_test.v @@ -0,0 +1,39 @@ +// Regression test for https://github.com/vlang/v/issues/27485 +// Assigning an `if`/`match` expression whose branches are `spawn` calls used to +// emit a C ternary with statement-level spawn setup inside it, which failed to +// compile (and, once that was fixed, produced an unjoinable/detached thread). + +fn ret(x int) int { + return x +} + +fn test_spawn_in_if_expr() { + c := true + t := if c { spawn ret(11) } else { spawn ret(22) } + assert t.wait() == 11 +} + +fn test_spawn_in_if_expr_false_branch() { + c := false + t := if c { spawn ret(11) } else { spawn ret(22) } + assert t.wait() == 22 +} + +fn test_spawn_in_match_expr() { + x := 2 + t := match x { + 1 { spawn ret(1) } + 2 { spawn ret(2) } + else { spawn ret(99) } + } + + assert t.wait() == 2 +} + +fn test_spawn_in_if_expr_collected_into_array() { + mut threads := []thread int{} + for i in 0 .. 4 { + threads << if i % 2 == 0 { spawn ret(i) } else { spawn ret(i * 10) } + } + assert threads.wait() == [0, 10, 2, 30] +} diff --git a/vlib/v/tests/error_sentinel_test.v b/vlib/v/tests/error_sentinel_test.v new file mode 100644 index 000000000..2d79cd8a9 --- /dev/null +++ b/vlib/v/tests/error_sentinel_test.v @@ -0,0 +1,39 @@ +// Test for https://github.com/vlang/v/issues/27508 +// `error_sentinel` is a reusable, allocation-free `IError` (a cached const, like +// `none`), for hot stateless/"not found" error paths in `!T` functions. + +fn find(x int) !int { + if x < 0 { + return error_sentinel + } + return x +} + +fn test_error_sentinel_is_returned_as_error() { + mut misses := 0 + for i := -3; i < 3; i++ { + v := find(i) or { + misses++ + continue + } + assert v == i + } + assert misses == 3 +} + +fn test_error_sentinel_message() { + if _ := find(-1) { + assert false + } else { + assert err.msg() == 'error' + assert err.code() == 0 + } +} + +fn test_error_sentinel_is_stable_identity() { + // Every call returns the same cached const instance (no per-call allocation). + a := error_sentinel + b := error_sentinel + assert a.msg() == b.msg() + assert a is MessageError +} -- 2.39.5