-
Notifications
You must be signed in to change notification settings - Fork 7
remove allocations in combine_surfaces #1595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
This change actually slightly worsens performance. I think it's because of runtime dispatch on the |
|
Ok, with the current state of this PR it does speed up simulations. For example comparing this build to main we see the following SYPD changes:
However runs with integrated land has the opposite trend:
|
|
physical results |
|
I did some investigating into why this PR slightly slows down the benchmark, and it comes down to the following. When we swap the roles of
julia> @allocated Interfacer.get_field!(csf.scalar_temp1, sim, Val(:area_fraction))
160
julia> @allocated Interfacer.get_field(sim, Val(:area_fraction), boundary_space)
32this is because:
julia> @allocated Interfacer.remap!(field2, field1)
128
julia> @allocated Interfacer.remap(field1, boundary_space)
32
julia> @allocated combined_field .+=
area_fraction .*
ifelse.(area_fraction .≈ 0, zero(combined_field), surface_field)
6544
julia> @allocated combined_field .+=
area_fraction .*
ifelse.(area_fraction .≈ 0, zero(eltype(combined_field)), surface_field)
272
julia> @allocated combined_field .+=
area_fraction .*
ifelse.(area_fraction .≈ 0, 0, surface_field)
272 |
Purpose
Before we were calling the allocating
get_fieldmethod. Now we remap and store the surface field in a temp coupler field, so we don't allocate. To me it's also more readable now which is nice.Content