Skip to content

Conversation

@hsugawa8651
Copy link
Contributor

@hsugawa8651 hsugawa8651 commented Sep 26, 2025

Summary

This PR fixes the type instability issue reported in #78, which causes excessive memory allocations when creating
MeshArray objects.

Problem

The mesh field in MeshArray struct was not type-stable, leading to:

  • Excessive allocations (>800MB for simple operations)
  • Poor performance in broadcast operations
  • Type inference failures

Solution

Implemented type stabilization through:

  1. Function barriers to isolate type-unstable code
  2. Type stabilization helpers (_stabilize_mesh_type, _create_mesharray_typed)
  3. Performance optimizations with @inline, @inbounds, @simd
  4. Improved broadcast implementation with type-stable copyto!

Changes

  • Modified src/mesharrays/dense.jl to add type stability improvements
    - Preserved original code as dense_original.jl for reference

Testing

Comprehensive testing shows:

  • ✅ All mesh types are now concrete
  • ✅ Memory allocations reduced dramatically
  • ✅ Function type stability verified with @code_warntype
  • ✅ All 1590 tests pass

Performance Verification

Here's a simple benchmark demonstrating the improvement:

using GreenFunc, GreenFunc.MeshArrays, CompositeGrids

# Create grids
k1 = CompositeGrids.SimpleG.Uniform([0.0, 1.0], 100)
k2 = CompositeGrids.SimpleG.Uniform([0.0, 1.0], 100)

# Create MeshArrays
ma1 = MeshArray(k1, k2; dtype=Float64)
ma2 = MeshArray(k1, k2; dtype=Float64)

# Fill with data
ma1.data .= rand(size(ma1.data)...)
ma2.data .= rand(size(ma2.data)...)

# Warmup
ma1 .+= ma2

# Measure allocations
@allocated ma1 .+= ma2  # Returns: 224 bytes
@allocated ma1 .*= 2.0  # Returns: 128 bytes

Results:

  • Broadcast .+= operation: 224 bytes (down from >800MB reported in Type-instability problem #78)
  • Broadcast .*= operation: 128 bytes
  • Consistent low allocations for both small and large arrays

Fixes #78

@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.58%. Comparing base (8e08d6a) to head (db75c05).
⚠️ Report is 31 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   75.81%   78.58%   +2.77%     
==========================================
  Files          12       11       -1     
  Lines         430      439       +9     
==========================================
+ Hits          326      345      +19     
+ Misses        104       94      -10     

☔ View full report in Codecov by Sentry.
📢 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.

- Added type stabilization helper functions with function barriers
- Improved type inference with @generated functions
- Added @inline and @inbounds optimizations for better performance
- Fixed broadcast operations to be type-stable
- Reduced memory allocations by 30-50%

This addresses the excessive memory allocation issue reported in Issue numericalEFT#78
by ensuring mesh types are concrete and using function barriers to isolate
type-unstable code.
Benchmark results show:
- Broadcast operations: 224 bytes (down from >800MB)
- Consistent low allocations for 10x10 and 100x100 arrays
- Type stability confirmed
@hsugawa8651 hsugawa8651 force-pushed the fix-type-instability-issue78 branch from e0920f0 to dfd3d30 Compare September 28, 2025 13:56
- Remove unused dense_original.jl reference file
- Add tests for type stability helper functions
- Add tests for broadcast type stability
- Coverage now includes _stabilize_mesh_type, _create_mesharray_typed,
  _infer_mesh_type, and _copyto_typed!
@hsugawa8651 hsugawa8651 force-pushed the fix-type-instability-issue78 branch from dfd3d30 to 4c2cfb2 Compare September 28, 2025 14:01
- Add test for non-concrete tuple stabilization
- Add test for same-type data handling in _create_mesharray_typed
- Improves coverage of edge cases in helper functions
- Test MeshArray construction with mesh as AbstractVector
- Covers line 149 in dense.jl (tuple conversion)
- Improves coverage for Issue numericalEFT#78 fix
@hsugawa8651 hsugawa8651 force-pushed the fix-type-instability-issue78 branch from 0e73316 to 9c2e012 Compare September 28, 2025 15:14
hsugawa8651 and others added 5 commits September 29, 2025 01:18
Implementation changes (dense.jl):
- Line 69: else branch in _stabilize_mesh_type (map(identity, mesh))
- Line 125: non-concrete mesh warning in first MeshArray constructor
- Line 155: non-concrete mesh warning in second MeshArray constructor

Test changes (test_MeshArrays.jl):
- Comment out test attempting to create non-concrete tuple (lines 108-113)

These branches are unreachable in practice because typeof() always
returns the actual runtime type, which is concrete. The code is kept
as commented out for defensive programming and documentation purposes.

Reason: typeof(x) evaluates to the runtime type, not the compile-time
inferred type, so isconcretetype(typeof(x)) is always true.
- Test conversion from Int to Float64
- Test conversion from Float64 to ComplexF64
- Covers line 175 in dense.jl (dtype != eltype(data) branch)
- Improves coverage for Issue numericalEFT#78 fix
…09-27-00-44-39-057-00783187251

CompatHelper: add new compat entry for Statistics at version 1, (keep existing compat)
…09-27-00-44-44-777-00730359842

CompatHelper: add new compat entry for DelimitedFiles at version 1, (keep existing compat)
Resolve conflict in Project.toml by combining both changes:
- Add Statistics = "1" compat entry from CompatHelper
- Keep Julia 1.10 and 1.11 compatibility
@hsugawa8651
Copy link
Contributor Author

Closing to incorporate CompatHelper dependency updates. Will resubmit with updated branch shortly.

@hsugawa8651
Copy link
Contributor Author

Reopening after incorporating CompatHelper dependency updates:

  • Added Statistics = "1" compat entry
  • Added DelimitedFiles = "1" compat entry
  • Maintains Julia 1.10 and 1.11 compatibility

@hsugawa8651 hsugawa8651 reopened this Nov 20, 2025
@hsugawa8651
Copy link
Contributor Author

Note: This PR includes the CompatHelper updates from PR #111 (DelimitedFiles) and PR #112 (Statistics), so those PRs can be closed upon merging this one.

@kunyuan
Copy link
Member

kunyuan commented Nov 21, 2025

Thank you very much for the updates!

@kunyuan kunyuan merged commit 47e5da2 into numericalEFT:master Nov 21, 2025
9 of 10 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.

Type-instability problem

3 participants