Обсуждение: 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@ip9.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
Tom Lane <tgl@sss.pgh.pa.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@2ndquadrant.com> writes: > On 9 November 2011 15:24, Tom Lane <tgl@sss.pgh.pa.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.Grittner@wicourts.gov> writes: > Tom Lane <tgl@sss.pgh.pa.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.Grittner@wicourts.gov> wrote: > Tom Lane <tgl@sss.pgh.pa.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
On 9 November 2011 15:24, Tom Lane <tgl@sss.pgh.pa.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
Robert Haas <robertmhaas@gmail.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.Grittner@wicourts.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 pointerto const List; I needed that so I could make foreach_const and forboth_const; they were needed to be able to makelist_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@sss.pgh.pa.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 toconst 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 transactionsin the database. Probably the most extreme and tedious way of introducing full const correctness in PostgreSQL would be to rename all filesto .cpp, and let c++ compiler tell you what you need to fix.
Robert Haas <robertmhaas@gmail.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.Grittner@wicourts.gov> wrote: > Robert Haas <robertmhaas@gmail.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.Grittner@wicourts.gov> wrote: > Robert Haas <robertmhaas@gmail.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@gmail.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.Grittner@wicourts.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@sss.pgh.pa.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.Grittner@wicourts.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@ip9.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@sss.pgh.pa.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@phlo.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.Grittner@wicourts.gov> writes: > Thomas Munro <munro@ip9.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@sss.pgh.pa.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@phlo.org> wrote: > On Nov9, 2011, at 22:54 , Kevin Grittner wrote: >> Tom Lane <tgl@sss.pgh.pa.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@gmx.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@gmx.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.Grittner@wicourts.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@sss.pgh.pa.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@sss.pgh.pa.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.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@phlo.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.Grittner@wicourts.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@sss.pgh.pa.us> wrote: > "Kevin Grittner" <Kevin.Grittner@wicourts.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@sss.pgh.pa.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.Grittner@wicourts.gov> wrote: > Tom Lane <tgl@sss.pgh.pa.us> wrote: >> "Kevin Grittner" <Kevin.Grittner@wicourts.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.Grittner@wicourts.gov> writes: >> Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > > "Kevin Grittner" <Kevin.Grittner@wicourts.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@commandprompt.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@svana.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@sss.pgh.pa.us> wrote: > > > "Kevin Grittner" <Kevin.Grittner@wicourts.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.Grittner@wicourts.gov> writes: > >> Tom Lane <tgl@sss.pgh.pa.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@bfk.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@bfk.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.Grittner@wicourts.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 rwload 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.652 11600.87 11659.33 12074.6312957.81 13353.544 18852.86 18918.32 19008.89 20006.49 20652.938 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.aasma@eesti.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.Grittner@wicourts.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@gmail.com> wrote: > Kevin Grittner <Kevin.Grittner@wicourts.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.