Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

On 12.01.2013 20:42, Andres Freund wrote:
> On 2013-01-12 13:16:56 -0500, Tom Lane wrote:
>> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com> writes:
>>> Here's one more construct to consider:
>>
>>> #define ereport_domain(elevel, domain, rest) \
>>> do { \
>>> const int elevel_ = elevel; \
>>> if (errstart(elevel_,__FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ||
>>> elevel_>= ERROR) \
>>> { \
>>> (void) rest; \
>>> if (elevel_>= ERROR) \
>>> errfinish_noreturn(1); \
>>> else \
>>> errfinish(1); \
>>> } \
>>> } while(0)
>>
>> I don't care for that too much in detail -- if errstart were to return
>> false (it shouldn't, but if it did) this would be utterly broken,
>> because we'd be making error reporting calls that elog.c wouldn't be
>> prepared to accept. We should stick to the technique of doing the
>> ereport as today and then adding something that tells the compiler we
>> don't expect to get here; preferably something that emits no added code.

With the current ereport(), we'll call abort() if errstart returns false
and elevel >= ERROR. That's even worse than making an error reporting
calls that elog.c isn't prepared for. If we take that risk seriously,
with bogus error reporting calls we at least have chance of catching it
and reporting an error.

> Yea, I didn't really like it all that much either - but anyway, I have
> yet to find *any* version with a local variable that doesn't lead to
> 200kb size increase :(.

Hmm, that's strange. Assuming the optimizer optimizes away the paths in
the macro that are never taken when elevel is a constant, I would expect
the resulting binary to be smaller than what we have now. All those
useless abort() calls must take up some space.

Here's what I got with and without my patch on my laptop:

-rwxr-xr-x 1 heikki heikki 24767237 tammi 12 21:27 postgres-do-while-mypatch
-rwxr-xr-x 1 heikki heikki 24623081 tammi 12 21:28 postgres-unpatched

But when I build without --enable-debug, the situation reverses:

-rwxr-xr-x 1 heikki heikki 5961309 tammi 12 21:48 postgres-do-while-mypatch
-rwxr-xr-x 1 heikki heikki 5986961 tammi 12 21:49 postgres-unpatched

I suspect you're also just seeing bloat caused by more debugging
symbols, which won't have any effect at runtime. It would be nice to
have smaller debug-enabled builds, too, of course, but it's not that
important.

>> However, using a do-block with a local variable is definitely something
>> worth considering. I'm getting less enamored of the __builtin_constant_p
>> idea after finding out that the gcc boys seem to have curious ideas
>> about what its semantics ought to be:
>> https://bugzilla.redhat.com/show_bug.cgi?id=894515
>
> I wonder whether __builtin_choose_expr is any better?

I forgot to mention (and it wasn't visible in the snippet I posted) the
reason I used __attribute__ ((noreturn)). We already use that attribute
elsewhere, so this optimization wouldn't require anything more from the
compiler than what already take advantage of. I think that noreturn is
more widely supported than these other constructs. Also, if I'm reading
the MSDN docs correctly, while MSVC doesn't understand __attribute__
((noreturn)) directly, it has an equivalent syntax _declspec(noreturn).
So we could easily support MSVC with this approach.

Attached is the complete patch I used for the above tests.

- Heikki