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

Some improvements to interfaces and performance #205

Closed
14 tasks done
valeriabarra opened this issue Jan 4, 2023 · 4 comments
Closed
14 tasks done

Some improvements to interfaces and performance #205

valeriabarra opened this issue Jan 4, 2023 · 4 comments
Assignees
Labels
🏅 SDI Software Design Issue

Comments

@valeriabarra
Copy link
Member

valeriabarra commented Jan 4, 2023

Purpose

This SDI proposes some clean-up of some interfaces in the current ClimaCoupler API so to improve on some of its aspects, such as type-safety and overall compiler optimization and performance.

Cost/Benefits/Risks

Costs
The costs could be shared across multiple co-authors that can concurrently work on separate files following the same improvement patterns or on the same pattern across different files.

Benefits
Benefits include:

  • an easier to read and follow interface
  • type-safety
  • reduced allocations
  • performance speed-up
  • dependencies stability
  • identification of missing interfaces in upstream packages (e.g., ClimaCore)

Risks
None identified at the moment.

People and Personnel

Components

Examples of components (not an exhaustive list) include:

In src/Utilities.jl:

In Regridder.jl:

Specifically, an example of how to avoid abstract types in structs in src/Utilities.jl is, instead of having

struct CoupledSimulation{FT, B, ...}
a::NamedTuple
...
end

we would have:

struct CoupledSimulation{FT <: Real, NT <: NamedTuple, B, ...}
a::NT
...
end

Also, it's useful to add helpers for extracting type information for types that are commonly needed in functions (e.g., the float type), for example:

float_type(::CoupledSimulation{FT}) where {FT} = FT
FT = float_type(coupler_sim)

Inputs

These inputs were kindly provided by @charleskawczynski and can be refined/modified further as we identify more points of improvement.

Results and deliverables

Better, type-safe, and more optimized interfaces in ClimaCoupler.
Update: So far the improvements have led to about 60% less allocations. See these CI perf jobs log.

Task Breakdown And Tentative Due Date

(A preliminary list of PRs and a preliminary timeline of PRs, milestones, and key results)

  • Documentation improvements fixing broken docstrings (week 1)
  • Improve on dependencies inclusions to avoid conflicted versions (week 1)
  • Remove abstract types in structs (week 2)
  • Improve type-stability of functions (week 3)
  • Reduction of allocations and identification of missing upstream interfaces (week 3 and 4)

Proposed Delivery Date

Since I am not going to work on this full time and there can be collaborative components it is a bit hard for me to propose a specific delivery date, but I could say it could take 4+ weeks.

Some potential future improvements

  • Something to keep in mind is the potential excessive requests when using @inline . This was observed in ClimaCore for instance (see this issue #1064). But it can still be beneficial for big functions or functions called multiple times.

  • Everywhere we are using directly ClimaCore internals, such as parent it means that ClimaCore is not providing that interface that users need. Identify those missing interfaces and report them upstream. Avoiding the use of parent throughout the ClimaCoupler code (which not only reduces allocation, but makes the code GPU compatible) should be a separate SDI, requiring more work.

  • QA and deliverables checked.

@valeriabarra valeriabarra added the 🏅 SDI Software Design Issue label Jan 4, 2023
@LenkaNovak LenkaNovak mentioned this issue Jan 5, 2023
13 tasks
@LenkaNovak
Copy link
Collaborator

Looks good, Valeria. Your timelines seem good. It would be good to pick one driver (e.g., experiments/AMIP/moist_mpi_earth and the modules in src/) and keep track of how the performance improves with each PR (even if just manually using @Btime for now). I will be implementing better performance logs this month, so when we move onto the less obvious performance improvements it will be more automated.

@LenkaNovak
Copy link
Collaborator

To answer your questions: FT_dot can be removed, and swap_space should be designed (or substituted) so it does not allocate.

@LenkaNovak
Copy link
Collaborator

As for timelines 4-6 days in total (as you suggest) seems reasonable. This estimate accounts for tricky dependency clash issues we've been encountering during the interface SDI.

bors bot added a commit that referenced this issue Jan 11, 2023
221: Some docstring improvements r=valeriabarra a=valeriabarra

## Purpose 
This PR addresses #220 , part of SDI #205 

Will close #220 


## To-do
- [x] Fix broken docstrings
- [x] Improve some math typesetting
- [x] Improve some other typesetting (e.g., code)

## Content
All of the above in the To-do list


I have:
- followed the codebase contribution guide: https://clima.github.io/ClimateMachine.jl/latest/Contributing/
- followed the style guide: https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/
- followed the documentation policy: https://github.com/CliMA/policies/wiki/Documentation-Policy
- checked that this PR does not duplicate an open PR.

In the Content, I have included 
- relevant unit tests, and integration tests, 
- appropriate docstrings on all functions, structs, and modules, and included relevant documentation.


- [x] I have read and checked the items on the review checklist.


Co-authored-by: Valeria Barra <valeriabarra21@gmail.com>
bors bot added a commit that referenced this issue Jan 12, 2023
223: Use relative module qualifiers for sibling modules r=charleskawczynski a=valeriabarra

## Purpose 
This is a very quick and short PR (3 lines!) that addresses the possible conflicted version of sibling module namespaces.
 
Closes #222 
(Part of #205 )

## To-do
- [x] Use relative module qualifiers in `using` and `import` statements.

## Content
- [x] In `ConservationChecker.jl` and `Regridder.jl`, avoid specifying the parent module as `ClimaCoupler`, but use the `..` path.


Review checklist

I have:
- followed the codebase contribution guide: https://clima.github.io/ClimateMachine.jl/latest/Contributing/
- followed the style guide: https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/
- followed the documentation policy: https://github.com/CliMA/policies/wiki/Documentation-Policy
- checked that this PR does not duplicate an open PR.

In the Content, I have included 
- relevant unit tests, and integration tests, 
- appropriate docstrings on all functions, structs, and modules, and included relevant documentation.

- [x] I have read and checked the items on the review checklist.


Co-authored-by: Valeria Barra <valeriabarra21@gmail.com>
bors bot added a commit that referenced this issue Feb 7, 2023
253: Remove FT_dot r=valeriabarra a=valeriabarra

## Purpose 

The purpose of this PR (a simple one-liner!) is to remove `FT_dot` since it was allocating and referencing an undefined variable. 

This is part of the clean-up/perf improvement work in #205 

Closes #252 

## Content
- Removes the `FT_dot` function.
 
Review checklist

I have:
- followed the codebase contribution guide: https://clima.github.io/ClimateMachine.jl/latest/Contributing/
- followed the style guide: https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/
- followed the documentation policy: https://github.com/CliMA/policies/wiki/Documentation-Policy
- checked that this PR does not duplicate an open PR.

In the Content, I have included 
- relevant unit tests, and integration tests, 
- appropriate docstrings on all functions, structs, and modules, and included relevant documentation.


- [x] I have read and checked the items on the review checklist.


Co-authored-by: Valeria Barra <valeriabarra21@gmail.com>
@valeriabarra
Copy link
Member Author

Cumulatively, the work to reduce allocations and to improve type-safety in this SDI have reduced allocations of about 60% (as it can also be seen from this build log)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏅 SDI Software Design Issue
Projects
None yet
Development

No branches or pull requests

5 participants