PR 4: Comparison expansion (Threading.Channels + Nim Channel + MoodyCamel)#23
Closed
elijahr wants to merge 10 commits intofeat/bench-rollup-pr3-comparison-mvpfrom
Closed
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces three new third-party adapters to the benchmark suite: MoodyCamel's ConcurrentQueue, Nim's stdlib system.Channel, and the threading package's Chan. The changes include vendoring the MoodyCamel header, implementing C++ shims and Nim adapters, and updating CI configurations and documentation. Feedback focuses on improving the MoodycamelAdapter by adding static assertions to ensure type safety for the 64-bit payload in push and pop operations, and dynamically reflecting the generic type in the name procedure for consistency.
Comment on lines
+69
to
+72
| proc push*[T](a: var MoodycamelAdapter[T], item: T): PushResult = | ||
| if a.queue == nil: | ||
| return prFull | ||
| if mc_push(a.queue, culonglong(uint64(item))) != cint(0): |
There was a problem hiding this comment.
The MoodycamelAdapter is generic over [T], but the underlying C++ wrapper mc_push is hardcoded to uint64_t (via culonglong). The current implementation uses uint64(item), which only works for ordinal types and will fail to compile for pointers or structs. Since the benchmark harness specifically targets uint64 payloads, you should add a static assertion to ensure type compatibility and prevent misuse with unsupported types.
proc push*[T](a: var MoodycamelAdapter[T], item: T): PushResult =
static: doAssert sizeof(T) <= 8, "MoodycamelAdapter only supports types up to 8 bytes"
if a.queue == nil:
return prFull
if mc_push(a.queue, culonglong(uint64(item))) != cint(0):
Comment on lines
+88
to
+89
| proc name*[T](a: MoodycamelAdapter[T]): string = | ||
| "moodycamel/ConcurrentQueue[uint64]" |
There was a problem hiding this comment.
The name procedure hardcodes [uint64] in the string. For consistency with other adapters in this PR (like NimChannelAdapter and ThreadingChannelsAdapter), it should use the string representation of the generic type parameter $T.
proc name*[T](a: MoodycamelAdapter[T]): string =
"moodycamel/ConcurrentQueue[" & $T & "]"
Comment on lines
+79
to
+84
| proc pop*[T](a: var MoodycamelAdapter[T]): PopResult[T] = | ||
| if a.queue == nil: | ||
| return PopResult[T](success: false) | ||
| var raw: culonglong | ||
| if mc_pop(a.queue, addr raw) != cint(0): | ||
| PopResult[T](success: true, value: T(uint64(raw))) |
There was a problem hiding this comment.
Similar to the push procedure, the pop implementation performs a type conversion T(uint64(raw)) which assumes T is compatible with a 64-bit unsigned integer. A static assertion should be added here as well to ensure type safety.
proc pop*[T](a: var MoodycamelAdapter[T]): PopResult[T] =
static: doAssert sizeof(T) <= 8
if a.queue == nil:
return PopResult[T](success: false)
var raw: culonglong
if mc_pop(a.queue, addr raw) != cint(0):
PopResult[T](success: true, value: T(uint64(raw)))
This was referenced May 1, 2026
elijahr
added a commit
that referenced
this pull request
May 1, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
force-pushed
the
feat/bench-rollup-pr3-comparison-mvp
branch
from
8b7a87f to
eb67e2b
Compare
elijahr
added a commit
that referenced
this pull request
May 1, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
9738889 to
5e3d07b
Compare
Contributor
Bencher Report
elijahr
force-pushed
the
feat/bench-rollup-pr3-comparison-mvp
branch
from
eb67e2b to
daf38c5
Compare
elijahr
added a commit
that referenced
this pull request
May 2, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
5e3d07b to
3531f0c
Compare
elijahr
force-pushed
the
feat/bench-rollup-pr3-comparison-mvp
branch
from
daf38c5 to
ac54727
Compare
elijahr
added a commit
that referenced
this pull request
May 2, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
3531f0c to
44d91df
Compare
elijahr
added a commit
that referenced
this pull request
May 2, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
44d91df to
170f37f
Compare
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request expands the benchmark suite by adding three new third-party adapters: MoodyCamel's concurrentqueue (MPMC unbounded), the threading package's Chan (MPMC bounded), and Nim's stdlib system.Channel (MPSC bounded). The changes include vendoring the MoodyCamel header with a C++ wrapper, adding smoke tests, and integrating these adapters into the benchmark binaries and CI workflows. Feedback focuses on maintaining naming consistency across the adapter suite, specifically suggesting the use of the make...Adapter and cleanup naming conventions for the new modules, and ensuring the name procedure correctly reflects the generic type parameter instead of hardcoding a specific type.
| PopResult[T](success: false) | ||
|
|
||
| proc name*[T](a: MoodycamelAdapter[T]): string = | ||
| "moodycamel/ConcurrentQueue[uint64]" |
There was a problem hiding this comment.
The name procedure hardcodes uint64 in the returned string, which is misleading since the adapter is generic over T. While the underlying C++ wrapper is specialized for uint64_t, the adapter's name should reflect the type parameter T for consistency with other adapters in the suite (e.g., NimChannelAdapter and ThreadingChannelsAdapter).
"moodycamel/ConcurrentQueue[" & $T & "]"
Comment on lines
+56
to
+57
| proc initNimChannelAdapter*[T](capacity: int = 1024 | ||
| ): NimChannelAdapter[T] = |
There was a problem hiding this comment.
To maintain consistency with the existing adapters in the benchmark suite (e.g., makeLoonyAdapter, makeBoostLockfreeQueueAdapter, makeMoodycamelAdapter), it is recommended to rename this procedure to makeNimChannelAdapter.
proc makeNimChannelAdapter*[T](capacity: int = 1024
): NimChannelAdapter[T] =
| result.chan = create(Channel[T]) | ||
| result.chan[].open(if capacity < 0: 0 else: capacity) | ||
|
|
||
| proc deinitNimChannelAdapter*[T](a: var NimChannelAdapter[T]) = |
There was a problem hiding this comment.
The established pattern for adapter teardown in this repository is to use a procedure named cleanup. Renaming deinitNimChannelAdapter to cleanup will align this adapter with the others (Loony, Boost, Crossbeam, MoodyCamel) and simplify the harness logic.
proc cleanup*[T](a: var NimChannelAdapter[T]) =
Comment on lines
+48
to
+49
| proc initThreadingChannelsAdapter*[T](capacity: int = 1024 | ||
| ): ThreadingChannelsAdapter[T] = |
There was a problem hiding this comment.
For consistency with the make...Adapter naming convention used throughout the benchmark suite, consider renaming this procedure to makeThreadingChannelsAdapter.
proc makeThreadingChannelsAdapter*[T](capacity: int = 1024
): ThreadingChannelsAdapter[T] =
Comment on lines
+57
to
+58
| proc deinitThreadingChannelsAdapter*[T]( | ||
| a: var ThreadingChannelsAdapter[T]) = |
There was a problem hiding this comment.
The benchmark harness and other adapters in this project use the name cleanup for resource teardown. Renaming deinitThreadingChannelsAdapter to cleanup ensures consistency across the comparison set.
proc cleanup*[T](
a: var ThreadingChannelsAdapter[T]) =
|
|
||
| when defined(adapter_threading_channels_available): | ||
| proc initThreadingChannelsQ(capacity: int): ThreadingChannelsAdapter[uint64] = | ||
| initThreadingChannelsAdapter[uint64](capacity) |
There was a problem hiding this comment.
Update the call to match the suggested renaming of the adapter's initialization procedure.
makeThreadingChannelsAdapter[uint64](capacity)
|
|
||
| when defined(adapter_nim_channel_available): | ||
| proc initNimChannelQ(capacity: int): NimChannelAdapter[uint64] = | ||
| initNimChannelAdapter[uint64](capacity) |
There was a problem hiding this comment.
Update the call to match the suggested renaming of the adapter's initialization procedure.
makeNimChannelAdapter[uint64](capacity)
Comment on lines
+126
to
+127
| initThreadingChannelsAdapter[uint64](capacity = 4096), | ||
| deinitThreadingChannelsAdapter(adapter) |
There was a problem hiding this comment.
Update the test case to use the consistent make... and cleanup procedure names.
makeThreadingChannelsAdapter[uint64](capacity = 4096),
cleanup(adapter)
Comment on lines
+137
to
+138
| initNimChannelAdapter[uint64](capacity = 4096), | ||
| deinitNimChannelAdapter(adapter) |
There was a problem hiding this comment.
Update the test case to use the consistent make... and cleanup procedure names.
makeNimChannelAdapter[uint64](capacity = 4096),
cleanup(adapter)
aa6dd1f to
fd5b154
Compare
elijahr
added a commit
that referenced
this pull request
May 2, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
added a commit
that referenced
this pull request
May 2, 2026
…el name Per Gemini review on PR #23. 1. nim_channel_adapter: rename initNimChannelAdapter → makeNimChannelAdapter and deinitNimChannelAdapter → cleanup, matching the makeLoonyAdapter / makeBoostLockfreeQueueAdapter / makeMoodycamelAdapter / makeCrossbeamArrayQueueAdapter convention. 2. threading_channels_adapter: same rename — initThreadingChannelsAdapter → makeThreadingChannelsAdapter, deinitThreadingChannelsAdapter → cleanup. Update the docstring's example accordingly. 3. moodycamel_adapter.name: drop the hardcoded `uint64` literal in the returned string and use $T. The C++ wrapper is uint64-specialized today, but the adapter is generic; keeping the name in sync with T matches every other adapter's name proc. 4. Update three call-site groups: bench_mpmc.nim's initThreadingChannelsQ wrapper, bench_mpsc.nim's initNimChannelQ wrapper, smoke_threading_channels.nim, and t_bench_adapters.nim suite blocks. Compile-checked bench_mpsc and bench_mpmc locally.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
170f37f to
6b86349
Compare
Owner
Author
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Owner
Author
|
/review |
Code Review by Qodo
1. FFI exception escapes C ABI
|
elijahr
force-pushed
the
feat/bench-rollup-pr3-comparison-mvp
branch
from
fd5b154 to
4ce6826
Compare
elijahr
added a commit
that referenced
this pull request
May 2, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
added a commit
that referenced
this pull request
May 2, 2026
…el name Per Gemini review on PR #23. 1. nim_channel_adapter: rename initNimChannelAdapter → makeNimChannelAdapter and deinitNimChannelAdapter → cleanup, matching the makeLoonyAdapter / makeBoostLockfreeQueueAdapter / makeMoodycamelAdapter / makeCrossbeamArrayQueueAdapter convention. 2. threading_channels_adapter: same rename — initThreadingChannelsAdapter → makeThreadingChannelsAdapter, deinitThreadingChannelsAdapter → cleanup. Update the docstring's example accordingly. 3. moodycamel_adapter.name: drop the hardcoded `uint64` literal in the returned string and use $T. The C++ wrapper is uint64-specialized today, but the adapter is generic; keeping the name in sync with T matches every other adapter's name proc. 4. Update three call-site groups: bench_mpmc.nim's initThreadingChannelsQ wrapper, bench_mpsc.nim's initNimChannelQ wrapper, smoke_threading_channels.nim, and t_bench_adapters.nim suite blocks. Compile-checked bench_mpsc and bench_mpmc locally.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
6b86349 to
042f763
Compare
elijahr
added a commit
that referenced
this pull request
May 3, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
added a commit
that referenced
this pull request
May 3, 2026
…el name Per Gemini review on PR #23. 1. nim_channel_adapter: rename initNimChannelAdapter → makeNimChannelAdapter and deinitNimChannelAdapter → cleanup, matching the makeLoonyAdapter / makeBoostLockfreeQueueAdapter / makeMoodycamelAdapter / makeCrossbeamArrayQueueAdapter convention. 2. threading_channels_adapter: same rename — initThreadingChannelsAdapter → makeThreadingChannelsAdapter, deinitThreadingChannelsAdapter → cleanup. Update the docstring's example accordingly. 3. moodycamel_adapter.name: drop the hardcoded `uint64` literal in the returned string and use $T. The C++ wrapper is uint64-specialized today, but the adapter is generic; keeping the name in sync with T matches every other adapter's name proc. 4. Update three call-site groups: bench_mpmc.nim's initThreadingChannelsQ wrapper, bench_mpsc.nim's initNimChannelQ wrapper, smoke_threading_channels.nim, and t_bench_adapters.nim suite blocks. Compile-checked bench_mpsc and bench_mpmc locally.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
d55a6b6 to
f184183
Compare
Owner
Author
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request expands the benchmark suite by integrating three new third-party adapters: MoodyCamel's concurrentqueue (which is now vendored), the Nimble threading package, and Nim's standard system.Channel. The changes include the adapter implementations, corresponding smoke tests, CI workflow enhancements, and updated documentation. Review feedback highlights the need for a null check after memory allocation in the Nim channel adapter to prevent potential crashes and suggests strengthening static assertions in the MoodyCamel adapter to exclude managed types like strings and sequences, ensuring memory safety across the C++ boundary.
Comment on lines
+62
to
+63
| result.chan = create(Channel[T]) | ||
| result.chan[].open(if capacity < 0: 0 else: capacity) |
There was a problem hiding this comment.
create(Channel[T]) (which uses alloc0) can return nil if the system is out of memory. Dereferencing it immediately with result.chan[].open(...) will cause a crash. Adding a check for nil and raising an OutOfMemDefect ensures a cleaner failure mode, consistent with the error handling in the MoodyCamel adapter.
result.chan = create(Channel[T])
if result.chan == nil:
raise newException(OutOfMemDefect, "Failed to allocate memory for system.Channel")
result.chan[].open(if capacity < 0: 0 else: capacity)
Comment on lines
+71
to
+74
| assert not (T is ref), | ||
| "MoodycamelAdapter cannot transport ref types: the C++ queue " & | ||
| "bypasses Nim's GC, so refcounts wouldn't be maintained across " & | ||
| "the boundary. Use a non-ref 64-bit payload (uint64, ptr, etc)." |
There was a problem hiding this comment.
The static assertion only checks for ref types, but other managed types like string and seq are also problematic when passed through an opaque C++ queue. Since the C++ implementation bypasses Nim's GC/ARC, reference counts for these types won't be maintained, leading to memory leaks or premature deallocation. It's safer to explicitly exclude them.
assert not (T is (ref | string | seq)),
"MoodycamelAdapter cannot transport managed types (ref, string, seq): " &
"the C++ queue bypasses Nim's GC, so memory wouldn't be maintained " &
"across the boundary. Use a non-managed 64-bit payload (uint64, ptr, etc)."
f76b84c to
b233388
Compare
elijahr
added a commit
that referenced
this pull request
May 3, 2026
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
elijahr
added a commit
that referenced
this pull request
May 3, 2026
…el name Per Gemini review on PR #23. 1. nim_channel_adapter: rename initNimChannelAdapter → makeNimChannelAdapter and deinitNimChannelAdapter → cleanup, matching the makeLoonyAdapter / makeBoostLockfreeQueueAdapter / makeMoodycamelAdapter / makeCrossbeamArrayQueueAdapter convention. 2. threading_channels_adapter: same rename — initThreadingChannelsAdapter → makeThreadingChannelsAdapter, deinitThreadingChannelsAdapter → cleanup. Update the docstring's example accordingly. 3. moodycamel_adapter.name: drop the hardcoded `uint64` literal in the returned string and use $T. The C++ wrapper is uint64-specialized today, but the adapter is generic; keeping the name in sync with T matches every other adapter's name proc. 4. Update three call-site groups: bench_mpmc.nim's initThreadingChannelsQ wrapper, bench_mpsc.nim's initNimChannelQ wrapper, smoke_threading_channels.nim, and t_bench_adapters.nim suite blocks. Compile-checked bench_mpsc and bench_mpmc locally.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
f184183 to
d1ac052
Compare
Owner
Author
|
/ai-review |
|
✅ Momus review posted — verdict APPROVE, 1 finding
|
Owner
Author
|
/ai-review |
2 similar comments
Owner
Author
|
/ai-review |
Owner
Author
|
/ai-review |
![axiomantic-momus[bot]](https://avatars.githubusercontent.com/u/230757868?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR adds three new queue adapters (MoodyCamel, threading.Chan, system.Channel) with CI soft-skip wiring, smoke tests, and bench-binary dispatch. The code is well-crafted, handles edge cases (null queue, failed init, cross-ABI exceptions, 32-bit capacity clamping), and follows existing adapter conventions. Test coverage exercises basic push/pop round-trips but — consistently with prior adapters — no concurrent multi-threaded unit tests. No blocking issues found.
Severity tally: 1 Low.
Low
- BOT-A1 (
benchmarks/nim/adapters/threading_channels_adapter.nim:20): Misleading docstring: references non-existentdeinitproc
Noteworthy
- MoodyCamel wrapper correctly handles cross-ABI exceptions by catching all C++ exceptions and returning nullptr, preventing undefined behavior at the C ABI boundary.
- Capacity edging in moodycamel_wrapper.cpp handles 32-bit SIZE_MAX clamping, avoiding wrap-around on ILP32 targets.
- Compile-time assertions in moodycamel_adapter.nim (sizeof(T)==8, T not ref) catch misuse at build time rather than at runtime.
Verdict: APPROVE.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
Comment on lines
+20
to
+24
| ## Memory management: ``Chan[T]`` is a reference-counted object that | ||
| ## owns its underlying buffer. We hold it as a value field on the | ||
| ## adapter so the destructor runs automatically when the adapter goes | ||
| ## out of scope; the optional ``deinit`` proc explicitly tears down a | ||
| ## migrated value before the adapter is dropped. |
There was a problem hiding this comment.
BOT-A1 — Low (quality)
Misleading docstring: references non-existent deinit proc
The module-level Memory management section states 'the optional deinit proc explicitly tears down a migrated value before the adapter is dropped'. The adapter defines no deinit proc — only a cleanup proc that is explicitly a no-op. The cleanup proc's own docstring correctly explains that Chan[T] self-cleans via its hooks. The module-level comment likely intended to describe Nim's general =destroy mechanism but reads as if the adapter itself provides a deinit procedure.
elijahr
force-pushed
the
feat/bench-rollup-pr3-comparison-mvp
branch
from
b233388 to
41a08f3
Compare
- benchmarks/vendor/concurrentqueue/{concurrentqueue.h,LICENSE.md,README.md,moodycamel_wrapper.cpp}: vendor MoodyCamel ConcurrentQueue at upstream commit d655418bb644b7f85159d94c591d7d983949fb81 (BSD-2-Clause / Boost dual). The single-header library is shimmed by an extern "C" wrapper exposing mc_init / mc_push / mc_pop / mc_destroy for uint64_t so the Nim adapter consumes plain importc, not the upstream template machinery.
- benchmarks/nim/adapters/moodycamel_adapter.nim: nim cpp adapter, topologiesSupported = {tMpmcUnbounded}, gated on -d:adapter_moodycamel_available.
- benchmarks/nim/adapters/threading_channels_adapter.nim: nimble threading.Chan[T] adapter, topologiesSupported = {tMpmc}, non-blocking trySend/tryRecv, gated on -d:adapter_threading_channels_available.
- benchmarks/nim/adapters/nim_channel_adapter.nim: stdlib system.Channel[T] adapter, topologiesSupported = {tMpsc}, blocking-on-full producer / non-blocking consumer (apples-to-oranges fairness caveat documented inline), gated on -d:adapter_nim_channel_available.
- benchmarks/nim/smoke/smoke_moodycamel.nim, smoke_threading_channels.nim: round-trip smokes for the new adapters.
- tests/t_bench_adapters.nim: extend with the three new gates.
- bench_mpsc.nim: when declared(initNimChannelQ) -> nim_channel/mpsc/<P>p1c slugs (P in {1,2,4}). Uses runThroughputHarness; producer side blocks on full per system.Channel semantics (fairness caveat documented in adapter file).
- bench_mpmc.nim: when declared(initThreadingChannelsQ) -> threading_channels/mpmc/<P>p<C>c slugs ({1,2,4}x{1,2,4}). Uses runThroughputHarness; non-blocking trySend / tryRecv.
- bench_unbounded.nim: when declared(initMoodycamelQ) -> moodycamel/ConcurrentQueue/mpmc_unbounded/<P>p<C>c slugs ({1,2,4}x{1,2,4}). Uses runThroughputHarness; vendored single-header + extern "C" shim, nim cpp only.
Verified by per-variant smoke runs at 1000 messages: each variant emits the full expected slug set. Default builds without any gate continue to compile clean and emit the existing pre-PR-4 slug set.
- workflow_dispatch inputs: force_skip_moodycamel, force_skip_threading_channels, force_skip_nim_channel. - env vars: FORCE_SKIP_MOODYCAMEL / FORCE_SKIP_THREADING_CHANNELS / FORCE_SKIP_NIM_CHANNEL gate the per-adapter steps. - MoodyCamel (bench_unbounded only): vendored-header presence check + nim cpp smoke; requires nim cpp -> sets NIM_MODE=cpp via ADAPTER_MOODYCAMEL. - threading.Chan (bench_mpmc only): nimble install threading + smoke; nim c. - system.Channel (bench_mpsc only): no install (stdlib); smoke via t_bench_adapters.nim under -d:adapter_nim_channel_available. - bench_mpsc compile step now consumes ADAPTER_FLAGS so nim_channel actually wires in (previously left blank because there was no MVP adapter for MPSC). - bench_unbounded compile step switches from hard-coded `nim c` to $NIM_MODE so MoodyCamel can request `nim cpp` while loony continues to be mode-neutral. actionlint clean.
- THIRD_PARTY_LICENSES.md: drop the 'Future entries (PR 4 / PR 5)' placeholder for concurrentqueue; add the real Comparison-expansion section with concrete blocks for concurrentqueue (vendored at d655418bb644b7f85159d94c591d7d983949fb81, BSD/Boost dual), nimble threading (MIT, unvendored), and Nim system.Channel (MIT, stdlib). - .gitattributes: new file with benchmarks/vendor/** linguist-vendored=true linguist-generated=true so the vendored MoodyCamel header doesn't skew GitHub language stats. Verified via git check-attr. - benchmarks/README.md: rename 'Comparison MVP — third-party adapters' to 'Comparison libraries' and extend the table from 5 to 8 rows (Loony, Boost x2, Crossbeam x2, MoodyCamel, threading.Chan, system.Channel). Add per-adapter local-run examples for the 3 new adapters. Add the apples-to-oranges fairness asterisk for system.Channel's blocking-on-full producer. - CHANGELOG.md: prepend the PR-4 / Track-4 'Added' bullets to [Unreleased] covering the four new adapters, vendored MoodyCamel, .gitattributes rule, three new force_skip flags, t_bench_adapters extension, and bench binary slug coverage.
merge_bmf.py rejects slugs that aren't <library>/<topology>/<P>p<C>c (exactly 3 segments). PR #23's CI failed on the moodycamel slug: invalid slug shape 'moodycamel/ConcurrentQueue/mpmc_unbounded/1p1c' The same 4-segment violations exist for loony and crossbeam_seg_queue in bench_unbounded.nim, and the crossbeam_array_queue dispatch was emitting a colliding "crossbeam_queue" lib name. Apply the canonical lib slugs from the design doc so all dispatch sites yield 3 segments after the runner appends "/<topology>/<shape>": loony/LoonyQueue -> loony crossbeam_queue/SegQueue -> crossbeam_seg_queue crossbeam_queue (Array) -> crossbeam_array_queue moodycamel/ConcurrentQueue -> moodycamel The PR #22 worktree applies the loony and crossbeam fixes too; git rebase will harmonize the duplicate edits. The adapter `name` constants (e.g. "moodycamel/ConcurrentQueue[uint64]") are human-readable echo strings, not BMF slugs, and stay as-is.
…el name Per Gemini review on PR #23. 1. nim_channel_adapter: rename initNimChannelAdapter → makeNimChannelAdapter and deinitNimChannelAdapter → cleanup, matching the makeLoonyAdapter / makeBoostLockfreeQueueAdapter / makeMoodycamelAdapter / makeCrossbeamArrayQueueAdapter convention. 2. threading_channels_adapter: same rename — initThreadingChannelsAdapter → makeThreadingChannelsAdapter, deinitThreadingChannelsAdapter → cleanup. Update the docstring's example accordingly. 3. moodycamel_adapter.name: drop the hardcoded `uint64` literal in the returned string and use $T. The C++ wrapper is uint64-specialized today, but the adapter is generic; keeping the name in sync with T matches every other adapter's name proc. 4. Update three call-site groups: bench_mpmc.nim's initThreadingChannelsQ wrapper, bench_mpsc.nim's initNimChannelQ wrapper, smoke_threading_channels.nim, and t_bench_adapters.nim suite blocks. Compile-checked bench_mpsc and bench_mpmc locally.
mc_init was an extern "C" function that constructed the queue with plain `new`, so an allocation failure (or any other constructor exception) would propagate across the C ABI boundary — undefined behavior. Switch to `new (std::nothrow)` and wrap a try/catch around the construction so init failure becomes a clean nullptr return. The Nim adapter's makeMoodycamelAdapter now validates the returned handle and raises OutOfMemDefect when mc_init returns nullptr. Without this check a failed init would manifest as the producer thread spin-retrying `push` with prFull (queue == nil branch) forever, turning init failure into an opaque benchmark hang.
moodycamel_adapter.nim:
- push/pop now use cast[uint64](item) / cast[T](uint64(raw)) instead
of value conversions. For T=uint64 this is identical, but it lets
the adapter round-trip non-numeric 64-bit payloads (pointers, refs,
distinct-int aliases) by their bit pattern.
moodycamel_wrapper.cpp:
- Clamp the uint64 capacity hint to SIZE_MAX before casting so on
32-bit hosts (size_t == uint32) a >4 GiB value doesn't truncate to
a tiny number. Bench CI is x86_64 and never exercises the clamp,
but the wrapper is exposed via the public concurrentqueue header
so the safer cast is worth carrying. Add <cstddef> for SIZE_MAX.
The C++ wrapper is hardcoded to uint64_t. The push/pop sites use cast[uint64](item) and cast[T](uint64(raw)) to round-trip the bit pattern, but those casts only behave when T is exactly 8 bytes. Add a static assertion in makeMoodycamelAdapter to fail fast at the instantiation site rather than corrupting state at run time. Also reject T=ref: the C++ queue is opaque to Nim's GC, so a refcount bump on push would never happen and the underlying object could be collected before pop. Restrict to integer / ptr-like 64-bit payloads.
…it proc The adapter never exposes a deinit proc; cleanup() is the no-op shape- parity stub and =destroy on Chan[T] does the actual buffer reclamation.
elijahr
force-pushed
the
feat/bench-rollup-pr4-comparison-expansion
branch
from
d1ac052 to
9b3e9f2
Compare
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Track 4 of the bench-rollup feature. Extends the comparison set from
PR 3's 5 adapters to 9 (across 7 upstream libraries) so each topology
has 3-or-more distinct implementations plotted on the Bencher
dashboard. The vendored MoodyCamel header lands the first vendored
third-party entry in
THIRD_PARTY_LICENSES.mdand proves thevendoring pattern for PR 5's uPlot bundle.
Depends on PR #19, PR #20, PR #21, PR #22.
Adapters added (4)
moodycamel_adapter.nim— MoodyCamelConcurrentQueue(BSD/Boostdual,
mpmc_unbounded). Vendored single-header atbenchmarks/vendor/concurrentqueue/concurrentqueue.h(pinned toupstream commit
d655418bb644b7f85159d94c591d7d983949fb81); athin
extern "C"shim (moodycamel_wrapper.cpp) isolates Nim fromupstream's template machinery (Risk M5).
threading_channels_adapter.nim— nimblethreadingpackage'sChan[T](MIT,mpmcbounded). Non-blockingtrySend/tryRecv.nim_channel_adapter.nim— Nim stdlibsystem.Channel[T](MIT,mpscbounded). Blocking-on-full producer; the apples-to-orangesfairness caveat is documented inline and the chart legend marks
the slug accordingly.
Plus a smoke for each new third-party adapter
(
smoke_moodycamel.nim,smoke_threading_channels.nim) and a thirdgate in
tests/t_bench_adapters.nimcovering 1000-item push/popround-trip set equality.
Bench wiring
bench_mpmc.nimemitsthreading_channels/mpmc/{1,2,4}p{1,2,4}c(9 shapes) when
-d:adapter_threading_channels_available.bench_mpsc.nimemitsnim_channel/mpsc/{1,2,4}p1c(3 shapes)when
-d:adapter_nim_channel_available. New: bench_mpsc compilestep now consumes
ADAPTER_FLAGS(it was unused in PR 3 becauseno MVP adapter targeted MPSC).
bench_unbounded.nimemitsmoodycamel/ConcurrentQueue/mpmc_unbounded/{1,2,4}p{1,2,4}c(9shapes) when
-d:adapter_moodycamel_available. Compile stepswitches from hard-coded
nim cto\$NIM_MODEso MoodyCamel canrequest
nim cppwhile loony continues to be mode-neutral.Default builds without any gate continue to compile clean and emit
the existing pre-PR-4 slug set; verified by per-binary smoke runs at
1000 messages.
CI
bench.ymlextends with three newworkflow_dispatchboolean inputs(
force_skip_moodycamel/force_skip_threading_channels/force_skip_nim_channel) and per-library install → smoke → set-flagpipelines mirroring the PR-3 soft-skip pattern (design §2.6).
MoodyCamel's "install" step is a
test -fagainst the vendoredheader so the bench is reproducible without network egress.
actionlintclean.Cross-cutting
THIRD_PARTY_LICENSES.mdlands its first vendored entry(
concurrentqueue (MoodyCamel), BSD/Boost dual, pinned SHAd655418bb644b7f85159d94c591d7d983949fb81) plus unvendoredentries for the nimble
threadingpackage and Nimsystem.Channel..gitattributesrulebenchmarks/vendor/** linguist-vendored=true linguist-generated=trueexcludes the vendored MoodyCamel header from GitHub language stats.
Verified via
git check-attr.benchmarks/README.mdcomparison table extends from 5 to 8 rowswith install commands and the apples-to-oranges fairness asterisk
for
nim_channel/*.CHANGELOG.md[Unreleased]gains the PR-4 / Track-4 bullets.Verification
nimble test: 200 OK / 0 FAILED / 0 SKIPPED../.tmp/smoke_moodycamel→ 32 push/pop OK../.tmp/smoke_threading_channels→ 32 push/pop OK.t_bench_adaptersround-trip: all three new gates(
adapter_moodycamel_availableundernim cpp,adapter_threading_channels_available,adapter_nim_channel_available) pass1 OK / 0 FAILED.expected slug is present in the BMF output.
Force-skip evidence
The three new
force_skip_<lib>workflow_dispatchinputs are wiredthrough to the per-library install step's
if:guard the same wayPR 3's
force_skip_boost/force_skip_loonyare wired. They haveNOT been exercised on
workflow_dispatchfrom this PR yet becausethe workflow runs against the merge target on push; once this PR is
merged the maintainer will run
workflow_dispatchwith each of thethree flags set in turn and record the resulting
::warning title= Adapter skipped::...annotations as evidence on the next release.