Re: [PATCH] mailinfo.c: move side-effects outside of assert

From: Jeff King <peff@peff.net>
Date: Dec 21, 2016, 15:55 UTC
Message-ID: <20161221155539.aykcmkuzqvq733ri@sigill.intra.peff.net>
In-Reply-To: <222ACFD4-ED9A-4B94-8BDD-3C70648A684B@gmail.com>

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.

Show 5 quoted lines

> > 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.

Show 8 quoted lines

> > 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