Oceananigans.jl icon indicating copy to clipboard operation
Oceananigans.jl copied to clipboard

Remove performance-/precompilation-time harmful `@eval`

Open simone-silvestri opened this issue 1 year ago • 4 comments

this PR fixes the issue of having @eval inside callable functions not resolved at compile time

closes #3555

cc @vchuravy

simone-silvestri avatar Apr 18 '24 14:04 simone-silvestri

The next one I ran into is https://github.com/CliMA/Oceananigans.jl/blob/00f028bb37f13692e24921588aeb8a9150f6dd55/src/Advection/reconstruction_coefficients.jl#L215

So one alternative is to use named tuples instead of variables.

@inline function compute_reconstruction_coefficients(grid, FT, scheme; order)

    method = scheme == :Centered ? 1 : scheme == :Upwind ? 2 : 3

    rect_metrics = (:xᶠᵃᵃ, :xᶜᵃᵃ, :yᵃᶠᵃ, :yᵃᶜᵃ, :zᵃᵃᶠ, :zᵃᵃᶜ)

    if grid isa Nothing
        coeffs = (; (m => nothing for m in rect_metrics)...)
    else
        metrics = coordinates(grid)
        dirsize = (:Nx, :Nx, :Ny, :Ny, :Nz, :Nz)

        arch       = architecture(grid)
        Hx, Hy, Hz = halo_size(grid)
        new_grid   = with_halo((Hx+1, Hy+1, Hz+1), grid)
        coeffs = (; (rect_metric => calc_reconstruction_coefficients(
                FT, getfield(new_grid, metric), arch, getfield(new_grid, dir), Val(method); order = order)
            for (dir, metric, rect_metric) in zip(dirsize, metrics, rect_metrics))...)
    end

    return tuple(coeffs...) # actually return named tuple to be order invariant
end

vchuravy avatar Apr 18 '24 16:04 vchuravy

I think we need to look at all eval usage

I hit:

function minimum_spacing(dir, grid, ℓx, ℓy, ℓz)
    spacing = eval(Symbol(dir, :spacing))
    LX, LY, LZ = map(destantiate, (ℓx, ℓy, ℓz))
    Δ = KernelFunctionOperation{LX, LY, LZ}(spacing, grid, ℓx, ℓy, ℓz)

    return minimum(Δ)
end

next. Which probably should be getglobal?

vchuravy avatar Apr 19 '24 17:04 vchuravy

@simone-silvestri are you going to shepherd this through to completion?

glwagner avatar May 14 '24 17:05 glwagner

I couldn't find any other @eval inside functions so I think we are clear. @vchuravy can you confirm?

simone-silvestri avatar May 15 '24 00:05 simone-silvestri

this should be ready to merge

simone-silvestri avatar Jun 19 '24 19:06 simone-silvestri