Skip to content

Add methods to bypass ConditionalOperation when condition==nothing#4639

Closed
tomchor wants to merge 19 commits intomainfrom
tc/speed-up-mean
Closed

Add methods to bypass ConditionalOperation when condition==nothing#4639
tomchor wants to merge 19 commits intomainfrom
tc/speed-up-mean

Conversation

@tomchor
Copy link
Copy Markdown
Member

@tomchor tomchor commented Jul 7, 2025

Related to #4633

Wrote this short script to time the slowdown we have (compared to directly accessing interior(u) when using mean(u) and Integral(u):

using Oceananigans, BenchmarkTools

grid = RectilinearGrid(size=(8, 8, 8), extent=(1, 1, 1), halo=(1, 1, 1));
u = XFaceField(grid);

bmean_baseline = @benchmark mean(interior(u))
bmean = @benchmark mean(u)
slowdown_mean = minimum(bmean.times) / minimum(bmean_baseline.times)
@info "Slowdown using mean(u): $slowdown_mean"= Field(Average(u))
bavg = @benchmark compute!(ū)
bsum_baseline = @benchmark sum(interior(u))
slowdown_avg = minimum(bavg.times) / minimum(bsum_baseline.times)
@info "Slowdown using compute!(ū): $slowdown_avg"

On main this produces

[ Info: Slowdown using mean(u): 8.200285408490902
[ Info: Slowdown using compute!(ū): 6.156982321501987

On this branch

[ Info: Slowdown using mean(u): 5.059299971073185
[ Info: Slowdown using compute!(ū): 1.8239949351060463

So this definitely speeds things up (when ConditionalOperation isn't needed), although there is more work to be done.

@tomchor tomchor requested a review from glwagner July 8, 2025 04:08
Comment thread src/AbstractOperations/conditional_operations.jl Outdated
Copy link
Copy Markdown
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the slow down occur?

@glwagner
Copy link
Copy Markdown
Member

glwagner commented Jul 8, 2025

If necessary we should do this, but I wonder if a better design would simply avoid creating the pointless ConditionalOperation in the first place?

@glwagner
Copy link
Copy Markdown
Member

glwagner commented Jul 8, 2025

Something is sketchy because the conditional operation should have no effect due to the constant here:

@inline evaluate_condition(::Nothing, i, j, k, grid, args...) = true

@glwagner
Copy link
Copy Markdown
Member

glwagner commented Jul 8, 2025

and moreover for example:

@propagate_inbounds Base.getindex(co::NoFuncNoConditionCO, i, j, k) = getindex(co.operand, i, j, k)

although I am not sure that method is being hit properly, maybe it isn't.

Comment thread src/AbstractOperations/metric_field_reductions.jl Outdated
Comment thread src/AbstractOperations/metric_field_reductions.jl Outdated
@simone-silvestri
Copy link
Copy Markdown
Collaborator

I think I found one issue. mean(interior(u)) is slower than it should be. Apparently, all the maps we have in interior are slowing down considerably all the computations, I ll open a different PR, then we can revisit this one. After fixing the maps, the additional cost of adding a conditional operation is concentrated in the Field constructor.

@simone-silvestri
Copy link
Copy Markdown
Collaborator

See #4668

@tomchor
Copy link
Copy Markdown
Member Author

tomchor commented Sep 22, 2025

Closing this since #4795 made this PR obsolete

@tomchor tomchor closed this Sep 22, 2025
@giordano giordano deleted the tc/speed-up-mean branch September 27, 2025 18:10
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.

3 participants