Skip to content
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

Reduce allocations #967

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Reduce allocations #967

wants to merge 2 commits into from

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Dec 24, 2024

This commit reduces allocations by changing turbulent_fluxes and net_radiation to be in place. In addition to this, the workaround introduced to enable GPU-compatibility in energy_hydrology resulted in lots of additional allocations, so I modified that too.

These allocations are due to the fact that there are broadcasted expression hidden in simple function calls, so that Julia cannot properly do operations in place.

There are still allocations. One of the easy ones to tackle is in surface_specificy_humidity for snow, which accounts for a large fraction of the total allocations for the snowy_land benchmark.

Closes #966

@Sbozzolo Sbozzolo added the Run benchmarks Add this label to run benchmarks on clima label Dec 24, 2024
@Sbozzolo Sbozzolo force-pushed the gb/allocations branch 3 times, most recently from 7c32282 to 9c0c61a Compare December 24, 2024 20:28
@Sbozzolo Sbozzolo force-pushed the gb/allocations branch 14 times, most recently from d6fb842 to dabcb52 Compare January 7, 2025 00:39
@Sbozzolo Sbozzolo requested a review from kmdeck January 7, 2025 00:39
@Sbozzolo Sbozzolo force-pushed the gb/allocations branch 5 times, most recently from 91d014d to fbe6886 Compare January 7, 2025 18:09
Copy link
Member

@kmdeck kmdeck left a comment

Choose a reason for hiding this comment

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

I think this looks good - to be thorough, maybe we can compare the buildkite (regular CI) plots for e.g. Ozark, snow col de porte, and snow soil to the current main before merging

@kmdeck
Copy link
Member

kmdeck commented Jan 7, 2025

also, if you dont mind, you can assign me an issue re dealing with the snow specific humidity allocation and the allocation in HydrologyEarthParameters

JULIA_CUDA_MEMORY_POOL="none" disables the CUDA memory pool. This is required
for MPI runs with certain versions of MPI, but it hurts performance
@Sbozzolo Sbozzolo force-pushed the gb/allocations branch 3 times, most recently from 4f63af6 to a74687b Compare January 7, 2025 22:42
@Sbozzolo Sbozzolo requested a review from kmdeck January 8, 2025 15:32
This commit reduces allocations by changing turbulent_fluxes and net_radiation
to be in place. In addition to this, the workaround introduced to enable
GPU-compatibility in energy_hydrology resulted in lots of additional
allocations, so I modified that too.

There are still allocations. One of the easy ones to tackle is in
surface_specificy_humidity for snow, which accounts for a large fraction of the
total allocations for the snowy_land benchmark
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run benchmarks Add this label to run benchmarks on clima
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants