Remove per-pixel atomics and dynamic scheduling from RTC _normalizeRtcArea (#341) - #355
Open
s-sasaki-earthsea-wizard wants to merge 1 commit into
Conversation
The gamma-naught normalization loop used 'omp parallel for schedule(dynamic) collapse(2)' with an unspecified chunk size, dispatching one iteration per pixel through a contended shared counter, plus an 'omp atomic' per pixel. Each (i, j) is only touched by its own iteration, so the atomics are unnecessary. At NISAR frequency-A scale (29240 x 21232 radar grid, two normalization calls) this amounted to 1.24e9 contended dynamic chunk acquisitions; profiling attributes ~20% of the RTC area-projection time (~11% of total GCOV wall time) to the dispatch and atomics alone, for a loop that is otherwise memory-bandwidth bound. The loop remains scalar with GCC 13 at -O2/-O3 either way (the NaN guard defeats if-conversion), so the entire gain comes from removing the dispatch and the atomics. Use a plain row-wise 'omp parallel for' and a conditional expression instead. Output is bit-identical: iteration-to-pixel mapping and per-pixel arithmetic are unchanged.
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
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
Fixes #341.
_normalizeRtcAreaincxx/isce3/geometry/RTC.cppran its gamma-naught normalization loop withomp parallel for schedule(dynamic) collapse(2)(no chunk size, i.e. one iteration per dispatch) and a per-pixelomp atomicwrite/update. Each(i, j)is read and written by exactly one iteration, so the loop is race-free without the atomics — they were pure overhead — and the pixel-granularity dynamic scheduling turned a memory-bound pass over twofloatarrays into ~1.2e9 contended acquisitions of the shared iteration counter per call pair at NISAR frequency-A scale.This PR replaces the loop body with a plain row-wise
omp parallel forand a conditional expression (one function, +8/−10). Iteration-to-pixel mapping and per-pixel arithmetic are unchanged, so the output is bit-identical by construction as well as by measurement.Measured effect
On a real NISAR L1 RSLC (frequency A, 29240 × 21232 radar grid) processed through the GCOV workflow on a 16-core host (full methodology, run matrix, logs and raw profiles in #341):
RTC-APGEO-AP(~89% of workflow wall)All four GTiff output products (
HHHH,HVHV,numberOfLooks,rtcGammaToSigmaFactor) are bit-identical across all 9 measurement runs (single md5 per product).An isolated microbenchmark of the loop alone (same grid, 16 threads, bit-identity asserted against the current loop) gives 34.1 s → 0.127 s per call; the fixed loop runs at the measured parallel-memcpy ceiling of the host, i.e. the pass is memory-bandwidth-bound, as expected. Details, plus a comparison against Eigen
select()variants, in this comment.Notes for review
perfprofiles attribute 10.9% (gomp_iter_dynamic_next) + 9.9% (the_normalizeRtcAreaomp region) of user CPU to the removed overhead before the patch; both symbols drop below 0.5% after. Absolute thread-seconds of unrelated symbols (e.g.Orbit::interpolate) are unchanged within noise._applyRtcMinValueDb(RTC.cpp:255) uses the identicalschedule(dynamic) collapse(2)+ atomic-write pattern and would take the same fix; it is left out of scope because it did not execute in the measured configuration. I can file a separate follow-up PR for it if desired.Disclosure: this investigation and the patch were developed with assistance from AI coding tools (Claude, Codex, and Gemini). All measurements were executed on real hardware, and the evidence linked above (profiles, logs, and the bitwise verification) was generated and reviewed by the author.