Repository navigation
test: make two GIL heartbeat workloads clear the 30 ms floor in release builds - #394
Conversation
…lds (#180) In a release build, test_count_releases_gil (7-10 ms) and test_search_pattern_count_parquet_parallel_releases_gil (17-18 ms) finished under the 30 ms MIN_ELAPSED_S floor, so the heartbeat self-check failed before it could assess the GIL. Both calls split their work across the fixture's eight row groups, so they get faster with each core up to eight. count() now counts rows past a shift window, which runs over the whole table in one partition. The parallel pattern scan now reads a one-row-group copy of the file, where it runs as one task, and adds a `value` term to every step, which takes it off the starts_with fast path onto per-row predicate evaluation, still on the parallel Parquet path. The other four tests the issue lists (pivot, insert, delete, modify) already clear the floor on current main after their workloads changed in b5e02bd.
AI Code Review — Round 1Reviewed HEAD: FindingsNone. SpecThe revised workloads address the release-build timing failures without lowering the 30 ms floor or weakening the heartbeat assertion. The count workload requires window evaluation, and the pattern workload remains on the Parquet scan path while avoiding row-group parallel speedup. All six originally reported workloads cleared the floor locally. StandardsConsistent with AGENTS.md, CLAUDE.md, the existing test structure, and ADR 0016. This test-only change introduces no public behavior requiring changelog, docstring, or stub updates. No reviewed files were modified. AssessmentNo correctness, security, or actionable coverage defects found. The change is narrowly scoped, preserves the GIL-release detector, and includes a semantic equivalence assertion for the heavier pattern predicates. ObservationsThe supplied PR context matches the bound HEAD and base main. All supplied CI checks were pending; no controller validation commands were configured. Local timing measurements establish margin on this machine, not a universal hardware guarantee. VerificationRead the full bound diff, issue, PR description, repository guidance, and related Python/Rust execution paths. The affected module passed all 19 tests with the existing debug extension in 27.89 seconds. Loading the existing release extension directly without replacing files produced 19 passes in 3.04 seconds, then another 19 passes in 3.14 seconds. Instrumented release calls measured count at 121.0 ms, pattern count at 112.3 ms, pivot at 55.2 ms, insert at 57.3 ms, delete at 55.5 ms, and modify at 50.5 ms. git diff --check passed. Final worktree status was clean and HEAD remained the bound commit. SummaryNo findings; another fix round is not needed. Debug and release testing supports the intended fix. Full CI results remain pending. Needs another fix round: NO |
Why
test_gil_release.pyrefuses to judge a call shorter thanMIN_ELAPSED_S(30 ms), because the heartbeat thread cannot tick enough in that time even with the GIL released. CI builds the extension in debug mode, where every workload is slow enough. In a release build, the build users install and the one a release-build test job (#328) would run, some calls finished below the floor and the self-check failed. The GIL was never the problem.What still failed on current
mainThe issue was filed before
b5e02bdchanged several workloads, so I rebuilt withmaturin develop --releaseand measured each test (5 runs, 8-core Linux):test_count_releases_giltest_search_pattern_count_parquet_parallel_releases_giltest_pivot_releases_giltest_insert_row_releases_giltest_delete_row_releases_giltest_modify_row_releases_gilThe last four already clear the floor, so they are left alone. The other 13 heartbeat tests take 70–320 ms.
The change
Both failing calls split their work across the fixture's eight row groups, so they run faster with each core up to eight, and a bigger machine would shrink them further. The replacement workloads do work that does not split that way:
shift(1)derive and filter. The shift is a window over the whole table in one partition, whichcount()has to evaluate. The test is still the control forcount()running detached.single_rg_parquet_path), so the per-row-group scan is a single task. It also adds& (r.value >= 0.0)to every step, which takes the scan off thestarts_withfast path (prefix matching onurlalone) and onto per-row evaluation of every step's predicate over two columns. The terms match every row, so the test also asserts the count equals the plain funnel's. These predicates stay on the parallel Parquet path: over the eight-row-group file they take ~27 ms, while the DataFusion fallback takes ~100 ms even on in-memory rows.The module docstring now says that workloads must clear the floor in a release build too, and on machines with more cores.
Deriving the floor from a calibration run would not fix this: a call as short as the old count call leaves the heartbeat only a handful of ticks, too close to the at most ~2 it makes while the GIL is held.
Testing
maturin develop --release, thenpytest py-ltseq/tests/test_gil_release.py, 5 runs: 19 passed each time (timings above).maturin develop(debug, as CI builds), thenpytest py-ltseq/tests/test_gil_release.py -v: 19 passed in 29.7 s; the slowest test took 3.9 s.Closes #180