On Fri, Oct 18, 2013 at 1:39 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Fri, Oct 11, 2013 at 1:14 AM, Noah Misch <n...@leadboat.com> wrote: > > On Thu, Oct 10, 2013 at 03:23:30PM +0200, Andres Freund wrote: > >> On 2013-10-10 08:59:47 -0400, Robert Haas wrote: > >> > On Tue, Oct 8, 2013 at 6:24 PM, Andres Freund <and...@2ndquadrant.com> > wrote: > >> > > Do you have a better alternative? Making the computation > unconditionally > >> > > 64bit will have a runtime overhead and adding a StaticAssert in the > >> > > existing macro doesn't work because we use it in array sizes where > gcc > >> > > balks. > >> > > We could try using inline functions, but that's not going to be > pretty > >> > > either. > >> > > > >> > > I don't really see that many further usecases that will align 64bit > >> > > values on 32bit platforms, so I think we're ok for now. > >> > > >> > I'd be inclined to make the computation unconditionally 64-bit. I > >> > doubt the speed penalty is enough to worry about, and I think we're > >> > going to have more and more cases where optimizing for 32-bit > >> > platforms is just not the right decision. > >> > >> MAXALIGN is used in several of PG's hottest functions in many > >> scenarios. att_align_nominal is used in slot_deform_tuple, > >> heap_deform_tuple, nocachegetattr, etc. So I don't think that's viable > >> yet. At least not with much more benefit than this... > > > > Agreed. Besides performance, aligning a wider-than-pointer value is an > > unusual need; authors should think thrice before doing that. I might > have > > even defined the MAXALIGN64 macro in xlog.c rather than a core header. > > > > Incidentally, why does MAXALIGN64 use unsigned math while MAXALIGN uses > signed > > math? > > Well, if this is the consensus, then I think the dynamic shared memory > patch may need some revision. In that patch, I used uint64 to > represent the size of the dynamic shared memory segment, sort of on > the theory that we were going to use this to be allocating big chunks > of dynamic shared memory for stuff like parallel sort. In follow-on > patches I'm currently developing to actually do stuff with dynamic > shared memory, this results in extensive use of MAXALIGN64, and it > really kind of looks like it wants the whole set of alignment macros, > not just that one. So option one is to leave the dsm code alone and > add the rest of the macros. > > For me I don't really see why there's a need to use MAXALIGN64 for any memory addresses related to RAM. I only created MAXALIGN64 because I needed it to fix the WAL code which needed as 64bit type on all platforms, not just 64bit ones. For me it made perfect sense, so I'm a bit confused at most of this fuss. Though I do understand that it's a bit weird that both macros are almost the same on a 64 bit machine... As for signed vs unsigned, I've not looked at all of the places where MAXALIGN is used, but I just assumed it was for memory addresses, if this is the case then I'm confused why we'd ever want a negative valued memory address? This might be an obvious one, but can anyone tell me why the casts are in the macro at all? Can a compiler not decide for itself which type it should be using? Regards David Rowley > But if we're bent on minimizing the use of 64-bit arithmetic on 32-bit > systems, then presumably I should instead go back and retrofit that > patch to use Size rather than uint64 to represent the size of a > segment. But then I have two concerns: > > 1. Is there any guarantee that sizeof(intptr_t) >= sizeof(size_t)? > (Note that Size is just a typedef for size_t, in c.h) > > 2. If intptr_t is a signed type, as it appears to be, and size_t is an > unsigned type, as I believe it to be, then is it safe to use the > macros written for the signed type with a value of the unsigned type? > Off-hand I can't see a problem there, but I'm not certain I'm not > missing something. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >