Optimizes fill_halo_regions_open.jl

[jagoosw]

I was doing some profiling on a model with no open boundaries and discovered that this function was causing a big slow down. I guess this is because the compiler isn't managing to work out its just a load of nothing operations but this change appears to make it completely go away.

[simone-silvestri]

what was the slowdown?

[glwagner]

Might be good to profile with fill_open_boundary_regions!(fields::NTuple, boundary_conditions, indices, loc, grid, args...; kwargs...) = nothing also to check this

[glwagner]

The profiles should be documented here. I'll tentatively approve but the documentation is important

[jagoosw]

Ah so I've realised this isn't the fix we needed, and I was just hiding it from myself in the profile because I replaced the function by writing it in the REPL. I made an MWE:

using Oceananigans

grid = RectilinearGrid(GPU(), topology = (Flat, Flat, Bounded), size = (100, ), extent = (400, ))

model = HydrostaticFreeSurfaceModel(; grid, velocities = PrescribedVelocityFields(), momentum_advection=nothing, buoyancy=nothing, tracers = ntuple(n->Symbol(:T, n), Val(30)))

You can see from this profile that `fill_open_boundary_regions!` takes a lot longer than `fill_halo_event!`, even though there are no velocity open boundaries. This is because it is launching a load of zero size kernels where as `fill_halo_event!` just returns nothing instead.

I've fixed this now and get this from the profile instead:

In numbers, the original version benchmarks time_step! at around 4.074 ms ± 581.472 μs and the new version 2.438 ms ± 501.642 μs

[glwagner]

I think it's good to go.

The only thing that doesn't quite make sense to me is why

fill_size = fill_halo_size(field, regular_fill_function, indices, boundary_conditions, loc, grid)

depends on regular_fill_function, since

fill_function, regular_fill_function = get_open_halo_filling_functions(loc) 

and loc is an argument to both functions. It doesn't seem that from a purely logical point of view we need regular_fill_function at all here.

[jagoosw]

I'll stream line those two line before I merge

[simone-silvestri]

Is this PR close to merging?

[tomchor]

cc @ali-ramadhan

[simone-silvestri]

I think this has been superseded by #3834. @jagoosw let us know if it is the case or not.

[jagoosw]

I think this has been superseded by #3834. @jagoosw let us know if it is the case or not.

Yeah definitely superseded!