Re: const correctness

Hi pgsql-hackers,

I am a long time user and fan of PostgreSQL and have built various
projects large and small on every major release since 6.5. Recently
I decided to try doing something more with the source than just
compiling it, and spent some time 'constifying' some parts of the code
as an exercise (this is an item I saw on the wiki to-do list, and I
figured it might provide me with an educational traversal of the
source tree).

I started looking at backend/nodes (thinking that operations on core
data structures need to be const-friendly before other bits of the
software can), and made the following apparently simple changes:

1. copyObject should take the object to be copied by pointer-to-const
(and therefore so should all the static functions in copyfuncs.c).

2. equal should take the objects to be compared by pointer-to-const
(and therefore so should all the static functions in
equalfuncs.c). Things started to get a bit trickier when dealing
with equality of lists... see below.

3. print, pprint, and various other print functions in print.c should
take the object to be printed/logged with a pointer-to-const
(except print_slot which modifies the slot object in code I don't
yet understand in heaptuple.c).

4. nodeToString should take the object to be written by
pointer-to-const (and therefore so should all the static functions
in outfuncs.c).

5. exprType, exprTypmod, exprIsLengthCoercion, exprCollation,
exprInputCollation, exprLocation should take the expression node
by pointer-to-const (but the functions in nodeFuncs.c that are
implemented via expression_tree_walker are trickier, see note at
end[1]).

6. list_length, check_list_invariants, list_copy...,
list_difference..., list_union..., list_intersection,
list_member..., list_nth... should take const List * (and probably
const void * where applicable for datum but only when it is not
captured).

So far so good, but I found that I also needed these extra functions:

7. I made a list_head_const function, which can be used used to get a
pointer to the head cell when you have a pointer to const List; I
needed that so I could make foreach_const and forboth_const; they
were needed to be able to make list_member, _equalList and various
other list-visiting functions work with const List objects.

Perhaps there should be a few more 'XXX_const' accessor function
variants, for example list_nth_const, to allow holders of pointers to
const lists to get a read-only view of the contained data, ie
preventing them from accessing pointers to non-const element (based on
the possibly flawed intuition that a const list should be considered
to have const elements, like std::list<T> in C++, for the constness to
be really useful). Or pehaps that's ridiculous overkill for little
gain in a language with such a blunt and frequently used cast
operator.

I have attached a patch illustrating the changes (don't worry I'm not
submitting it for consideration, I just wanted to discuss the idea at
this stage and generally test the water). I would be grateful for
opinions on whether this approach could be useful. Being new to all
this I have no idea if destructive vs non-destructive confusion (for
example lcons and lappend modifying their arguments) is really a
source of bugs, and more generally what the project's take is on the
appropriate uses of const.

What did the author of the to-do item "Add use of 'const' for
variables in source tree" have in mind, and am I even barking up the
right tree?

Thanks,

Thomas Munro

[1] For functions built on top of expression_tree_walker (like
expression_returns_set), it seems that it and therefore they can be
modified to work entirely on pointers to const Node without any
complaints from GCC at least, but only because the walker function
pointer is of type bool (*)() (that is, pointer to a function with
unspecified arguments, thereby side-stepping all static type
checking). But I guess it would only be honest to change the
signature of expression_tree_walker to take const Node * if the type
of the walker function pointer were changed to bool (*)(const Node *,
void *), and the walker mechanism were declared in the comments not to
permit any mutation at all (rather than the weaker restriction that
routines can modify nodes in-place but not add/delete/replace nodes).

Thomas Munro <munro(at)ip9(dot)org> writes:
> I am a long time user and fan of PostgreSQL and have built various
> projects large and small on every major release since 6.5. Recently
> I decided to try doing something more with the source than just
> compiling it, and spent some time 'constifying' some parts of the code
> as an exercise (this is an item I saw on the wiki to-do list, and I
> figured it might provide me with an educational traversal of the
> source tree).

There was some discussion of this just a couple days ago in connection
with a patch Peter offered ... did you see that?
http://archives.postgresql.org/pgsql-hackers/2011-11/msg00314.php

> Perhaps there should be a few more 'XXX_const' accessor function
> variants, for example list_nth_const,

