26028: Reduces threadpool overhead - #648
Conversation
howso-mario
left a comment
There was a problem hiding this comment.
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.
howso-mario
left a comment
There was a problem hiding this comment.
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.
howso-mario
left a comment
There was a problem hiding this comment.
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.
howso-mario
left a comment
There was a problem hiding this comment.
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.
No description provided.