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

Fix memory issues #1113

Merged
merged 1 commit into from
Feb 1, 2023
Merged

Fix memory issues #1113

merged 1 commit into from
Feb 1, 2023

Conversation

dennisYatunin
Copy link
Member

@dennisYatunin dennisYatunin commented Jan 31, 2023

This PR reduces ClimaCore memory usage due to excessive code inlining, and it closes #1064.

Specifically, it

  • Removes the @inline annotations to Base.copyto! that were added in Inline copyto! to fix Ref allocations #945 and Extend and fix Ref broadcasting with StencilCoefs #978.
  • Removes all slurm_mem increases from the CI pipeline, so that the default value of 8GB applies to every task.
  • Extends broadcasting over Fields and DataLayouts so that it can handle single-element Tuples. In theory, broadcasting with a single-element Tuple should be equivalent to broadcasting with a Ref. However, since a Ref is mutable, it looks like the compiler needs to perform runtime allocations when using a Ref, unless we force it to inline the majority of the broadcasting machinery.
  • Replaces every Ref in test/Fields/field_opt.jl with a single-element Tuple, so that the runtime allocation unit tests can pass.
  • Replaces two of the @test_brokens in test/Operators/finitedifference/opt_examples.jl with @tests, since those tests are now passing, even though they don't appear to have anything to do with Refs.

Note: It is still necessary to go through the rest of our code (both in ClimaCore and elsewhere) and replace every Ref in a Field/DataLayout broadcast with a single-element Tuple. Now that the @inline annotations have been removed, broadcasting with a Ref will trigger runtime allocations, which will pose a problem for our long runs.

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

@simonbyrne
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Feb 1, 2023
@dennisYatunin
Copy link
Member Author

bors try

@dennisYatunin dennisYatunin marked this pull request as ready for review February 1, 2023 20:19
bors bot added a commit that referenced this pull request Feb 1, 2023
@bors
Copy link
Contributor

bors bot commented Feb 1, 2023

try

Build failed:

@dennisYatunin
Copy link
Member Author

bors r+

@bors bors bot merged commit 40ac99d into main Feb 1, 2023
@bors bors bot deleted the dy/memory branch February 1, 2023 23:51
@charleskawczynski
Copy link
Member

I think it would have been better if we had added new tests in this PR, and converted any passing tests to failing tests, rather than change existing tests, which makes understanding the changes more complicated.

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.

Simple integration tests/examples cannot be run locally for excessive memory usage
3 participants