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

2013-01-23 Thread Heikki Linnakangas
For the record, on MSVC we can use __assume(0) to mark unreachable code. It does the same as gcc's __builtin_unreachable(). I tested it with the same Pavel's palloc-heavy test case that you used earlier, with the one-shot plan commit temporarily reverted, and saw a similar speedup you reported

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

2013-01-21 Thread Steve Singer
On 13-01-21 02:28 AM, Andres Freund wrote: I haven't removed it from the patch afaik, so it would be great to get a profile here! Its only for xlogdump, but that tool helped me immensely and I don't want to maintain it independently... Here is the output from tprof Here is the baseline:

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

2013-01-21 Thread Andres Freund
On 2013-01-21 11:59:18 -0500, Steve Singer wrote: On 13-01-21 02:28 AM, Andres Freund wrote: I haven't removed it from the patch afaik, so it would be great to get a profile here! Its only for xlogdump, but that tool helped me immensely and I don't want to maintain it independently... Here

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

2013-01-21 Thread Steve Singer
On 13-01-21 12:15 PM, Andres Freund wrote: On 2013-01-21 11:59:18 -0500, Steve Singer wrote: On 13-01-21 02:28 AM, Andres Freund wrote: I haven't removed it from the patch afaik, so it would be great to get a profile here! Its only for xlogdump, but that tool helped me immensely and I don't

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

2013-01-20 Thread Andres Freund
On 2013-01-19 17:33:05 -0500, Steve Singer wrote: On 13-01-09 03:07 PM, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Well, I *did* benchmark it as noted elsewhere in the thread, but thats obviously just machine (E5520 x 2) with one rather restricted workload (pgbench -S -jc 40

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

2013-01-19 Thread Steve Singer
On 13-01-09 03:07 PM, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Well, I *did* benchmark it as noted elsewhere in the thread, but thats obviously just machine (E5520 x 2) with one rather restricted workload (pgbench -S -jc 40 -T60). At least its rather palloc heavy. Here are

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

2013-01-18 Thread Andres Freund
On 2013-01-09 15:07:10 -0500, Tom Lane wrote: I would like to know if other people get comparable results on other hardware (non-Intel hardware would be especially interesting). If this result holds up across a range of platforms, I'll withdraw my objection to making palloc a plain function.

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

2013-01-13 Thread Andres Freund
On 2013-01-12 15:39:16 -0500, Tom Lane wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: On 12.01.2013 20:42, Andres Freund wrote: 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, With the

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

2013-01-13 Thread Andres Freund
On 2013-01-12 19:47:18 -0500, Tom Lane wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: When I compile with gcc -O0, I get one warning with this: datetime.c: In function �DateTimeParseError�: datetime.c:3575:1: warning: �noreturn� function does return [enabled by default]

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

2013-01-13 Thread Andres Freund
On 2013-01-12 16:36:39 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: It does *not* combine elog_start and elog_finish into one function if varargs are available although that brings a rather measurable size/performance benefit. Since you've apparently already done

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

2013-01-13 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: the numbers are: old definition: 10393.658ms, 5497912 bytes old definition + unreachable: 10011.102ms, 5469144 bytes stmt, two calls, unreachable: 10036.132ms, 5468792 bytes stmt,

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

2013-01-13 Thread Andres Freund
On 2013-01-13 14:17:52 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: the numbers are: old definition: 10393.658ms, 5497912 bytes old definition + unreachable: 10011.102ms, 5469144 bytes stmt, two calls, unreachable:

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

2013-01-13 Thread Andres Freund
On 2013-01-13 15:44:58 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: And if you look at the disassembly of ERROR codepaths: I think your numbers are being twisted by -fno-omit-frame-pointer. What I get, with the __builtin_unreachable version of the macro, looks more

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

2013-01-12 Thread Heikki Linnakangas
On 11.01.2013 23:49, Tom Lane wrote: Andres Freundand...@2ndquadrant.com writes: On 2013-01-11 16:28:13 -0500, Tom Lane wrote: I'm not very satisfied with that answer. Right now, Peter's patch has added a class of bugs that never existed before 9.3, and yours would add more. It might well

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

2013-01-12 Thread Andres Freund
On 2013-01-12 12:26:37 +0200, Heikki Linnakangas wrote: On 11.01.2013 23:49, Tom Lane wrote: Andres Freundand...@2ndquadrant.com writes: On 2013-01-11 16:28:13 -0500, Tom Lane wrote: I'm not very satisfied with that answer. Right now, Peter's patch has added a class of bugs that never

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

2013-01-12 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: It does *not* combine elog_start and elog_finish into one function if varargs are available although that brings a rather measurable size/performance benefit. Since you've apparently already done the measurement: how much? It would be a bit

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

2013-01-12 Thread Tom Lane
Heikki Linnakangas hlinnakan...@vmware.com writes: When I compile with gcc -O0, I get one warning with this: datetime.c: In function ‘DateTimeParseError’: datetime.c:3575:1: warning: ‘noreturn’ function does return [enabled by default] That suggests that the compiler didn't correctly

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

2013-01-11 Thread Andres Freund
On 2013-01-10 10:55:20 +0100, Andres Freund wrote: On 2013-01-10 10:31:04 +0100, Andres Freund wrote: On 2013-01-10 00:05:07 +0100, Andres Freund wrote: On 2013-01-09 17:28:35 -0500, Tom Lane wrote: (We know this doesn't matter, but gcc doesn't; this version of gcc apparently isn't

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

2013-01-11 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: The attached patch: * adds configure checks for __VA_ARGS__ and __builtin_unreachable support * provides a pg_unreachable() macro that expands to __builtin_unreachable() or abort() * changes #define elog ... into #define elog(elevel, ...) if

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

2013-01-11 Thread Andres Freund
On 2013-01-11 14:01:40 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: The attached patch: * adds configure checks for __VA_ARGS__ and __builtin_unreachable support * provides a pg_unreachable() macro that expands to __builtin_unreachable() or abort() *

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

2013-01-11 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: [ patch to mark elog() as non-returning if elevel = ERROR ] It strikes me that both in this patch, and in Peter's previous patch to do this for ereport(), there is an opportunity for improvement. Namely, that the added code only has any use if elevel

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

2013-01-11 Thread Andres Freund
On 2013-01-11 15:05:54 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: [ patch to mark elog() as non-returning if elevel = ERROR ] It strikes me that both in this patch, and in Peter's previous patch to do this for ereport(), there is an opportunity for improvement.

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

2013-01-11 Thread Andres Freund
On 2013-01-11 15:52:19 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-11 15:05:54 -0500, Tom Lane wrote: And another thing: what if the elevel argument isn't safe for multiple evaluation? No such hazard ever existed before these patches, so I'm not very

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

2013-01-11 Thread Andres Freund
On 2013-01-11 16:16:58 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-11 15:52:19 -0500, Tom Lane wrote: I agree the scenario doesn't seem all that probable, but what scares me here is that if we use __builtin_constant_p(elevel) (elevel) = ERROR in some

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

2013-01-11 Thread Andres Freund
On 2013-01-11 16:28:13 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-11 16:16:58 -0500, Tom Lane wrote: Uh ... because it's *not* unreachable if elevel ERROR. Otherwise we'd just mark errfinish as __attribute((noreturn)) and be done. Of course, that's a

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

2013-01-10 Thread Andres Freund
On 2013-01-10 00:05:07 +0100, Andres Freund wrote: On 2013-01-09 17:28:35 -0500, Tom Lane wrote: (We know this doesn't matter, but gcc doesn't; this version of gcc apparently isn't doing much with the knowledge that elog won't return.) Afaics one reason for that is that we don't have any

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

2013-01-10 Thread Andres Freund
On 2013-01-10 10:31:04 +0100, Andres Freund wrote: On 2013-01-10 00:05:07 +0100, Andres Freund wrote: On 2013-01-09 17:28:35 -0500, Tom Lane wrote: (We know this doesn't matter, but gcc doesn't; this version of gcc apparently isn't doing much with the knowledge that elog won't return.)

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

2013-01-09 Thread Andres Freund
On 2013-01-09 11:57:35 -0500, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2013-01-09 11:37:46 -0500, Tom Lane wrote: I agree that what dirmod is doing is pretty ugly, but it's localized enough that I don't care particularly. (Really, the only reason it's a hack and not

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

2013-01-09 Thread Andres Freund
On 2013-01-09 12:32:20 -0500, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2013-01-09 11:57:35 -0500, Tom Lane wrote: My objection is that you're forcing *all* backend code to go through the trampoline. That probably has a negative impact on performance, and if you think

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

2013-01-09 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: Well, I *did* benchmark it as noted elsewhere in the thread, but thats obviously just machine (E5520 x 2) with one rather restricted workload (pgbench -S -jc 40 -T60). At least its rather palloc heavy. Here are the numbers: before:

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

2013-01-09 Thread Tom Lane
I wrote: I then applied the palloc.h and mcxt.c hunks of your patch and rebuilt. Now I get an average runtime of 1 ms, a full 2% faster, which is a bit astonishing, particularly because the oprofile results haven't moved much: I studied the assembly code being generated for palloc(), and

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

2013-01-09 Thread Andres Freund
On 2013-01-09 15:43:19 -0500, Tom Lane wrote: I wrote: I then applied the palloc.h and mcxt.c hunks of your patch and rebuilt. Now I get an average runtime of 1 ms, a full 2% faster, which is a bit astonishing, particularly because the oprofile results haven't moved much: I studied

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

2013-01-09 Thread Andres Freund
On 2013-01-09 15:07:10 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Well, I *did* benchmark it as noted elsewhere in the thread, but thats obviously just machine (E5520 x 2) with one rather restricted workload (pgbench -S -jc 40 -T60). At least its rather palloc

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

2013-01-09 Thread Andres Freund
On 2013-01-09 17:28:35 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-09 15:43:19 -0500, Tom Lane wrote: I had thought of proposing that we code palloc() like this: void * palloc(Size size) { MemoryContext context = CurrentMemoryContext;