Skip to content

26028: Reduces threadpool overhead - #648

Merged
howsohazard merged 9 commits into
mainfrom
26028-threadpool-overhead
Sep 22, 2026
Merged

howsohazard merged 9 commits into
mainfrom
26028-threadpool-overhead

Conversation

@howsohazard

Copy link
Copy Markdown
Contributor

No description provided.

@howsohazard
howsohazard requested review from a team as code owners September 21, 2026 21:33

@howso-mario howso-mario left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 944d7fd501e0f3c6c3b4f12babffb970eb6a5c01. The current small-buffer task wrapper has blocking portability and lifetime defects (inline comments), and the PR does not remove the reported per-task queue-mutex contention: producers and every worker still serialize on threadsMutex around the FIFO, while task destruction occurs after the worker re-locks it. Linux/Windows CI lanes pass, but macOS fails and final-check is red. I also ran an exact-head GCC 14 ASan/UBSan probe: FIFO, 20,000 exactly-once jobs, 12-job low concurrency, 40 reuse batches, resizing, and shutdown passed; a std::string capture failed with stack-use-after-scope and an alignas(64) callable failed with a misaligned store. I do not recommend adding a speculative lock-free queue here without the Engine workflow benchmarks and a complete wakeup/reclamation design; first make the wrapper portable/safe, destroy dequeued tasks before relocking, and benchmark a reusable mutex-protected ring/count-limited wakeup experiment against this exact head.

Comment thread src/Amalgam/ThreadPool.h Outdated
Comment thread src/Amalgam/ThreadPool.h Outdated
Comment thread src/Amalgam/ThreadPool.h Outdated

@howso-mario howso-mario left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head e930e84e4bb667f2415cece53b019d66682a10bb. The AppleClang initialization blocker is fixed, and the earlier bytewise-lifetime UB is no longer reproduced for ordinary captures. However, the new alignment gate disables SBO for typical tasks, and the type-erased relocation hook copies from a const source inside noexcept Task moves. Focused GCC 14 ASan/UBSan probes show align-8 string and unique_ptr captures always allocate; an align-1 move-only capture fails to compile; and a throwing inline copy calls std::terminate through move construction, move assignment, and public EnqueueTask. The public variadic/result-returning APIs also remain removed. Current CI is green at this exact head, including macOS and final-check, but it does not exercise these cases. See inline comments. I am not proceeding with the separate queue redesign on this insufficient head.

Comment thread src/Amalgam/ThreadPool.h Outdated
Comment thread src/Amalgam/ThreadPool.h
Comment thread src/Amalgam/ThreadPool.h

@howso-mario howso-mario left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected exact-head re-review of 34322edfa35c1c739a4be2548e91f74cd860871b.

Correction to my prior review: I withdraw the public/future-return API compatibility finding. Removal is intentional, and a repository-wide check finds EnqueueTaskWithResult / BatchEnqueueTaskWithResult only as declarations in the base header and no in-repo call sites; this is not a blocker.

The earlier AppleClang initialization, storage-alignment gate, and noexcept-relocation eligibility issues are addressed. However, this head introduces one remaining blocking compile defect in the relocation thunk (inline comment): MoveFunc now expects (void *, void *), but the assigned lambda still takes (void *, const void *) and then casts away const. GCC 14 rejects every eligible nontrivially-copyable inline capture tested, including alignment-1 nothrow move-only, std::string, and std::unique_ptr captures. Aggregate CI can miss this because current in-repo captures are trivially copyable and do not instantiate the thunk.

Focused GCC 14 C++20 ASan/UBSan evidence: over-aligned and potentially-throwing-move heap fallbacks passed; controlled one-worker FIFO, 12-task batches, dynamic increase/decrease, shutdown/join, and 100 reuse batches / 1,200 exactly-once tasks passed without sanitizer diagnostics. The three inline nontrivial cases fail at compile time at lines 374-375.

Verdict remains CHANGES_REQUESTED solely for that compile blocker. I am not proceeding with the separate queue redesign or Engine benchmark campaign on this head.

Comment thread src/Amalgam/ThreadPool.h Outdated

@howso-mario howso-mario left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head re-review of da4c8500d3693ac7a7d9c9941d69a006a386aca3: APPROVE / no blocking findings for the Task-wrapper changes. The latest commit changes the relocation lambda source from const void * to mutable void *, matching MoveFunc and enabling a true nothrow move. GCC 14.2 C++20 ASan+UBSan verification passed for alignment-1 nothrow move-only, std::string, and std::unique_ptr inline captures; over-aligned and potentially-throwing-move heap fallbacks; controlled one-worker FIFO; 12-task batches; 120 reuse batches / 1,440 exactly-once tasks; dynamic increase/decrease; and shutdown/join. Both normal and -fno-elide-constructors builds passed three runs each; the prior const-source header fails the same harness as expected. AppleClang initialization, alignment eligibility, and noexcept-relocation findings remain resolved. The intentional unused future/variadic API removal remains accepted. CI run 35742151906 is fully green across Linux amd64/glibc-2.28/arm64/arm64_8a, WASM release/debug, macOS, Windows, smoke tests, docs regeneration, and final-check; release-only jobs are expected skips. Independent Claude/Fable 5.1 read-only review found no blocker. Residual non-blocking risks: current production captures may not instantiate the nontrivial inline move path, Task is 144 bytes on GCC x86-64, and task destruction still occurs after reacquiring threadsMutex as before.

@howsohazard
howsohazard merged commit 144d671 into main Sep 22, 2026
24 checks passed
@howsohazard
howsohazard deleted the 26028-threadpool-overhead branch September 22, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants