Conversation
… threading Extends the prior fix (49ed886, cb/BUG-003) with a full redesign per review feedback. Coupling is no longer set up once at buffer-construction time; `couple_buffers` is now called internally, once per `work!` call, sourcing each coupled partner directly from whatever `CoupledSimulations` is supplied. Threaded coupled work is now supported (previously fell back to sequential): each task reuses its coupled partner's own per-task buffer directly - no partner content is copied - so partner and primary task counts must match (ArgumentError otherwise). Each reused partner TaskLocals is scattered once per work! call so a threaded partner's Δt can't go stale from reuse. AutoDiffCellBuffer reuses the existing "rebuild only if type changed" pattern from _replace_material_with, avoiding repeated JacobianConfig rebuilds. Testing: full Pkg.test() passes (couple_buffers 338/338, two allocation testsets covering plain and AutoDiff, sequential and threaded coupling). The real bidirectional coupled solve in the phasefield_fracture tutorial verified directly (not just via include, since its solve() call sits in a documentation-only markdown block) - matches the pre-redesign baseline. Reviewed by Codex (dual-review skill) across several rounds: accepted fixes for hoisting per-task buffer construction outside the per-color loop (was rebuilding per color instead of once per work! call), documenting that coupled simulations must not be mutated by a concurrent work! call or shared as a coupled partner by two concurrently-running work! calls, adding mismatched-task-count and allocation regression tests, and correcting allocation-cost documentation to precisely describe the small, non-cell- scaling per-task linking cost that remains. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND6mqzQ1JLuWdC8LPAhkvY
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
- Coverage 96.94% 96.68% -0.26%
==========================================
Files 30 30
Lines 1210 1236 +26
==========================================
+ Hits 1173 1195 +22
- Misses 37 41 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The "couple_buffers threaded allocations" AutoDiffCellBuffer steady-state check asserted nalloc_ad_30a == nalloc_ad_30b, but @allocated for threaded work can vary by a small, fixed amount between otherwise-identical calls (task scheduling, GC) - this showed up as a genuine but tiny 128-byte difference on Julia 1.13, unrelated to any regression. Replaced with the same "doesn't blow up" style bound already used elsewhere in this testset, which still catches the real regression this test guards against (a repeated, expensive JacobianConfig rebuild) without being sensitive to run-to-run noise. Verified: Pkg.test() passes with 2 threads on Julia 1.11.9, 1.12.7, and 1.13.0 (juliaup channels 1.11, 1.12, release). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND6mqzQ1JLuWdC8LPAhkvY
… reinit coupled buffers and join before work single
…rebuild Finishes the coupled-buffer rework: work_domain_threaded! now links each task's own persistent buffer directly to its coupled partner's per-task buffer (via TaskLocals), instead of allocating a temporary cib::Vector. Fixes bugs from the WIP version: a wrong-buffer typo when fetching the partner's per-task buffer, relinking being rebuilt once per mesh color instead of once per work! call, an unhelpful @Assert instead of ArgumentError on task-count mismatch, and an unguarded scatter! that would error for a sequential coupled partner. Adds a persistent per-buffer cache (CellBuffer.coupled_cache) so couple_itembuffers returns the previously-built linked wrapper when called again with the same (===) coupled NamedTuple, instead of reconstructing. This is what lets AutoDiffCellBuffer skip rebuilding its expensive ForwardDiff.JacobianConfig on steady-state repeated work! calls while still resolving coupling fresh each call. Also fixes a bug found during review: replace_material was carrying over the old buffer's coupled_cache Ref, which could return a stale, pre-replacement wrapper (wrong material, or a TypeError if the material type changed) on the next coupled work! call. Tests: full Pkg.test() suite passes (3383 tests), including a new "couple_buffers cache identity" testset that directly proves (via ===) that repeated coupling reuses the cached wrapper/JacobianConfig and that replace_material does not leak a stale cache entry. Reviewed independently via Codex; one high-severity finding (the replace_material cache leak) was found and fixed, with a regression test added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND6mqzQ1JLuWdC8LPAhkvY
Owner
|
Closed by #92 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Investigating BUG-003 (stale time increments in coupled threaded buffers) surfaced more: coupled material/cache could also go stale after
replace_material; setup-timecouple_bufferscouldn't support symmetric bidirectional coupling; and threaded coupled work had no safe way to reinit a partner's buffer without racing across tasks. API change:couple_buffersis no longer called at setup — coupling is resolved fresh eachwork!call from whateverCoupledSimulationsis supplied. Threaded coupled work now reuses each partner's own per-task buffers directly, so partner and primary task counts must match (errors otherwise).work!call, always sourced directly from the suppliedCoupledSimulations— no persistent link that can drift from what's actually being worked.ArgumentErrorif task counts don't match.AutoDiffCellBufferavoids rebuilding itsJacobianConfigwhen the coupling structure is unchanged.couple_buffersis no longer public/exported — it's purely internal now.Test plan
Pkg.test()— full suite passes,couple_buffers338/338, plus new allocation regression tests (plain and AutoDiff buffers, sequential and threaded) confirming coupling cost is small and doesn't scale with cell countsolve_single_part, not justinclude, since the driving code lives in a documentation-only markdown block) — matches pre-change baseline🤖 Generated with Claude Code
https://claude.ai/code/session_01ND6mqzQ1JLuWdC8LPAhkvY