On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:
> > I wasn't aware anybody actually built with NDEBUG at all. You'd have to
> > explicitly ask for it via CFLAGS, so I assume most people don't.
>
> Not a good assumption. You know what happens when you assume[1], right? ;)
Kind of. If it's a configuration that nobody[1] in the Git development
community intended to support or test, then isn't the person triggering
it the one making assumptions?
At any rate, I agree that setting NDEBUG should not create a broken
program, and some solution like your patch is a good idea here. I was
mainly speaking to the "do not bother" comment. It is not that I do not
bother to build with NDEBUG, it is that I think it is actively a bad
idea.
[1] Maybe I am alone in my surprise, and everybody working on Git is
using assert() with the intention that it can be disabled. But if
that were the case, I'd expect more push-back against "die(BUG)"
which does not have this feature. I don't recall a single discussion
to that effect, and searching for NDEBUG in the list archives turns
up hardly any mentions.
> I've been defining NDEBUG whenever I make a release build for quite some
> time (not just for Git) in order to squeeze every last possible drop of
> performance out of it.
I think here you are getting into superstition. Is there any single
assert() in Git that will actually have an impact on performance?
I'd be more impressed if you could show some operation that is faster
when built with NDEBUG than without. Running all of t/perf does not seem
to show any difference, and looking at the asserts themselves, they're
almost all single-instruction compares in code that isn't performance
critical anyway.
> > So from my perspective it is not so much "do not bother with release
> > builds" as "are release builds even a thing for git?"
>
> They should be if you're deploying Git in a performance critical
> environment.
I hope my history of patches shows that I do care about deploying Git in
a performance critical environment. But I only care about performance
tradeoffs that have a _measurable_ gain.
> Perhaps Git should provide a "verify" macro. Works like "assert" except
> that it doesn't go away when NDEBUG is defined. Being Git-provided it could
> also use Git's die function. Then Git could do a global replace of assert
> with verify and institute a no-assert policy.
What would be the advantage of that over `if(...) die("BUG: ...")`? It
does not require you to write a reason in the die(), but I am not sure
that is a good thing.
> > I do notice that we set NDEBUG for nedmalloc, though if I am reading the
> > Makefile right, it is just for compiling those files. It looks like
> > there are a ton of asserts there that _are_ potentially expensive, so
> > that makes sense.
>
> So there's no way to get a non-release build of nedmalloc inside Git then
> without hacking the Makefile? What if you need those assertions enabled?
> Maybe NDEBUG shouldn't be defined by default for any files.
AFAICT, yes. I'd leave it to people who actually build with nedmalloc to
decide whether it is worth caring about, and whether the asserts there
have a noticeable performance impact.
-Peff