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

RFC: change default build to always be optimized (even for developers) #1417

Merged
merged 2 commits into from
Mar 17, 2016

Conversation

jsquyres
Copy link
Member

@jsquyres jsquyres commented Mar 2, 2016

A not-uncommon complaint we hear is someone who gets lower-than-expected performance, and it is later discovered that they used a debugging build. This commit enables ortertun to automatically print a stern warning/admonishion to not use this build for performance tests if it is a debug build. You can use the orterun --nowarn-debug-build CLI option to disable this message, or export OMPI_MCA_nowarn_debug_build=1.

The rationale for this option is to make it totally, completely obvious that you are using a debug build. Yes, this extra output will screw up the output of running benchmarks and other applications. That's kinda the point -- if you're running benchmarks / parsing stdout to get performance data, you shouldn't be using a debugging build, and we want to smack you in the face with that fact.

That being said, there are many useful scenarios for a debug build. Hence, you can disable the warning with an mpirun CLI option and/or setting an env var.

Comments?

@jsquyres jsquyres added this to the v2.1.0 milestone Mar 2, 2016
@rhc54
Copy link
Contributor

rhc54 commented Mar 2, 2016

Why not a different, less annoying approach? The comments in the code explicitly state that defaulting to a debug build when using a repo was done for "early development" purposes. So why not just turn that off? It would be much less annoying to have to remember to enable-debug.

@jsquyres
Copy link
Member Author

jsquyres commented Mar 2, 2016

Yeah, I just replied on the mailing list -- I think we agreed to do this last week in Dallas, anyway (disable debug-by-default builds). I think these could still be complimentary features though -- if debug builds are not common (because they're not the default), then perhaps a warning like this is less obtrusive, anyway.

I'll add a "don't do a debug build by default" commit to this PR.

@jsquyres
Copy link
Member Author

jsquyres commented Mar 2, 2016

Ok, I added a 2nd commit that disables debug-build-by-default behavior if a .git directory is found.

@rhc54
Copy link
Contributor

rhc54 commented Mar 2, 2016

I believe @gpaulsen was going to do it, but I see you've done it here. Pretty trivial change.

I would advocate to remove the warning and just do your 2nd commit.

@gpaulsen
Copy link
Member

gpaulsen commented Mar 2, 2016

No worries. I made a comment on the commit. Is that easily visible to you Jeff? I assume it emailed you.
Is this for 2.0?

@jsquyres jsquyres force-pushed the rfc/mpirun-warn-if-debug-build branch from f38fdad to b47f2e5 Compare March 8, 2016 16:56
@jsquyres
Copy link
Member Author

jsquyres commented Mar 8, 2016

@gpaulsen The milestone on this PR is v2.1.0.

Per discussion on the call today, it seems like everyone is comfortable with:

  • Change the default to build optimized (i.e., never magically make a debug build -- debug builds now must be specifically requested via --enable-debug --enable-mem-debug --enable-picky)
  • Remove the warning that was initially added in this PR

I.e., you only get a debug build if you ask for it, and if you asked for it, we don't need to give you a warning.

PR has been updated.

@goodell
Copy link
Member

goodell commented Mar 10, 2016

  • Change the default to build optimized

I'm very happy to see this change, I've found it to be surprising since basically forever.

@jsquyres jsquyres changed the title RFC: orterun: warn if this is a debug build RFC: change default build to always be optimized (even for developers) Mar 10, 2016
@ggouaillardet
Copy link
Contributor

👍
i would also have mpirun --version and the likes clearly state this is a debug build

@gpaulsen
Copy link
Member

👍

@jsquyres
Copy link
Member Author

Point made 15 Mar 2016 webex: it would be nice to leave --enable-picky enabled by default for developer builds.

Ok, will do.

After 11 years, it's probably ok to say that we're no longer in "early
development" -- disable the "build a debug version of Open MPI by
default if we find a .git directory" behavior.

However, we are keeping the "use compiler picky flags if we find a
.git directory" behavior.  That's useful behavior for developers, and
has no effect on performance.
Make the help messages for --enable-mem-debug and --enable-mem-profile
the same as other help messages.
@jsquyres jsquyres force-pushed the rfc/mpirun-warn-if-debug-build branch from b47f2e5 to 54687d0 Compare March 15, 2016 23:51
@jsquyres
Copy link
Member Author

PR updated per the above comment.

@jsquyres
Copy link
Member Author

bot:retest

@jsquyres
Copy link
Member Author

@miked-mellanox Is the Mellanox Jenkins busted at the moment?

jsquyres added a commit that referenced this pull request Mar 17, 2016
RFC: change default build to always be optimized (even for developers)
@jsquyres jsquyres merged commit cb1837e into open-mpi:master Mar 17, 2016
@jsquyres jsquyres deleted the rfc/mpirun-warn-if-debug-build branch March 17, 2016 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants