-
Notifications
You must be signed in to change notification settings - Fork 19
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
Group NCDatasets.sync calls #2828
Conversation
07e07f6
to
bd47df2
Compare
I think we should also avoid calling sync for every diagnostic. Instead, let's flush the output once per diagnostic cycle. As in #2623 |
9e9521d
to
4c87928
Compare
Yeah, I agree, but first I'd like to see how this performs, since it's functionally equivalent. Updating it to be called every diagnostics lcd cycle is a much simpler change than this is turning out to be. |
Do you mean functionally equivalent to main? |
4c87928
to
a605698
Compare
Yeah. It's technically different in terms of order of compute / io, but it should at least be calling |
95b2a8b
to
269bfab
Compare
writer = diag.output_writer | ||
if writer isa NetCDFWriter && sync_needed(step, diag) | ||
output_path = outpath_name(output_dir, diag) | ||
# TODO: find out why Threads.@spawn segfaults in multithreaded IO test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something we might want to keep in mind is this:
https://juliaparallel.org/MPI.jl/stable/reference/environment/#MPI.ThreadLevel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It should not be a problem at the moment because sync does not contain an MPI call.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought that only the root process does this, but it's a good point.
7d71b4e
to
5ea2d97
Compare
5ea2d97
to
01be37c
Compare
In light of #2827, this PR hoists the
NCDatasets.sync
to a separate callback so that we can (eventually) and useThreads.@spawn
to leverage threads (if we have them).I've also named/annotated
compute_callback
andoutput_callback
, so that they show up in the NVTX reports.