Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-30 Thread Stephen R. van den Berg
Tom Lane wrote: Accordingly, I'm going to go ahead with this: #ifdef __GNUC__ /* With GCC, we can use a compound statement within an expression */ #define newNode(size, tag) \ ({ Node *__result__; \ Please avoid identifiers starting with __ . ANSI reserves those for the implementation

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-30 Thread Tom Lane
Stephen R. van den Berg [EMAIL PROTECTED] writes: Tom Lane wrote: ({ Node *__result__; \ Please avoid identifiers starting with __ . Yeah, I had misremembered which way that rule went. It's _result as committed. regards, tom lane -- Sent via pgsql-hackers

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-29 Thread Tom Lane
Peter Eisentraut [EMAIL PROTECTED] writes: Tom Lane wrote: I considered that one, but since part of my argument is that inlining this is a waste of code space, it seems like a better inlining technology isn't really the answer. The compiler presumably has the intelligence and the

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-29 Thread Tom Lane
I wrote: In theory the above implementation of newNode should be a clear win, so I'm thinking this result must be an artifact of some kind. I'm going to go try it on PPC and HPPA machines next; does anyone want to try it on something else? Repeating the explain test on several machines, I

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-29 Thread Alex Hunsaker
On Fri, Aug 29, 2008 at 1:16 PM, Tom Lane [EMAIL PROTECTED] wrote: In theory the above implementation of newNode should be a clear win, so I'm thinking this result must be an artifact of some kind. I'm going to go try it on PPC and HPPA machines next; does anyone want to try it on something

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-29 Thread Tom Lane
Alex Hunsaker [EMAIL PROTECTED] writes: Hrm, I tried it on my x86_64 (quad core 2.66ghz, sorry no exotic machines here :)) and did not see any real noticeable difference between the two... Thanks. That confirms my feeling that I was seeing some weird artifact on my own x86_64 box.

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-27 Thread Heikki Linnakangas
Tom Lane wrote: I happened to be looking at nodes.h and started wondering just how sane this coding really is: extern PGDLLIMPORT Node *newNodeMacroHolder; #define newNode(size, tag) \ ( \ AssertMacro((size) = sizeof(Node)),/* need the tag, at least */ \ newNodeMacroHolder =

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-27 Thread Tom Lane
Heikki Linnakangas [EMAIL PROTECTED] writes: Tom Lane wrote: I happened to be looking at nodes.h and started wondering just how sane this coding really is: Note that the MemSetLoop macro used in palloc0fast is supposed to be evaluated at compile time, Oooh, good point, I had forgotten

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-27 Thread Heikki Linnakangas
Tom Lane wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: Note that the MemSetLoop macro used in palloc0fast is supposed to be evaluated at compile time, Oooh, good point, I had forgotten about that little detail. Yeah, we'll lose that optimization if we move the code out-of-line. Well,

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-27 Thread Tom Lane
Heikki Linnakangas [EMAIL PROTECTED] writes: Well, we could still have the MemSetTest outside the function, and evaluated at compile-time, if we provided an aligned and unaligned version of newNode: Yeah, that should work fine, since we expect MemSetTest to reduce to a compile-time constant.

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-27 Thread Heikki Linnakangas
Tom Lane wrote: I'll have a go at this later ... unless you want to do it? Nah, I don't care enough. BTW, it would be nice to have a test case that shows it's worth messing with that. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-27 Thread Peter Eisentraut
Tom Lane wrote: I considered that one, but since part of my argument is that inlining this is a waste of code space, it seems like a better inlining technology isn't really the answer. The compiler presumably has the intelligence and the command-line options to control how much inlining one

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-27 Thread Greg Stark
On Wed, Aug 27, 2008 at 4:05 PM, Heikki Linnakangas [EMAIL PROTECTED] wrote: Tom Lane wrote: BTW, it would be nice to have a test case that shows it's worth messing with that. At a guess the parser would be a good place to look. Perhaps a benchmark of a parsing a very large query? One thing I

[HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-26 Thread Tom Lane
I happened to be looking at nodes.h and started wondering just how sane this coding really is: extern PGDLLIMPORT Node *newNodeMacroHolder; #define newNode(size, tag) \ ( \ AssertMacro((size) = sizeof(Node)),/* need the tag, at least */ \ newNodeMacroHolder = (Node *)

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-26 Thread Stephen R. van den Berg
Tom Lane wrote: I happened to be looking at nodes.h and started wondering just how sane this coding really is: extern PGDLLIMPORT Node *newNodeMacroHolder; #define newNode(size, tag) \ ( \ AssertMacro((size) = sizeof(Node)),/* need the tag, at least */ \ newNodeMacroHolder =

Re: [HACKERS] Is it really such a good thing for newNode() to be a macro?

2008-08-26 Thread Tom Lane
Stephen R. van den Berg [EMAIL PROTECTED] writes: b. Create a function newNode() which is declared as inline, which basically gives you the same code as under (a). I considered that one, but since part of my argument is that inlining this is a waste of code space, it seems like a better