This is exactly what was bothering Robert and me about Peter's patch.
If you go down this road you soon start needing duplicate functions
for no other reason than that one takes/returns "const" and one doesn't.
That might be acceptable on a very small scale, but any serious attempt
to const-ify the PG code is going to require it on a very large scale.
The maintenance costs of duplicate code are significant (not even
considering whether there's a performance hit), and it just doesn't seem
likely to be repaid in easier or safer development. Offhand I cannot
remember the last bug in PG which would have been prevented by better
const-ification.

So I'm starting to feel that this idea is a dead end, and we should take
it off the TODO list.

> [1] For functions built on top of expression_tree_walker (like
> expression_returns_set), it seems that it and therefore they can be
> modified to work entirely on pointers to const Node without any
> complaints from GCC at least, but only because the walker function
> pointer is of type bool (*)() (that is, pointer to a function with
> unspecified arguments, thereby side-stepping all static type
> checking). But I guess it would only be honest to change the
> signature of expression_tree_walker to take const Node * if the type
> of the walker function pointer were changed to bool (*)(const Node *,
> void *), and the walker mechanism were declared in the comments not to
> permit any mutation at all (rather than the weaker restriction that
> routines can modify nodes in-place but not add/delete/replace nodes).

Yeah, that was an alternative I considered when designing the tree
walker infrastructure. However, if we force walker functions to be
declared just like that, we lose rather than gain type safety. In the
typical usage, there's a startup function that sets up a context
structure and calls the walker function directly. As things stand,
that call is type-checked: you pass the wrong kind of context object,
you'll get a bleat. Changing the walkers to use void * would remove
that check, while adding a need for explicit casting of the argument
inside the walkers, and gain nothing of value.

As for the question of whether we should insist that walkers never
modify the tree ... yeah, we could, but there are enough instances
of nominal walkers that do modify the tree to make me not enthused
about it. We would have to change each one of those walkers to
instead create a new copy of the tree, with attendant memory consumption
and palloc overhead. It would almost certainly be a noticeable
performance hit, and the benefit seems debatable at best.

There would, I think, be both performance and safety benefits from
getting certain entire modules to not scribble on their input trees;
especially the planner. But that is a high-level consideration and
I'm not sure that const-decoration would really do much to help us
achieve it.

regards, tom lane

On 9 November 2011 15:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:.
> If you go down this road you soon start needing duplicate functions
> for no other reason than that one takes/returns "const" and one doesn't.

Why would you have to do that?

To my mind, the fact that const "spreads" is a feature, not a deficiency.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Perhaps there should be a few more 'XXX_const' accessor function
>> variants, for example list_nth_const,
>
> This is exactly what was bothering Robert and me about Peter's
> patch.If you go down this road you soon start needing duplicate
> functions for no other reason than that one takes/returns "const"
> and one doesn't.

What about existing functions which are not intended to modify their
inputs, don't actually do so, and can be marked to indicate that
just by adding "const" to the current declarations? Aside from any
possible value in code optimization by the compiler, I find it helps
me understand unfamiliar code more quickly, by making the contract
of the API more explicit in the declaration. Perhaps it's worth
going after the low-hanging fruit?

-Kevin

Peter Geoghegan <peter(at)2ndquadrant(dot)com> writes:
> On 9 November 2011 15:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:.
>> If you go down this road you soon start needing duplicate functions
>> for no other reason than that one takes/returns "const" and one doesn't.

> Why would you have to do that?

list_nth is an example. Now admittedly you can hack it, in the same
spirit as the C library functions that are declared to take const
pointers and return non-const pointers to the very same data; but that
hardly satisfies anyone's idea of const cleanliness. In particular
it doesn't fix what Peter E. was on about, which was getting rid of
cast-away-const warnings, since such a function will have to do that
internally.

regards, tom lane

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> This is exactly what was bothering Robert and me about Peter's
>> patch.If you go down this road you soon start needing duplicate
>> functions for no other reason than that one takes/returns "const"
>> and one doesn't.

> What about existing functions which are not intended to modify their
> inputs, don't actually do so, and can be marked to indicate that
> just by adding "const" to the current declarations? Aside from any
> possible value in code optimization by the compiler, I find it helps
> me understand unfamiliar code more quickly, by making the contract
> of the API more explicit in the declaration. Perhaps it's worth
> going after the low-hanging fruit?

I have no objection to const-ifying anything that can be done with one
or two localized changes. The difficulty comes in when you try to make
core infrastructure like expression_tree_walker do it. (And of course
the problem then is that there are not so many functions that don't use
any of that core infrastructure, so if you try to const-ify them you end
up having to cast away const internally; which does not seem like a net
advance to me.)

regards, tom lane

On Wed, Nov 9, 2011 at 10:45 AM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>  Perhaps there should be a few more 'XXX_const' accessor function
>>> variants, for example list_nth_const,
>>
>> This is exactly what was bothering Robert and me about Peter's
>> patch.If you go down this road you soon start needing duplicate
>> functions for no other reason than that one takes/returns "const"
>> and one doesn't.
>
> What about existing functions which are not intended to modify their
> inputs, don't actually do so, and can be marked to indicate that
> just by adding "const" to the current declarations?  Aside from any
> possible value in code optimization by the compiler, I find it helps
> me understand unfamiliar code more quickly, by making the contract
> of the API more explicit in the declaration.  Perhaps it's worth
> going after the low-hanging fruit?

My feeling is that there's no harm (and possibly some benefit) in
const-ifying functions that do very simple things. But as soon as you
get to functions where the const-ness starts growing all over the
system like kudzu, it's time to run away screaming. Moreover, I don't
really want to see us spend a lot of time figuring out exactly what we
can or can't const-ify. I feel as virtuous as the next guy when I
mark something const, but my experience over the years is that it
rapidly turns a huge amount of work. That by itself is not enough
reason not to do it; many worthwhile things are hard. The kicker is
that it's a lot of work for an unbelievably tiny benefit, sometimes a
negative benefit.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> My feeling is that there's no harm (and possibly some benefit) in
> const-ifying functions that do very simple things. But as soon as
> you get to functions where the const-ness starts growing all over
> the system like kudzu, it's time to run away screaming.

The patch attached to Thomas's original post is an example of what I
consider low-hanging fruit worth going after. It applies cleanly,
causes no new warnings, and adds no new objects -- it just clarifies
the API. (It was in checking for new warnings that I found the one
I mentioned in the other post.)

> Moreover, I don't really want to see us spend a lot of time
> figuring out exactly what we can or can't const-ify.

Well, nobody is asking you to do so. Thomas said that he was
looking for something to do which would lead him through the code so
he could learn it.

> I feel as virtuous as the next guy when I mark something const,
> but my experience over the years is that it rapidly turns a huge
> amount of work. That by itself is not enough reason not to do it;
> many worthwhile things are hard.

If Thomas wants to do this as an exercise in learning PostgreSQL
code, it seems like a win/win to me. We get minor clarifications of
our APIs and another person with some understanding of the code
base.

> The kicker is that it's a lot of work for an unbelievably tiny
> benefit, sometimes a negative benefit.

Assuming duplicate declarations with and without const are off the
table, where do you see the negative?

-Kevin

On Wed, Nov 9, 2011 at 11:28 AM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>> The kicker is that it's a lot of work for an unbelievably tiny
>> benefit, sometimes a negative benefit.
>
> Assuming duplicate declarations with and without const are off the
> table, where do you see the negative?

If it doesn't uglify the code, there aren't any negatives. I'm just
saying we may not be able to get very far before we run up against
that issue. For example, in the OP, Thomas wrote:

7. I made a list_head_const function, which can be used used to get a
pointer to the head cell when you have a pointer to const List; I
needed that so I could make foreach_const and forboth_const; they
were needed to be able to make list_member, _equalList and various
other list-visiting functions work with const List objects.

So that's already duplicating list_head, foreach, and forboth.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9 Nov 2011, at 15:33, Peter Geoghegan wrote:

> On 9 November 2011 15:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:.
>> If you go down this road you soon start needing duplicate functions
>> for no other reason than that one takes/returns "const" and one doesn't.
>
> Why would you have to do that?
>
> To my mind, the fact that const "spreads" is a feature, not a deficiency.
>
+1.

I would go as far as compiling most of my stuff using C++ compiler, because it is much more strict about const-correctness. (but then I have rule about making source files small).
C compilers (and standard) allows you to do silly things like :

char *foo = "barbar";
foo[1] = '4';

Not an option in C++ and if you use const correctness.
I had few bugs like that in the past, where pointer was passed around (in C code), and one of the pointers was pointing to const string - but since compiler was fine with it... You know what happened.
And that was on an embedded platform which made it even harder to trace down.

The point is, const correctness is deeply unappreciated.
Added bonus is the fact that compiler can make extra optimisations based on the const keyword. Kind of like read-only transactions in the database.

Probably the most extreme and tedious way of introducing full const correctness in PostgreSQL would be to rename all files to .cpp, and let c++ compiler tell you what you need to fix.

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> If it doesn't uglify the code, there aren't any negatives. I'm
> just saying we may not be able to get very far before we run up
> against that issue. For example, in the OP, Thomas wrote:
>
> 7. I made a list_head_const function, which can be used used to
> get a pointer to the head cell when you have a pointer to
> const List; I needed that so I could make foreach_const and
> forboth_const; they were needed to be able to make
> list_member, _equalList and various other list-visiting
> functions work with const List objects.
>
> So that's already duplicating list_head, foreach, and forboth.

OK, I failed to pick up on that properly. With that stripped out,
you get the attached patch, which does nothing but add "const" to
661 lines. It still applies cleanly, builds with no warnings, and
passes regression tests.

It is a bit disappointing that without creating two flavors of the
list_head function and the foreach and forboth macros, there are a
number of functions which aren't intended to modify their inputs
which can't be declared with const parameters; but unless there is
some demonstrable performance benefit from those changes, I agree
that the argument for having the two flavors is thin.

-Kevin

On Wed, Nov 9, 2011 at 2:35 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> If it doesn't uglify the code, there aren't any negatives.  I'm
>> just saying we may not be able to get very far before we run up
>> against that issue.  For example, in the OP, Thomas wrote:
>>
>> 7.  I made a list_head_const function, which can be used used to
>>     get a pointer to the head cell when you have a pointer to
>>     const List; I needed that so I could make foreach_const and
>>     forboth_const; they were needed to be able to make
>>     list_member, _equalList and various other list-visiting
>>     functions work with const List objects.
>>
>> So that's already duplicating list_head, foreach, and forboth.
>
> OK, I failed to pick up on that properly.  With that stripped out,
> you get the attached patch, which does nothing but add "const" to
> 661 lines.  It still applies cleanly, builds with no warnings, and
> passes regression tests.

So what happens when someone wants to use list_nth in one of the
outfuncs? Would we then rip all these back out? Or would we then
bite the bullet and duplicate the code?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9 November 2011 19:35, Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> So that's already duplicating list_head, foreach, and forboth.
>
> OK, I failed to pick up on that properly.  With that stripped out,
> you get the attached patch, which does nothing but add "const" to
> 661 lines.  It still applies cleanly, builds with no warnings, and
> passes regression tests.
>
> It is a bit disappointing that without creating two flavors of the
> list_head function and the foreach and forboth macros, there are a
> number of functions which aren't intended to modify their inputs
> which can't be declared with const parameters; but unless there is
> some demonstrable performance benefit from those changes, I agree
> that the argument for having the two flavors is thin.

There is another option: if list_head is changed to take a pointer to
const List and return a pointer to non-const ListCell (something I was
trying to avoid before), then no XXX_const functions/macros are
necessary, and all of the functions from the first patch can keep
their 'const', adding const to 930 lines.

I've attached a new patch, which simply adds the keyword 'const' in
lots of places, no new functions etc. This version generates no
warnings under -Wcast-qual (now that I've read Peter E's thread and
been inspired to fix up some places that previously cast away const)
for all code under backend/nodes. To achieve that I had to stray
outside backend/nodes and change get_leftop and get_rightop (from
clauses.h).

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> So what happens when someone wants to use list_nth in one of the
> outfuncs? Would we then rip all these back out?

If we just go this far and don't create a separate const flavor of
the one function and two macros, then we would at least need to rip
out the const keyword on the parameter of the affected function(s).

> Or would we then bite the bullet and duplicate the code?

I'm not sure we shouldn't go that far right up front. The entire
body of the only duplicated function is:

return l ? l->head : NULL;

As cloned code goes, I've seen worse.

Of the two new macros, one has three lines of body, the other has
one line.

If people aren't inclined to support this on the grounds of API
clarity, maybe we should do some sort of benchmark run while we have
a patch which applies cleanly before writing off the possible
performance impact, but I'm not sure what makes a good stress-test
for the affected code.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> If people aren't inclined to support this on the grounds of API
> clarity, maybe we should do some sort of benchmark run while we have
> a patch which applies cleanly before writing off the possible
> performance impact, but I'm not sure what makes a good stress-test
> for the affected code.

I don't doubt that just duplicating macros and inlineable functions is
a wash performance-wise (in fact, in principle it shouldn't change
the generated code at all). My objection is the one Robert already
noted: it takes extra brain cells to remember which function/macro
to use, and I have seen not a shred of evidence that that extra
development/maintenance effort will be repaid.

I think that "const" works materially better in C++ where you can
overload foo(struct *) and foo(const struct *) and let the compiler sort
out which is being called. In C, the impedance match is a lot worse,
so you have to pick and choose where const is worth the trouble.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I don't doubt that just duplicating macros and inlineable
> functions is a wash performance-wise (in fact, in principle it
> shouldn't change the generated code at all).

I had the impression that compilers these days could sometimes
better optimize across calls to functions with const parameters,
because previously-referenced elements of the structures could be
trusted to be unchanged across the call. I'm not talking about
calls to the inlineable function or macros themselves, but the
higher level functions which can then use const.

> My objection is the one Robert already noted: it takes extra brain
> cells to remember which function/macro to use, and I have seen not
> a shred of evidence that that extra development/maintenance effort
> will be repaid.

Well, for me at least, seeing a parameter flagged as const helps me
be sure that it will be use only for input to the function, and thus
more quickly grasp the semantics of the API. For someone who is
already familiar with an API, I doubt it helps much; and it may be
one of those cognitive differences that just exist between people.

As far as which to use when there is a const and a non-const version
-- how is that unclear? For me it seems intuitively obvious
(although perhaps my intuition is off-base) that I would use const
when I didn't want the called function to change what was pointed at
by the parameter. Maybe you're looking at the slippery slope more
than this one function and two macros, though.

> In C, the impedance match is a lot worse, so you have to pick and
> choose where const is worth the trouble.

Agreed. And I'm not sure how much of what Thomas is proposing is
worth it; it just seems prudent to consider it while the offer is
being made to do the work.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> In C, the impedance match is a lot worse, so you have to pick and
>> choose where const is worth the trouble.
>
> Agreed. And I'm not sure how much of what Thomas is proposing is
> worth it; it just seems prudent to consider it while the offer is
> being made to do the work.

If the gain is for human readers of the API rather than the compiler and
some level of automated checking, what about this trick:

#define constp

Then you can use it wherever you want to instruct readers that the
parameter is a constant, it's now a noise word as far as the compiler is
concerned (thanks to the precompiler replacing it with an empty string).

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Nov9, 2011, at 22:38 , Tom Lane wrote:
> I think that "const" works materially better in C++ where you can
> overload foo(struct *) and foo(const struct *) and let the compiler sort
> out which is being called. In C, the impedance match is a lot worse,
> so you have to pick and choose where const is worth the trouble.

Yup. In fact, C++ even *forces* you to use const in a few instances - you
aren't, for example, allowed to call non-const member functions on temporary
objects (i.e., myclass().nonconstmember() fails to compile where as
myclass().constmember() works as expected). Also, in C++ const influences
actual run-time behaviour - there's a very real difference in the life-time
of temporary objects depending on whether they're assigned to a const or
a non-const reference.

So, while C++ and C are similar in a lot of aspects, the situation regarding
const is very different.

best regards,
Florian Pflug

Thomas Munro <munro(at)ip9(dot)org> wrote:

> There is another option: if list_head is changed to take a pointer
> to const List and return a pointer to non-const ListCell
> (something I was trying to avoid before), then no XXX_const
> functions/macros are necessary, and all of the functions from the
> first patch can keep their 'const', adding const to 930 lines.

Now that you mention it, I think that's better anyway. Just because
you don't want the *called* function to change something doesn't
seem like it should imply anything about whether the *caller* should
be able to change something. Leave that to the caller unless the
function is quite sure that it is returning a pointer to something
which should be immutable in all cases.

> I've attached a new patch, which simply adds the keyword 'const'
> in lots of places, no new functions etc. This version generates
> no warnings under -Wcast-qual (now that I've read Peter E's thread
> and been inspired to fix up some places that previously cast away
> const) for all code under backend/nodes. To achieve that I had to
> stray outside backend/nodes and change get_leftop and get_rightop
> (from clauses.h).

On this end it applies cleanly, compiles without warning, and passes
check-world regression tests.

-Kevin

On Nov9, 2011, at 22:54 , Kevin Grittner wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> I don't doubt that just duplicating macros and inlineable
>> functions is a wash performance-wise (in fact, in principle it
>> shouldn't change the generated code at all).
>
> I had the impression that compilers these days could sometimes
> better optimize across calls to functions with const parameters,
> because previously-referenced elements of the structures could be
> trusted to be unchanged across the call. I'm not talking about
> calls to the inlineable function or macros themselves, but the
> higher level functions which can then use const.

I don't think that's true. Const (for pointer types) generally only
means "you cannot modify the value through *this* pointer. But there
may very well be other pointers to the same object, and those may
very well be used to modify the value at any time.

So unless both the calling and the called function are in the same
compilation unit, the compiler needs to assume that any non-local
(and even local values whose address was taken previously) value
in the calling function may change as a result of the function call.
Or at least I think so.

If we're concerned about helping the compiler produce better code,
I think we should try to make our code safe under strict aliasing
rules. AFAIK, that generally helps much more than const-correctness.
(Dunno how feasible that is, though)

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> If we're concerned about helping the compiler produce better code,
> I think we should try to make our code safe under strict aliasing
> rules. AFAIK, that generally helps much more than const-correctness.
> (Dunno how feasible that is, though)

The last time we talked about that, we gave up and added
-fno-strict-aliasing, mainly because nobody trusted gcc to warn us about
violations of the aliasing rules. That was quite some time ago though.
Perhaps recent gcc versions do better?

regards, tom lane

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Thomas Munro <munro(at)ip9(dot)org> wrote:
>> There is another option: if list_head is changed to take a pointer
>> to const List and return a pointer to non-const ListCell
>> (something I was trying to avoid before), then no XXX_const
>> functions/macros are necessary, and all of the functions from the
>> first patch can keep their 'const', adding const to 930 lines.

> Now that you mention it, I think that's better anyway.

IOW, the strchr() trick? If the C standards committee couldn't find
any better answer than that, maybe we shouldn't expect to either.

In general I don't have an objection to adding "const" to individual
routines, so long as it doesn't create propagating requirements to
const-ify other code. This may be the only way to do it.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> In general I don't have an objection to adding "const" to
> individual routines, so long as it doesn't create propagating
> requirements to const-ify other code. This may be the only way to
> do it.

As I understand it (although I'm no C expert), a "const" qualifier
on a function parameter declaration is a promise that the function
will not modify what is thus qualified. That means that it can't
pass a const parameter to another function as a parameter not also
declared const. It doesn't say anything about the object itself or
what is returned from the function.

So a non-const parameter in can be passed to a const parameter in a
call, but not vice versa. And a variable need not be declared const
to pass it to a function as a const parameter. I don't know if this
meets your conditions for non-propagation.

-Kevin

Florian Pflug <fgp(at)phlo(dot)org> wrote:
> On Nov9, 2011, at 22:54 , Kevin Grittner wrote:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>>> I don't doubt that just duplicating macros and inlineable
>>> functions is a wash performance-wise (in fact, in principle it
>>> shouldn't change the generated code at all).
>>
>> I had the impression that compilers these days could sometimes
>> better optimize across calls to functions with const parameters,
>> because previously-referenced elements of the structures could be
>> trusted to be unchanged across the call. I'm not talking about
>> calls to the inlineable function or macros themselves, but the
>> higher level functions which can then use const.
>
> I don't think that's true. Const (for pointer types) generally
> only means "you cannot modify the value through *this* pointer.
> But there may very well be other pointers to the same object, and
> those may very well be used to modify the value at any time.
>
> So unless both the calling and the called function are in the same
> compilation unit, the compiler needs to assume that any non-local
> (and even local values whose address was taken previously) value
> in the calling function may change as a result of the function
> call. Or at least I think so.

You two seem to be right. I checked some generated code where I
would have expected it to help if it was ever going to, and the
generated code was absolutely identical. It appears that the *only*
real argument for this is to document the function's contract.
Whether the benefit of that outweighs any distraction it causes
seems to be the key argument to be had here.

> If we're concerned about helping the compiler produce better code,
> I think we should try to make our code safe under strict aliasing
> rules. AFAIK, that generally helps much more than
> const-correctness. (Dunno how feasible that is, though)

I hacked my configure file to use strict aliasing and -O3, and my
usual set of regression tests passed. (make check-world, make
installcheck-world against a cluster with
default_transaction_isolation = 'serializable' and
max_prepared_transactions = 10, and make -C src/test/isolation
installcheck against the same cluster)

I did get 10 warnings like this:

warning: dereferencing type-punned pointer will break
strict-aliasing rules

I haven't yet compared code or run benchmarks.

Since 9.2 seems to be shaping up mainly as a performance release,
now might be a good time to review these compile options to see how
far we can now safely push them.

-Kevin

On ons, 2011-11-09 at 10:49 -0500, Tom Lane wrote:
> Now admittedly you can hack it, in the same
> spirit as the C library functions that are declared to take const
> pointers and return non-const pointers to the very same data

Which C library functions do that?

Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On ons, 2011-11-09 at 10:49 -0500, Tom Lane wrote:
>> Now admittedly you can hack it, in the same
>> spirit as the C library functions that are declared to take const
>> pointers and return non-const pointers to the very same data
>
> Which C library functions do that?

Tom mentioned the strchr() function, which does do that. I don't
actually find that surprising given my understanding of the
semantics. That means that the function is promising not to modify
the character array, but is not asserting that it knows the
character array to be immutable. Makes sense to me. It's up to the
caller to assign it to a "const char *" if it knows it passed in an
immutable object.

-Kevin

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On ons, 2011-11-09 at 10:49 -0500, Tom Lane wrote:
>> Now admittedly you can hack it, in the same
>> spirit as the C library functions that are declared to take const
>> pointers and return non-const pointers to the very same data

> Which C library functions do that?

strchr() is the classic example, but I believe there are some others.

regards, tom lane

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom mentioned the strchr() function, which does do that. I don't
> actually find that surprising given my understanding of the
> semantics. That means that the function is promising not to modify
> the character array, but is not asserting that it knows the
> character array to be immutable. Makes sense to me. It's up to the
> caller to assign it to a "const char *" if it knows it passed in an
> immutable object.

The problem with it of course is that mistaken use could have the
effect of casting-away-const, which is exactly what we hoped to prevent.
Still, there may not be a better solution.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> The problem with it of course is that mistaken use could have the
> effect of casting-away-const, which is exactly what we hoped to
> prevent. Still, there may not be a better solution.

Yeah, I've come to the conclusion that the compiler doesn't do the
apparently-available optimizations using const precisely because it
is so easy to cast away the property maliciously or accidentally.

-Kevin

Kevin Grittner wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > The problem with it of course is that mistaken use could have the
> > effect of casting-away-const, which is exactly what we hoped to
> > prevent. Still, there may not be a better solution.
>
> Yeah, I've come to the conclusion that the compiler doesn't do the
> apparently-available optimizations using const precisely because it
> is so easy to cast away the property maliciously or accidentally.

Right. The compiler would have to look at the function code, and all
functions called by that function, to determine if const was honored ---
not something that is easily done.

I agree that the strchr() approach is best. I realize the patch only
added 1-2 new const functions, but this is only a small area of the code
being patched --- a full solution would have many more complex
duplicates, and awkward changes as we add features.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> I realize the patch only added 1-2 new const functions

No, version 2 of the patch used the strchr() technique and has
*zero* new functions and *zero* new macros.

> but this is only a small area of the code being patched --- a full
> solution would have many more complex duplicates, and awkward
> changes as we add features.

I'm not convinced of that, and I don't think it really has a bearing
on doing where it can be done with no new functions and no changes
to the code other than adding "const" to existing lines of code.

-Kevin

Kevin Grittner wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> > I realize the patch only added 1-2 new const functions
>
> No, version 2 of the patch used the strchr() technique and has
> *zero* new functions and *zero* new macros.

Right. I was referring to the non-strchr() approach in the initial
patch.

> > but this is only a small area of the code being patched --- a full
> > solution would have many more complex duplicates, and awkward
> > changes as we add features.
>
> I'm not convinced of that, and I don't think it really has a bearing
> on doing where it can be done with no new functions and no changes
> to the code other than adding "const" to existing lines of code.

Right, again I was referring to the non-strchr() approach, e.g. new
functions.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> wrote:

>> No, version 2 of the patch used the strchr() technique and has
>> *zero* new functions and *zero* new macros.
>
> Right. I was referring to the non-strchr() approach in the
> initial patch.

I'm sorry that I misunderstood you.

So, I don't think I've heard any argument against version 2 of this
patch. Does anyone oppose this version? Is any committer willing
to commit it? I'm not sure there's much point putting it into the
CF application, since in spot-checks of object files I thought were
most likely to be affected, I found that identical object code was
generated. It seems to be strictly a matter of whether the code is
more or less readily understood with the patch applied.

-Kevin

Florian Pflug <fgp(at)phlo(dot)org> wrote:

> If we're concerned about helping the compiler produce better code,
> I think we should try to make our code safe under strict aliasing
> rules. AFAIK, that generally helps much more than
> const-correctness. (Dunno how feasible that is, though)

To get a preliminary feel for how much this might help, I set my
workstation with an i7-2600 and 16GB RAM to run Robert Haas's
pgbench concurrency tests against PostgreSQL built with (default)
-O2 and no strict aliasing versus -O3 and strict aliasing. I
ignored the ten warnings about punning under strict aliasing. Both
builds were with asserts disabled. No other changes from Friday's
HEAD. All runs were at the REPEATABLE READ isolation level. I
scheduled it for a window of time where the box wasn't running any
scheduled maintenance.

The results were interesting. While the small overlap between
samples from the two builds at most levels means that this was
somewhat unlikely to be just sampling noise, there could have been
alignment issues that account for some of the differences. In
short, the strict aliasing build always beat the other with 4
clients or fewer (on this 4 core machine), but always lost with more
than 4 clients.

1 client: +0.8%
2 clients: +2.0%
4 clients: +3.2%
8 clients: -0.9%
16 clients: -0.5%
32 clients: -0.9%

I wouldn't want to make too much out of this without repeating the
tests and trying different hardware, but I'm wondering whether the
abrupt difference at the number of cores makes sense to anybody.
Also, is there something I should do to deal with the warnings
before this would be considered a meaningful test?

Raw numbers:

no-strict-aliasing.1 tps = 7140.253910
no-strict-aliasing.1 tps = 7291.465297
no-strict-aliasing.1 tps = 7219.054359
no-strict-aliasing.2 tps = 16592.613779
no-strict-aliasing.2 tps = 15418.602945
no-strict-aliasing.2 tps = 16826.200551
no-strict-aliasing.4 tps = 48145.694444
no-strict-aliasing.4 tps = 47141.611960
no-strict-aliasing.4 tps = 47263.175254
no-strict-aliasing.8 tps = 93466.397174
no-strict-aliasing.8 tps = 93757.111493
no-strict-aliasing.8 tps = 93422.349453
no-strict-aliasing.16 tps = 88758.623319
no-strict-aliasing.16 tps = 88976.546555
no-strict-aliasing.16 tps = 88521.025343
no-strict-aliasing.32 tps = 87799.019143
no-strict-aliasing.32 tps = 88006.881881
no-strict-aliasing.32 tps = 88295.826711

strict-aliasing.1 tps = 7067.461710
strict-aliasing.1 tps = 7415.244823
strict-aliasing.1 tps = 7277.643321
strict-aliasing.2 tps = 14576.820162
strict-aliasing.2 tps = 16928.746994
strict-aliasing.2 tps = 19958.285834
strict-aliasing.4 tps = 48780.830247
strict-aliasing.4 tps = 49067.751657
strict-aliasing.4 tps = 48303.413578
strict-aliasing.8 tps = 93155.601896
strict-aliasing.8 tps = 92279.973490
strict-aliasing.8 tps = 92629.332125
strict-aliasing.16 tps = 88328.799197
strict-aliasing.16 tps = 88283.503270
strict-aliasing.16 tps = 88463.673815
strict-aliasing.32 tps = 87148.701204
strict-aliasing.32 tps = 87398.233624
strict-aliasing.32 tps = 87201.021722

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> The results were interesting. While the small overlap between
> samples from the two builds at most levels means that this was
> somewhat unlikely to be just sampling noise, there could have been
> alignment issues that account for some of the differences. In
> short, the strict aliasing build always beat the other with 4
> clients or fewer (on this 4 core machine), but always lost with more
> than 4 clients.

That is *weird*.

> Also, is there something I should do to deal with the warnings
> before this would be considered a meaningful test?

Dunno ... where were the warnings exactly? Also, did you run the
regression tests (particularly the parallel version) against the
build?

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> The results were interesting. While the small overlap between
>> samples from the two builds at most levels means that this was
>> somewhat unlikely to be just sampling noise, there could have
>> been alignment issues that account for some of the differences.
>> In short, the strict aliasing build always beat the other with 4
>> clients or fewer (on this 4 core machine), but always lost with
>> more than 4 clients.
>
> That is *weird*.

Yeah, my only theories are that it was an unlucky set of samples
(which seems a little thin looking at the numbers) or that some of
the optimizations in -O3 are about improving pipelining at what
would otherwise be an increase in cycles, but that context switching
breaks up the pipelining enough that it's a net loss at high
concurrency. That doesn't seem quite as thin as the other
explanation, but it's not very satisfying without some sort of
confirmation.

>> Also, is there something I should do to deal with the warnings
>> before this would be considered a meaningful test?
>
> Dunno ... where were the warnings exactly?

All 10 were like this:

warning: dereferencing type-punned pointer will break
strict-aliasing rules

The warning is about reading a union using a different type than was
last stored there. It seems like that might sometimes be legitimate
reasons to do that, and that if it was broken with strict aliasing
it might be broken without. But strict aliasing is new territory
for me.

> Also, did you run the regression tests (particularly the parallel
> version) against the build?

Yes. The normal parallel `make check-world`, the `make
installcheck-world` against an install with
default_transaction_isolation = 'serializable' and
max_prepared_transactions = 10, and `make -C src/test/isolation
installcheck`. All ran without problem.

I'm inclined to try -O3 and -strict-aliasing separately, with a more
iterations; but I want to fix anything that's wrong with the
aliasing first.

-Kevin

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Dunno ... where were the warnings exactly?

Ah, you asked "where", not "what". I don't think I saved that, and
I had to reboot for a new kernel, so I don't have the buffer sitting
around. I'll do a new build and let you know shortly.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:

>>> Also, is there something I should do to deal with the warnings
>>> before this would be considered a meaningful test?
>>
>> Dunno ... where were the warnings exactly?
>
> All 10 were like this:
>
> warning: dereferencing type-punned pointer will break
> strict-aliasing rules

>From HEAD checkout of a few minutes ago I now see only 9:

parse_type.c: In function *typenameTypeMod*:
parse_type.c:313:4
parse_type.c:318:4
parse_type.c:319:7

guc.c: In function *flatten_set_variable_args*:
guc.c:6036:3
guc.c:6087:7

plpython.c: In function *PLy_plan_status*:
plpython.c:3213:3

btree_utils_var.c: In function *gbt_var_node_truncate*:
btree_utils_var.c:213:2

trgm_gist.c: In function *gtrgm_consistent*:
trgm_gist.c:262:5
trgm_gist.c:262:5

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Dunno ... where were the warnings exactly?

> From HEAD checkout of a few minutes ago I now see only 9:

Hmm ... well, none of those look likely to be in performance-sensitive
areas. But I wonder just how good the trouble-detection code is these
days.

regards, tom lane

Excerpts from Kevin Grittner's message of lun nov 14 17:30:50 -0300 2011:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:

> >> Also, is there something I should do to deal with the warnings
> >> before this would be considered a meaningful test?
> >
> > Dunno ... where were the warnings exactly?
>
> All 10 were like this:
>
> warning: dereferencing type-punned pointer will break
> strict-aliasing rules

Uhm, shouldn't we expect there to be one warning for each use of a Node
using some specific node pointer type as well as something generic such
as inside a ListCell etc?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, Nov 14, 2011 at 06:25:19PM -0300, Alvaro Herrera wrote:
> > All 10 were like this:
> >
> > warning: dereferencing type-punned pointer will break
> > strict-aliasing rules
>
> Uhm, shouldn't we expect there to be one warning for each use of a Node
> using some specific node pointer type as well as something generic such
> as inside a ListCell etc?

Maybe they're safe? But in any case given the use of Node, a may be an
idea to mark it with attribute((__may_alias__)), that should clear up
most of the problems in that area.

http://ohse.de/uwe/articles/gcc-attributes.html#type-may_alias

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
-- Arthur Schopenhauer

On Monday, November 14, 2011 10:25:19 PM Alvaro Herrera wrote:
> Excerpts from Kevin Grittner's message of lun nov 14 17:30:50 -0300 2011:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> > >> Also, is there something I should do to deal with the warnings
> > >> before this would be considered a meaningful test?
> > >
> > > Dunno ... where were the warnings exactly?
> >
> > All 10 were like this:
> > warning: dereferencing type-punned pointer will break
> >
> > strict-aliasing rules
>
> Uhm, shouldn't we expect there to be one warning for each use of a Node
> using some specific node pointer type as well as something generic such
> as inside a ListCell etc?
The case with Node's being accessed by SomethingNode is legal to my knowledge
as the individual memory locations are accessed by variables of the same type.
That follows from the rules "an aggregate or union type that includes one of
the aforementioned types among its members (including, recursively, a member
of a subaggregate or contained union)" and "a type compatible with the
effective type of the object".

And the ListCell case is ok as well unless there is a wrong cast in code using
the ListCell somewhere.

E.g. its afaics safe to do something like:

void do_something_int(int);

int bla;
void* foo = &bla;
...
do_something_int(*(int*)foo);

but

do_something_short(*(short*)foo);
is illegal.

The compiler obviously cant be able to prove all misusage of the void*
pointers in e.g. ListCell's though...

Andres

On Monday, November 14, 2011 10:22:52 PM Tom Lane wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> >> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> Dunno ... where were the warnings exactly?
> >
> > From HEAD checkout of a few minutes ago I now see only 9:
> Hmm ... well, none of those look likely to be in performance-sensitive
> areas. But I wonder just how good the trouble-detection code is these
> days.
No idea about how good it is but you can make the detection code more
aggressive by -Wstrict-aliasing=1 (which will produce more false positives).

I don't gcc will ever be able to call all possible misusages. E.g. The List
api is a case where its basically impossible to catch everything (as gcc won't
be able to figure out what the ListCell.data.ptr_value pointed to originally
in the general case).

Andres

* Andres Freund:

> I don't gcc will ever be able to call all possible misusages. E.g. The
> List api is a case where its basically impossible to catch everything
> (as gcc won't be able to figure out what the ListCell.data.ptr_value
> pointed to originally in the general case).

Correct, if code is not strict-aliasing-safe and you compile with
-f-strict-aliasing, GCC may silently produce wrong code. (Same with
-fwrapv, by the way.)

--
Florian Weimer <fweimer(at)bfk(dot)de>
BFK edv-consulting GmbH http://www.bfk.de/
Kriegsstraße 100 tel: +49-721-96201-1
D-76133 Karlsruhe fax: +49-721-96201-99

Florian Weimer <fweimer(at)bfk(dot)de> wrote:
> * Andres Freund:
>
>> I don't gcc will ever be able to call all possible misusages.
>> E.g. The List api is a case where its basically impossible to
>> catch everything (as gcc won't be able to figure out what the
>> ListCell.data.ptr_value pointed to originally in the general
>> case).
>
> Correct, if code is not strict-aliasing-safe and you compile with
> -f-strict-aliasing, GCC may silently produce wrong code. (Same
> with -fwrapv, by the way.)

I've spent a little time trying to get my head around what to look
for in terms of unsafe code, but am not really there yet. Meanwhile,
I've run a few more benchmarks of -fstrict-aliasing (without also
changing to the -O3 switch) compared to a normal build. In no
benchmark so far has strict aliasing by itself performed better on
my i7, and in most cases it is slightly worse. (This means that
some of the optimizations in -O3 probably *did* have a small net
positive, since the benchmarks combining both showed a gain as long
as there weren't more clients than cores, and the net loss on just
strict aliasing would account for the decrease at higher client
counts.)

>From my reading, it appears that if we get safe code in terms of
strict aliasing, we might be able to use the "restrict" keyword to
get further optimizations which bring it to a net win, but I think
there is currently lower-hanging fruit than monkeying with these
compiler options. I'm letting this go, although I still favor the
const-ifying which started this discussion, on the grounds of API
clarity.

-Kevin

On Tue, Nov 15, 2011 at 9:02 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> From my reading, it appears that if we get safe code in terms of
> strict aliasing, we might be able to use the "restrict" keyword to
> get further optimizations which bring it to a net win, but I think
> there is currently lower-hanging fruit than monkeying with these
> compiler options.  I'm letting this go, although I still favor the
> const-ifying which started this discussion, on the grounds of API
> clarity.

Speaking of lower-hanging fruit...

I ran a series of tests to see how different optimization flags
affect performance. I was particularly interested in what effect
link time optimization has. The results are somewhat interesting.

Benchmark machine is my laptop, Intel Core i5 M 540 @ 2.53GHz.
2 cores + hyperthreading for a total of 4 threads. Ubuntu 11.10.
Compiled with GCC 4.6.1-9ubuntu3.

I ran pgbench read only test with scale factor 10, default
options except for shared_buffers = 256MB. The dataset fits fully
in shared buffers.

I tried following configurations:
default: plain old ./configure; make; make install
-O3: what it says on the label
lto: CFLAGS="-O3 -flto" This should do some global optimizations
at link time.
PGO: compiled with CFLAGS="-O3 -fprofile-generate", then ran
pgbench -T 30 on a scalefactor 100 database (IO bound rw load
to mix the profile up a bit). Then did
# sed -i s/-fprofile-generate/-fprofile-use/ src/Makefile.global
and recompiled and installed.
lto + PGO: same as previous, but with added -flto.

Median tps of 3 5 minute runs at different concurrency levels:

-c default -O3 lto PGO lto + PGO
==================================================
1 6753.40 6689.76 6498.37 6614.73 5918.65
2 11600.87 11659.33 12074.63 12957.81 13353.54
4 18852.86 18918.32 19008.89 20006.49 20652.93
8 15232.30 15762.70 14568.06 15880.19 16091.24
16 15693.93 15625.87 16563.91 17088.28 18223.02

Percentage increase from default flags:

-c default -O3 lto PGO lto + PGO
==================================================
1 6753.40 -0.94% -3.78% -2.05% -12.36%
2 11600.87 0.50% 4.08% 11.70% 15.11%
4 18852.86 0.35% 0.83% 6.12% 9.55%
8 15232.30 3.48% -4.36% 4.25% 5.64%
16 15693.93 -0.43% 5.54% 8.88% 16.12%

Concurrency 8 results should probably be ignored - variance was huge,
definitely more than the differences. For other results, variance was
~1%.

I don't know what to make of the single client results, why they seem
to be going in the opposite direction of all other results. Other than
that both profile guided optimization and link time optimization give
a pretty respectable boost. If anyone can suggest some more diverse
workloads to test, I could try to see if the PGO results persist when
profiling and benchmark loads differ more. These results suggest that
giving the compiler information about hot and cold paths results in a
significant improvement in generated code quality.

--
Ants Aasma

Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:

> Concurrency 8 results should probably be ignored - variance was
> huge, definitely more than the differences.

I'm not so sure it should be ignored -- one thing I noticed in
looking at the raw numbers from my benchmarks was that the -O2 code
was much more consistent from run to run than the -O3 code. I doubt
that the more aggressive optimizations were developed under NUMA
architecture, and I suspect that the aggressively optimized code may
be more sensitive to whether memory is directly accessed by the core
running the process or routed though the memory controller on
another core. (I hit on this idea this morning when I remembered
seeing similar variations in run times of STREAM against our new
servers with NUMA.)

This suggests that in the long term, it might be worth investigating
whether we can arrange for a connection's process to have some
degree of core affinity and encourage each process to allocate local
memory from RAM controlled by that core. To some extent I would
expect the process-based architecture of PostgreSQL to help with
that, as you would expect a NUMA-aware OS to try to arrange that to
some degree.

-Kevin

On Wed, Nov 16, 2011 at 9:47 AM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> This suggests that in the long term, it might be worth investigating
> whether we can arrange for a connection's process to have some
> degree of core affinity and encourage each process to allocate local
> memory from RAM controlled by that core.  To some extent I would
> expect the process-based architecture of PostgreSQL to help with
> that, as you would expect a NUMA-aware OS to try to arrange that to
> some degree.

I've done some testing on HP/UX-Itanium and have not been able to
demonstrate any significant performance benefit from overriding the
operating system's default policies regarding processor affinity. For
example, I hacked the code to request that the shared memory be
allocated as cell-local memory, then used mpsched with the FILL_TREE
policy to bind everything to a single cell, and sure enough it all ran
in that cell, but it wasn't any better than 4 clients running on
different cells with the shared memory segment allocated interleaved.
This result didn't really make much sense to me, because it seemed
like it SHOULD have helped. So it's possible I did something wrong.
But if so, I couldn't find it. The other possibility is that the OS
is smart enough about moving things around to get good locality that
sticking locality hints on top doesn't really make any difference.
Certainly, I would expect any OS to be smart enough to allocate
backend-local memory on the same processor where the task is running,
and to avoid moving processes between cells more than necessary.

Regarding results instability, on some patch sets I've tried, I've
seen very unstable performance. I've also noticed that a very short
run sometimes gives much higher performance than a longer run. My
working theory is that this is the result of spinlock contention.
Suppose you have a spinlock that is getting passed around very quickly
between, say, 32 processes. Since the data protected by the spinlock
is on the same cache line as the lock, what ideally happens is that
the process gets the lock and then finishes its work and releases the
lock before anyone else tries to pull the cache line away. And at the
beginning of the run, that's what does actually happen. But then for
some reason a process gets context-switched out while it holds the
lock, or maybe it's just that somebody gets unlucky enough to have the
cache line stolen before they can dump the spinlock and can't quite
get it back fast enough. Now people start to pile up trying to get
that spinlock, and that means trouble, because now it's much harder
for any given process to get the cache line and finish its work before
the cache line gets stolen away. So my theory is that now the
performance goes down more or less "permanently", unless or until
there's some momentary break in the action that lets the queue of
people waiting for that spinlock drain out. This is just a wild-ass
guess, and I might be totally wrong...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:

>> This suggests that in the long term, it might be worth [...]

> The other possibility is that the OS is smart enough about moving
> things around to get good locality that sticking locality hints on
> top doesn't really make any difference. Certainly, I would expect
> any OS to be smart enough to allocate backend-local memory on the
> same processor where the task is running, and to avoid moving
> processes between cells more than necessary.

Right. I'm not sure that it will make any more sense to do this
than to do raw access to a disk partition. I don't think it's a
given that we can do a better job of this than the OS does.

> Regarding results instability [...] My working theory is that this
> is the result of spinlock contention.

> So my theory is that now the performance goes down more or less
> "permanently", unless or until there's some momentary break in the
> action that lets the queue of people waiting for that spinlock
> drain out. This is just a wild-ass guess, and I might be totally
> wrong...

Well, I suspect that you're basing that guess on enough evidence
that it's more likely to be right than the wild-assed guesses I've
been throwing out there. :-) I can't say it's inconsistent with
anything I've seen.

-Kevin

On ons, 2011-11-09 at 21:15 +0000, Thomas Munro wrote:
> I've attached a new patch, which simply adds the keyword 'const' in
> lots of places, no new functions etc. This version generates no
> warnings under -Wcast-qual (now that I've read Peter E's thread and
> been inspired to fix up some places that previously cast away const)
> for all code under backend/nodes. To achieve that I had to stray
> outside backend/nodes and change get_leftop and get_rightop (from
> clauses.h).

Patch committed.