From 59da6c6b964dbffbd81c66dedb271a003259a1d7 Mon Sep 17 00:00:00 2001 From: "KOKO\\Mimi" Date: Sun, 9 Aug 2026 19:06:24 +0900 Subject: [PATCH] feat(linear-static-3d-euler-beam): step 17 - parallel-for-tbb-review-fix --- ...tic-3d-euler-beam-implementation-report.md | 35 +++++++++++++ tests/unit/assembly/parallel_for_test.cpp | 52 +++++++++++++------ 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/docs/implementation-plans/linear-static-3d-euler-beam-implementation-report.md b/docs/implementation-plans/linear-static-3d-euler-beam-implementation-report.md index 0bd163b..c59c65f 100644 --- a/docs/implementation-plans/linear-static-3d-euler-beam-implementation-report.md +++ b/docs/implementation-plans/linear-static-3d-euler-beam-implementation-report.md @@ -681,3 +681,38 @@ - handoff: Step 18 can inject either backend for independent element-local computation into caller-owned stable element-order slots before its separate deterministic COO-to-CSR reduction. + +### Step 17 Review Fix Round 1 — atomic test evidence + +- classification: test-evidence reliability hardening; no production defect + was observed or corrected. +- baseline_GREEN: before changing the test, + `ctest --test-dir .harness/build -C Debug -R ParallelFor --output-on-failure` + exited 0 with the existing production adapter passing 3/3. Because the + review changes only how a test observes duplicate callbacks, no + implementation-owned RED was expected or manufactured. +- reliability_issue: the prior `std::vector` scalar writes were race-free only + if the exact-once contract was already true. A duplicate concurrent callback + for one index could write the same non-atomic scalar concurrently, making the + negative case undefined instead of a deterministic test failure. +- correction: T17-PFOR-001 now uses an explicitly initialized atomic zero-count + counter and explicitly initialized per-index atomic visit counters. + T17-PFOR-002 uses explicitly initialized atomic serial/TBB output slots plus + per-index atomic visit counters. All callback increments use + `fetch_add(1, memory_order_relaxed)` and post-execution assertions require + every count to equal one, so duplicate same-index callbacks are observed + without a data race. Output stores/loads are relaxed atomics because only + per-slot observation is required after the blocking adapter returns. +- scope: only `tests/unit/assembly/parallel_for_test.cpp`, this cumulative + Step 17 section, and the ignored `task-17-report.md` changed. Production, + CMake, the phase index, upstream contracts, and reference artifacts were not + modified in this review round; the exact three test names remain unchanged. + +| stage | exact command | exit_code | observed_result | +| --- | --- | ---: | --- | +| REVIEW1-baseline-GREEN | `ctest --test-dir .harness/build -C Debug -R ParallelFor --output-on-failure` | 0 | existing correct production passed 3/3 before test hardening; no implementation RED expected | +| REVIEW1-VERIFY-build | `cmake --build .harness/build --config Debug` | 0 | C++17 atomic test evidence compiled and linked under MSVC `/W4 /WX` without a warning | +| REVIEW1-VERIFY-targeted | `ctest --test-dir .harness/build -C Debug -R ParallelFor --output-on-failure` | 0 | hardened exact three ParallelFor tests passed 3/3 | +| REVIEW1-VERIFY-discovery | `ctest --test-dir .harness/build -C Debug --show-only=json-v1` | 0 | 42 tests discovered, including the unchanged three exact ParallelFor names | +| REVIEW1-VERIFY-full | `ctest --test-dir .harness/build -C Debug --output-on-failure` | 0 | full accumulated suite passed 42/42 | +| REVIEW1-VERIFY-scans | public TBB leak, production scope, atomic reliability, reference, and diff/whitespace scans | 0 | public leaks 0; forbidden production scope 0; atomic vectors 5; relaxed duplicate counters 4; legacy non-atomic visit/output paths 0; reference unchanged; diff clean | diff --git a/tests/unit/assembly/parallel_for_test.cpp b/tests/unit/assembly/parallel_for_test.cpp index ee2d57f..f59f548 100644 --- a/tests/unit/assembly/parallel_for_test.cpp +++ b/tests/unit/assembly/parallel_for_test.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -28,43 +29,64 @@ TEST(ParallelFor, ZeroOneManyExecuteExactlyOnce) { const TbbParallelFor tbb; for (const ParallelFor& parallelFor : parallelForBackends(serial, tbb)) { - bool zeroBodyCalled = false; - parallelFor.execute(0U, [&zeroBodyCalled](std::size_t) { - zeroBodyCalled = true; + std::atomic zeroVisits{0U}; + parallelFor.execute(0U, [&zeroVisits](std::size_t) { + zeroVisits.fetch_add(1U, std::memory_order_relaxed); }); - EXPECT_FALSE(zeroBodyCalled); + EXPECT_EQ(zeroVisits.load(std::memory_order_relaxed), 0U); for (const std::size_t count : {1U, 257U}) { - std::vector visits(count, 0); + std::vector> visits(count); + for (auto& visit : visits) { + visit.store(0U, std::memory_order_relaxed); + } parallelFor.execute(count, [&visits](std::size_t index) { - ++visits[index]; + visits[index].fetch_add(1U, std::memory_order_relaxed); }); - EXPECT_EQ(visits, std::vector(count, 1)); + for (std::size_t index = 0; index < count; ++index) { + EXPECT_EQ(visits[index].load(std::memory_order_relaxed), 1U); + } } } } TEST(ParallelFor, SerialAndTbbProduceStableIndexedOutput) { constexpr std::size_t count = 1024U; - std::vector serialOutput(count, 0U); - std::vector tbbOutput(count, 0U); + std::vector> serialOutput(count); + std::vector> tbbOutput(count); + std::vector> serialVisits(count); + std::vector> tbbVisits(count); + for (std::size_t index = 0; index < count; ++index) { + serialOutput[index].store(0U, std::memory_order_relaxed); + tbbOutput[index].store(0U, std::memory_order_relaxed); + serialVisits[index].store(0U, std::memory_order_relaxed); + tbbVisits[index].store(0U, std::memory_order_relaxed); + } const auto valueForIndex = [](std::size_t index) { return (index + 17U) * (index + 3U); }; const SerialParallelFor serial; - serial.execute(count, [&serialOutput, &valueForIndex](std::size_t index) { - serialOutput[index] = valueForIndex(index); + serial.execute(count, [&serialOutput, &serialVisits, &valueForIndex](std::size_t index) { + serialOutput[index].store(valueForIndex(index), std::memory_order_relaxed); + serialVisits[index].fetch_add(1U, std::memory_order_relaxed); }); const TbbParallelFor tbb; - tbb.execute(count, [&tbbOutput, &valueForIndex](std::size_t index) { - tbbOutput[index] = valueForIndex(index); + tbb.execute(count, [&tbbOutput, &tbbVisits, &valueForIndex](std::size_t index) { + tbbOutput[index].store(valueForIndex(index), std::memory_order_relaxed); + tbbVisits[index].fetch_add(1U, std::memory_order_relaxed); }); - EXPECT_EQ(tbbOutput, serialOutput); for (std::size_t index = 0; index < count; ++index) { - EXPECT_EQ(tbbOutput[index], valueForIndex(index)); + EXPECT_EQ(serialVisits[index].load(std::memory_order_relaxed), 1U); + EXPECT_EQ(tbbVisits[index].load(std::memory_order_relaxed), 1U); + EXPECT_EQ( + tbbOutput[index].load(std::memory_order_relaxed), + serialOutput[index].load(std::memory_order_relaxed)); + EXPECT_EQ( + tbbOutput[index].load(std::memory_order_relaxed), + valueForIndex(index)); } }