[HACKERS] [PATCH v4] [libpq] Try to avoid manually masking SIGPIPEs on every send()
Currently, libpq will wrap each send() call on the connection with
two system calls to mask SIGPIPEs. This results in 3 syscalls instead
of one, and (on Linux) can lead to high contention on the signal
mask locks in threaded apps.
We have a couple of other methods to avoid SIGPIPEs:
sockopt(SO_NOSIGPIPE) and the MSG_NOSIGNAL flag to send().
This change attempts to use these if they're available at compile-
and run-time. If not, we drop back to manipulating the signal mask as
before.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
v4: roll into one patch, use macros
---
src/interfaces/libpq/fe-connect.c | 42
src/interfaces/libpq/fe-secure.c | 131 ++
src/interfaces/libpq/libpq-int.h |2
3 files changed, 136 insertions(+), 39 deletions(-)
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***
*** 1089,1094 keep_going: /* We
will come back to here until there is
--- 1089,1097
while (conn-addr_cur != NULL)
{
struct addrinfo *addr_cur =
conn-addr_cur;
+ #ifdef SO_NOSIGPIPE
+ int optval;
+ #endif /* SO_NOSIGPIPE */
/* Remember current address for
possible error msg */
memcpy(conn-raddr.addr,
addr_cur-ai_addr,
***
*** 1153,1158 keep_going: /* We
will come back to here until there is
--- 1156,1200
}
#endif /* F_SETFD */
+ /* We have three methods of blocking
sigpipe during
+* send() calls to this socket:
+*
+* - setsockopt(sock, SO_NOSIGPIPE)
+* - send(sock, ..., MSG_NOSIGNAL)
+* - setting the signal mask to
SIG_IGN during send()
+*
+* The first two reduce the number of
syscalls (for the
+* third, we require three syscalls to
implement a send()),
+* so use them if they're available.
Their availability is
+* flagged in the following members of
PGconn:
+*
+* conn-sigpipe_so - we
have set up SO_NOSIGPIPE
+* conn-sigpipe_flag - we're
specifying MSG_NOSIGNAL
+*
+* If we can use SO_NOSIGPIPE, then set
sigpipe_so here and
+* we don't need to care about anything
else. Otherwise,
+* try MSG_NOSIGNAL by setting
sigpipe_flag. If we get an
+* error with MSG_NOSIGNAL, we clear
the flag and revert
+* to manual masking.
+*/
+ conn-sigpipe_so = false;
+ #ifdef MSG_NOSIGNAL
+ conn-sigpipe_flag = true;
+ #else /* !MSG_NOSIGNAL */
+ conn-sigpipe_flag = false;
+ #endif /* MSG_NOSIGNAL */
+
+ #ifdef SO_NOSIGPIPE
+ optval = 1;
+ if (!setsockopt(conn-sock, SOL_SOCKET,
SO_NOSIGPIPE,
+ (char *)optval,
sizeof(optval)))
+ {
+ conn-sigpipe_so = true;
+ conn-sigpipe_flag = false;
+ }
+ #endif /* SO_NOSIGPIPE */
+
+
/*
* Start/make connection. This should
not block, since we
* are in nonblock mode. If it does,
well, too bad.
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***
*** 118,161 static long win32_ssl_create_mutex = 0;
/*
* Macros to handle disabling and then restoring the state of SIGPIPE
handling.
- * Note that DISABLE_SIGPIPE() must appear at the start of a block.
*/
#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY
! #define DISABLE_SIGPIPE(failaction) \
! sigset_tosigmask; \
! boolsigpipe_pending; \
! boolgot_epipe = false
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Hi Tom,
That code is not broken as it stands, and doesn't appear to really
gain anything from the proposed change. Why should we risk any
portability questions when the code isn't going to get either simpler
or shorter?
OK, after attempting a macro version of this, we end up with:
#define DECLARE_SIGPIPE_INFO(var) struct sigpipe_info var;
#define DISABLE_SIGPIPE(info, conn) \
((SIGPIPE_MASKED(conn)) ? 0 : \
((info)-got_epipe = false, \
pg_block_sigpipe((info)-oldsigmask,
(info)-sigpipe_pending)) 0)
- kinda ugly, uses the ?: and , operators to return the result of
pg_block_sigpipe. We could avoid the comma with a block though.
If we want to keep the current 'failaction' style of macro:
#define DISABLE_SIGPIPE(info, conn, failaction) \
do { \
if (!SIGPIPE_MASKED(conn)) { \
(info)-got_epipe = false; \
if (pg_block_sigpipe((info)-oldsigmask, \
(info)-sigpipe_pending)) 0) { \
failaction; \
} \
} \
} while (0)
We could ditch struct sigpipe info, but that means we need to declare
three separate vars in DECLARE_SIGPIPE_INFO() instead of the one, and it
doesn't clean up the macro code much. I'd rather reduce the amount of
stuff that we declare behind the caller's back.
Compared to the static-function version:
static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info
*info)
{
if (sigpipe_masked(conn))
return 0;
info-got_epipe = false;
return pq_block_sigpipe(info-oldsigmask, info-sigpipe_pending) 0;
}
Personally, I think the static functions are a lot cleaner, and don't
think we lose any portability from using these (since inline is #define-
ed out on compilers that don't support it). On non-inlining compilers,
we do gain an extra function call, but we're about to enter the kernel
anyway, so this will probably be lost in the noise (especially if we
save the sigpipe-masking syscalls).
But in the end, it's not up to me - do you still prefer the macro
approach?
Cheers,
Jeremy
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v4] Avoid manual shift-and-test logic in AllocSetFreeIndex
Hi Greg, Thanks for the benchmark app, thought I'd pitch in with some ppc results: clz 1.530s lookup table 1.720s float hack 4.424s unrolled 5.280s normal 5.369s POWER5+: clz 2.046s lookup table 2.188s float hack 7.786s unrolled 6.353s normal 7.033s POWER6: clz 1.063s lookup table 1.199s float hack 3.575s unrolled 2.290s normal 3.456s Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Hi Robert, Perhaps we should use macros. I was trying to avoid macros, as this means we lose type- and syntax- checking at the call-site, and end up with slightly messier code. However, I understand that this is probably personal preference for me :) How about just 'static' functions? (ie, drop the 'inline'). This way, the compiler is free to inline where suitable, and non-inlining compilers will do the right thing too. However, I'd rather make decisions on data, rather than guessing. Is the actual problem here that some compilers just don't support the 'inline' keyword? Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v3] Avoid manual shift-and-test logic in AllocSetFreeIndex
Hi Robert, That having been said, Jeremy, you probably want to take a look at those comments and I have a few responses to them as well. OK, thanks for the heads-up. following comment: Applied and built cleanly. Regress passes. Trying to hunt down ppc box to see if performance enhancement can be seen. Question: are we only doing this because of PowerPC? No, this patch will benefit any architecture that has a gcc implementation of __builtin_clz. I know that both x86 and powerpc gcc support this. What is going to happen on x86 and other architectures? x86 is not the center of the universe, but at a very minimum we need to make sure that things don't go backwards on what is a very common platform. Has anyone done any benchmarking of this? Yes, Atsushi Ogawa did some benchmarking with this patch on x86, his numbers are here: http://archives.postgresql.org/message-id/4a2895c4.9050...@hi-ho.ne.jp In fact, Atushi originally submitted a patch using inline asm (using bsr) to do this on x86. Coincidentally, I was working on a powerpc implementation (using cntlz) at the same time, so submitted a patch using the gcc builtin that would work on both (and possibly other) architectures. A related question: the original email for this patch says that it results in a performance increase of about 2% on PPC. But since it gives no details on exactly what the test was that improved by 2%, it's hard to judge the impact of this. If this means that AllocSetFreeIndex() is 2% faster, I think we should reject this patch and move on. It's not worth introducing architecture-specific code to get a performance improvement that probably won't even move the needle on a macro-benchmark. On the other hand, if the test was something like a pgbench run, then this is really very impressive. But we don't know which it is. The 2% improvement I saw is for a sysbench OLTP run. I'm happy to re-do the run and report specific numbers if you need them. Zdenek Kotala added this comment: I have several objections: - inline is forbidden to use in PostgreSQL - you need exception or do it differently - type mismatch - Size vs unsigned int vs 32. you should use only Size and sizeof(Size) OK, happy to make these changes. What's the commitfest process here, should I redo the patch and re-send to -hackers? (inline again: should I just make this a static, the compiler can inline where possible? or do you want a macro?) And general comment: Do we want to have this kind of code which is optimized for one compiler and platform. One compiler, multiple platforms. See openSSL or MySQL, they have many optimizations which work fine on one platform with specific version of compiler and specific version of OS. But if you select different version of compiler or different compiler you can get very different performance result (e.g. MySQL 30% degradation, OpenSSL RSA 3x faster and so on). I don't think we're going to see a lot of variation here (besides the difference where __builtin_clz isn't available). Given that this is implemented with a couple of instructions, I'm confident that we won't see any degradation for platforms that support __builtin_clz. For the other cases, we just use the exiting while-loop algorithm so there should be no change (unless we mess up the inlining...). I think in this case, it makes sense to have optimization here, but we should do it like spinlocks are implemented and put this code into /port. Unless I'm missing something, spinlocks are done the same way - we have one file, src/include/storage/s_lock.h, which is mostly different implementations of the lock primitives for different platforms, separated by preprocessor tests. Essentially, this is just one line of code, protected by HAVE_BUILTIN_CLZ (which is a feature-test, rather than a specific platform or compiler test), and is only used in one place. I don't think that warrants a separate file. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Tom, However, I think the whole patch is pretty useless. That code is not broken as it stands, and doesn't appear to really gain anything from the proposed change. Why should we risk any portability questions when the code isn't going to get either simpler or shorter? This patch clears the way for the proceeding change (2/2). We use the new inline functions to implement the proper checks to see if the sigpipe-masking syscalls are needed. We also need disable_sigpipe to be called when it's not the start of a block, hence the separate type definition. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH v4] Avoid manual shift-and-test logic in AllocSetFreeIndex
Rather than testing single bits in a loop, change AllocSetFreeIndex to
use the __builtin_clz() function to calculate the chunk index.
This requires a new check for __builtin_clz in the configure script.
Results in a ~2% performance increase on sysbench on PowerPC.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
v4: don't use separate fls() function, remove hardcoded 32
---
configure.in | 13 +
src/backend/utils/mmgr/aset.c | 14 +-
2 files changed, 26 insertions(+), 1 deletion(-)
*** a/configure.in
--- b/configure.in
***
*** 1361,1366 case $host_os in
--- 1361,1379
AC_FUNC_FSEEKO;;
esac
+ # GCC builtins
+ #
+ # We need AC_TRY_LINK here, as the prototype generated by AC_CHECK_FUNC
+ # will cause gcc to try to reference a non-builtin symbol.
+
+ AC_MSG_CHECKING([for __builtin_clz])
+ AC_TRY_LINK([],
+ [__builtin_clz(0);],
+ [AC_DEFINE(HAVE_BUILTIN_CLZ, 1,
+ [Define to 1 if you have __builtin_clz().])
+ AC_MSG_RESULT(yes)],
+ [AC_MSG_RESULT(no)])
+
#
# Pthreads
*** a/src/backend/utils/mmgr/aset.c
--- b/src/backend/utils/mmgr/aset.c
***
*** 268,281 AllocSetFreeIndex(Size size)
{
int idx = 0;
! if (size 0)
{
size = (size - 1) ALLOC_MINBITS;
while (size != 0)
{
idx++;
size = 1;
}
Assert(idx ALLOCSET_NUM_FREELISTS);
}
--- 268,293
{
int idx = 0;
! if (size (1 ALLOC_MINBITS))
{
size = (size - 1) ALLOC_MINBITS;
+
+ #ifdef HAVE_BUILTIN_CLZ
+ /* If possible, use __builtin_clz to calculate the chunk index
- this
+* should be O(1) rather than O(n). The builtin takes an
unsigned int,
+* so we need to cast from the possibly 64-bit Size type. This
cast
+* won't overflow, as the limit is at 2^(32 + ALLOC_MINBITS)
bytes,
+* which is larger than ALLOC_CHUNK_LIMIT.
+*/
+ idx = sizeof(unsigned int) * BITS_PER_BYTE -
+ __builtin_clz((unsigned int)size);
+ #else
while (size != 0)
{
idx++;
size = 1;
}
+ #endif
Assert(idx ALLOCSET_NUM_FREELISTS);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Hi, Alvaro, Does this work in compilers other than GCC? I think we use some kludges to protect against them ... see pg_list.h for the canonical example. As I understand it, we're not using static inlines in pg_list.h to prevent multiple objects from exporting the same symbols if the functions don't end up as 'static inline' (ie, because the compiler doesn't support that). In this case, we're only compiling the inlines into a single object, so even if the compiler doesn't support inlines, we'll just end up with out-of-line function calls, which should work too. However, this is only my assumption about those compilers (I don't have access to other compilers to test); happy to fix these up if the inlines won't work. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 0/2 v3] SIGPIPE masking in local socket connections
A new approach to avioding manipulating the signal mask during for every
send - this time round, use SO_NOSIGPIPE and MSG_NOSIGNAL if available.
The patches have been tested on Linux and OSX, and I've confirmed that
'struct foo { };' will compile with a MSVC compiler. I'd still like a
little more testing though, is there a machine that allows both
SO_NOSIGPIPE and MSG_NOSIGNAL?
v3: respin as context diffs
Again, comments most welcome,
Jeremy
---
Jeremy Kerr (2):
[libpq] rework sigpipe-handling macros
[libpq] Try to avoid manually masking SIGPIPEs on every send()
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 2/2 v3] [libpq] Try to avoid manually masking SIGPIPEs on every send()
Currently, libpq will wrap each send() call on the connection with
two system calls to mask SIGPIPEs. This results in 3 syscalls instead
of one, and (on Linux) can lead to high contention on the signal
mask locks in threaded apps.
We have a couple of other methods to avoid SIGPIPEs:
sockopt(SO_NOSIGPIPE) and the MSG_NOSIGNAL flag to send().
This change attempts to use these if they're available at compile-
and run-time. If not, we drop back to manipulating the signal mask as
before.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/interfaces/libpq/fe-connect.c | 40 ++
src/interfaces/libpq/fe-secure.c | 83 +-
src/interfaces/libpq/libpq-int.h |2
3 files changed, 107 insertions(+), 18 deletions(-)
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***
*** 1085,1090 keep_going: /* We
will come back to here until there is
--- 1085,1091
while (conn-addr_cur != NULL)
{
struct addrinfo *addr_cur =
conn-addr_cur;
+ int optval;
/* Remember current address for
possible error msg */
memcpy(conn-raddr.addr,
addr_cur-ai_addr,
***
*** 1149,1154 keep_going: /* We
will come back to here until there is
--- 1150,1194
}
#endif /* F_SETFD */
+ /* We have three methods of blocking
sigpipe during
+* send() calls to this socket:
+*
+* - setsockopt(sock, SO_NOSIGPIPE)
+* - send(sock, ..., MSG_NOSIGNAL)
+* - setting the signal mask to
SIG_IGN during send()
+*
+* The first two reduce the number of
syscalls (for the
+* third, we require three syscalls to
implement a send()),
+* so use them if they're available.
Their availability is
+* flagged in the following members of
PGconn:
+*
+* conn-sigpipe_so - we
have set up SO_NOSIGPIPE
+* conn-sigpipe_flag - we're
specifying MSG_NOSIGNAL
+*
+* If we can use SO_NOSIGPIPE, then set
sigpipe_so here and
+* we don't need to care about anything
else. Otherwise,
+* try MSG_NOSIGNAL by setting
sigpipe_flag. If we get an
+* error with MSG_NOSIGNAL, we clear
the flag and revert
+* to manual masking.
+*/
+ conn-sigpipe_so = false;
+ #ifdef MSG_NOSIGNAL
+ conn-sigpipe_flag = true;
+ #else /* !MSG_NOSIGNAL */
+ conn-sigpipe_flag = false;
+ #endif /* MSG_NOSIGNAL */
+
+ #ifdef SO_NOSIGPIPE
+ optval = 1;
+ if (!setsockopt(conn-sock, SOL_SOCKET,
SO_NOSIGPIPE,
+ (char *)optval,
sizeof(optval)))
+ {
+ conn-sigpipe_so = true;
+ conn-sigpipe_flag = false;
+ }
+ #endif /* SO_NOSIGPIPE */
+
+
/*
* Start/make connection. This should
not block, since we
* are in nonblock mode. If it does,
well, too bad.
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***
*** 122,127 static long win32_ssl_create_mutex = 0;
--- 122,139
*/
#ifndef WIN32
+
+ static inline int sigpipe_masked(PGconn *conn)
+ {
+ /* If we're on an SSL connection, we can only use SO_NOSIGPIPE masking.
+* Otherwise, we can handle SO_NOSIGPIPE or the MSG_NOSIGNAL flag */
+ #ifdef USE_SSL
+ if (conn-ssl)
+ return conn-sigpipe_so;
+ #endif
+ return conn-sigpipe_so || conn-sigpipe_flag;
+ }
+
#ifdef ENABLE_THREAD_SAFETY
struct sigpipe_info {
***
*** 130,137
[HACKERS] [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros
Currently, the sigpipe-masking code in libpq is implemented as
a set of macros, which depend on declaring local variables.
This change adds a (private) struct sigpipe_info to contain the
compile-dependent data required for sigpipe masking and restoring.
The caller can then declare a struct sigpipe info explicitly, and
pass this to the subsequent sigpipe-masking code.
This allows us to separate the variable declarations from the code,
and gives the caller more flexibility for controlling the scope of
these variables.
Also, since we don't need to declare variables in the macros, we
can change the code to be implemented as static inlines.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/interfaces/libpq/fe-secure.c | 88 ---
1 file changed, 55 insertions(+), 33 deletions(-)
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***
*** 119,163 static long win32_ssl_create_mutex = 0;
/*
* Macros to handle disabling and then restoring the state of SIGPIPE
handling.
- * Note that DISABLE_SIGPIPE() must appear at the start of a block.
*/
#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY
! #define DISABLE_SIGPIPE(failaction) \
! sigset_tosigmask; \
! boolsigpipe_pending; \
! boolgot_epipe = false; \
! \
! if (pq_block_sigpipe(osigmask, sigpipe_pending) 0) \
! failaction
! #define REMEMBER_EPIPE(cond) \
! do { \
! if (cond) \
! got_epipe = true; \
! } while (0)
! #define RESTORE_SIGPIPE() \
! pq_reset_sigpipe(osigmask, sigpipe_pending, got_epipe)
#else /* !ENABLE_THREAD_SAFETY */
! #define DISABLE_SIGPIPE(failaction) \
! pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN)
! #define REMEMBER_EPIPE(cond)
! #define RESTORE_SIGPIPE() \
! pqsignal(SIGPIPE, oldsighandler)
#endif/* ENABLE_THREAD_SAFETY */
#else /* WIN32 */
! #define DISABLE_SIGPIPE(failaction)
! #define REMEMBER_EPIPE(cond)
! #define RESTORE_SIGPIPE()
#endif/* WIN32 */
--- 119,180
/*
* Macros to handle disabling and then restoring the state of SIGPIPE
handling.
*/
#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY
! struct sigpipe_info {
! sigset_toldsigmask;
! boolsigpipe_pending;
! boolgot_epipe;
! };
! static inline int disable_sigpipe(struct sigpipe_info *info)
! {
! info-got_epipe = false;
! return pq_block_sigpipe(info-oldsigmask, info-sigpipe_pending) 0;
! }
! static inline void remember_epipe(struct sigpipe_info *info, bool cond)
! {
! if (cond)
! info-got_epipe = true;
! }
!
! static inline void restore_sigpipe(struct sigpipe_info *info)
! {
! pq_reset_sigpipe(info-oldsigmask, info-sigpipe_pending,
info-got_epipe);
! }
#else /* !ENABLE_THREAD_SAFETY */
! struct sigpipe_info {
! pqsigfunc oldhandler;
! };
! static inline int disable_sigpipe(struct sigpipe_info *info)
! {
! info-oldhandler = pqsignal(SIGPIPE, SIG_IGN);
! return 0;
! }
!
! static inline void remember_epipe(struct sigpipe_info *info, bool cond)
! {
! }
! static inline void restore_sigpipe(struct sigpipe_info *info)
! {
! pqsignal(SIGPIPE, info-oldhandler);
! }
#endif/* ENABLE_THREAD_SAFETY */
#else /* WIN32 */
! struct sigpipe_info { };
! static inline int disable_sigpipe(struct sigpipe_info *info) { return 0; }
! static inline void remember_epipe(struct sigpipe_info *info, bool cond) { }
! static inline void restore_sigpipe(struct sigpipe_info *info) { }
#endif/* WIN32 */
***
*** 286,294 pqsecure_read(PGconn *conn, void *ptr, size_t len)
if (conn-ssl)
{
int err;
/* SSL_read can write to the socket, so we need to disable
SIGPIPE */
! DISABLE_SIGPIPE(return -1);
rloop:
n = SSL_read(conn-ssl, ptr, len);
--- 303,313
if (conn-ssl)
{
int err;
+ struct sigpipe_info info;
/* SSL_read can write to the socket, so we need to disable
SIGPIPE */
! if (disable_sigpipe(info))
! return -1;
rloop:
n = SSL_read(conn-ssl, ptr, len);
***
*** 315,321 rloop:
if (n == -1)
{
! REMEMBER_EPIPE(SOCK_ERRNO ==
EPIPE);
printfPQExpBuffer(conn-errorMessage,
libpq_gettext(SSL SYSCALL error: %s\n),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf
[HACKERS] [PATCH v3] Avoid manual shift-and-test logic in AllocSetFreeIndex
Move the shift-and-test login into a separate fls() function, which
can use __builtin_clz() if it's available.
This requires a new check for __builtin_clz in the configure script.
Results in a ~2% performance increase on PowerPC.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
v3: respin as context diff
---
configure.in | 13 +
src/backend/utils/mmgr/aset.c | 34 +++---
2 files changed, 40 insertions(+), 7 deletions(-)
*** a/configure.in
--- b/configure.in
***
*** 1361,1366 case $host_os in
--- 1361,1379
AC_FUNC_FSEEKO;;
esac
+ # GCC builtins
+ #
+ # We need AC_TRY_LINK here, as the prototype generated by AC_CHECK_FUNC
+ # will cause gcc to try to reference a non-builtin symbol.
+
+ AC_MSG_CHECKING([for __builtin_clz])
+ AC_TRY_LINK([],
+ [__builtin_clz(0);],
+ [AC_DEFINE(HAVE_BUILTIN_CLZ, 1,
+ [Define to 1 if you have __builtin_clz().])
+ AC_MSG_RESULT(yes)],
+ [AC_MSG_RESULT(no)])
+
#
# Pthreads
*** a/src/backend/utils/mmgr/aset.c
--- b/src/backend/utils/mmgr/aset.c
***
*** 255,260 static MemoryContextMethods AllocSetMethods = {
--- 255,285
#define AllocAllocInfo(_cxt, _chunk)
#endif
+ /*
+ * fls: find last set bit.
+ *
+ * Returns the 1-based index of the most-significant bit in x. The MSB
+ * is bit number 32, the LSB is bit number 1. If x is zero, the result is
+ * undefined.
+ */
+ static inline int
+ fls(unsigned int x)
+ {
+ #ifdef HAVE_BUILTIN_CLZ
+ return 32 - __builtin_clz(x);
+ #else
+ int ls = 0;
+
+ while (x != 0)
+ {
+ ls++;
+ x = 1;
+ }
+
+ return ls;
+ #endif
+ }
+
/* --
* AllocSetFreeIndex -
*
***
*** 268,281 AllocSetFreeIndex(Size size)
{
int idx = 0;
! if (size 0)
{
! size = (size - 1) ALLOC_MINBITS;
! while (size != 0)
! {
! idx++;
! size = 1;
! }
Assert(idx ALLOCSET_NUM_FREELISTS);
}
--- 293,301
{
int idx = 0;
! if (size (1 ALLOC_MINBITS))
{
! idx = fls((size - 1) ALLOC_MINBITS);
Assert(idx ALLOCSET_NUM_FREELISTS);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Stephen, Is it just the size that matters, or is it when there are few spaces at the end? It's the number of spaces at the end. If we knew this number, then we wouldn't have to do any comparisons at all :) Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Stephen, If the updated function is always faster when the overall string is at least, say, 16 characters long, But that's not the case - the cost of the function (and the speedup from the previous version) depends on the number of spaces that there are at the end. For the new function to be faster, we need to know that there are more than 6 (on average, depending on alignment) trailing spaces. The number of non-space characters in the string won't affect performance (other than that there is probably a correlation between total string length and number of spaces, but we can't be certain of that). If the new function is always slower, regardless of overall string length, when there's only 1 extra space at the end Yes, that is correct. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Hi Stephen,
What would be really useful would be best case and worst case
scenarios.
I've put together some data from a microbenchmark of the bcTrulen
function, patched and unpatched.
As for best-case, if you have a long string of trailing spaces, we can
go through them at theoretically one quarter of cost (a test benchmark
on x86 shows an actual reduction of 11 to 3 sec with a string of 100
spaces).
Worst-case behaviour is with smaller numbers of spaces. Here are the
transition points (ie, where doing the word-wise comparison is faster
than byte-wise) that I see from my benchmark:
align spaces
3 7
2 6
1 5
0 4
- where 'align' is the alignment of the first byte to compare (ie, at
the end of the string). This is pretty much as-expected, as these
transition points are the first opportunity that the new function has to
do a word compare.
In the worst cases, I see a 53% cost increase on x86 (with the string
'aaa ') and a 97% increase on PowerPC ('a ').
So, it all depends on the number of padding spaces we'd expect to see on
workload data. Fortunately, we see the larger reductions on the more
expensive operations (ie, longer strings).
Cheers,
Jeremy
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Tom, I've put together some data from a microbenchmark of the bcTrulen function, patched and unpatched. Uh, where's the data? If you're after the raw data for a run, I've put it up: http://ozlabs.org/~jk/projects/db/data/bctruelen.csv I've also packaged up the quick-and-dirty benchmark I've been using: http://ozlabs.org/~jk/projects/db/bctrulen.tar.gz to recreate as-needed. ./benchmark.sh will run tests for varying-length strings (so changing the alignment) and varying numbers of trailing spaces. ./benchmark-bctruelen str will perform an individual old/new comparison. Unfortunately, the cases with lots of padding spaces are probably much less probable than the cases with fewer. It would be unpleasant for example if this patch resulted in a severe performance degradation for a canonical example of char(n) being used properly, such as char(2) for US state abbreviations. Yep, makes sense. The other consideration is stock-ticker symbols, I assume they may also be stored in CHAR(small n) columns. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
The current bcTruelen function uses a simple reverse array scan to
find the legth of a space-padded string. On some workloads (it shows
in sysbench), this can result in a lot of time spent in this function.
This change introduces a word-at-a-time comparison in bcTruelen, aiming
to reduce the number of compares where possible. Word-size compares
are performed on aligned sections of the string.
Results in a small performance increase; around 1-2% on my POWER6 test
box.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
Resend: context diff this time
---
src/backend/utils/adt/varchar.c | 24 +---
1 file changed, 21 insertions(+), 3 deletions(-)
*** a/src/backend/utils/adt/varchar.c
--- b/src/backend/utils/adt/varchar.c
***
*** 624,639 varchartypmodout(PG_FUNCTION_ARGS)
static int
bcTruelen(BpChar *arg)
{
char *s = VARDATA_ANY(arg);
int i;
- int len;
! len = VARSIZE_ANY_EXHDR(arg);
! for (i = len - 1; i = 0; i--)
{
if (s[i] != ' ')
break;
}
return i + 1;
}
--- 624,657
static int
bcTruelen(BpChar *arg)
{
+ const uint32_t spaces = 0x20202020;
+ const int wordsize = sizeof(spaces);
char *s = VARDATA_ANY(arg);
int i;
! i = VARSIZE_ANY_EXHDR(arg) - 1;
!
! /* compare down to an aligned boundary */
! for (; i 0 !PointerIsAligned(s + i - (wordsize - 1), uint32_t);
i--)
{
if (s[i] != ' ')
+ return i + 1;
+ }
+
+ /* now that we're aligned, compare word at a time */
+ for (; i = wordsize - 1; i -= wordsize)
+ {
+ if (*(uint32_t *)(s + i - (wordsize - 1)) != spaces)
break;
}
+
+ /* check within the last non-matching word */
+ for (; i = 0; i--)
+ {
+ if (s[i] != ' ')
+ break;
+ }
+
return i + 1;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Robert, I'd still like to know the workload and exact numbers. From up-thread: http://ozlabs.org/~jk/projects/db/data/postgres.bcTruelen/ - or were there other details you were looking for? Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Hi all, Speaking of which, what about some performance numbers? OK, benchmarks done: http://ozlabs.org/~jk/projects/db/data/postgres.bcTruelen/ Summary: small increase in performance (~1-2% on my machine), at about 1.5 standard deviations from the mean. Profiles show a decent drop in hits within bcTruelen. However: Sysbench seems to be quite heavy with the fixed-width char types, so may end up calling bcTruelen more than most workloads. Would be nice to get some x86 numbers too, but I don't have a suitable machine here. So: The increase in performance is positive on this workload, albeit fairly minor. Downside is increased code complexity. Will re-send the patch once I work out how to get git to create a context diff... Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Hi all,
That's correct. To check the alignment you would have to look at the
actual pointer. I would suggest using the existing macros to handle
alignment. Hm, though the only one I see offhand which is relevant is
the moderately silly PointerIsAligned(). Still it would make the code
clearer even if it's pretty simple.
Yes, the code (incorrectly) assumes that any multiple-of-4 index into
the char array is aligned, and so the array itself must be aligned for
this to work.
I'll rework the patch, testing the pointer alignment directly instead.
Incidentally, the char foo[4] = {' ',' ',' ',' '} suggestion is, I
think, bogus. There would be no alignment guarantee on that array.
Personally I'm find with 0x20202020 with a comment explaining what it
is.
The variable is called 'spaces', but I can add extra comments if
preferred.
Cheers,
Jeremy
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Hi Tom, Speaking of which, what about some performance numbers? Like Heikki, I'm quite suspicious of whether there is any real-world gain to be had from this approach. Will send numbers tomorrow, with the reworked patch. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] char() overhead on read-only workloads not so insignifcant as the docs claim it is...
Alvaro, Maybe bcTruelen could be optimized to step on one word at a time (perhaps by using XOR against a precomputed word filled with ' '), instead of one byte at a time ... I have a patch for this, will send soon. Regards, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/backend/utils/adt/varchar.c | 24 +---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/varchar.c b/src/backend/utils/adt/varchar.c
index 5f3c658..6889dff 100644
--- a/src/backend/utils/adt/varchar.c
+++ b/src/backend/utils/adt/varchar.c
@@ -624,16 +624,34 @@ varchartypmodout(PG_FUNCTION_ARGS)
static int
bcTruelen(BpChar *arg)
{
+ const unsigned int spaces = 0x20202020;
+ const int wordsize = sizeof(spaces);
char *s = VARDATA_ANY(arg);
int i;
- int len;
- len = VARSIZE_ANY_EXHDR(arg);
- for (i = len - 1; i = 0; i--)
+ i = VARSIZE_ANY_EXHDR(arg) - 1;
+
+ /* compare down to an aligned boundary */
+ for (; i = 0 i % wordsize != wordsize - 1; i--)
{
if (s[i] != ' ')
+ return i + 1;
+ }
+
+ /* now that we're aligned, compare word at a time */
+ for (; i = wordsize - 1; i -= wordsize)
+ {
+ if (*(unsigned int *)(s + i - (wordsize - 1)) != spaces)
break;
}
+
+ /* check within the last non-matching word */
+ for (; i = 0; i--)
+ {
+ if (s[i] != ' ')
+ break;
+ }
+
return i + 1;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] backend: compare word-at-a-time in bcTruelen
Robert, This looks very non-portable to me. Unsurprisingly, I'm new to postgres hacking and the large number of supported platforms :) I was considering something like: unsigned int spaces; const unsigned int wordsize = sizeof(unsigned int); memset(spaces, ' ', wordsize); In most cases, the compiler should be able to optimise the memset out, but it may introduce overhead where this is not possible. However, are there any supported platforms where sizeof(unsigned int) != 4 ? Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 0/2] SIGPIPE masking in local socket connections, v2
A new approach to avioding manipulating the signal mask during for every
send - this time round, use SO_NOSIGPIPE and MSG_NOSIGNAL if available.
The patches have been tested on Linux and OSX, and I've confirmed that
'struct foo { };' will compile with a MSVC compiler. I'd still like a
little more testing though, is there a machine that allows both
SO_NOSIGPIPE and MSG_NOSIGNAL?
Again, comments most welcome,
Jeremy
---
Jeremy Kerr (2):
[libpq] rework sigpipe-handling macros
[libpq] Try to avoid manually masking SIGPIPEs on every send()
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 1/2] [libpq] rework sigpipe-handling macros
Currently, the sigpipe-masking code in libpq is implemented as
a set of macros, which depend on declaring local variables.
This change adds a (private) struct sigpipe_info to contain the
compile-dependent data required for sigpipe masking and restoring.
The caller can then declare a struct sigpipe info explicitly, and
pass this to the subsequent sigpipe-masking code.
This allows us to separate the variable declarations from the code,
and gives the caller more flexibility for controlling the scope of
these variables.
Also, since we don't need to declare variables in the macros, we
can change the code to be implemented as static inlines.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/interfaces/libpq/fe-secure.c | 88 ---
1 file changed, 55 insertions(+), 33 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index ee0a91e..13c97ac 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -119,45 +119,62 @@ static long win32_ssl_create_mutex = 0;
/*
* Macros to handle disabling and then restoring the state of SIGPIPE handling.
- * Note that DISABLE_SIGPIPE() must appear at the start of a block.
*/
#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY
-#define DISABLE_SIGPIPE(failaction) \
- sigset_tosigmask; \
- boolsigpipe_pending; \
- boolgot_epipe = false; \
-\
- if (pq_block_sigpipe(osigmask, sigpipe_pending) 0) \
- failaction
+struct sigpipe_info {
+ sigset_toldsigmask;
+ boolsigpipe_pending;
+ boolgot_epipe;
+};
-#define REMEMBER_EPIPE(cond) \
- do { \
- if (cond) \
- got_epipe = true; \
- } while (0)
+static inline int disable_sigpipe(struct sigpipe_info *info)
+{
+ info-got_epipe = false;
+ return pq_block_sigpipe(info-oldsigmask, info-sigpipe_pending) 0;
+}
-#define RESTORE_SIGPIPE() \
- pq_reset_sigpipe(osigmask, sigpipe_pending, got_epipe)
+static inline void remember_epipe(struct sigpipe_info *info, bool cond)
+{
+ if (cond)
+ info-got_epipe = true;
+}
+
+static inline void restore_sigpipe(struct sigpipe_info *info)
+{
+ pq_reset_sigpipe(info-oldsigmask, info-sigpipe_pending,
info-got_epipe);
+}
#else /* !ENABLE_THREAD_SAFETY */
-#define DISABLE_SIGPIPE(failaction) \
- pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN)
+struct sigpipe_info {
+ pqsigfunc oldhandler;
+};
-#define REMEMBER_EPIPE(cond)
+static inline int disable_sigpipe(struct sigpipe_info *info)
+{
+ info-oldhandler = pqsignal(SIGPIPE, SIG_IGN);
+ return 0;
+}
+
+static inline void remember_epipe(struct sigpipe_info *info, bool cond)
+{
+}
-#define RESTORE_SIGPIPE() \
- pqsignal(SIGPIPE, oldsighandler)
+static inline void restore_sigpipe(struct sigpipe_info *info)
+{
+ pqsignal(SIGPIPE, info-oldhandler);
+}
#endif /* ENABLE_THREAD_SAFETY */
#else /* WIN32 */
-#define DISABLE_SIGPIPE(failaction)
-#define REMEMBER_EPIPE(cond)
-#define RESTORE_SIGPIPE()
+struct sigpipe_info { };
+static inline int disable_sigpipe(struct sigpipe_info *info) { return 0; }
+static inline void remember_epipe(struct sigpipe_info *info, bool cond) { }
+static inline void restore_sigpipe(struct sigpipe_info *info) { }
#endif /* WIN32 */
@@ -286,9 +303,11 @@ pqsecure_read(PGconn *conn, void *ptr, size_t len)
if (conn-ssl)
{
int err;
+ struct sigpipe_info info;
/* SSL_read can write to the socket, so we need to disable
SIGPIPE */
- DISABLE_SIGPIPE(return -1);
+ if (disable_sigpipe(info))
+ return -1;
rloop:
n = SSL_read(conn-ssl, ptr, len);
@@ -315,7 +334,7 @@ rloop:
if (n == -1)
{
- REMEMBER_EPIPE(SOCK_ERRNO ==
EPIPE);
+ remember_epipe(info,
SOCK_ERRNO == EPIPE);
printfPQExpBuffer(conn-errorMessage,
libpq_gettext(SSL SYSCALL error: %s\n),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
@@ -351,7 +370,7 @@ rloop:
break;
}
- RESTORE_SIGPIPE();
+ restore_sigpipe(info);
}
else
#endif
@@ -367,8 +386,10 @@ ssize_t
pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{
ssize_t n;
+ struct sigpipe_info info;
- DISABLE_SIGPIPE(return -1);
+ if (disable_sigpipe(info))
+ return -1;
#ifdef USE_SSL
[HACKERS] [PATCH 2/2] [libpq] Try to avoid manually masking SIGPIPEs on every send()
Currently, libpq will wrap each send() call on the connection with
two system calls to mask SIGPIPEs. This results in 3 syscalls instead
of one, and (on Linux) can lead to high contention on the signal
mask locks in threaded apps.
We have a couple of other methods to avoid SIGPIPEs:
sockopt(SO_NOSIGPIPE) and the MSG_NOSIGNAL flag to send().
This change attempts to use these if they're available at compile-
and run-time. If not, we drop back to manipulating the signal mask as
before.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/interfaces/libpq/fe-connect.c | 39 +
src/interfaces/libpq/fe-secure.c | 83 +-
src/interfaces/libpq/libpq-int.h |2
3 files changed, 106 insertions(+), 18 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 7f4ae4c..8265268 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1085,6 +1085,7 @@ keep_going:
/* We will come back to here until there is
while (conn-addr_cur != NULL)
{
struct addrinfo *addr_cur =
conn-addr_cur;
+ int optval;
/* Remember current address for
possible error msg */
memcpy(conn-raddr.addr,
addr_cur-ai_addr,
@@ -1149,6 +1150,44 @@ keep_going:
/* We will come back to here until there is
}
#endif /* F_SETFD */
+ /* We have three methods of blocking
sigpipe during
+* send() calls to this socket:
+*
+* - setsockopt(sock, SO_NOSIGPIPE)
+* - send(sock, ..., MSG_NOSIGNAL)
+* - setting the signal mask to
SIG_IGN during send()
+*
+* The first two reduce the number of
syscalls (for the
+* third, we require three syscalls to
implement a send()),
+* so use them if they're available.
Their availability is
+* flagged in the following members of
PGconn:
+*
+* conn-sigpipe_so - we
have set up SO_NOSIGPIPE
+* conn-sigpipe_flag - we're
specifying MSG_NOSIGNAL
+*
+* If we can use SO_NOSIGPIPE, then set
sigpipe_so here and
+* we don't need to care about anything
else. Otherwise,
+* try MSG_NOSIGNAL by setting
sigpipe_flag. If we get an
+* error with MSG_NOSIGNAL, we clear
the flag and revert
+* to manual masking.
+*/
+ conn-sigpipe_so = false;
+#ifdef MSG_NOSIGNAL
+ conn-sigpipe_flag = true;
+#else /* !MSG_NOSIGNAL */
+ conn-sigpipe_flag = false;
+#endif /* MSG_NOSIGNAL */
+
+#ifdef SO_NOSIGPIPE
+ if (!setsockopt(conn-sock, SOL_SOCKET,
SO_NOSIGPIPE,
+ (char *)optval,
sizeof(optval)))
+ {
+ conn-sigpipe_so = true;
+ conn-sigpipe_flag = false;
+ }
+#endif /* SO_NOSIGPIPE */
+
+
/*
* Start/make connection. This should
not block, since we
* are in nonblock mode. If it does,
well, too bad.
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 13c97ac..949cd0f 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -122,6 +122,18 @@ static long win32_ssl_create_mutex = 0;
*/
#ifndef WIN32
+
+static inline int sigpipe_masked(PGconn *conn)
+{
+ /* If we're on an SSL connection, we can only use SO_NOSIGPIPE masking.
+* Otherwise, we can handle SO_NOSIGPIPE or the MSG_NOSIGNAL flag */
+#ifdef USE_SSL
+ if (conn-ssl)
+ return conn-sigpipe_so;
+#endif
+ return conn-sigpipe_so || conn-sigpipe_flag;
+}
+
#ifdef
Re: [HACKERS] [PATCH 2/2] [libpq] Try to avoid manually masking SIGPIPEs on every send()
Marko, optval seems used without initialization. Dang, I was checking for the SO_NOSIGPIPE flag, and didn't check for optval. New patch coming... Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH v2] [libpq] Try to avoid manually masking SIGPIPEs on every send()
Currently, libpq will wrap each send() call on the connection with
two system calls to mask SIGPIPEs. This results in 3 syscalls instead
of one, and (on Linux) can lead to high contention on the signal
mask locks in threaded apps.
We have a couple of other methods to avoid SIGPIPEs:
sockopt(SO_NOSIGPIPE) and the MSG_NOSIGNAL flag to send().
This change attempts to use these if they're available at compile-
and run-time. If not, we drop back to manipulating the signal mask as
before.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
v2: initialise optval
---
src/interfaces/libpq/fe-connect.c | 40 ++
src/interfaces/libpq/fe-secure.c | 83 +-
src/interfaces/libpq/libpq-int.h |2
3 files changed, 107 insertions(+), 18 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 7f4ae4c..92ab4e0 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1085,6 +1085,7 @@ keep_going:
/* We will come back to here until there is
while (conn-addr_cur != NULL)
{
struct addrinfo *addr_cur =
conn-addr_cur;
+ int optval;
/* Remember current address for
possible error msg */
memcpy(conn-raddr.addr,
addr_cur-ai_addr,
@@ -1149,6 +1150,45 @@ keep_going:
/* We will come back to here until there is
}
#endif /* F_SETFD */
+ /* We have three methods of blocking
sigpipe during
+* send() calls to this socket:
+*
+* - setsockopt(sock, SO_NOSIGPIPE)
+* - send(sock, ..., MSG_NOSIGNAL)
+* - setting the signal mask to
SIG_IGN during send()
+*
+* The first two reduce the number of
syscalls (for the
+* third, we require three syscalls to
implement a send()),
+* so use them if they're available.
Their availability is
+* flagged in the following members of
PGconn:
+*
+* conn-sigpipe_so - we
have set up SO_NOSIGPIPE
+* conn-sigpipe_flag - we're
specifying MSG_NOSIGNAL
+*
+* If we can use SO_NOSIGPIPE, then set
sigpipe_so here and
+* we don't need to care about anything
else. Otherwise,
+* try MSG_NOSIGNAL by setting
sigpipe_flag. If we get an
+* error with MSG_NOSIGNAL, we clear
the flag and revert
+* to manual masking.
+*/
+ conn-sigpipe_so = false;
+#ifdef MSG_NOSIGNAL
+ conn-sigpipe_flag = true;
+#else /* !MSG_NOSIGNAL */
+ conn-sigpipe_flag = false;
+#endif /* MSG_NOSIGNAL */
+
+#ifdef SO_NOSIGPIPE
+ optval = 1;
+ if (!setsockopt(conn-sock, SOL_SOCKET,
SO_NOSIGPIPE,
+ (char *)optval,
sizeof(optval)))
+ {
+ conn-sigpipe_so = true;
+ conn-sigpipe_flag = false;
+ }
+#endif /* SO_NOSIGPIPE */
+
+
/*
* Start/make connection. This should
not block, since we
* are in nonblock mode. If it does,
well, too bad.
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 13c97ac..949cd0f 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -122,6 +122,18 @@ static long win32_ssl_create_mutex = 0;
*/
#ifndef WIN32
+
+static inline int sigpipe_masked(PGconn *conn)
+{
+ /* If we're on an SSL connection, we can only use SO_NOSIGPIPE masking.
+* Otherwise, we can handle SO_NOSIGPIPE or the MSG_NOSIGNAL flag */
+#ifdef USE_SSL
+ if (conn-ssl)
+ return conn-sigpipe_so
Re: [HACKERS] [RFC,PATCH] Avoid manual shift-and-test logic in AllocSetFreeIndex
Hi Atsushi,
If size = 8, fls((size - 1) ALLOC_MINBITS) is fls(0).
The result of fls(0) is undefined.
Yep, got caught out by this because my previous fls() supported zero.
I think we have to never call fls(0) from AllocSetFreeIndex().
My proposal code:
if (size (1 ALLOC_MINBITS))
{
idx = fls((size - 1) ALLOC_MINBITS);
Assert(idx ALLOCSET_NUM_FREELISTS);
}
Looks good, I'll send an updated patch.
Also, are you still seeing the same improvement with the __builtin_clz
as your inline asm implementation?
Cheers,
Jeremy
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH v2] Avoid manual shift-and-test logic in AllocSetFreeIndex
Move the shift-and-test login into a separate fls() function, which
can use __builtin_clz() if it's available.
This requires a new check for __builtin_clz in the configure script.
Results in a ~2% performance increase on PowerPC.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
v2: prevent fls(0)
---
configure.in | 13 +
src/backend/utils/mmgr/aset.c | 34 +++---
2 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/configure.in b/configure.in
index b8d2685..6a317b0 100644
--- a/configure.in
+++ b/configure.in
@@ -1361,6 +1361,19 @@ case $host_os in
AC_FUNC_FSEEKO;;
esac
+# GCC builtins
+#
+# We need AC_TRY_LINK here, as the prototype generated by AC_CHECK_FUNC
+# will cause gcc to try to reference a non-builtin symbol.
+
+AC_MSG_CHECKING([for __builtin_clz])
+AC_TRY_LINK([],
+ [__builtin_clz(0);],
+ [AC_DEFINE(HAVE_BUILTIN_CLZ, 1,
+ [Define to 1 if you have __builtin_clz().])
+ AC_MSG_RESULT(yes)],
+ [AC_MSG_RESULT(no)])
+
#
# Pthreads
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0e2d4d5..9eb3117 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -255,6 +255,31 @@ static MemoryContextMethods AllocSetMethods = {
#define AllocAllocInfo(_cxt, _chunk)
#endif
+/*
+ * fls: find last set bit.
+ *
+ * Returns the 1-based index of the most-significant bit in x. The MSB
+ * is bit number 32, the LSB is bit number 1. If x is zero, the result is
+ * undefined.
+ */
+static inline int
+fls(unsigned int x)
+{
+#ifdef HAVE_BUILTIN_CLZ
+ return 32 - __builtin_clz(x);
+#else
+ int ls = 0;
+
+ while (x != 0)
+ {
+ ls++;
+ x = 1;
+ }
+
+ return ls;
+#endif
+}
+
/* --
* AllocSetFreeIndex -
*
@@ -268,14 +293,9 @@ AllocSetFreeIndex(Size size)
{
int idx = 0;
- if (size 0)
+ if (size (1 ALLOC_MINBITS))
{
- size = (size - 1) ALLOC_MINBITS;
- while (size != 0)
- {
- idx++;
- size = 1;
- }
+ idx = fls((size - 1) ALLOC_MINBITS);
Assert(idx ALLOCSET_NUM_FREELISTS);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC,PATCH] Avoid manual shift-and-test logic in AllocSetFreeIndex
Hi Atsushi, In my benchmark program, it is a little different performance in fls implementation and inline asm implementation. However, the result of a pgbench is almost the same improvement. Here is the result of my benchmark. Excellent, thank you for getting this extra set of numbers. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v2] Add bit operations util header
Florian, +#if defined(__GNUC__) \ + (defined(__ppc__) || defined(__powerpc__) || \ +defined(__ppc64__) || defined (__powerpc64__)) If you require GCC anyway, you can use __builtin_clz instead. (It's been available since GCC 4.1 at least.) Because now we have to test the compiler *and* the version as well? But I do agree that using the builtins makes for much better code; I'm looking at a future change that does this. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v2] Add bit operations util header
Hi Tom, The other thing I didn't like about the patch was the assumption that it's okay to have a static inline function in a header. You can get away with that in gcc but *not* in other compilers. Gee, you user-space guys have it tough! :D Point taken, will rework. Look at the existing coding patterns for, eg, list_head; then go thou and do likewise. Or, since there's currently no need for the code outside aset.c, forget about putting it in a header and just plop it into aset.c. OK, I'll add a configure check and conditionally use the builtin if it's available. I have some other patches that could be improved by using other builtins, so it would be a good opportunity to figure out a nice pattern for doing this. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [RFC,PATCH] Avoid manual shift-and-test logic in AllocSetFreeIndex
Move the shift-and-test login into a separate fls() function, which
can use __builtin_clz() if it's available.
This requires a new check for __builtin_clz in the configure script.
Results in a ~2% performance increase on PowerPC.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
configure.in | 13 +
src/backend/utils/mmgr/aset.c | 32 ++--
2 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/configure.in b/configure.in
index b8d2685..6a317b0 100644
--- a/configure.in
+++ b/configure.in
@@ -1361,6 +1361,19 @@ case $host_os in
AC_FUNC_FSEEKO;;
esac
+# GCC builtins
+#
+# We need AC_TRY_LINK here, as the prototype generated by AC_CHECK_FUNC
+# will cause gcc to try to reference a non-builtin symbol.
+
+AC_MSG_CHECKING([for __builtin_clz])
+AC_TRY_LINK([],
+ [__builtin_clz(0);],
+ [AC_DEFINE(HAVE_BUILTIN_CLZ, 1,
+ [Define to 1 if you have __builtin_clz().])
+ AC_MSG_RESULT(yes)],
+ [AC_MSG_RESULT(no)])
+
#
# Pthreads
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0e2d4d5..af352b8 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -255,6 +255,31 @@ static MemoryContextMethods AllocSetMethods = {
#define AllocAllocInfo(_cxt, _chunk)
#endif
+/*
+ * fls: find last set bit.
+ *
+ * Returns the 1-based index of the most-significant bit in x. The MSB
+ * is bit number 32, the LSB is bit number 1. If x is zero, the result is
+ * undefined.
+ */
+static inline int
+fls(unsigned int x)
+{
+#ifdef HAVE_BUILTIN_CLZ
+ return 32 - __builtin_clz(x);
+#else
+ int ls = 0;
+
+ while (x != 0)
+ {
+ ls++;
+ x = 1;
+ }
+
+ return ls;
+#endif
+}
+
/* --
* AllocSetFreeIndex -
*
@@ -270,12 +295,7 @@ AllocSetFreeIndex(Size size)
if (size 0)
{
- size = (size - 1) ALLOC_MINBITS;
- while (size != 0)
- {
- idx++;
- size = 1;
- }
+ idx = fls((size - 1) ALLOC_MINBITS);
Assert(idx ALLOCSET_NUM_FREELISTS);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC,PATCH] SIGPIPE masking in local socket connections
Tom, The consideration is that the application fails completely on server disconnect (because it gets SIGPIPE'd). This was long ago deemed unacceptable, and we aren't likely to change our opinion on that. OK, understood. I'm guessing MSG_NOSIGNAL on the send() isn't portable enough here? What disturbs me about your report is the suggestion that there are paths through that code that fail to protect against SIGPIPE. If so, we need to fix that. I just missed the comment that pqsecure_read may end up writing to the socket in the SSL case, so looks like all is fine here. We shouldn't see a SIGPIPE from the recv() alone. Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC,PATCH] SIGPIPE masking in local socket connections
Tom, A feature that is exercised via setsockopt is probably fairly safe, since you can check for failure of the setsockopt call and then do it the old way. MSG_NOSIGNAL is a recv() flag, no? It's a flag to send(). The question is whether you could expect that the recv() would fail if it had any unrecognized flags. Not sure if I trust that. SO_NOSIGPIPE seems safer. Yep, a once-off test would be better. However, I don't seem to have a NOSIGPIPE sockopt here :( Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] faster version of AllocSetFreeIndex for x86 architecture
Hi, I made a faster version of AllocSetFreeIndex for x86 architecture. Neat, I have a version for PowerPC too. In order to prevent writing multiple copies of AllocSetFreeIndex, I propose that we add a fls() function (find last set); this can be defined in an architecture-independent manner (ie, shift mask test in a loop), and re-defined for arches that have faster ways of doing the same (ie, cntlz instruction on powerpc). We can then change AllocSetFreeIndex to use fls(). Patches coming... Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 1/2] Add bit operations util header
Add a utility header for simple bit operatios - bitops.h.
At present, just contains the fls() (find last set bit) function.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/include/utils/bitops.h | 52 +
1 file changed, 52 insertions(+)
diff --git a/src/include/utils/bitops.h b/src/include/utils/bitops.h
new file mode 100644
index 000..de11624
--- /dev/null
+++ b/src/include/utils/bitops.h
@@ -0,0 +1,52 @@
+/*-
+ *
+ * bitops.h
+ * Simple bit operations.
+ *
+ * Portions Copyright (c) 2009, PostgreSQL Global Development Group
+ *
+ * $PostgreSQL$
+ *
+ *-
+ */
+#ifndef BITOPS_H
+#define BITOPS_H
+
+#if defined(__ppc__) || defined(__powerpc__) || \
+ defined(__ppc64__) || defined (__powerpc64__)
+
+static inline int
+fls(unsigned int x)
+{
+ int lz;
+ asm(cntlz %0,%1 : =r (lz) : r (x));
+ return 32 - lz;
+}
+
+#else /* !powerpc */
+
+/* Architecture-independent implementations */
+
+/*
+ * fls: find last set bit.
+ *
+ * Returns the 1-based index of the most-significant bit in x. The MSB
+ * is bit number 32, the LSB is bit number 1. If x is zero, returns zero.
+ */
+static inline int
+fls(unsigned int x)
+{
+ int ls = 0;
+
+ while (x != 0)
+ {
+ ls++;
+ x = 1;
+ }
+
+ return ls;
+}
+
+#endif
+
+#endif /* BITOPS_H */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH 2/2] Use fls() to find chunk set
Results in a ~2% performance increase by using the powerpc fls()
implementation.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/backend/utils/mmgr/aset.c |8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0e2d4d5..762cf72 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -65,6 +65,7 @@
#include postgres.h
#include utils/memutils.h
+#include utils/bitops.h
/* Define this to detail debug alloc information */
/* #define HAVE_ALLOCINFO */
@@ -270,12 +271,7 @@ AllocSetFreeIndex(Size size)
if (size 0)
{
- size = (size - 1) ALLOC_MINBITS;
- while (size != 0)
- {
- idx++;
- size = 1;
- }
+ idx = fls((size - 1) ALLOC_MINBITS);
Assert(idx ALLOCSET_NUM_FREELISTS);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH v2] Add bit operations util header
Add a utility header for simple bit operatios - bitops.h.
At present, just contains the fls() (find last set bit) function.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
v2: only use inline asm with gcc
---
src/include/utils/bitops.h | 53 +
1 file changed, 53 insertions(+)
diff --git a/src/include/utils/bitops.h b/src/include/utils/bitops.h
new file mode 100644
index 000..4f2bbc9
--- /dev/null
+++ b/src/include/utils/bitops.h
@@ -0,0 +1,53 @@
+/*-
+ *
+ * bitops.h
+ * Simple bit operations.
+ *
+ * Portions Copyright (c) 2009, PostgreSQL Global Development Group
+ *
+ * $PostgreSQL$
+ *
+ *-
+ */
+#ifndef BITOPS_H
+#define BITOPS_H
+
+#if defined(__GNUC__) \
+ (defined(__ppc__) || defined(__powerpc__) || \
+defined(__ppc64__) || defined (__powerpc64__))
+
+static inline int
+fls(unsigned int x)
+{
+ int lz;
+ asm(cntlz %0,%1 : =r (lz) : r (x));
+ return 32 - lz;
+}
+
+#else /* !(gcc powerpc) */
+
+/* Architecture-independent implementations */
+
+/*
+ * fls: find last set bit.
+ *
+ * Returns the 1-based index of the most-significant bit in x. The MSB
+ * is bit number 32, the LSB is bit number 1. If x is zero, returns zero.
+ */
+static inline int
+fls(unsigned int x)
+{
+ int ls = 0;
+
+ while (x != 0)
+ {
+ ls++;
+ x = 1;
+ }
+
+ return ls;
+}
+
+#endif
+
+#endif /* BITOPS_H */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [RFC,PATCH] Only disable sigpipe during SSL write
If the connection isn't over SSL, there's no need to do the disable.
This effectively halves the number of syscalls performed by libpq when
SSL is not in use.
Signed-off-by: Jeremy Kerr j...@ozlabs.org
---
src/interfaces/libpq/fe-secure.c |7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index eb579cf..2101315 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -368,13 +368,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{
ssize_t n;
- DISABLE_SIGPIPE(return -1);
-
#ifdef USE_SSL
if (conn-ssl)
{
int err;
+ DISABLE_SIGPIPE(return -1);
+
n = SSL_write(conn-ssl, ptr, len);
err = SSL_get_error(conn-ssl, n);
switch (err)
@@ -433,15 +433,14 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
n = -1;
break;
}
+ RESTORE_SIGPIPE();
}
else
#endif
{
n = send(conn-sock, ptr, len, 0);
- REMEMBER_EPIPE(n 0 SOCK_ERRNO == EPIPE);
}
- RESTORE_SIGPIPE();
return n;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [RFC,PATCH] SIGPIPE masking in local socket connections
Currently, I'm seeing the psecure_{red,write} functions being invoked
when connecting to postgres via a unix domain socket. psecure_write
seems to alter the signal mask of the process to disable sigpipe
reporting. psecure_read only does this when the connection is using SSL.
When using a multithreaded client application on Linux, this can result
in poor scalability. Each change to the signal mask requires an
current-sighand-siglock, which becomes highly contended between
the client threads. It also means we do 3 syscalls per write: mask
sigpipe, write, unmask sigpipe.
The following patch changes psecure_write to be more like psecure_read -
it only alters the signal mask if the connection is over SSL. It's only
an RFC, as I'm not entirely sure about the reasoning behind blocking
SIGPIPE for the non-SSL case - there may be other considerations here.
With this change I see the following performance improvement
during a sysbench OLTP run:
http://ozlabs.org/~jk/projects/db/data/sigpipe-perf.png
load: sysbench --test=oltp --oltp-read-only=on, connecting locally,
machine: POWER6, 64-way, 4.2GHz
Comments most welcome,
Jeremy
---
Jeremy Kerr (1):
Only disable sigpipe during SSL write
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
