On 2015-12-20 02:49:13 +1300, David Rowley wrote: > So I choose to ignore that, and give
> it a try anyway... elog(ERROR) for example, surely that branch should > always be in a cold path? ... ereport() too perhaps?
Hm, not exactly what you were thinking of, but it'd definitely be interesting to add unlikely annotations to debug elogs, in some automated manner (__builtin_choose_expr?). I've previously hacked elog/ereport to only support >= ERROR, and that resulted in a speedup, removing a log of elog(DEBUG*) triggered jumps.
Good thinking. It would be interesting to see benchmarks with the hacked version of elog()
To make sure I understand correctly: Before doing that you manually added the errcheck()'s manually? At each elog(ERROR) callsite?
Yeah manually at most call sites. Although I did nothing in cases like;
if (somethinggood)
dosomethinggood();
else
elog(ERROR, ...);
Quite possibly in that case we could use likely(), but there's also cases where elog() is in the default: in a switch(), or contained in an else in an if/else if/else block.
I also left out errcheck() in places where the condition called a function with some side affect. I did this as I also wanted to test a hard coded false condition to see how much overhead remained. I did nothing with ereport().
One way to do this would be to add elog_on() / ereport_on() macros, directly containing the error message. Like #define elog_on(cond, elevel, ...) \ do { \ if (unlikely(cond)) \ { \ elog(elevel, __VA_ARGS__) \ } \ } while(0)
Interesting idea. Would you think that would be something we could do a complete replace on, or are you thinking just for the hotter code paths?
I've attached the patch this time, it should apply cleanly to bbbd807. I'd would be good to see some other performance tests on some other hardware.