Re: [HACKERS] Windows warnings from VS 2017

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