Skip to content

Extend the concurrent stress test #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
maxburian wants to merge 1 commit into from
Closed

Extend the concurrent stress test #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions docs/audits/NEGGIA-004.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
# Audit: NEGGIA-004 — Concurrent-test fixture upgrade

## Spec Reference
../neggia/docs/specs/NEGGIA-004.md

## Symbol / File Inventory
(from neggia-archaeologist, 2026-08-18)

- `Test_XdsPluginConcurrent.cpp` (149 lines): `NUM_THREADS=16`, `CALLS_PER_THREAD=100` (:44-45); class derives from `TestDatasetArtificialSmall001` (:48) — hard-bound to the 5-frame synthetic master; frame count from `plugin_get_header`'s 6th out-param (:81-84); serial 1-based reference loop (:89-96); 16 threads × per-thread `std::mt19937(0xC0FFEE ^ t)` uniform draws over [1,total] (:106-129); bit-equality via `std::equal` over nx·ny int32 (:122-123); own `main()` (:145-149).
- `DatasetsFixture.h:18-22`: `WIDTH=11; HEIGHT=13; N_FRAMES_PER_DATASET=5`; Small001 = 1 dataset (5 frames); **Large001 = 1000 datasets → 5000 frames / 1000 external data files** (`DatasetsFixture.h:33-47`, dir verified: 1001 files).
- Eiger tests use **no fixture class** — hardcoded relative literals (`Test_XdsPluginWithData.cpp:97-151`) resolved through the build-dir symlink (`test/CMakeLists.txt:3-7`); only compile definition is the absolute `PATH_TO_XDS_PLUGIN` (:8). Eiger fixtures: 4 frames each; 2-datafile variants = 2 frames/file; eiger1 1030×1065 uint16, eiger2 1028×512 uint8/16/32; bslz4 + lz4.
- **No TEST_P precedent, and the vendored gtest is 1.9.0-dev (pre-1.10): `INSTANTIATE_TEST_SUITE_P` does not exist** — only deprecated `INSTANTIATE_TEST_CASE_P` (spec OQ3 resolved: use a fixture-path array/loop or per-case TEST_Fs, not parameterized-test macros).
- Dispatch on master (`H5ToXds.cpp:151-156, :514`): slot = raw **1-based** frame `% 16`; `plugin_get_header` pins worker 0 (:461).

## Threading Model
Test-side threads only (existing pattern). New interleaving the upgrade observes: two threads dispatched to the *same* worker slot concurrently calling `readDataset` on one immutable `H5DataCache` — per-call `Dataset` + per-call buffer (`H5ToXds.cpp:395-405`) → should stay race-free; with bslz4/lz4 eiger data the **decompression paths run under concurrency for the first time ever** (the 5-frame synthetic is effectively uncompressed).

## Call Graph
Test → dlopen'd four symbols (unchanged). No production symbol modified.

## ABI Surface Impact
**NONE** (test-only). ABI HALT does not trigger.

## Invariants
1. Zero production diff (spec inv 1) — everything under `test/`
2. **Worker-coverage math (spec OQ resolved + spec inv 2 sharpened):** with slot = 1-based-frame % 16, contiguous frames 1..16 are the minimum covering all 16 slots (slot 0 first served by frame 16). Under NEGGIA-005's planned dataset-index dispatch (`globalFrame / nframesPerDataset % K`), covering all 16 slots needs ≥16 datasets → **≥ K × N_FRAMES_PER_DATASET = 80 contiguous frames of Large001**. Decision: the Large001 case tests frames 1..N_test with **N_test = 80** — covers all 16 slots under BOTH dispatch keys (frames 1..80 mod 16 = all residues; datasets 0..15 = all slots), so the assert survives NEGGIA-005 unchanged. Expressed as an assertion on the tested frame range (N_test == 80, contiguous, 1-based), never a literal `% 16` (spec inv 2).
3. **Bounded Helgrind cost:** capping Large001 at 80 frames (not 5000) keeps the serial reference loop + stress phase small enough for NEGGIA-003's ≤15-min Helgrind budget — the per-frame Dataset re-parse anomaly (`docs/learnings/NEGGIA-001.md`, → NEGGIA-005) makes a 5000-frame loop needlessly expensive under valgrind
4. Stress-phase draws are uniform over [1, N_test]; with N_test=80 and 1600 draws, residue coverage is morally deterministic (P(miss) ≤ 16·(15/16)^1600 ≈ 6×10⁻⁴³) and the *reference loop* provides the deterministic guarantee regardless
5. Bit-equality asserts preserved per fixture (spec inv 4); reference = single-threaded serial reads of the same master
6. Large001 caveat: **no plugin-level test has ever opened it** — `number_of_frames == 5000` is expected from fixture arithmetic but unverified; the new case asserts `total_frames == 5000` before capping to 80 (turns the unknown into a checked fact)
7. **Spec erratum (inv 6):** test files are cap-EXEMPT per NEGGIA-001's recorded accounting ("test-cap-exempt", CMakeLists "framework-exempt"); the spec's self-imposed ≤50 declaration is withdrawn — the ~30-line projection stands as a guideline, and the patch may take the clean-refactor route (extract a `runStressOn(master, nTestFrames)` helper + 4 cases) rather than a duplication-minimizing contortion

## Risks
- **Medium → Low after invariant 2/3 decisions**: (i) dispatch-key survivability — closed by the 80-frame rule; (ii) Helgrind wall-time — closed by the cap; (iii) Large001 plugin-level behavior unverified — converted into an explicit assertion; (iv) eiger cases add ~4-frame stress domains (slots 1-4 only) — they are for real-compression concurrency, not coverage; the audit records that division of labor so nobody "fixes" it later.

## Existing ctest Coverage on Surface
`Test_XdsPluginConcurrent` (Small001 only, workers 1-5, no real compression); `Test_XdsPluginWithData` (eiger, single-threaded); `Test_EigerData` (dataset API, per-datafile dims — source of the 2-frames/file constants); `Test_Dataset` (Small001 + Large001 via C++ API, one frame/dataset). Gap being closed: no concurrent test on real compression, external links, or slots 0 and 6-15.

## Challenge Questions Answered
- ≥32-frame fixture exists? Yes — Large001, 5000 frames; no generation tooling needed; h5-testfiles submodule is owned by upstream `dectris` org (extending it = governance decision), so reusing Large001 also avoids a cross-org write (OQ1).
- Path plumbing? Relative literals + build-dir symlink; nothing to extend in CMake beyond (possibly) nothing at all (OQ2).
- Parameterization? No TEST_P (pre-1.10 gtest); helper + per-case TEST_F (OQ3).
- What interleaving is newly observed? Same-slot concurrent `readDataset` on real compressed multi-datafile data — precisely NEGGIA-005/007's future surface.

## Devil's Advocate
- **Strongest argument against:** capping Large001 at 80 frames means the test never exercises deep external-link fan-out (data files 17..1000) or late-frame paths, so a hypothetical bug appearing only past frame 80 (e.g., B-tree deep-node traversal under concurrency) stays invisible — the cap trades completeness for Helgrind budget.
- **Resolution:** the 80-frame window spans 16 external data files and every worker slot, which is the concurrency surface this ticket exists to observe; deep-fan-out correctness is *single-threaded* HDF5-parsing territory already covered by `Test_Dataset`'s Large001 pass over all 1000 datasets. If NEGGIA-005's cache changes fan-out behavior, its own verify step must extend the window — recorded here as a forward note for the NEGGIA-005 spec.
- **Second-order effects:** ctest wall-time grows (4 cases instead of 1, one with an 80-frame reference loop) — minutes at worst natively; the NEGGIA-003 interplay is handled by the cap. No production or ABI effect.
- **What would make this audit wrong:** if Large001's master declared nimages ≠ 5000 (invariant 6 converts this to a first-run assertion failure, not a silent wrong test); if the vendored gtest's TEST_F machinery behaved differently across the 4 added cases — implausible, same macro as today.

## Cap-Unit Projection (sum-counted)
Test files cap-exempt (NEGGIA-001 precedent; spec inv 6 erratum above). Indicative size for review: helper extraction + 4 cases ≈ 60-80 sum lines in `Test_XdsPluginConcurrent.cpp`, ~0-6 in `DatasetsFixture.h` (Large001 already exists; likely zero), 0 in CMakeLists (same target). No headers weighted.

## Audit Verdict
- **READY_FOR_PATCH**
- Rationale: scope matches the spec (test/ only); all 3 OQs resolved — the decisive finding is that Large001 already provides the ≥32-frame multi-datafile fixture, eliminating generation tooling and submodule governance entirely; the 80-frame rule makes the coverage assert survive NEGGIA-005's dispatch change and bounds Helgrind cost; the one spec erratum (cap-exemption for tests) is recorded.

## Minimal Patch Proposal

Source-line count: **cap-exempt** (all changes under `src/dectris/neggia/test/` — test additions encouraged, not penalised; NEGGIA-001 precedent). Indicative size: +121/−70 in `Test_XdsPluginConcurrent.cpp`; zero changes to `DatasetsFixture.*` (Large001 already existed) or `test/CMakeLists.txt` (same target). No headers.

### Diff
Full unified diff at **`docs/patches/NEGGIA-004.patch`** (validated `git apply --check` — clean). Human applies:
```
git apply docs/patches/NEGGIA-004.patch
```
Shape: the NEGGIA-001 stress body becomes the fixture member `runStressOn(masterPath, capFrames, expectedTotal)` (byte-preserved logic; three textual parameterizations: master path, `n_test` in place of `total_frames` for the reference size/dist, plus the cap/expected-total block after `ASSERT_GT(total_frames, 0)`); four `TEST_F` cases — original Small001, eiger1 bslz4 2-datafile, eiger2 bslz4 uint32 2-datafile, and Large001 capped at `16*5 = 80` frames with `expectedTotal=5000`.

### Per-hunk justification
1. Header note + `#include <string>` — documentation + helper signature hygiene
2. `runStressOn` helper — spec invariants 4 (bit-equality preserved verbatim), 2 (cap logic → contiguous 1..80 coverage under both dispatch keys), audit invariant 6 (`expectedTotal` pins Large001's never-before-verified plugin-level count)
3. Eiger TEST_Fs — spec invariant 3 (real bslz4 + external-link multi-datafile under ≥16 threads)
4. Large001 TEST_F — spec invariant 2 (worker coverage) + audit invariant 3 (80-frame Helgrind budget cap)

### Verification (executed 2026-08-18 in a scratch copy outside the repo — the repo's src/ untouched per skill rule 3)
- Build: clean compile, Release, master toolchain ✓
- ctest: `Test_XdsPluginConcurrent` → **4 tests ran, all passed, 2.6 s total** (Large001 cap keeps it cheap) ✓
- Large001 plugin-level `number_of_frames == 5000` — **empirically confirmed** (was audit invariant 6's unknown) ✓
- Bit-exact regression: no production change; `.so` byte-identical — trivially satisfied (skill rule 5's TSan/Helgrind/bit-exact trio applies to threading-state changes; this ticket only *observes*; the TSan+Helgrind signal lands via NEGGIA-003's lanes once both are merged)
- ABI: untouched ✓

### Verdict
- **READY_FOR_HUMAN_APPLY** (apply the patch on this branch, push — the PR's CI incl. the NEGGIA-003 lanes, if merged first, is the final gate)
216 changes: 216 additions & 0 deletions docs/patches/NEGGIA-004.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
--- a/src/dectris/neggia/test/Test_XdsPluginConcurrent.cpp
+++ b/src/dectris/neggia/test/Test_XdsPluginConcurrent.cpp
@@ -8,6 +8,12 @@
// Helgrind. Both expected to report zero races / zero errors because each
// worker's H5DataCache is thread-confined post-NEGGIA-001 (per-worker
// ownership; dispatch by frame_number % NUM_WORKERS in plugin_get_data).
+//
+// NEGGIA-004 extends the suite: the shared stress body runStressOn() runs
+// against real bslz4 external-link multi-datafile masters (eiger1/2) and
+// against Large001 frames 1..80 for full worker-slot coverage (80 = 16
+// workers x 5 frames/dataset -- covers every slot under BOTH frame-modulo
+// and NEGGIA-005's planned dataset-index dispatch).

#include <dectris/neggia/user/H5File.h>
#include <dlfcn.h>
@@ -18,6 +24,7 @@
#include <iostream>
#include <random>
#include <thread>
+#include <string>
#include <vector>
#include "DatasetsFixture.h"

@@ -68,78 +75,122 @@
plugin_close_file close_file;
int error_flag;
int info_array[1024];
+
+ // NEGGIA-004: shared stress body. capFrames > 0 limits the tested range
+ // to frames 1..capFrames (contiguous -- deterministic worker-slot
+ // coverage); expectedTotal > 0 pins the master's plugin-level frame
+ // count (Large001 was never opened at plugin level before).
+ void runStressOn(const std::string& masterPath,
+ int capFrames,
+ int expectedTotal) {
+ // 1. Open and read header (single-threaded — happens-before any concurrent
+ // get_data calls per std::thread's spawn-creates-happens-before rule).
+ open_file(masterPath.c_str(), info_array, &error_flag);
+ ASSERT_EQ(error_flag, 0);
+
+ int nx, ny, nbytes, total_frames;
+ float qx, qy;
+ get_header(&nx, &ny, &nbytes, &qx, &qy, &total_frames, info_array,
+ &error_flag);
+ ASSERT_EQ(error_flag, 0);
+ ASSERT_GT(total_frames, 0);
+ if (expectedTotal > 0) {
+ ASSERT_EQ(total_frames, expectedTotal);
+ }
+ int n_test = total_frames;
+ if (capFrames > 0) {
+ ASSERT_GE(total_frames, capFrames);
+ n_test = capFrames;
+ }
+ const size_t frame_pixels = (size_t)nx * (size_t)ny;
+
+ // 2. Build single-threaded reference: read every available frame once,
+ // serialise the int32 buffer into `reference[frame_index]`.
+ std::vector<std::vector<int32_t>> reference(n_test);
+ for (int f = 0; f < n_test; ++f) {
+ reference[f].resize(frame_pixels);
+ int frame_number = f + 1; // plugin uses 1-indexed frame numbers
+ get_data(&frame_number, &nx, &ny, reference[f].data(), info_array,
+ &error_flag);
+ ASSERT_EQ(error_flag, 0) << "reference read failed at frame " << frame_number;
+ }
+
+ // 3. Spawn NUM_THREADS workers; each calls plugin_get_data
+ // CALLS_PER_THREAD times with frame numbers picked from a per-thread
+ // deterministic random sequence (different seed per thread so frame
+ // orderings differ across threads).
+ std::atomic<int> mismatch_count{0};
+ std::atomic<int> error_count{0};
+ std::vector<std::thread> workers;
+ workers.reserve(NUM_THREADS);
+ for (int t = 0; t < NUM_THREADS; ++t) {
+ workers.emplace_back([&, t]() {
+ std::mt19937 rng(0xC0FFEE ^ t); // per-thread deterministic seed
+ std::uniform_int_distribution<int> frame_dist(1, n_test);
+ std::vector<int32_t> buffer(frame_pixels);
+ int thread_info[1024];
+ std::memset(thread_info, 0, sizeof(thread_info));
+ int thread_error = 0;
+ for (int call = 0; call < CALLS_PER_THREAD; ++call) {
+ int frame_number = frame_dist(rng);
+ get_data(&frame_number, &nx, &ny, buffer.data(), thread_info,
+ &thread_error);
+ if (thread_error != 0) {
+ ++error_count;
+ return;
+ }
+ const auto& ref = reference[frame_number - 1];
+ if (!std::equal(buffer.begin(), buffer.end(), ref.begin())) {
+ ++mismatch_count;
+ return;
+ }
+ }
+ });
+ }
+ for (auto& w : workers) {
+ w.join();
+ }
+
+ EXPECT_EQ(error_count.load(), 0)
+ << "one or more workers reported a plugin error";
+ EXPECT_EQ(mismatch_count.load(), 0)
+ << "one or more workers read frame data that differed from "
+ "single-threaded reference";
+
+ // 4. Close (single-threaded — happens-after all worker joins).
+ close_file(&error_flag);
+ ASSERT_EQ(error_flag, 0);
+ }
};

-TEST_F(TestXdsPluginConcurrent, ConcurrentGetDataMatchesSingleThreadedReference) {
- // 1. Open and read header (single-threaded — happens-before any concurrent
- // get_data calls per std::thread's spawn-creates-happens-before rule).
- open_file(getPathToSourceFile().c_str(), info_array, &error_flag);
- ASSERT_EQ(error_flag, 0);
-
- int nx, ny, nbytes, total_frames;
- float qx, qy;
- get_header(&nx, &ny, &nbytes, &qx, &qy, &total_frames, info_array,
- &error_flag);
- ASSERT_EQ(error_flag, 0);
- ASSERT_GT(total_frames, 0);
- const size_t frame_pixels = (size_t)nx * (size_t)ny;
-
- // 2. Build single-threaded reference: read every available frame once,
- // serialise the int32 buffer into `reference[frame_index]`.
- std::vector<std::vector<int32_t>> reference(total_frames);
- for (int f = 0; f < total_frames; ++f) {
- reference[f].resize(frame_pixels);
- int frame_number = f + 1; // plugin uses 1-indexed frame numbers
- get_data(&frame_number, &nx, &ny, reference[f].data(), info_array,
- &error_flag);
- ASSERT_EQ(error_flag, 0) << "reference read failed at frame " << frame_number;
- }
-
- // 3. Spawn NUM_THREADS workers; each calls plugin_get_data
- // CALLS_PER_THREAD times with frame numbers picked from a per-thread
- // deterministic random sequence (different seed per thread so frame
- // orderings differ across threads).
- std::atomic<int> mismatch_count{0};
- std::atomic<int> error_count{0};
- std::vector<std::thread> workers;
- workers.reserve(NUM_THREADS);
- for (int t = 0; t < NUM_THREADS; ++t) {
- workers.emplace_back([&, t]() {
- std::mt19937 rng(0xC0FFEE ^ t); // per-thread deterministic seed
- std::uniform_int_distribution<int> frame_dist(1, total_frames);
- std::vector<int32_t> buffer(frame_pixels);
- int thread_info[1024];
- std::memset(thread_info, 0, sizeof(thread_info));
- int thread_error = 0;
- for (int call = 0; call < CALLS_PER_THREAD; ++call) {
- int frame_number = frame_dist(rng);
- get_data(&frame_number, &nx, &ny, buffer.data(), thread_info,
- &thread_error);
- if (thread_error != 0) {
- ++error_count;
- return;
- }
- const auto& ref = reference[frame_number - 1];
- if (!std::equal(buffer.begin(), buffer.end(), ref.begin())) {
- ++mismatch_count;
- return;
- }
- }
- });
- }
- for (auto& w : workers) {
- w.join();
- }
-
- EXPECT_EQ(error_count.load(), 0)
- << "one or more workers reported a plugin error";
- EXPECT_EQ(mismatch_count.load(), 0)
- << "one or more workers read frame data that differed from "
- "single-threaded reference";
-
- // 4. Close (single-threaded — happens-after all worker joins).
- close_file(&error_flag);
- ASSERT_EQ(error_flag, 0);
+// Original NEGGIA-001 case: 5-frame synthetic master (slots 1-5 only).
+TEST_F(TestXdsPluginConcurrent,
+ ConcurrentGetDataMatchesSingleThreadedReference) {
+ runStressOn(getPathToSourceFile(), 0, 0);
+}
+
+// NEGGIA-004: real bslz4 compression + external-link multi-datafile layout
+// under 16-thread stress -- the concurrency surface NEGGIA-005/007 change.
+TEST_F(TestXdsPluginConcurrent, Eiger1Bslz4MultiDatafileConcurrent) {
+ runStressOn(
+ "h5-testfiles/datasets_eiger1/"
+ "eiger1_testmode10_2datafiles_4images_bslz4_master.h5",
+ 0, 4);
+}
+
+TEST_F(TestXdsPluginConcurrent, Eiger2Bslz4Uint32MultiDatafileConcurrent) {
+ runStressOn(
+ "h5-testfiles/datasets_eiger2/"
+ "eiger2_simread7_2datafiles_4images_bslz4_uint32_master.h5",
+ 0, 4);
+}
+
+// NEGGIA-004: full worker-slot coverage on Large001 (1000 datasets x 5
+// frames). Frames 1..80 contiguous cover all 16 slots under the current
+// frame-modulo dispatch AND under dataset-index dispatch (datasets 0..15).
+TEST_F(TestXdsPluginConcurrent, Large001AllWorkerSlotsCovered) {
+ runStressOn("h5-testfiles/dataset_artificial_large_001/test_master.h5",
+ 16 * 5, 5000);
}

int main(int argc, char** argv) {
Loading
Loading