mfi refactor batched read assign - #8
Open
yslan wants to merge 29 commits into
Open
Conversation
…Nek5000#900) Refactor the multi-file-input restart readers mfi_getv/mfi_gets (core/ic.f) so reading large checkpoints no longer blows the fixed buffers. - Fuse read+redistribute+assign into one batched loop across all three transport paths (crystal router, MPI-RMA, np==1); eliminate the full-size storage buffer wkg (was real*4 wkg(2*lwk)). - Widen the crystal-router tuple item to lelem_mx = 2*[ldim*](lx1+6)^3 with column count lbrst_max, lifting the per-element 'c' limit that aborted high-order FP64 restarts (the PR Nek5000#900 failure). - Fix pre-existing bugs surfaced by new coverage: * np==1 + h-refine index (ei=er(e), matches upstream PR Nek5000#908). * RMA window / crystal-router setup guarded to np>1 (MPI_Win_create at np==1 aborted). Tests (short_tests): add IO_Test variants closing gaps G1 (RMA, ifcrrs=F), G7 (np==1 serial), and a high-order FP64 acceptance case for PR Nek5000#900 (fails on pre-refactor code at 'mfi_getv c', passes after). All four IO_Test variants pass; verified byte-identical (err-max==0) plus a negative control. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
De-duplicate the per-element assign block (byte_reverse + copy-vs-interp ladder) that was inlined in 6 places (3 transport paths x mfi_gets/mfi_getv) into a single shared helper mfi_assign_elem. mfi_gets calls it once per element; mfi_getv calls it per component (u,v,w). mfi_gets/mfi_getv stay separate routines. Byte-identical to the prior code; all four IO_Test variants pass. Note: the getv call sites initially hit the fixed-form column-72 limit (the comma after 'npts' landed in col 73 and was silently dropped, fusing npts+nxr into a garbage arg and corrupting getv fields). Call lines are broken before 'npts' so all code stays within column 72. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 2 -- correct the message-buffer check 'a'. It was an always-on full-field check lim_chk(nxyzr*nelt_hr0, size(wk)); but only the RMA path (.not.ifcrrs) uses wk, and the CR path never touches it. Make 'a' RMA-only so a high-order FP64 CR restart no longer aborts spuriously on wk. Phase 3 -- batch-scale the read buffer w2 and set batch-capacity defaults: - w2 sized lrbs_loc*lread_mx (was lrbs_loc*lelt); lread_mx defaults to lelt (no memory change) and is the read-block memory knob. lread_mx/lrbs_loc are kept identical in mfi_gets/mfi_getv so the shared /vrthov/ common matches. - lbrst_max default min(1024,lelt) (was lelt), matching lbrst's own default, so the CR tuple vi is batch-scaled and stays under the old wkg+vi peak. RMA multi-block fix: reading in file-order blocks (small w2) cannot also bound the destination-indexed RMA window under a sequential fd. So RMA now uses a single FULL-FIELD window (nbatch=1) that accumulates Puts across all read blocks and assigns only on the last block (i.eq.nread); it batch-scales w2 but not the window. CR still batches the redistribute over lbrst windows. Without this, small lread_mx AND small lbrst together corrupted RMA output. Verified: all four IO_Test variants pass at default sizing; a reduced-sizing spot check (lread_mx=lbrst_max=lbrst=4) passes CR/RMA/serial, exercising the multi read-block x multi window path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Plan D) D1b -- when a field is present in the file but not requested (mfi_get[vs](...,iskip=.true.)), the getter previously did the full read AND the full redistribute (CR crystal-router all-to-all, or RMA MPI_Puts + window sync), discarding only the assign. Now the byte_read is kept (it advances the serial non-MPIIO fd correctly and is harmless for MPIIO -- the driver sets an absolute byte_set_view per field, so positioning is unaffected) but the entire redistribute+assign block is guarded by .not.iskip. This removes the per-skipped-field all-to-all. np==1 already skipped only the assign (no redistribute there). D2 -- the RMA path (ifcrrs=.false.) uses a full-field one-sided window and, unlike CR, cannot batch it below the full read-order field (assign only reduces size after data is in the window). A high-order/large file overflows wk and hit an opaque "buffer too small" abort. Add an actionable rank-0 message directing the user to the crystal-router path (ifcrrs=.true., the default, which handles high order via per-element sub-batching) or to enlarge wk. Batched (drain-per-block) RMA was designed and deferred: it gives no worst-case window bound (worst case = full field = the common contiguous-partition case), is a no-op at default lread_mx=lelt, and costs nread extra collectives + reader collision machinery. See reports/report_D.md. All four IO_Test variants pass; byte-identical for read fields. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eps 1-2) Readability cleanup of short_tests/io_test/io_test.usr (no behavior change): - Replace the 17 identical PASSED/FAILED tail blocks with a single report() subroutine. The bare 'PASSED'/'FAILED' tokens and the 'All I/O tests PASSED' banner (grepped by NekTests.py) are preserved byte-for-byte. - Replace the 3 copies of the /COMPDATA/ reference-field common-block declaration with a shared case-dir include io_test_cmp.inc (makenek's -I<casedir> picks it up). io_test.usr 2440 -> 2338 lines. All four IO_Test variants pass; the final run's PASSED/PASSED:field/All-I/O counts (6/47/1) match the pre-refactor baseline exactly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Continue the readability refactor of io_test.usr (no behavior change): - Replace the 17 near-identical ~60-90 line "check reading options" header verification blocks with one chk_restart_hdr() helper, driven by a 14-char flag spec string plus wdsize/nfiler/byteswap/nelgr/time expectations. The hrefine double-precision PnPn/PnPn-2 pressure branch is expressed via a mutable spec (pos6=ifgetp, pos14=if_full_pres). Per-block time tolerances preserved (.fld blocks use eps4, others eps8). - Replace the 7 write prologues (param(63/65/66)+if_full_pres+time+ fields_restore+prepost) with one run_write(iprec,nfiles,ifmt,ffp,prefix). io_test.usr 2440 -> 1262 lines (~48% smaller). All four IO_Test variants pass; the final run's PASSED/PASSED:field/All-I/O counts (6/47/1) match the pre-refactor baseline exactly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
All pre-existing (present in base 41d1794), each verified against the suite: - (a) hrefine-schedule gate was tautological: 'if (hrefcuts(1).gt.2 .OR. hrefcuts(1).le.4)' is always true. Corrected to '.ge.2 .AND. .le.4' (keeps the tested schedules 2 and 3 valid). - (b) filename-prefix if/else was inverted in the hrefine double-precision read block (blank prefix built a prefixed name and vice-versa) vs all other blocks. Fixed to match; the '2;2' run (the only caller affected, via 'hr2') stays green. - (c) removed dead code: commented coord-zeroing in fields_clean, a stray 'csk' marker, and a commented 'c ifcrrs = .true.'. All four IO_Test variants pass (6/47/1 counts unchanged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Dev-facing README: how to run, the four IO_Test variants and their toggles, what io_test.usr does + the shared helpers, the committed fixtures vs generated artifacts, the coverage matrix, and the known gaps (MHD B-field restart, odd-np multi-file, dedicated iskip, passive scalars in full_restart). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
io_test_rs.usr (the full_restart round-trip case) shares none of io_test.usr's duplicated patterns, so only the applicable Plan E cleanups were made: - remove the dead commented-out 'outpost' block in check(); - add a file-header comment describing the full_restart test flow; - add a one-line purpose comment on check(). No executable logic changed; the full_restart err-max==0 assertion (PnPn-2 and PnPn) still passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rework mfi_gets/mfi_getv from the two-nested-loop shape
(do i=1,nread { read; do ibatch=1,nbatch { CR/RMA; assign } }) into the
single fused loop the PR Nek5000#900 author specified: each iteration reads one
file-order batch, routes the whole batch via the crystal router, and
assigns it. The read buffer w2 holds only one batch; nread and nbatch
collapse into one global iteration count.
- CR path (ifcrrs=.true., default) and np==1 use the fused loop; the RMA
path (ifcrrs=.false.) keeps its full-field-window variant, branched via
if (ifcrrs.or.np.eq.1) ... else if (RMA) ...
- The crystal transfer (fgslib_crystal_tuple_transfer -> crystal_router)
is a butterfly collective over all np, so the loop uses a global
niter=iglmax(...) and calls the transfer on every rank (n=0 legal on
non-readers). This also fixes a latent non-MPIIO+CR deadlock when
nfiler < np, where the old reader-gated transfer would hang.
- Dropping the jeln dest-window means a rank can receive its whole local
field per transfer, so the CR receive tuple vi moves to a common block
(/mfi_vis/, /mfi_viv/) sized lrst_mx=lelt. CR peak memory relocates
from w2 to vi (bounded either/or under one sequential file pass).
- np==1 folds in: transfer skipped, columns stay file-ordered, assign
uses ei=er(k+iloc) (preserves the PR Nek5000#908 np==1 h-refine fix).
- Preserved: iskip skip-redistribute (D1b), h-refine, wdsizr 4/8,
scalar/vector assign, rst_etime timers, mfi_assign_elem, lim_chk a-d.
All four short_tests/io_test IO_Test variants pass (CR/RMA/Serial/
HighOrderFP64), counts 47/1/0/0, including a forced niter=6 multi-batch
stress run.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Slim the in-code comments in mfi_gets/mfi_getv/mfi_assign_elem (core/ic.f) and the high-order test (io_test.usr): drop internal project tags (W1/W2/W3, D1b/D2, B6, report_X.md references) and collapse multi-line rationale to concise notes. Comments only -- the compiled code is byte-identical (verified by diffing with inline comments stripped). Upstream references (PR Nek5000#900/Nek5000#908) and genuine cross-routine constraints (lrbs_loc/lread_mx must match) are kept. The full original commentary is preserved in reports/mfi_code_comments_archive.md (outside this repo). All four short_tests/io_test IO_Test variants still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rename the shared reference-field cache include to a Nek5000-style ALL-CAPS, no-extension common-block header REFDATA (resolved via the case-dir include path -I$(CASEDIR)), and rename the common block /COMPDATA/ -> /REFDATA/ to match. Updates the three include lines in io_test.usr and the README helper note. Variable names (cmp_v/cmp_t/ cmp_pr/cmp_x) are unchanged. All four IO_Test variants pass after rebuild. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Polish the concise comments in the mfi_gets CR fused-loop region (fixing typos and an inaccuracy: the per-exchange receive capacity is lrst_mx = a rank's full local field, not lbrst) and apply the same tidied wording to mfi_getv so the two routines stay parallel. Also set lbrst_max=1024 (a plain cap; it only bounds lbrst via check 'd' and no longer dimensions vi, which is sized by lrst_mx). Comments + the lbrst_max constant only; no logic change. All four IO_Test variants pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Factor the crystal-router redistribution out of the Plan F fused loop into a shared subroutine mfi_redist_cr(vi,li,nmax,n,w2,nxyzr,ke,nb,ierr): pack nb file-order elements er(ke+1..ke+nb) from the read buffer into the tuple vi=[nid,iel,payload] keyed by owner, then crystal-route (np>1); np==1 packs only (file order); recv overflow (n>nmax) -> ierr. The payload is an opaque nxyzr-word blob, so one routine serves both mfi_gets (scalar) and mfi_getv (vector). Both loops now read as read -> redist -> assign; the ~24-line pack+transfer block that was duplicated in each routine is gone. Pure code motion (net -4 lines); rst_etime(2/3) timing breakdown preserved; RMA path left inline (does not fit the pack-into-vi interface; later). All four IO_Test variants pass, incl. a forced niter=6 multi-batch stress. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Give the crystal-router restart path a small receive buffer with a hard
no-overflow guarantee, instead of sizing the receive tuple vi to the full
local field (lelt). Per read batch:
- build an ELL/CSR index of the batch elements grouped by destination
rank (local fgslib_crystal_ituple_sort + one O(nb) scan);
- crystal fan-in/fan-out handshake (fgslib_crystal_ituple_transfer):
fan-in gathers per-destination counts so each rank learns recvcnt and
each sender's exclusive-prefix base offset boff(d); fan-out sends the
offsets back;
- nrounds = iglmax(ceil(recvcnt/cap)); each round packs only the elements
whose global stream position lands in [r*cap,(r+1)*cap) (a contiguous
per-destination slice via the index) and crystal-routes them, so a
receiver gets <= cap per round. Assign runs per round.
vi shrinks from (2+lelem_mx,lelt) to (2+lelem_mx,lrcv_mx=lbrst_max).
Invariant cap>=nb (send and receive share vi). np==1 bypasses the
handshake and assigns straight from w2 (file order, PR Nek5000#908). Replaces the
Plan G mfi_redist_cr with mfi_redist_plan + mfi_redist_round; new /mfi_hs/
index common; RMA arm untouched.
All four short_tests/io_test IO_Test variants pass; round-splitting
verified via a forced nrounds=2 stress (fields byte-clean). A temporary
per-batch diagnostic (FIXME (dbg)) is left in pending a loglevel-gated
verbose mode.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the temporary per-batch FIXME(dbg) redist print with a proper verbose summary gated by loglevel>2 (the standard Nek verbose threshold; default loglevel=2, so silent unless set via general:loglevel in the .par). Per mfi_gets/mfi_getv call, at loglevel>2, print two lines: - nbatch, rounds/batch min/max/avg, recvmax (redistribution shape) - cap, nb, lbrst, and buffer word counts w2 / vi / handshake (footprint) Round stats accumulate over the batch loop (nrounds is global via iglmax, so min/max/avg is consistent across ranks); nio.eq.0 prints. No effect at default loglevel. All four IO_Test variants pass; verbose output verified at loglevel=3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a runtime knob `lrcv` (INPUT, default 0=auto) for the bounded-receive
CR round cap, and decouple the crystal transfer's nmax from the round cap
so the cap can be set below the read batch safely:
- INPUT: new integer lrcv next to lbrst; drive2 initdat sets lrcv=0.
- mfi_gets/getv: cap = lrcv_mx, overridden by lrcv when lrcv>0.
- mfi_redist_round: new nmx arg (vi column capacity); the transfer and
overflow backstop use nmx (>= send nb and recv cap), so a small cap
just makes more, smaller rounds without overflowing the buffer.
CI: new IO_Test.test_PnPn2_Parallel_MultiRound runs the hrefine 1 then 2;2
cases with lbrst=3 (many read batches) and lrcv=2 (recv=3 per batch ->
nrounds=2), loglevel=3, and asserts both 'All I/O tests PASSED' and that
the verbose 'rounds/batch' max is 2 -- i.e. multi-batch AND multi-round are
now actually exercised (previously every batch was nbatch=1/nrounds=1).
io_test.usr:usrdat maps uparam06->lbrst, uparam07->lrcv.
All five IO_Test variants pass (Parallel, MultiRound, HighOrderFP64, RMA,
Serial).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…+ demo - ic.f: the loglevel>2 verbose line printed lrcv_mx (compile max) as 'cap='; print the actual runtime cap instead (label bug only; nrounds math was already correct). - io_test/README.md: add a section on testing the bounded-receive CR path (lbrst/lrcv/loglevel knobs, the MultiRound pytest, how to read the log) with a real demo excerpt (nbatch=6, rounds/batch=2, cap=2, buffer sizes). All five IO_Test variants pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Make the MPI-RMA redistribution path batched + bounded like the CR path,
reusing the CR fan-in/fan-out handshake, instead of a full-field window.
- new mfi_redist_round_rma: reuse mfi_redist_plan's /mfi_hs/ ELL index +
boff; per round r, MPI_Put each element's tuple into the owner's COMPACT
stream-position window at disp=(boff(d)+j - r*cap)*li. boff tiles each
owner's stream disjointly so Puts never collide; the receiver assigns
n=min(cap,recvcnt-r*cap) slots from the window, identical to CR.
- bounded window: new /mfi_rwin/ common (real*4 rwin4 in mfi, integer view
iwin in gets/getv), sized (2+lelem_mv)*lrcv_mx (vector bound, serves
both); MPI_Win_create over it; epochs via MPI_Win_fence (collective, so
equal epoch counts follow from global nrounds). Replaces the full-field
window over wk and the D2 overflow guard.
- RMA branch now runs plan/epochs/assign on ALL ranks (readers gate only
read+Put-pack), fixing a latent bug where non-reader Put targets never
assigned (masked before because MPIIO made every rank a reader).
- crystal handshake (fgslib_crystal_setup/free) now set up for BOTH paths
(RMA borrows mfi_redist_plan). lim_chk 'c'/'d' apply to both (np>1).
CI: new IO_Test.test_PnPn2_Parallel_RMA_MultiRound (RMA, lbrst=3, lrcv=2 ->
nrounds=2, asserts fields pass AND max rounds==2) and
test_PnPn2_Parallel_RMA_HighOrderFP64 (reads the lx1=10 FP64 checkpoint via
RMA into lx1=6 -- the old full-field window overflowed; the bounded window
fits). All seven IO_Test variants pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(Plan J)
Collapse the two near-identical branches (CR: ifcrrs/np==1; RMA: np>1) in
mfi_gets and mfi_getv into one batched loop. The loop branches on ifcrrs only
at the two points where CR and RMA actually differ, per round:
- transport: mfi_redist_round (crystal -> vi)
vs mfi_redist_round_rma (MPI_Put -> iwin window)
- assign src: inline assign from vi vs from iwin
np==1 stays a distinct inner branch (assign straight from w2, no tuple header).
The per-element assign stays inline (a short loop over mfi_assign_elem) rather
than a wrapper subroutine: a shared helper would have to pass ~12 arguments
straight through to mfi_assign_elem just to select the source buffer, so it
added boilerplate without real reuse.
Behavior-preserving refactor: ~100 fewer lines vs the Plan I two-branch shape.
All seven IO_Test variants pass (CR/RMA x plain/MultiRound/HighOrderFP64 +
Serial).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Behavior-preserving cleanup of the batched restart readers: - Rename the batch variables to a consistent, self-describing scheme: nb->nbe (elems/batch), nb_this->nbe_cur (this batch, last partial), local_nb->nbatch (batches on this rank), niter->nbatch_g (global loop count). Prefix the verbose-only stats v_* (v_nbat,v_rmn,v_rmx,v_rsum, v_rcvmx) so they read as non-essential. - Add short same-line descriptions to the introduced vars/buffers (w2, vi, iwin, cap, k, l, nw, lbrst_max, lrcv_mx, nxyzr) and 2-3 line intros to the read-batching setup and plan/round/assign phases. - Drop dead locals (e, ei, disp, dnxyzr, nelt_hr0) in both routines; fix the stale wk comment (window is /mfi_rwin/, wk is an unused legacy arg). - Remove three redundant nekgsync calls per routine, each shadowed by an adjacent collective (iglmax before the loop, the crystal transfer / MPI_Win_fence between rounds, err_chk's iglsum at the end). - Concise comments in mfi_redist_plan/_round/_round_rma/_assign_elem/mfi; drop internal plan/PR numbers; call the /mfi_hs/ index CSR (not ELL). - Mirror the mfi_gets cleanup into mfi_getv so the two stay symmetric. Verbose write() token layout unchanged (CI greps col 6 of "rounds/batch min/max/avg="). All seven IO_Test variants pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8b406ecc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three defects surfaced by a strict (clang/flang) build and code review:
1. Implicit-REAL loop indices in mfi_redist_plan. `t` (and `n1`) fell in the
o-z / default range, so `do t=1,n1` and `it(3,t)` used a REAL index —
gfortran silently converted, clang/flang errors ("Must have INTEGER type,
but is REAL(8)"). Declared `integer i,j,t,n1`. Audited all three redist
routines + gets/getv: every introduced non-(i..n) variable is now
explicitly typed.
2. np==1 serial assign was compiled out under `#ifdef MPI`. In a real MPI=0
build (makenek NOMPIIO: no -DMPI, mpi_dummy.h -> mpif.h), the whole
redist+assign block vanished, so a serial restart read the bytes but left
the field untouched. The original kept the np==1 read+assign OUTSIDE the
guard. Moved the np==1 direct-from-w2 assign out of `#ifdef MPI` (only the
CR/RMA redistribute, which genuinely needs MPI, stays guarded), in both
mfi_gets and mfi_getv. Verified: ic.f MPI=0 syntax-compiles (same 13
pre-existing unrelated errors as the original; none in the mfi region).
3. Handshake fan-in capacity bounded by lbrst_max. `n1` = number of source
ranks routing to a destination in a batch, which is O(np) (MPIIO: all ranks
read), NOT bounded by lbrst. Sizing the /mfi_hs/ `it` tuple and the
ituple_transfer caps + `mfi hs r` check by lbrst_max=1024 aborted large-np
restarts before the bounded rounds could act. `n1 <= recvcnt <= nelt_hr0
<= lelt`, so give the handshake its own bound `lhs_mx=lelt` (integer tuples,
cheap) in all three redist routines; add a diagnostic print on overflow.
All seven IO_Test variants pass; ic.f compiles under MPI=1 and MPI=0.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@codex review this commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e6366a5bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…quence
The CR/RMA multi-round tests used hrefine="2;2", but the h-refine schedule
separator is a COMMA and ';' is an inline-comment char in the .par parser
(iniparser), so "2;2" was silently truncated to "2" (nhref=1) -- the 2-level
hierarchical restart (nhref=2, the hr2-checkpoint read path) was never actually
exercised. They passed only because "2;2"=="2" self-consistently writes+reads
hr2 within one run.
Fix: use the real comma separator "2,2" and add "2" to the sequence so the
hr2io_test0.f* checkpoint is written before the 2,2 run reads it:
("1","2;2") -> ("1","2","2,2")
Now the multi-round tests are self-contained on a clean checkout AND genuinely
test the hierarchical restart (verified: nhref=2, "Testing hrefine href=2;2",
reads hr2, max rounds/batch==2 on every step). All seven IO_Test variants pass
from a clean tree.
(The pre-existing main/_RMA/_Serial tests still use "2;2"; they have "2" before
it so they don't fail, but likewise only test hrefine=2 -- left as-is here.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@codex review this commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45a0f8e979
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review findings: 1. Fixed receive cap over-reserved memory for small lelt. lrcv_mx was lbrst_max=1024, so vi (/mfi_vis/,/mfi_viv/) and the /mfi_rwin/ window each reserved 1024 tuple columns of full high-order element size even when a rank can receive at most its local element count (<=lelt) -- hundreds of MB/rank before any restart I/O in high-order/small-lelt runs. A rank never receives more than nelt_hr0<=lelt elements, and the send side is bounded by nbe<=lbrst<=min(1024,lelt), so define lrcv_mx=min(lbrst_max,lelt) consistently across mfi_gets/mfi_getv/mfi_redist_round_rma/mfi (all four sites that size vi and the window). No effect where lelt>=1024. 2. io_test h-refine schedule used "2;2", but the separator is a COMMA and ';' is an inline-comment char in the .par parser, so "2;2" was silently truncated to "2" (nhref=1) -- the RMA and Serial matrices duplicated the hrefine=2 case instead of exercising the 2-level hierarchical restart, and the main test's "test 3" step did too. Switched to "2,2" (all three already have a "2" step before, so the hr2 checkpoint exists). Now genuinely tests hierarchical restart on CR/RMA/serial (matches the pre-existing EthierRefine test, which already uses "2,2"). The main test change touches a pre-existing upstream line. All seven IO_Test variants pass from a clean tree (real nhref=2 now exercised). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The batched read/redistribute loop had three error checks that were correct but not collective-safe: if they ever fired on a subset of ranks they would deadlock (some ranks branch to err_chk/exitt while the rest enter the next collective) instead of aborting cleanly. All are provably unreachable under the maintained bounds, and the ierr/goto pattern is inherited from the original reader, but harden them before the PR: - H1: reduce ierr with iglsum before every "if(ierr.ne.0) goto 100" inside the collective batch/round loop (after mfi_redist_plan and after each mfi_redist_round[_rma]), so all ranks bail together. - H2: the /mfi_hs/ backstops lim_chk(ndest,...) and lim_chk(n1,...) tested rank-local values and abort via exitt; check the GLOBAL max instead (iglmax) so all ranks abort together, and report the true max. - H3: reduce ierr with iglsum right after the batch read so a per-rank I/O error can't leave some ranks skipping the collective read/redistribute (matters on iskip batches, which have no plan/round to propagate it). iglsum/iglmax are Nek global ops (math.f:925), serial-safe, so the additions outside #ifdef MPI compile under MPI=0. Extra collectives only during restart. All seven IO_Test variants pass; MPI=0 syntax-clean in the mfi region. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@codex review again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b93f20fbcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When a single restart element's payload exceeds the read buffer w2 (nxyzr >
lrbs, e.g. a small-lelt job reading an FP64 high-order vector checkpoint,
nxr<=lx1+6), the batched read overflowed w2 instead of failing cleanly:
- nread = ceil(nxyzr*nelr/lrbs) is large, so nelrr = nelr/nread truncates
to 0, and the 'b' check lim_chk(nxyzr*nelrr,lrbs) = lim_chk(0,lrbs) passes;
- the 'c' check bounds nxyzr by lelem_mx (the CR-tuple payload bound, which
is >= lrbs for small lelt), so it doesn't protect w2;
- nbe = min(lbrst, lrbs/nxyzr) = 0 -> forced to 1, and byte_read(w2,nxyzr)
then reads nxyzr > size(w2) words -> memory corruption.
This is a partial regression: the original 'c' check bounded nxyzr by
lrbs_loc (<= lrbs), aborting cleanly on the CR path. Restore a clean abort by
checking nxyzr <= lrbs (one element must fit the read buffer) before sizing
the batch, in both mfi_gets and mfi_getv. nxyzr is broadcast-consistent
(header-derived), so the abort is collective-safe. Once nxyzr <= lrbs,
lrbs/nxyzr >= 1 so nbe >= 1 and the batch read nxyzr*nbe <= lrbs fits.
All seven IO_Test variants pass (io_test lrbs >> nxyzr, so the guard is a
no-op there).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ime width
Stage 1 of moving the restart redistribution buffers off dedicated commons onto
the already-allocated /scrns/ scratch (the reviewer's memory-footprint concern;
w2 already reuses GFLDR's /vrthov/, so only the vi/window commons were "new").
- Drop /mfi_vis/ and /mfi_viv/ (the dedicated CR receive commons, ~56 MB/rank
at io_test sizing). The CR send/recv buffer AND the RMA send buffer are now
carved from wk (the /scrns/ arg mfi already passes, previously unused).
- Runtime tuple width li = 2+nxyzr (not compile-time 2+lelem_mx): the buffer
holds the whole local field in one round when it fits (prior-proven default),
batching only for high-order/oversized checkpoints. mtup = 2*lwk/li tuples
fit wk; nbe (read batch) and cap (recv round) are bounded by mtup so the
send/recv fits wk. Default cap = mtup (whole field if it fits).
- New helper mfi_assign_recv(recv,li,...) with an adjustable integer recv(li,1)
-- required because li is now runtime (a fixed-li version would just inline).
Shared by CR (recv=wk slice) and RMA (recv=window slice), gets (nfld=1) and
getv (nfld=ldim).
- Guards: nxyzr<=lrbs ('e', w2), li<=2*lwk ('f', one tuple fits wk), plus the
existing 'c'/'d' and (RMA) li<=lrwin ('g'). All broadcast-consistent.
/scrns/ safety: mfi uses it only transiently during the restart call; verified
no restart caller (setics/full_restart/auto_averager) holds live /scrns/ across
restart (auto_averager keeps /scrsf/, a different GFLDR common). CR has no
persistent window, so no coupling. The RMA window is still dedicated /mfi_rwin/
here (stage 2 moves it to /scrns/ per-restart). All seven IO_Test variants pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stage 2 of moving the restart buffers off dedicated commons. The MPI-RMA window was the last one (/mfi_rwin/, ~42 MB/rank at io_test sizing, resident for the whole run even on the default CR path). It now lives in wk (/scrns/): - wk (real*4, 2*lwk words) is split for RMA: 1st half = send-pack buffer, 2nd half (wk(lwk+1..)) = the MPI window. CR is unchanged (whole wk in place). mtup/mwin are computed per path (CR: 2*lwk/li; RMA: lwk/li each half). - The window is created in mfi over wk(lwk+1) (byte count lwk*4, disp_unit 4) and FREED at the end of the same mfi call -- so between restarts /scrns/ is completely free (the mid-run-restart aliasing concern). commrs (the comm dup) is kept for possible reuse; drive1.f no longer frees rsH (nothing persists). - Drop /mfi_rwin/ entirely (mfi, mfi_gets, mfi_getv). Guard 'g' now checks one tuple fits the window half (li <= lwk). Buffer-type audit (per review): rzero operates on default real (=real*8), so rzero(wk(lwk+1),lwk) would zero lwk*8 bytes and overrun wk by 4*lwk bytes (masked in tests only because physical /scrns/ is larger). Fixed to rzero(wk,lwk) -- lwk real*8 == the whole 2*lwk real*4 wk, aligned, no overflow (matches the original's zeroing). Verified the payload punning too: mfi_assign_elem's real*4 src is read as real*8 for wdsizr=8 (copy/mapab/byte_reverse8 with 2x counts); mfi_assign_recv's integer->real*4 view of the window slice is 4-byte-consistent; MPI_Put count/disp and win_size are all in the correct units. All seven IO_Test variants pass (CR/RMA x plain/MultiRound/HighOrderFP64 + Serial). Only /mfi_hs/ (56 KB handshake index) remains dedicated (stage 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stage 4: make the verbose sizing report reviewable so a user can confirm the
default is right for a given run. The 'rounds/batch min/max/avg=' line is kept
verbatim (the CI col-6 multi-round check parses it). The second line is expanded
into three:
avail : wk / w2 ceilings (words), and per-element need (nxyzr, li)
batch : nbe and the three limits it is min() of (lbrst, w2fit, wkfit)
round : cap and what binds it -- 'wk default' (whole field in 1 round when it
fits, mtup/mwin) or 'lrcv=<n>' (user knob overriding it)
Confirmed against io_test at loglevel=3: the DEFAULT (lrcv=0) gives cap=mtup and
rounds/batch=1 (whole local field in one round -- the prior-proven single-pass
behavior), while uparam7->lrcv forces cap=lrcv and >1 round. No behavior change;
diagnostics only.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
@codex this reduce the mem usage by reusing scratch space. review this. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
1 participant
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.
internal PR check