Skip to content

Profiling-driven fixes (2026-07-25) - #226

Merged
gaelforget merged 10 commits into
masterfrom
various_20260718_exp1
Jul 28, 2026
Merged

Profiling-driven fixes (2026-07-25)#226
gaelforget merged 10 commits into
masterfrom
various_20260718_exp1

Conversation

@gaelforget

@gaelforget gaelforget commented Jul 25, 2026

Copy link
Copy Markdown
Member

part 1

Fix A — Polygons.jl SparseArrays wrong dispatch (done)

to_Polygons and to_LineStrings2D used [a b c d e] syntax for 1×5 matrices.
With contaminated upstream types this dispatched through SparseArrays (~1,093 samples, ~81% of demo_grid runtime).
Fix: Float64[a b c d e] — forces typed_hcat, plain Matrix{Float64}.

Fix B — CatView deep nesting in define_regions (done)

Two sub-fixes:

src/types/gcmvector.jlview(gcmarray, gcmvector): was building CatView(CatView(...)) iteratively — O(nFaces) nesting, O(depth) cost per element write (~6,254 samples).
Fix: views = [view(A[a], B[a]) for a in eachindex(A)]; CatView(views...) — single-level flat CatView.

src/Integration.jldefine_regions hot call sites: mask[findall(...)].=l desugared to dotview → view → CatView → broadcast.
Fix: changed .=l to =l — routes to setindex!(gcmarray, Number, gcmvector) which does a direct face-by-face loop.

Fix C — face-by-face broadcast copyto! (done)

Root cause: A .* A, A .+ B, scalar broadcasts etc. all went through an element-by-element gcmarray_getindex_evalf loop, allocating a new matrix per face per broadcast. Inconsistent with the fast face-by-face *, +, -, / methods in main.jl.

Fix in src/types/gcmarray.jl and src/types/gcmvector.jl: rewrote copyto! to loop for I in CartesianIndices(dest.f) and call broadcast!(bc.f, dest.f[I], face_args...). Helper _bc_face rewrites a Broadcasted tree by replacing each AbstractMeshArray arg with its face-I inner matrix.

Four bugs encountered and fixed during this rewrite:

  1. gcmvector.similar left inner Array{T,1} as undef references — fixed to allocate each f[I] explicitly.
  2. eachindex(dest.f) returns linear integers for Julia Arrays; I[1] on Int is a no-op. Fixed to CartesianIndices(dest.f) so I[1] always gives the face dimension.
  3. Mixed 1D/2D gcmarray broadcast (e.g. GM_PsiX[:,k] .* DYG): _bc_face must use I[1] for 1D sources and full I for 2D. Fix: ndims(a.f) == 1 ? a.f[I[1]] : a.f[I].
  4. exch_UV replaces face matrices with larger halo-padded ones but does NOT update fSize. So similar allocates dest faces at wrong size → DimensionMismatch. Fix: copyto! recomputes result shape from actual face args via Base.Broadcast.combine_axes, reallocates dest.f[I] in-place if size mismatches. filter(a -> isa(a, AbstractArray), face_args) guards against scalar args which have no axes.

part 2

  • Integration.func: replace collect comprehension with explicit loop,
    hoist xymsk(b) mask out of inner k-loop — zero Vector allocations per call
  • Operations.land_mask: use setindex! (=) not broadcast (.=) on gcmvector
    results to bypass CatView materialize! and its ~40-level recursion
  • transport test: replace side-effect comprehension with for loop

part 3

  • pre-compute basin mask in define_sums; replace func/func_h with face-level loops to eliminate per-call gcmarray allocations
  • replace gcmarray broadcast+findall pattern in define_regions with face-level loop, eliminating CatView nesting

…_hcat instead of generic hcat and avoids the SparseArrays dispatch
…n define_regions \n Replace element-by-element broadcast copyto! with face-by-face broadcast! for gcmarray/gcmvector
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.92308% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.50%. Comparing base (b039c36) to head (124cc5a).

Files with missing lines Patch % Lines
src/types/gcmarray.jl 86.36% 3 Missing ⚠️
src/Polygons.jl 50.00% 2 Missing ⚠️
src/types/gcmvector.jl 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   87.02%   87.50%   +0.47%     
==========================================
  Files          37       37              
  Lines        3446     3538      +92     
==========================================
+ Hits         2999     3096      +97     
+ Misses        447      442       -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

  results to bypass CatView materialize! and its ~40-level recursion
…ce-level loops to eliminate per-call gcmarray allocations ; 2) replace gcmarray broadcast+findall pattern in define_regions with face-level loop, eliminating CatView nesting
@gaelforget
gaelforget merged commit 32555ac into master Jul 28, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant