Skip to content

Jjhursey topic/timer gettimeofday #3201

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

Merged
merged 4 commits into from
Mar 19, 2017

Conversation

hppritcha
Copy link
Member

add some PR feedback to @jjhursey 's original PR #3184

@jsquyres this is it. have commitment this evening so its this or wait till tomorrow.

jsquyres and others added 3 commits March 15, 2017 21:03
This variable is only used in one file, so make it static.

Signed-off-by: Jeff Squyres <[email protected]>
Several component-specific functions were named with a prefix of
"opal_timer_base", which was quite confusing.  Rename them to have a
prefix "opal_timer_linux" to make it clear that they are here in this
component (and different than *actual* opal_timer_base symbols).

Signed-off-by: Jeff Squyres <[email protected]>
 * See open-mpi#3003 for a discussion about
   this patch. Once we get a better version in place we can revert this
   change.

Signed-off-by: Joshua Hursey <[email protected]>
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

The additional merge commit here in this PR is a little odd, too. Can that be removed?

@@ -40,6 +41,12 @@ double MPI_Wtick(void)
{
OPAL_CR_NOOP_PROGRESS();

/*
* See https://github.com/open-mpi/ompi/issues/3003
* For now we are forcing the use of gettimeofday() until we find a
Copy link
Member

Choose a reason for hiding this comment

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

This comment is no longer correct.

#endif
#else
#if defined(__linux__) && OPAL_HAVE_CLOCK_GETTIME
/* QAD no call clock_getres */
Copy link
Member

Choose a reason for hiding this comment

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

What's "QAD" -- why not call clock_getres?

Copy link
Member Author

Choose a reason for hiding this comment

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

quick and dirty

#if defined(__linux__) && OPAL_HAVE_CLOCK_GETTIME
struct timespec tp = {.tv_sec = 0, .tv_nsec = 0};
(void) clock_gettime(CLOCK_MONOTONIC, &tp);
return (tp.tv_sec + tp.tv_nsec/1.0e+9);
Copy link
Member

Choose a reason for hiding this comment

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

This return is not quite right -- all paths through the original code assign wtime and fall through to call OPAL_CR_NOOP_PROGRESS.

I know the checkpoint/restart code has fallen into disrepair, but the disparity of treatment here is a little odd.

@hppritcha hppritcha force-pushed the jjhursey-topic/timer-gettimeofday branch from 2d8482c to c564ab5 Compare March 18, 2017 20:01
@hppritcha
Copy link
Member Author

@jsquyres updated per your feedback. the original merge commit was showing up because I did a merge of @jjhursey stuff on to a fork of master - i.e. doing github PR like thing by hand.

@hppritcha hppritcha force-pushed the jjhursey-topic/timer-gettimeofday branch from c564ab5 to 12bd4e9 Compare March 18, 2017 20:06
better solution needed later
 workaround for open-mpi#3003

Signed-off-by: Howard Pritchard <[email protected]>
@hppritcha hppritcha force-pushed the jjhursey-topic/timer-gettimeofday branch from 12bd4e9 to b933152 Compare March 18, 2017 20:09
@jsquyres jsquyres merged commit ce0e1cd into open-mpi:master Mar 19, 2017
@hppritcha hppritcha deleted the jjhursey-topic/timer-gettimeofday branch May 2, 2018 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants