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.

Sign up for GitHub

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

Jump to bottom

Define copyto! for diagonalized field vectors #478

Merged
merged 1 commit into from
Jan 27, 2022
Merged

Define copyto! for diagonalized field vectors #478

merged 1 commit into from
Jan 27, 2022

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Jan 27, 2022 •
edited
Loading

Based on some analysis, we're hoping this improves performance a bit. cc @simonbyrne

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jan 27, 2022
@bors
Try #478:
a545eb0
@bors
Copy link
Contributor

bors bot commented Jan 27, 2022

try

Build succeeded:

simonbyrne
simonbyrne reviewed Jan 27, 2022
View reviewed changes
Copy link
Member

@simonbyrne simonbyrne left a comment

Choose a reason for hiding this comment

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

Why not add it to src?

@charleskawczynski
Copy link
Member Author

charleskawczynski commented Jan 27, 2022 •
edited
Loading

@simonbyrne, @szy21, @jiahe23, @dennisYatunin, it looks like the performance for our implicit cases are improved with this PR:

                                                   |  main   | this PR
examples/3dsphere/baroclinic_wave_rho_etot.jl      | 23m 23s | 10m 18s
examples/3dsphere/baroclinic_wave_rho_theta.jl     | 23m 57s | 10m 21s
examples/3dsphere/held_suarez_rho_etot.jl          | 24m 21s | 12m 11s
examples/3dsphere/held_suarez_rho_theta.jl         | 26m 14s | 10m 30s

Also, allocations in the implicit baroclinic wave go from 968.233 GiB to 7.197 GiB. 🎉

@charleskawczynski
Copy link
Member Author

Why not add it to src?

Oh, whoops 😆, I thought I did.

@charleskawczynski
Copy link
Member Author

Well, now I'm curious if any other cases might improve from this.

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jan 27, 2022
@bors
Try #478:
a39bac0
@bors
Copy link
Contributor

bors bot commented Jan 27, 2022

try

Build succeeded:

@charleskawczynski charleskawczynski changed the title Define copyto for diagonalized field vectors Define copyto! for diagonalized field vectors Jan 27, 2022
@charleskawczynski
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 27, 2022

Build succeeded:

@bors bors bot merged commit eac7eaf into main Jan 27, 2022
@bors bors bot deleted the ck/perf branch January 27, 2022 03:29
@charleskawczynski
Copy link
Member Author

charleskawczynski commented Jan 27, 2022 •
edited
Loading

Ok, maybe it'll be helpful to look at Rising bubble 2D hybrid in the same way, since that one still allocates 1TB (which is very strange for a 2D case). Ah, and some other cases, too.

@charleskawczynski
Copy link
Member Author

Related issue: #275

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbyrne simonbyrne simonbyrne approved these changes

Assignees
No one assigned
Labels
performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@charleskawczynski @simonbyrne