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

Bugfix/mpi wtime profiler #317

Merged
merged 38 commits into from
Nov 2, 2023
Merged

Bugfix/mpi wtime profiler #317

merged 38 commits into from
Nov 2, 2023

Conversation

balos1
Copy link
Member

@balos1 balos1 commented Aug 10, 2023 •
edited
Loading

Todo:

  • Check profiling unit test output on windows

@balos1 balos1 removed the dont-merge label Aug 11, 2023
@balos1
Copy link
Member Author

balos1 commented Aug 11, 2023

Fixes #312.

@balos1 balos1 added this to the SUNDIALS Next milestone Aug 11, 2023
gardner48
gardner48 requested changes Sep 5, 2023
View reviewed changes
Copy link
Member

@gardner48 gardner48 left a comment

Choose a reason for hiding this comment

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

Looks like all the other MPI calls are guarded by checks for a NULL comm, so I think this will resolve the issue but we should add a CI test with MPI and profiling enabled to test this as well.

cmake/SundialsSetupCompilers.cmake Outdated Show resolved Hide resolved
src/sundials/sundials_profiler.c Outdated Show resolved Hide resolved
@balos1
Copy link
Member Author

balos1 commented Sep 5, 2023

I need to add a unit test which does a sleep or something to verify the timer is giving the value we would expect.

@gardner48
Copy link
Member

gardner48 commented Sep 5, 2023 •
edited
Loading

I need to add a unit test which does a sleep or something to verify the timer is giving the value we would expect.

Take a look at the profiler-csv-output branch I've added a test like that under test/unit_tests/profiling

@balos1 balos1 modified the milestones: SUNDIALS 6.6.1, SUNDIALS Next Sep 5, 2023
@gardner48
Copy link
Member

gardner48 commented Oct 29, 2023 •
edited
Loading

The CI is failing with

/var/lib/jenkins/workspace/SUNDIALS_GitHub_Test_PR-317/test/tarballs/sundials-6.6.1/include/sundials/sundials_profiler.h:122:3: error: C++ style comments are not allowed in ISO C90 [-Werror]
 } // namespace sundials

Maybe there's a misplaced #ifdef __cplusplus. For some of the other classes we've started putting the convenience wrappers for C++ into a .hpp file. We should probably do the same here as well.

@balos1 balos1 requested a review from gardner48 October 30, 2023 18:46
gardner48
gardner48 requested changes Oct 30, 2023
View reviewed changes
Copy link
Member

@gardner48 gardner48 left a comment

Choose a reason for hiding this comment

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

A couple minor changes, otherwise this looks good to me

include/sundials/sundials_profiler.h Show resolved Hide resolved
doc/shared/sundials/Profiling.rst Outdated Show resolved Hide resolved
@balos1
Copy link
Member Author

balos1 commented Oct 30, 2023 •
edited
Loading

I have addressed #139 in this too so that all of the unit tests can build on Windows.

@gardner48 gardner48 merged commit 26317e2 into develop Nov 2, 2023
16 checks passed
@gardner48 gardner48 deleted the bugfix/mpi-wtime-profiler branch November 2, 2023 05:08
This was referenced Nov 2, 2023
Closed
Closed
gardner48 added a commit that referenced this pull request Dec 18, 2023
@balos1 @gardner48
Bugfix: Remove MPI wtime in profiler (#317)
3719cd2 
Updated `SUNProfiler` to note rely on `MPI_WTime`. Fixes #312

---------

Co-authored-by: David Gardner <[email protected]>
balos1 added a commit that referenced this pull request Dec 18, 2023
@balos1 @gardner48
Bugfix: Remove MPI wtime in profiler (#317)
dac7cd9 
Updated `SUNProfiler` to note rely on `MPI_WTime`. Fixes #312

---------

Co-authored-by: David Gardner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gardner48 gardner48 gardner48 approved these changes

@drreynolds drreynolds drreynolds left review comments

@cswoodward cswoodward Awaiting requested review from cswoodward cswoodward is a code owner

Assignees
No one assigned
Labels
archos-windows bug pkg-core
Projects
None yet
Milestone
SUNDIALS v6.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@balos1 @gardner48 @drreynolds