Repository navigation
Optimize IB Index Lookup - #1916
Merged
sbryngelson merged 13 commits intoOct 12, 2026
Merged
Conversation
|
Claude Code Review Head SHA: 84737ea Files changed:
Findings:
|
Its sandiego.yaml mechanism is the only non-h2o2 one in the suite, so it forces a second chemistry build of every target just for this case, which takes too long to compile. It stays in the example skip list.
Conflict in m_ibm.fpp: this branch moves s_get_neighborhood_idx and s_update_ib_lookup to m_ib_patches; master (MFlowCode#1914) added s_check_every_patch_marked between them. Kept the new check and this branch's removal.
Member
|
Merged master into this branch ( |
Member
Author
|
@sbryngelson Would I be able to request that we not do upstream merges into draft PRs until they are opened as a full PR? Upstream branches that I am not aware of cause me to keep diverging from main |
Member
|
@danieljvickers yes good idea. sometimes it's me trying to save people trouble but i know what you mean |
Lines of Code
|
danieljvickers
marked this pull request as ready for review
October 9, 2026 14:03
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1916 +/- ##
==========================================
+ Coverage 61.79% 61.95% +0.15%
==========================================
Files 86 86
Lines 22773 22834 +61
Branches 3353 3356 +3
==========================================
+ Hits 14073 14147 +74
+ Misses 6211 6193 -18
- Partials 2489 2494 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
|
to get this to pass CI you'll need to fast forward your branch @danieljvickers -- some changes in the runners now require this |
sbryngelson
added a commit
to sbryngelson/MFC
that referenced
this pull request
Oct 12, 2026
Resolve conflict in s_finalize_ibm_module: master's MFlowCode#1916 removed ib_gbl_idx_lookup, so keep only the new ib_face_ft deallocation.
sbryngelson
added a commit
to sbryngelson/MFC
that referenced
this pull request
Oct 13, 2026
_fc()'s inline default is what the toolchain uses when src/ is absent, as in a Homebrew install, so a stale one silently gives the wrong patch_ib namelist bound there. master's MFlowCode#1916 moved the Fortran constant to 810000 while this branch still carried 216000, and merging master in brought the two together without a textual conflict -- they live in different files. This is the drift the new TestFortranConstantFallbacks exists to catch, and it caught it. It is the only stale fallback of the ten.
sbryngelson
added a commit
that referenced
this pull request
Oct 13, 2026
* Sync NIB fallback with m_constants.fpp and test all _fc defaults definitions.py falls back to inline defaults when src/ is unavailable (e.g. Homebrew). The num_ib_patches_max_namelist default was still 54000 while m_constants.fpp has 216000, so such installs accepted far fewer patch_ib entries than the Fortran build. Nothing enforced the match. Update the default to 216000 and add a test that parses every _fc(name, default) call in definitions.py and checks the default against the parsed m_constants.fpp value. Verified: the new test fails with the old 54000 and passes now; all params_tests pass. (cherry picked from commit 3269dd2) * Accept every index of indexed families in the case-file JSON schema json_schema_gen listed only the index-1 example of each IndexedFamily (patch_ib, ib_airfoil, stl_models, particle_cloud) and set additionalProperties to false, so IDEs using mfc-case-schema.json flagged patch_ib(2)%... and every other index > 1 as invalid. Add a patternProperties entry per family attribute, reusing the index-1 property schema (type, constraints, description). The family regex is factored into registry.family_param_pattern and shared with REGISTRY.get_json_schema. Attributes are matched exactly, as _resolve_family does; max_index is not encoded in the regex. Verified: new fastjsonschema-based test validates patch_ib(2)/(1000), ib_airfoil(3), particle_cloud(4), stl_models(2) entries and rejects index 0, unknown attrs and out-of-range sub-indices; it fails without the change. All params_tests pass. (cherry picked from commit ce14f37) * Match the num_ib_patches_max_namelist fallback to m_constants.fpp _fc()'s inline default is what the toolchain uses when src/ is absent, as in a Homebrew install, so a stale one silently gives the wrong patch_ib namelist bound there. master's #1916 moved the Fortran constant to 810000 while this branch still carried 216000, and merging master in brought the two together without a textual conflict -- they live in different files. This is the drift the new TestFortranConstantFallbacks exists to catch, and it caught it. It is the only stale fallback of the ten.
sbryngelson
added a commit
to sbryngelson/MFC
that referenced
this pull request
Oct 13, 2026
Master's MFlowCode#1916 rewrites the IB index lookup: ib_gbl_idx_lookup (direct-indexed) is replaced by sorted ib_lookup_keys/vals with a binary search, and s_get_neighborhood_idx moves from m_ibm to m_ib_patches. Three conflicts, resolved as follows. - m_helper.fpp, m_ib_patches.fpp: union of the public lists, keeping this branch's s_pack_ib_state and f_cylinder_axis alongside master's lookup routines and its new lookup arrays. - m_ibm.fpp finalize: dropped the deallocate for ib_gbl_idx_lookup, which no longer exists; kept ours for ib_face_ft. - m_ibm.fpp handoff: **master's refactor does not carry this branch's MFlowCode#1973 duplicate protection.** It appends arrivals and only merges the lookup after the unpack loop, and the search is bounded to the pre-loop count, so a rank that fills several neighbour slots across a periodic direction receives the same patch once per slot and would append it twice; s_merge_ib_lookup is a plain sorted merge and does not dedupe. The protection is kept, expressed in master's idiom: scan this loop's arrivals before appending. Master's new owner-change branch is kept as well. Verified: builds clean; IBM 65/70 and Chemistry 30/31 under amdflang CPU, the failing set identical to the pre-merge amdflang baseline (0387594F, 4646053A, B0CE19C5, B7A6CC79, F2F0B673 -- four tolerance-marginal at ~1e-10 against a 1e-10 tolerance, one the hot-wall corner case). So the merge changes no result these tests cover. Not verified: the duplicate-handoff path itself. It needs multiple ranks and a periodic direction, which the serial suite does not exercise. Made with Claude Code.
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.
Contribution Policy
The IBM weak scaling runs performed previously achieved 64% scaling efficiency on all of Frontier, with the major slow down coming from the IB ownership handoff. A closer look at this subroutine reveals that the issue is a growing cost to rebuilt the IB lookup array as the number of IBs approaches 100s of millions of entries.
This PR optimizes that subroutine by refactoring the IB lookup array.
Old Lookup Behavior
The lookup was a sparce array of length
num_gbl_ibswhich held null values except for a few entries where global IBs were being tracked by the local neighborhood rank. At the largest simulation of 500 million IBs, this is about 2 GB of integers where only a few kilobytes hold required information.New Behavior
I have replaced the old lookup with a set of key-value-pair arrays that are sorted by key. Because we expect few IBs to be crossing boundaries, I opted to do an insertion sort where new IBs are inserted into the previously sorted array of elements. This reduces the size to track particles to the order of 100s of kilobytes. The lookup time slows slightly from O(1) to O(log(num_ibs)), but that time is still very negligible compared to the performance gain, which reduces the array rebuilding from O(num_gbl_ibs) to O(log(num_ibs)).
Performance
I have run this PR on Frontier using the old weak scaling case. I skipped the final data point because the trend was clear. The refactor shows significant improvement in performance.