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

Add timing output to the end of ngen runs #696

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

PhilMiller
Copy link
Contributor

@PhilMiller PhilMiller commented Jan 3, 2024

This will allow for slightly more informative benchmark results, in which we can distinguish different elements of the runtime, and extrapolate accordingly.

Changes

  • Simplify the mixed MPI/Routing #ifdef logic
  • Add time calls and prints

Testing

  1. Unit testing still passes

Screenshot

From the CI job running on example data:

Running timestep 700
Finished 720 timesteps.
NGen top-level timings:
	NGen::init: 0.207376
	NGen::simulation: 0.230584
	NGen::routing: 1.232e-06

Todos

  • Improve numeric formatting

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux
  • macOS

The inter-mixed conditions of MPI and Routing being active were confusing.

Additionally, running routing after calling MPI_Finalize is
inadvisable. Per the MPICH documentation,

> The number of processes running after this routine is called is
> undefined; it is best not to perform much more than a return rc
> after calling MPI_Finalize.
@PhilMiller
Copy link
Contributor Author

The refactoring of the ifdef logic was because I wanted to use the same test for printing the timings, and didn't want to make a mess there.

src/NGen.cpp Outdated Show resolved Hide resolved
src/NGen.cpp Outdated Show resolved Hide resolved
src/NGen.cpp Show resolved Hide resolved
src/NGen.cpp Outdated Show resolved Hide resolved
@PhilMiller
Copy link
Contributor Author

Ugh, and of course in tweaking things between my last local build and pushing, I broke it a bit. That'll be fixed shortly.

@ZacharyWills
Copy link
Contributor

This will greatly help with hardware affinity testing. Ideally we should be able to give some rough numbers about the relationship between domain extent, available hardware, and expected runtime.

Thanks,
Zach

@robertbartel
Copy link
Contributor

I'll leave it to others to formally approve, but I like it overall. Thanks for putting this together @PhilMiller.

@PhilMiller
Copy link
Contributor Author

Ideally, the textual numeric output would be presented a little more readably, using something like https://github.com/martinmoene/EngFormat-Cpp but that's an inessential refinement, compared to just getting meaningful measurements in place.

All previous comments on the code addressed.

@PhilMiller PhilMiller merged commit b357ee5 into NOAA-OWP:master Jan 8, 2024
19 checks passed
@PhilMiller PhilMiller deleted the phil/timing branch January 8, 2024 19:36
@ZacharyWills
Copy link
Contributor

@PhilMiller
Just for clarity, what units are the timings in?

	NGen::init: 0.207376
	NGen::simulation: 0.230584
	NGen::routing: 1.232e-06

@PhilMiller
Copy link
Contributor Author

@ZacharyWills Seconds

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.

4 participants