Thread: [HACKERS] Windows warnings from VS 2017
I'm working through testing the changes to allow building with Visual Studio 2017. I've noticed that there are some warnings that we don't see in earlier compilers. First, it warns about a couple of unused variables at lines 4553 and 4673 of src/backend/optimizer/path/costsize.c. I think we can do a little rearrangement to keep it happy there. It's also warning that it will copy 16 bytes to a 13 byte structure at lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't seen any ill effects of this so far, but it seems to indicate that something is possibly amiss on this compiler with the MemSet macros. The regression tests are currently failing on my test platform (Windows Server 2016) because it says it can't change permissions on the testtablespace directory. I have no idea what's going on there, still investigating. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > First, it warns about a couple of unused variables at lines 4553 and > 4673 of src/backend/optimizer/path/costsize.c. I think we can do a > little rearrangement to keep it happy there. Those are around for some time, see here: https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com But there has been no actual agreement about how to fix them.. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 09/20/2017 06:13 PM, Michael Paquier wrote: > On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan > <andrew.dunstan@2ndquadrant.com> wrote: >> First, it warns about a couple of unused variables at lines 4553 and >> 4673 of src/backend/optimizer/path/costsize.c. I think we can do a >> little rearrangement to keep it happy there. > Those are around for some time, see here: > https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com > But there has been no actual agreement about how to fix them.. Oh. Missed that. My solution was going to be slightly different. I was going to enclose the #ifdef'd code in a bare block and move the rte declaration inside that block. I think the difference between these cases and other cases of the PG_USER_FOR_ASSERTS_ONLY macro is that these variables are only even written to by assert-enabled code, whereas in the other cases the variable is written to by non-assert-enabed code but only read by assert-enabled code. The Microsoft compilers don't seem to mind that so much. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 21, 2017 at 7:49 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > On 09/20/2017 06:13 PM, Michael Paquier wrote: >> On Thu, Sep 21, 2017 at 7:08 AM, Andrew Dunstan >> <andrew.dunstan@2ndquadrant.com> wrote: >>> First, it warns about a couple of unused variables at lines 4553 and >>> 4673 of src/backend/optimizer/path/costsize.c. I think we can do a >>> little rearrangement to keep it happy there. >> Those are around for some time, see here: >> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com >> But there has been no actual agreement about how to fix them.. > > Oh. Missed that. > > My solution was going to be slightly different. I was going to enclose > the #ifdef'd code in a bare block and move the rte declaration inside > that block. That looks like a fine approach to me. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > On 09/20/2017 06:13 PM, Michael Paquier wrote: >> Those are around for some time, see here: >> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com >> But there has been no actual agreement about how to fix them.. > Oh. Missed that. > My solution was going to be slightly different. I was going to enclose > the #ifdef'd code in a bare block and move the rte declaration inside > that block. Of the various solutions proposed in the previous thread, I think the most salable alternative is probably ilmari's: get rid of the variable and write the assert like Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY); That's a pretty minimal change and it doesn't add any cycles to the non-Assert case. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> It's also warning that it will copy 16 bytes to a 13 byte structure at
> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't
> seen any ill effects of this so far, but it seems to indicate that
> something is possibly amiss on this compiler with the MemSet macros.
That's weird. Is it too stupid to figure out that the if() inside
MemSet evaluates to constant false in these calls? It seems hard to
see how it would realize that the loop will write 16 bytes if it doesn't
propagate the constant value forward.
However ... on some other compilers, I've noticed that the compiler seems
more likely to make "obvious" deductions of that sort if the variables in
question are marked const. Does it help if you do
- void *_vstart = (void *) (start); \
- int _val = (val); \
- Size _len = (len); \
+ void * const _vstart = (void *) (start); \
+ const int _val = (val); \
+ const Size _len = (len); \
I don't think there's any strong reason not to just do s/MemSet/memset/
in these calls and nearby ones, but it would be good to understand just
what's wrong here. And why it's only showing up in that file; seems
nearly certain that we have similar coding elsewhere.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 09/20/2017 07:32 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 09/20/2017 06:13 PM, Michael Paquier wrote: >>> Those are around for some time, see here: >>> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1JVvYWd_G0WrKOT+4uFQjGGrv8osQbUZzXGXdA@mail.gmail.com >>> But there has been no actual agreement about how to fix them.. >> Oh. Missed that. >> My solution was going to be slightly different. I was going to enclose >> the #ifdef'd code in a bare block and move the rte declaration inside >> that block. > Of the various solutions proposed in the previous thread, I think the > most salable alternative is probably ilmari's: get rid of the variable > and write the assert like > > Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY); > > That's a pretty minimal change and it doesn't add any cycles to the > non-Assert case. > > I can live with that. Will do. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 09/20/2017 07:54 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> It's also warning that it will copy 16 bytes to a 13 byte structure at >> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't >> seen any ill effects of this so far, but it seems to indicate that >> something is possibly amiss on this compiler with the MemSet macros. > That's weird. Is it too stupid to figure out that the if() inside > MemSet evaluates to constant false in these calls? It seems hard to > see how it would realize that the loop will write 16 bytes if it doesn't > propagate the constant value forward. > > However ... on some other compilers, I've noticed that the compiler seems > more likely to make "obvious" deductions of that sort if the variables in > question are marked const. Does it help if you do > > - void *_vstart = (void *) (start); \ > - int _val = (val); \ > - Size _len = (len); \ > + void * const _vstart = (void *) (start); \ > + const int _val = (val); \ > + const Size _len = (len); \ > > > I don't think there's any strong reason not to just do s/MemSet/memset/ > in these calls and nearby ones, but it would be good to understand just > what's wrong here. And why it's only showing up in that file; seems > nearly certain that we have similar coding elsewhere. > > I'll test it. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 09/20/2017 08:18 PM, Andrew Dunstan wrote: > > On 09/20/2017 07:54 PM, Tom Lane wrote: >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >>> It's also warning that it will copy 16 bytes to a 13 byte structure at >>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't >>> seen any ill effects of this so far, but it seems to indicate that >>> something is possibly amiss on this compiler with the MemSet macros. >> That's weird. Is it too stupid to figure out that the if() inside >> MemSet evaluates to constant false in these calls? It seems hard to >> see how it would realize that the loop will write 16 bytes if it doesn't >> propagate the constant value forward. >> >> However ... on some other compilers, I've noticed that the compiler seems >> more likely to make "obvious" deductions of that sort if the variables in >> question are marked const. Does it help if you do >> >> - void *_vstart = (void *) (start); \ >> - int _val = (val); \ >> - Size _len = (len); \ >> + void * const _vstart = (void *) (start); \ >> + const int _val = (val); \ >> + const Size _len = (len); \ >> >> >> I don't think there's any strong reason not to just do s/MemSet/memset/ >> in these calls and nearby ones, but it would be good to understand just >> what's wrong here. And why it's only showing up in that file; seems >> nearly certain that we have similar coding elsewhere. >> >> > > I'll test it. > Doesn't make a difference. I agree it would be good to understand what's going on. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 21, 2017 at 12:26 PM, Andrew Dunstan <andrew.dunstan@2ndquadrant.
Doesn't make a difference. I agree it would be good to understand what's
On 09/20/2017 08:18 PM, Andrew Dunstan wrote:
>
> On 09/20/2017 07:54 PM, Tom Lane wrote:
>> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>>> It's also warning that it will copy 16 bytes to a 13 byte structure at
>>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't
>>> seen any ill effects of this so far, but it seems to indicate that
>>> something is possibly amiss on this compiler with the MemSet macros.
>> That's weird. Is it too stupid to figure out that the if() inside
>> MemSet evaluates to constant false in these calls? It seems hard to
>> see how it would realize that the loop will write 16 bytes if it doesn't
>> propagate the constant value forward.
>>
>> However ... on some other compilers, I've noticed that the compiler seems
>> more likely to make "obvious" deductions of that sort if the variables in
>> question are marked const. Does it help if you do
>>
>> - void *_vstart = (void *) (start); \
>> - int _val = (val); \
>> - Size _len = (len); \
>> + void * const _vstart = (void *) (start); \
>> + const int _val = (val); \
>> + const Size _len = (len); \
>>
>>
>> I don't think there's any strong reason not to just do s/MemSet/memset/
>> in these calls and nearby ones, but it would be good to understand just
>> what's wrong here. And why it's only showing up in that file; seems
>> nearly certain that we have similar coding elsewhere.
>>
>>
>
> I'll test it.
>
going on.
These warnings are not produced when the build is run in DEBUG mode.
Because of this I didn't see these warning when I was working with VS 2017.
This may be an issue with VS 2017 compiler.
Regards,
Hari Babu
Fujitsu Australia
On 09/21/2017 02:53 AM, Haribabu Kommi wrote: > > > On Thu, Sep 21, 2017 at 12:26 PM, Andrew Dunstan > <andrew.dunstan@2ndquadrant.com > <mailto:andrew.dunstan@2ndquadrant.com>> wrote: > > > > On 09/20/2017 08:18 PM, Andrew Dunstan wrote: > > > > On 09/20/2017 07:54 PM, Tom Lane wrote: > >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com > <mailto:andrew.dunstan@2ndquadrant.com>> writes: > >>> It's also warning that it will copy 16 bytes to a 13 byte > structure at > >>> lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. > I haven't > >>> seen any ill effects of this so far, but it seems to indicate that > >>> something is possibly amiss on this compiler with the MemSet > macros. > >> That's weird. Is it too stupid to figure out that the if() inside > >> MemSet evaluates to constant false in these calls? It seems > hard to > >> see how it would realize that the loop will write 16 bytes if > it doesn't > >> propagate the constant value forward. > >> > >> However ... on some other compilers, I've noticed that the > compiler seems > >> more likely to make "obvious" deductions of that sort if the > variables in > >> question are marked const. Does it help if you do > >> > >> - void *_vstart = (void *) (start); \ > >> - int _val = (val); \ > >> - Size _len = (len); \ > >> + void * const _vstart = (void *) (start); \ > >> + const int _val = (val); \ > >> + const Size _len = (len); \ > >> > >> > >> I don't think there's any strong reason not to just do > s/MemSet/memset/ > >> in these calls and nearby ones, but it would be good to > understand just > >> what's wrong here. And why it's only showing up in that file; > seems > >> nearly certain that we have similar coding elsewhere. > >> > >> > > > > I'll test it. > > > > > Doesn't make a difference. I agree it would be good to understand > what's > going on. > > > These warnings are not produced when the build is run in DEBUG mode. > Because of this I didn't see these warning when I was working with VS > 2017. > > This may be an issue with VS 2017 compiler. > Yeah, it seems like it, particularly when identical code in another function in the same file doesn't generate a complaint. The speed of memset is hardly going to be the dominating factor in a 'CREATE DATABASE' command, so we could certainly afford to change to plain memset calls here. I'll see if I can find someone to report this to :-) cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> The speed of memset is hardly going to be the dominating factor in a
> 'CREATE DATABASE' command, so we could certainly afford to change to
> plain memset calls here.
Another thought is that it may be time for our decennial debate about
whether MemSet is worth the electrons it's printed on. I continue to
think that any modern compiler+libc ought to do an equivalent or better
optimization given just a plain memset(). If that seems to be true
for recent MSVC, we could consider putting an #if into c.h to define
MemSet as just memset for MSVC. Maybe later that could be extended
to other compilers.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 21, 2017 at 10:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Another thought is that it may be time for our decennial debate about > whether MemSet is worth the electrons it's printed on. I continue to > think that any modern compiler+libc ought to do an equivalent or better > optimization given just a plain memset(). If that seems to be true > for recent MSVC, we could consider putting an #if into c.h to define > MemSet as just memset for MSVC. Maybe later that could be extended > to other compilers. +1. Things change quickly in IT. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-09-21 09:30:31 -0400, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > > The speed of memset is hardly going to be the dominating factor in a > > 'CREATE DATABASE' command, so we could certainly afford to change to > > plain memset calls here. > > Another thought is that it may be time for our decennial debate about > whether MemSet is worth the electrons it's printed on. I continue to > think that any modern compiler+libc ought to do an equivalent or better > optimization given just a plain memset(). If that seems to be true > for recent MSVC, we could consider putting an #if into c.h to define > MemSet as just memset for MSVC. Maybe later that could be extended > to other compilers. +many. glibc's memset is nearly an order of magnitude faster than MemSet on my machine for medium+ length copies, and still 3-4 four times ~100 bytes. And both gcc and clang optimize way the memcpy entirely when the length is a fixed short length. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 09/21/2017 09:41 PM, Andres Freund wrote: > On 2017-09-21 09:30:31 -0400, Tom Lane wrote: >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >>> The speed of memset is hardly going to be the dominating factor in a >>> 'CREATE DATABASE' command, so we could certainly afford to change to >>> plain memset calls here. >> Another thought is that it may be time for our decennial debate about >> whether MemSet is worth the electrons it's printed on. I continue to >> think that any modern compiler+libc ought to do an equivalent or better >> optimization given just a plain memset(). If that seems to be true >> for recent MSVC, we could consider putting an #if into c.h to define >> MemSet as just memset for MSVC. Maybe later that could be extended >> to other compilers. > +many. glibc's memset is nearly an order of magnitude faster than MemSet > on my machine for medium+ length copies, and still 3-4 four times ~100 > bytes. And both gcc and clang optimize way the memcpy entirely when the > length is a fixed short length. Perhaps we need some sort of small benchmark program that we can run on a representative set of platforms? I presume if you can make that assertion you already have something along those lines? cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
> On 09/21/2017 09:41 PM, Andres Freund wrote: > > On 2017-09-21 09:30:31 -0400, Tom Lane wrote: > >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > >>> The speed of memset is hardly going to be the dominating factor in a > >>> 'CREATE DATABASE' command, so we could certainly afford to change to > >>> plain memset calls here. > >> Another thought is that it may be time for our decennial debate about > >> whether MemSet is worth the electrons it's printed on. I continue to > >> think that any modern compiler+libc ought to do an equivalent or better > >> optimization given just a plain memset(). If that seems to be true > >> for recent MSVC, we could consider putting an #if into c.h to define > >> MemSet as just memset for MSVC. Maybe later that could be extended > >> to other compilers. > > +many. glibc's memset is nearly an order of magnitude faster than MemSet > > on my machine for medium+ length copies, and still 3-4 four times ~100 > > bytes. And both gcc and clang optimize way the memcpy entirely when the > > length is a fixed short length. > Perhaps we need some sort of small benchmark program that we can run on > a representative set of platforms? I personally don't think that's needed here, given how incredibly widely used memset is in our codebase. > I presume if you can make that assertion you already have something > along those lines? Not really. I just replaced memset with MemSetAligned in a bunch of places in the code and looked at profiles. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> I presume if you can make that assertion you already have something
>> along those lines?
> Not really. I just replaced memset with MemSetAligned in a bunch of
> places in the code and looked at profiles.
Well, it's not that much work to try it and see. I compared results
of this simplistic test case:
pgbench -S -c 1 -T 60 bench
(using a scale-factor-10 pgbench database) on current HEAD and HEAD
with the attached patch, which just lobotomizes all the MemSet
macros into memset(). Median of 3 runs is 9698 tps for HEAD and
9675 tps with the patch; the range is ~100 tps though, making
this difference well within the noise level.
I did this using RHEL6's gcc (Red Hat 4.4.7-18), which is pretty
far from bleeding-edge so I think it's okay as a baseline for
what optimizations we can expect to find used in the field.
So I can't sustain Andres' assertion that memset is actually faster
for the cases we care about, but it certainly doesn't seem any
slower either. It would be interesting to check compromise
patches, such as keeping only the MemSetLoop case.
BTW, I got a couple of warnings with this patch:
pmsignal.c: In function 'PMSignalShmemInit':
pmsignal.c:104: warning: passing argument 1 of 'memset' discards qualifiers from pointer target type
/usr/include/string.h:65: note: expected 'void *' but argument is of type 'volatile struct PMSignalData *'
procsignal.c: In function 'ProcSignalInit':
procsignal.c:119: warning: passing argument 1 of 'memset' discards qualifiers from pointer target type
/usr/include/string.h:65: note: expected 'void *' but argument is of type 'volatile sig_atomic_t *'
Those seem like maybe something to look into. MemSet is evidently
casting away pointer properties that it perhaps shouldn't.
regards, tom lane
diff --git a/src/include/c.h b/src/include/c.h
index b6a9697..f7ea3a5 100644
*** a/src/include/c.h
--- b/src/include/c.h
*************** typedef NameData *Name;
*** 843,874 ****
* memset() functions. More research needs to be done, perhaps with
* MEMSET_LOOP_LIMIT tests in configure.
*/
! #define MemSet(start, val, len) \
! do \
! { \
! /* must be void* because we don't know if it is integer aligned yet */ \
! void *_vstart = (void *) (start); \
! int _val = (val); \
! Size _len = (len); \
! \
! if ((((uintptr_t) _vstart) & LONG_ALIGN_MASK) == 0 && \
! (_len & LONG_ALIGN_MASK) == 0 && \
! _val == 0 && \
! _len <= MEMSET_LOOP_LIMIT && \
! /* \
! * If MEMSET_LOOP_LIMIT == 0, optimizer should find \
! * the whole "if" false at compile time. \
! */ \
! MEMSET_LOOP_LIMIT != 0) \
! { \
! long *_start = (long *) _vstart; \
! long *_stop = (long *) ((char *) _start + _len); \
! while (_start < _stop) \
! *_start++ = 0; \
! } \
! else \
! memset(_vstart, _val, _len); \
! } while (0)
/*
* MemSetAligned is the same as MemSet except it omits the test to see if
--- 843,849 ----
* memset() functions. More research needs to be done, perhaps with
* MEMSET_LOOP_LIMIT tests in configure.
*/
! #define MemSet(start, val, len) memset(start, val, len)
/*
* MemSetAligned is the same as MemSet except it omits the test to see if
*************** typedef NameData *Name;
*** 876,901 ****
* that the pointer is suitably aligned (typically, because he just got it
* from palloc(), which always delivers a max-aligned pointer).
*/
! #define MemSetAligned(start, val, len) \
! do \
! { \
! long *_start = (long *) (start); \
! int _val = (val); \
! Size _len = (len); \
! \
! if ((_len & LONG_ALIGN_MASK) == 0 && \
! _val == 0 && \
! _len <= MEMSET_LOOP_LIMIT && \
! MEMSET_LOOP_LIMIT != 0) \
! { \
! long *_stop = (long *) ((char *) _start + _len); \
! while (_start < _stop) \
! *_start++ = 0; \
! } \
! else \
! memset(_start, _val, _len); \
! } while (0)
!
/*
* MemSetTest/MemSetLoop are a variant version that allow all the tests in
--- 851,857 ----
* that the pointer is suitably aligned (typically, because he just got it
* from palloc(), which always delivers a max-aligned pointer).
*/
! #define MemSetAligned(start, val, len) memset(start, val, len)
/*
* MemSetTest/MemSetLoop are a variant version that allow all the tests in
*************** typedef NameData *Name;
*** 911,925 ****
MEMSET_LOOP_LIMIT != 0 && \
(val) == 0 )
! #define MemSetLoop(start, val, len) \
! do \
! { \
! long * _start = (long *) (start); \
! long * _stop = (long *) ((char *) _start + (Size) (len)); \
! \
! while (_start < _stop) \
! *_start++ = 0; \
! } while (0)
/*
--- 867,873 ----
MEMSET_LOOP_LIMIT != 0 && \
(val) == 0 )
! #define MemSetLoop(start, val, len) memset(start, val, len)
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-10-11 18:20:24 -0400, Tom Lane wrote:
> Well, it's not that much work to try it and see. I compared results
> of this simplistic test case:
> pgbench -S -c 1 -T 60 bench
> (using a scale-factor-10 pgbench database) on current HEAD and HEAD
> with the attached patch, which just lobotomizes all the MemSet
> macros into memset(). Median of 3 runs is 9698 tps for HEAD and
> 9675 tps with the patch; the range is ~100 tps though, making
> this difference well within the noise level.
>
> I did this using RHEL6's gcc (Red Hat 4.4.7-18), which is pretty
> far from bleeding-edge so I think it's okay as a baseline for
> what optimizations we can expect to find used in the field.
>
> So I can't sustain Andres' assertion that memset is actually faster
> for the cases we care about, but it certainly doesn't seem any
> slower either. It would be interesting to check compromise
> patches, such as keeping only the MemSetLoop case.
Well, that'd be because that workload doesn't exercise either version of
memset to a meaningful degree, right?
> +#define MemSetAligned(start, val, len) memset(start, val, len)
If we actually care about this optimization, which seems to solely serve
palloc0fast, we could tell the compiler about the guaranteed alignment
etc. I'm doubtful it's worth making this work for palloc0fast alone,
but I think it might be worthwhile to make palloc0 an inline function
like
#ifdef YES_I_AM_A_GOOD_COMPILER
/* not needed here, but probably good for plenty other places */
#define pg_attribute_assume_aligned(align) __attribute__((assume_aligned(align)))
#define pg_assume_aligned(ptr, align) __builtin_assume_aligned(ptr, align)
#else
...
#end
extern void *palloc(Size size) pg_attribute_assume_aligned(MAXALIGN);
static inline void *
palloc0(Size size)
{ void *mem = palloc(size);
memset(mem, 0, size); return mem;
}
that should allow the compiler to always optimize the memset based on
the alignment - at least x86 gcc does so - and on the size if a
built-time constant (nearly every compiler does so).
I assume we probably should do this dance not just for palloc, but for
the other allocation functions too.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers