Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Robert Haas writes: > There are an awful lot of places in our source tree where the error > level is fixed. We could invent a new construct, say ereport_error or > so, that is just like ereport except that it takes no error-level > parameter because it's hard-coded to ERROR. > It would be a bit of a pain to change all of the existing call sites, > but presumably it would dodge a lot of these issues about the way > compilers optimize things, because we could simply say categorically > that ereport_error NEVER returns. Meh. We've already got it working, and in a way that doesn't require the compiler to understand __attribute__((noreturn)) --- it only has to be aware that abort() doesn't return, in one fashion or another. So I'm disinclined to run around and change a few thousand call sites, much less expect extension authors to do so too. (By my count there are about six thousand places we'd have to change.) Note that whatever's going on on dugong is not a counterexample to "got it working", because presumably dugong would also be misbehaving if we'd used a different method of flagging all the ereports/elogs as nonreturning. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On Sun, Jan 13, 2013 at 4:16 PM, Tom Lane wrote: > Andres Freund writes: >>> Basically, the aspects of this that I think are likely to be >>> reproducible wins across different platforms are (a) teaching the >>> compiler that elog(ERROR) doesn't return, and (b) reducing code size as >>> much as possible. The single-function change isn't going to help on >>> either ground --- maybe it would have helped on (b) without the errno >>> problem, but that's going to destroy any possible code size savings. > >> Agreed. I am happy to produce an updated patch unless youre already on >> it? > > On it now (busy testing on some old slow boxes, else I'd be done already). Just a random thought here... There are an awful lot of places in our source tree where the error level is fixed. We could invent a new construct, say ereport_error or so, that is just like ereport except that it takes no error-level parameter because it's hard-coded to ERROR. It would be a bit of a pain to change all of the existing call sites, but presumably it would dodge a lot of these issues about the way compilers optimize things, because we could simply say categorically that ereport_error NEVER returns. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund writes: >> Basically, the aspects of this that I think are likely to be >> reproducible wins across different platforms are (a) teaching the >> compiler that elog(ERROR) doesn't return, and (b) reducing code size as >> much as possible. The single-function change isn't going to help on >> either ground --- maybe it would have helped on (b) without the errno >> problem, but that's going to destroy any possible code size savings. > Agreed. I am happy to produce an updated patch unless youre already on > it? On it now (busy testing on some old slow boxes, else I'd be done already). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund writes: > On 2013-01-13 14:17:52 -0500, Tom Lane wrote: >> I find these numbers pretty hard to credit. > There are quite some elog(DEBUG*)s in the backend, and those are taken, > so I don't find it unreasonable that doing less work in those cases is > measurable. Meh. If there are any elog(DEBUG)s in time-critical places, they should be changed to ereport's anyway, so as to eliminate most of the overhead when they're not firing. > 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 like MemoryContextAlloc: cmpq$1073741823, %rsi pushq %rbx movq%rsi, %rbx ja .L53 movq8(%rdi), %rax movb$0, 48(%rdi) popq%rbx movq(%rax), %rax jmp *%rax .L53: movl$__func__.5262, %edx movl$576, %esi movl$.LC2, %edi callelog_start movq%rbx, %rdx movl$.LC3, %esi movl$20, %edi xorl%eax, %eax callelog_finish With -fno-omit-frame-pointer it's a little worse, but still not what you show --- in particular, for me gcc still pushes the elog calls out of the main code path. I don't think that the main path will get any better with one elog function instead of two. It could easily get worse. On many machines, the single-function version would be worse because of needing to use more parameter registers, which would translate into more save/restore work in the calling function, which is overhead that would likely be paid whether elog actually gets called or not. (As an example, in the above code gcc evidently isn't noticing that it doesn't need to save/restore rbx so far as the main path is concerned.) In any case, results from a single micro-benchmark on a single platform with a single compiler version (and single set of compiler options) don't convince me a whole lot here. Basically, the aspects of this that I think are likely to be reproducible wins across different platforms are (a) teaching the compiler that elog(ERROR) doesn't return, and (b) reducing code size as much as possible. The single-function change isn't going to help on either ground --- maybe it would have helped on (b) without the errno problem, but that's going to destroy any possible code size savings. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund writes:
> On 2013-01-12 15:39:16 -0500, Tom Lane wrote:
>> This is actually a disadvantage of the proposal to replace the abort()
>> calls with __builtin_unreachable(), too. The gcc boys interpret the
>> semantics of that as "if control reaches here, the behavior is
>> undefined"
> We could make add an Assert() additionally?
I see little value in that. In a non-assert build it will do nothing at
all, and in an assert build it'd just add code bloat.
I think there might be a good argument for defining pg_unreachable() as
abort() in assert-enabled builds, even if the compiler has
__builtin_unreachable(): that way, if the impossible happens, we'll find
out about it in a predictable and debuggable way. And by definition,
we're not so concerned about either performance or code bloat in assert
builds.
> Where my variant is:
> #define ereport_domain(elevel, domain, rest)\
> do {\
> const int elevel_ = elevel; \
> if (errstart(elevel_,__FILE__, __LINE__, PG_FUNCNAME_MACRO, domain))\
> { \
> errfinish rest; \
> } \
> if (elevel_>= ERROR)\
> pg_unreachable(); \
> } while(0)
This is the same implementation I'd arrived at after looking at
Heikki's. I think we should probably use this for non-gcc builds.
However, for recent gcc (those with __builtin_constant_p) I think that
my original suggestion is better, ie don't use a local variable but
instead use __builtin_constant_p(elevel) && (elevel) >= ERROR in the
second test. That way we don't have the problem with unreachability
tests failing at -O0. We may still have some issues with bogus warnings
in other compilers, but I think most PG developers are using recent gcc.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-12 18:15:17 -0500, Tom Lane wrote: > Andres Freund writes: > > On 2013-01-12 13:16:56 -0500, Tom Lane wrote: > >> 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? > > Right offhand I don't see how that helps us. AFAICS, > __builtin_choose_expr is the same as x?y:z except that y and z are not > required to have the same datatype, which is okay because they require > the value of x to be determinable at compile time. So we couldn't just > write __builtin_choose_expr((elevel) >= ERROR, ...). That would fail > if elevel wasn't compile-time-constant. We could conceivably do > > __builtin_choose_expr(__builtin_constant_p(elevel) && (elevel) >= ERROR, ...) > > with the idea of making real sure that that expression reduces to a > compile time constant ... but stacking two nonstandard constructs on > each other seems to me to probably increase our exposure to gcc's > definitional randomness rather than reduce it. I mean, if > __builtin_constant_p can't already be trusted to act like a constant, > why should we trust that __builtin_choose_expr doesn't also have a > curious definition of constant-ness? I was thinking of something like the above, yes. It seems to me to generate code the compiler would need to really make sure the condition in __builtin_choose_expr() is really constant, so I hoped the checks inside are somewhat strict... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund writes: > On 2013-01-12 13:16:56 -0500, Tom Lane wrote: >> 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? Right offhand I don't see how that helps us. AFAICS, __builtin_choose_expr is the same as x?y:z except that y and z are not required to have the same datatype, which is okay because they require the value of x to be determinable at compile time. So we couldn't just write __builtin_choose_expr((elevel) >= ERROR, ...). That would fail if elevel wasn't compile-time-constant. We could conceivably do __builtin_choose_expr(__builtin_constant_p(elevel) && (elevel) >= ERROR, ...) with the idea of making real sure that that expression reduces to a compile time constant ... but stacking two nonstandard constructs on each other seems to me to probably increase our exposure to gcc's definitional randomness rather than reduce it. I mean, if __builtin_constant_p can't already be trusted to act like a constant, why should we trust that __builtin_choose_expr doesn't also have a curious definition of constant-ness? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Heikki Linnakangas 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 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. No it isn't: you'd get a clean and easily interpretable abort. I am not sure exactly what would happen if we plow forward with calling elog.c functions after errstart returns false, but it wouldn't be good, and debugging a field report of such behavior wouldn't be easy. This is actually a disadvantage of the proposal to replace the abort() calls with __builtin_unreachable(), too. The gcc boys interpret the semantics of that as "if control reaches here, the behavior is undefined" --- and their notion of undefined generally seems to amount to "we will screw you as hard as we can". For example, they have no problem with using the assumption that control won't reach there to make deductions while optimizing the rest of the function. If it ever did happen, I would not want to have to debug it. The same goes for __attribute__((noreturn)), BTW. >> 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. I was wondering about that too, but haven't tried it locally yet. It could be that Andres was looking at an optimization level in which a register still got allocated for the local variable. > 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 Yes, I noticed that too: these patches make the debug overhead grow substantially for some reason. It's better to look at the output of "size" rather than the executable's total file size. That way you don't need to rebuild without debug to see reality. (I guess you could also "strip" the executables for comparison purposes.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [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 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
diff --git a/src/backend/bootstrap/bootscanner.l b/src/backend/bootstrap/bootscanner.l
index bdd7dcb..b97bee4 100644
--- a/src/backend/bootstrap/bootscanner.l
+++ b/src/backend/bootstrap/bootscanner.l
@@ -42,8 +42,13 @@
/* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */
#undef fprintf
-#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg)))
+#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg)
+static void
+fprintf_to_ereport(char *fmt, const char *msg)
+{
+ ereport(ERROR, (errmsg_internal("%s", msg)));
+}
static int yyline = 1; /* line number for error reporting */
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 622cb1f..877345b 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -42,7 +42,14 @@
/* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */
#undef fprintf
-#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg)))
+#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg)
+
+static void
+fprintf_to_ereport(char *fmt, const char *msg)
+{
+ ereport(ERROR, (errmsg_internal("%s", msg)));
+}
+
/*
* GUC variables. This is a DIRECT violation of the warning given at the
diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index 9ccf02a..b62625b 100644
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -19,7 +19,13 @@
/* Avoid exit() on fatal scanner errors (a
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-12 13:16:56 -0500, Tom Lane wrote:
> Heikki Linnakangas writes:
> > On 11.01.2013 23:49, Tom Lane wrote:
> >> Hm ... well, that's a point. I may be putting too much weight on the
> >> fact that any such bug for elevel would be a new one that never existed
> >> before. What do others think?
>
> > It sure would be nice to avoid multiple evaluation. At least in xlog.c
> > we use emode_for_corrupt_record() to pass the elevel, and it does have a
> > side-effect.
>
> Um. I had forgotten about that example, but its existence is sufficient
> to reinforce my opinion that we must not risk double evaluation of the
> elevel argument.
Unfortunately I aggree :(
> > 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.
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 :(.
> 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?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Heikki Linnakangas writes:
> On 11.01.2013 23:49, Tom Lane wrote:
>> Hm ... well, that's a point. I may be putting too much weight on the
>> fact that any such bug for elevel would be a new one that never existed
>> before. What do others think?
> It sure would be nice to avoid multiple evaluation. At least in xlog.c
> we use emode_for_corrupt_record() to pass the elevel, and it does have a
> side-effect.
Um. I had forgotten about that example, but its existence is sufficient
to reinforce my opinion that we must not risk double evaluation of the
elevel argument.
> 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.
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
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund 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 be that those classes are empty ... but >> *we can't know that*. I don't think that keeping a new optimization for >> non-gcc compilers is worth that risk. Postgres is already full of >> gcc-only optimizations, anyway. > ISTM that ereport() already has so strange behaviour wrt evaluation of > arguments (i.e doing it only when the message is really logged) that its > doesn't seem to add a real additional risk. Hm ... well, that's a point. I may be putting too much weight on the fact that any such bug for elevel would be a new one that never existed before. What do others think? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund 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 gcc-ism too. > Well, I mean with the double evaluation risk. Oh, are you saying you don't want to make the __builtin_constant_p addition? 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 be that those classes are empty ... but *we can't know that*. I don't think that keeping a new optimization for non-gcc compilers is worth that risk. Postgres is already full of gcc-only optimizations, anyway. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund 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 builds, and just "(elevel) >= ERROR" in others, then if there is >> any code with a multiple-evaluation hazard, it is only buggy in the >> latter builds. That's sufficiently nasty that I'm willing to give up >> an optimization that we never had before 9.3 anyway. > Well, why use it at all then and not just rely on > __builtin_unreachable() in any recent gcc (and llvm fwiw) and abort() > otherwise? Then the code is small for anything recent (gcc 4.4 afair) > and always consistently buggy. 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 gcc-ism too. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund 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 comfortable with adding one. (Even if all our own code is >> safe, there's third-party code to worry about.) > Hm. I am not really too scared about those dangers I have to admit. 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 builds, and just "(elevel) >= ERROR" in others, then if there is any code with a multiple-evaluation hazard, it is only buggy in the latter builds. That's sufficiently nasty that I'm willing to give up an optimization that we never had before 9.3 anyway. > I dislike the latter somewhat as it would mean not to give that > information at all to msvc and others which seems a bit sad. But I don't > feel particularly strongly. It's not like our code isn't rather gcc-biased anyway. I'd keep the optimization for other compilers if possible, but not at the cost of introducing bugs that occur only on those compilers. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund writes: > I wonder whether it makes sense to "inline" the contents pstrdup() > additionally? My gut feeling is not, but... I don't recall ever having seen MemoryContextStrdup amount to much, so probably not worth the extra code space. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund 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;
>>
>> AssertArg(MemoryContextIsValid(context));
>>
>> if (!AllocSizeIsValid(size))
>> elog(ERROR, "invalid memory alloc request size %lu",
>> (unsigned long) size);
>>
>> context->isReset = false;
>>
>> return (*context->methods->alloc) (context, size);
>> }
>>
>> but at least on this specific hardware and compiler that would evidently
>> be a net loss compared to direct use of CurrentMemoryContext. I would
>> not put a lot of faith in that result holding up on other machines
>> though.
> Thats not optimized to the same? ISTM the compiler should produce
> exactly the same code for both.
No, that's exactly the point here, you can't just assume that you get
the same code; this is tense enough that a few instructions matter.
Remember we're considering non-assert builds, so the AssertArg vanishes.
So in the form where CurrentMemoryContext is only read after the elog
call, the compiler can see that it requires but one fetch - it can't
change within the two lines where it's used. In the form given above,
the compiler is required to fetch CurrentMemoryContext into a local
variable and keep it across the elog call. (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.) Since the extra local
variable adds several instructions to the overall function's entry/exit
sequences, you come out behind. At least on this platform. On other
machines with different code generation behavior, the story could easily
be very different.
>> In any case this doesn't explain the whole 2% speedup, but it probably
>> accounts for palloc() showing as slightly cheaper than
>> MemoryContextAlloc had been in the oprofile listing.
> I'd guess that a good part of the cost is just smeared across all
> callers and not individually accountable to any function visible in the
> profile.
Yeah, I think this is most likely showing that there is a real,
measurable cost of code bloat.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund 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 it'll get committed without a thorough proof that there's >> no such impact, you're mistaken. Perhaps you're not aware of exactly >> how hot a hot-spot palloc is? It's not unlikely that this patch could >> hurt backend performance by several percent all by itself. > There is no additional overhead except some minor increase in code size. > If you look at the patch palloc() now simply directly calls > CurrentMemoryContext->methods->alloc. So there is no additional function > call relative to the previous state. Apparently we're failing to communicate, so let me put this as clearly as possible: an unsupported assertion that this patch has zero cost isn't worth the electrons it's written on. We're talking about a place where single instructions can make a large difference. I see that you've avoided an extra level of function call by duplicating code, but there are (at least) two reasons why that could be a loser: * now there are two hotspots not one, ie both MemoryContextAlloc and palloc will be competing for L1 cache, likewise for MemoryContextAllocZero and palloc0; * depending on machine architecture and compiler optimizations, multiple fetches of the global variable CurrentMemoryContext are quite likely to cost more than fetching it once into a parameter register. As I said, it's possible that this is a wash. But it's very possible that it isn't. In any case it's not clear to me why duplicating code like that is a less ugly approach than having different macro definitions for frontend and backend. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund 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 The Solution is that at the moment there's only one >> file doing it that way. A trampoline function for use only by code >> that needs to work in both frontend and backend isn't an unreasonable >> idea.) > Well, after the patch the trampoline function would be palloc() itself. 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 it'll get committed without a thorough proof that there's no such impact, you're mistaken. Perhaps you're not aware of exactly how hot a hot-spot palloc is? It's not unlikely that this patch could hurt backend performance by several percent all by itself. Now on the other side of the coin, it's also possible that it's a wash, particularly on machines where fetching a global variable is a relatively expensive thing to do. But the driving force behind any proposed change to the backend's palloc mechanism has to be performance, and nothing but. Convenience for a patch such as this not only doesn't trump performance, I'm unwilling to grant it any standing at all. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-09 11:37:46 -0500, Tom Lane wrote: > Andres Freund writes: > > On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: > >> I don't understand the need for this change. Can't you just: > >> #define palloc(s) pg_malloc(s) > >> in the frontend context? > > > Yes, that would be possible, but imo its the inferior solution: > > I'm with Heikki: in fact, I will go farther and say that this approach > is DOA. The only way you get to change the backend's definition of > palloc is if you can show that doing so yields a performance boost > in the backend. Otherwise, we use a macro or whatever is necessary > to allow frontend code to have a different underlying implementation > of palloc(x). Whats the usecase for being able to redefine it after the patch? Its not like you can do anything interesting given that pfree() et. al. is not a macro. > > * removing allows us to get rid of the following ugliness in dirmod.c: > > 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 The Solution is that at the moment there's only one > file doing it that way. A trampoline function for use only by code > that needs to work in both frontend and backend isn't an unreasonable > idea.) Well, after the patch the trampoline function would be palloc() itself. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Andres Freund writes: > On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: >> I don't understand the need for this change. Can't you just: >> #define palloc(s) pg_malloc(s) >> in the frontend context? > Yes, that would be possible, but imo its the inferior solution: I'm with Heikki: in fact, I will go farther and say that this approach is DOA. The only way you get to change the backend's definition of palloc is if you can show that doing so yields a performance boost in the backend. Otherwise, we use a macro or whatever is necessary to allow frontend code to have a different underlying implementation of palloc(x). > * it precludes ever sharing code without compiling twice That's never going to happen anyway, or at least there are plenty more problems in the way of it. > * removing allows us to get rid of the following ugliness in dirmod.c: 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 The Solution is that at the moment there's only one file doing it that way. A trampoline function for use only by code that needs to work in both frontend and backend isn't an unreasonable idea.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: > On 09.01.2013 13:27, Andres Freund wrote: > >- makes palloc() into a real function so CurrentMemoryContext doesn't > > need to be provided > > I don't understand the need for this change. Can't you just: > > #define palloc(s) pg_malloc(s) > > in the frontend context? Yes, that would be possible, but imo its the inferior solution: * it precludes ever sharing code without compiling twice * removing allows us to get rid of the following ugliness in dirmod.c: -#ifndef FRONTEND - -/* - * On Windows, call non-macro versions of palloc; we can't reference - * CurrentMemoryContext in this file because of PGDLLIMPORT conflict. - */ -#if defined(WIN32) || defined(__CYGWIN__) -#undef palloc -#undef pstrdup -#define palloc(sz) pgport_palloc(sz) -#define pstrdup(str) pgport_pstrdup(str) -#endif -#else /* FRONTEND */ - * it opens the window for moving more stuff from utils/palloc.h to memutils.h Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
On 09.01.2013 13:27, Andres Freund wrote: - makes palloc() into a real function so CurrentMemoryContext doesn't need to be provided I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the frontend context? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
