Thread: libpq Win32 Mutex performance patch
I noticed several months ago, and came across it again today, that libpq's pthread-win32.c implementation is using CreateMutex rather than CRITICAL_SECTION. CreateMutex is like a semaphore in that it is designed to be accessible via name system-wide. Even when you don't give it a name, thus bound to process that created it, it still carries significant overhead compared to using win32 CRITICAL_SECTIONs. The attached patch replaces the win32 mutex calls with critical section calls. The change will not affect the behavior of the windows pthread_xxx functions. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/port/pthread-win32.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/port/pthread-win32.h,v retrieving revision 1.2 diff -c -r1.2 pthread-win32.h *** src/port/pthread-win32.h 18 Apr 2007 08:32:40 -0000 1.2 --- src/port/pthread-win32.h 11 Apr 2008 18:35:33 -0000 *************** *** 2,8 **** #define __PTHREAD_H typedef ULONG pthread_key_t; ! typedef HANDLE pthread_mutex_t; typedef int pthread_once_t; DWORD pthread_self(void); --- 2,8 ---- #define __PTHREAD_H typedef ULONG pthread_key_t; ! typedef CRITICAL_SECTION *pthread_mutex_t; typedef int pthread_once_t; DWORD pthread_self(void); Index: src/interfaces/libpq/pthread-win32.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/pthread-win32.c,v retrieving revision 1.15 diff -c -r1.15 pthread-win32.c *** src/interfaces/libpq/pthread-win32.c 1 Jan 2008 19:46:00 -0000 1.15 --- src/interfaces/libpq/pthread-win32.c 11 Apr 2008 18:35:33 -0000 *************** *** 35,51 **** void pthread_mutex_init(pthread_mutex_t *mp, void *attr) { ! *mp = CreateMutex(0, 0, 0); } void pthread_mutex_lock(pthread_mutex_t *mp) { ! WaitForSingleObject(*mp, INFINITE); } void pthread_mutex_unlock(pthread_mutex_t *mp) { ! ReleaseMutex(*mp); } --- 35,69 ---- void pthread_mutex_init(pthread_mutex_t *mp, void *attr) { ! if(mp) ! { ! *mp = (CRITICAL_SECTION *)malloc(sizeof(CRITICAL_SECTION)); ! if(*mp) ! InitializeCriticalSection(*mp); ! } } void pthread_mutex_lock(pthread_mutex_t *mp) { ! if(mp && *mp) ! EnterCriticalSection(*mp); } void pthread_mutex_unlock(pthread_mutex_t *mp) { ! if(mp && *mp) ! LeaveCriticalSection(*mp); } + + /* If ever needed + pthread_mutex_destroy(pthread_mutex_t *mp) + { + if(mp && *mp) + { + DeleteCriticalSection(*mp); + *mp = NULL; + } + } + */ \ No newline at end of file
Andrew Chernow wrote: > I noticed several months ago, and came across it again today, that > libpq's pthread-win32.c implementation is using CreateMutex rather > than CRITICAL_SECTION. CreateMutex is like a semaphore in that it is > designed to be accessible via name system-wide. Even when you don't > give it a name, thus bound to process that created it, it still > carries significant overhead compared to using win32 > CRITICAL_SECTIONs. > > The attached patch replaces the win32 mutex calls with critical > section calls. The change will not affect the behavior of the > windows pthread_xxx functions. First of all, I like this in general :-) But a couple of comments. It changes the behavior when the pointer passed in is invalid from crash to silent working, right? This shouldn't actually matter, since these functions are only ever supposed to run from callers *inside libpq*, so it probalby doesn't matter... Which brings up the second point - is there any actual reason for adding the pthread_mutex_destroy call? Since libpq never calls it, and it's never used from outside libpq (it's not exported outside the library even), isn't it just going to end up as dead code? //Magnus
Magnus Hagander wrote: >It changes the behavior when the pointer passed in is invalid from >crash to silent working, right? Correct, it a Habit. I sub-consciously write code that checks pointers. We can remove the pointer checks and let the thing dump core if people prefer. > Which brings up the second point - is there any actual reason for > adding the pthread_mutex_destroy call? Since libpq never calls it, and > it's never used from outside libpq (it's not exported outside the > library even), isn't it just going to end up as dead code? > > //Magnus > The destroy call is within a comment. I only put it there in case it is ever needeed. BTW, I just noticed the commented destroy call forgot to free(*mp) ... ooppssseee. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
On Fri, Apr 11, 2008 at 2:49 PM, Magnus Hagander <magnus@hagander.net> wrote: > Andrew Chernow wrote: > > I noticed several months ago, and came across it again today, that > > libpq's pthread-win32.c implementation is using CreateMutex rather > > than CRITICAL_SECTION. CreateMutex is like a semaphore in that it is > > designed to be accessible via name system-wide. Even when you don't > > give it a name, thus bound to process that created it, it still > > carries significant overhead compared to using win32 > > CRITICAL_SECTIONs. > > > > The attached patch replaces the win32 mutex calls with critical > > section calls. The change will not affect the behavior of the > > windows pthread_xxx functions. > > First of all, I like this in general :-) But a couple of comments. > > It changes the behavior when the pointer passed in is invalid from > crash to silent working, right? This shouldn't actually matter, > since these functions are only ever supposed to run from callers > *inside libpq*, so it probalby doesn't matter... I noticed you conjured up a ecpg threading patch sometime around early 2007. You used a mutex there deliberately because that's what libpq did. Maybe that patch should be adjusted? merlin
Andrew Chernow <ac@esilo.com> writes:
> The attached patch replaces the win32 mutex calls with critical section
> calls. The change will not affect the behavior of the windows
> pthread_xxx functions.
Why have you defined the lock/unlock functions as willing to fall
through silently if handed a null pointer? I think a crash in
such a case is what we *want*. Silently not locking is surely
not very safe.
regards, tom lane
Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: >> The attached patch replaces the win32 mutex calls with critical section >> calls. The change will not affect the behavior of the windows >> pthread_xxx functions. > > Why have you defined the lock/unlock functions as willing to fall > through silently if handed a null pointer? I think a crash in > such a case is what we *want*. Silently not locking is surely > not very safe. > > regards, tom lane > Yeah, both naughty. These functions were not implemented to spec. These pthread functions are all supposed to return an int (which is an errno value). I was trying not to change the existing prototypes ... should I? I can return EINVAL if something is NULL and ENOMEM if the malloc fails ... or just dump core. If you like the return value idea, I can make all occurances of pthread_xxx check the return value. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote: >Silently not locking is surely > not very safe. > Here is the dump code version of the patch. If anyone wants the return value idea, let me know. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/port/pthread-win32.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/port/pthread-win32.h,v retrieving revision 1.2 diff -c -r1.2 pthread-win32.h *** src/port/pthread-win32.h 18 Apr 2007 08:32:40 -0000 1.2 --- src/port/pthread-win32.h 11 Apr 2008 19:22:17 -0000 *************** *** 2,8 **** #define __PTHREAD_H typedef ULONG pthread_key_t; ! typedef HANDLE pthread_mutex_t; typedef int pthread_once_t; DWORD pthread_self(void); --- 2,8 ---- #define __PTHREAD_H typedef ULONG pthread_key_t; ! typedef CRITICAL_SECTION *pthread_mutex_t; typedef int pthread_once_t; DWORD pthread_self(void); Index: src/interfaces/libpq/pthread-win32.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/pthread-win32.c,v retrieving revision 1.15 diff -c -r1.15 pthread-win32.c *** src/interfaces/libpq/pthread-win32.c 1 Jan 2008 19:46:00 -0000 1.15 --- src/interfaces/libpq/pthread-win32.c 11 Apr 2008 19:22:17 -0000 *************** *** 35,51 **** void pthread_mutex_init(pthread_mutex_t *mp, void *attr) { ! *mp = CreateMutex(0, 0, 0); } void pthread_mutex_lock(pthread_mutex_t *mp) { ! WaitForSingleObject(*mp, INFINITE); } void pthread_mutex_unlock(pthread_mutex_t *mp) { ! ReleaseMutex(*mp); } --- 35,61 ---- void pthread_mutex_init(pthread_mutex_t *mp, void *attr) { ! *mp = (CRITICAL_SECTION *)malloc(sizeof(CRITICAL_SECTION)); ! InitializeCriticalSection(*mp); } void pthread_mutex_lock(pthread_mutex_t *mp) { ! EnterCriticalSection(*mp); } void pthread_mutex_unlock(pthread_mutex_t *mp) { ! LeaveCriticalSection(*mp); } + + /* If ever needed + void pthread_mutex_destroy(pthread_mutex_t *mp) + { + DeleteCriticalSection(*mp); + free(*mp); + *mp = NULL; + } + */ \ No newline at end of file
Andrew Chernow wrote: > Tom Lane wrote: >> Silently not locking is surely >> not very safe. >> > > Here is the dump code version of the patch. If anyone wants the return > value idea, let me know. > > > ------------------------------------------------------------------------ > > A more graceful solution would be to print something to stderr and then exit. This allows an app's atexit funcs to run. I don't think libpq should core dump an app by choice. I'd be pretty upset if a library I was using core dumped in some cases rather than exiting. andrew
Andrew Chernow <ac@esilo.com> writes:
> A more graceful solution would be to print something to stderr and then
> exit.
stderr doesn't exist, or point to a useful place, in many environments.
And a forced exit() is no better than a crash for most purposes.
> I don't think libpq should core dump an app by choice.
The case that we are talking about is a bug, or would be a bug if it
could happen (the fact that we've gotten along happily with no similar
test in the existing code shows that it can't). Many forms of bug can
result in core dumps; it's foolish to try to prevent them all. And
bloating one line of code into five or more lines to defend against
can't-happen cases is a good way to end up with unreadable,
unmaintainable software.
regards, tom lane
On Fri, Apr 11, 2008 at 06:25:53PM -0400, Tom Lane wrote: > Andrew Chernow <ac@esilo.com> writes: > > A more graceful solution would be to print something to stderr and then > > exit. > > stderr doesn't exist, or point to a useful place, in many environments. > And a forced exit() is no better than a crash for most purposes. > > > I don't think libpq should core dump an app by choice. > > The case that we are talking about is a bug, or would be a bug if it > could happen (the fact that we've gotten along happily with no similar > test in the existing code shows that it can't). Many forms of bug can > result in core dumps; it's foolish to try to prevent them all. And > bloating one line of code into five or more lines to defend against > can't-happen cases is a good way to end up with unreadable, > unmaintainable software. > > regards, tom lane +1 -dg -- David Gould daveg@sonic.net 510 536 1443 510 282 0869 If simplicity worked, the world would be overrun with insects.
daveg wrote: > On Fri, Apr 11, 2008 at 06:25:53PM -0400, Tom Lane wrote: >> Andrew Chernow <ac@esilo.com> writes: >>> A more graceful solution would be to print something to stderr and then >>> exit. >> stderr doesn't exist, or point to a useful place, in many environments. >> And a forced exit() is no better than a crash for most purposes. >> >>> I don't think libpq should core dump an app by choice. >> The case that we are talking about is a bug, or would be a bug if it >> could happen (the fact that we've gotten along happily with no similar >> test in the existing code shows that it can't). Many forms of bug can >> result in core dumps; it's foolish to try to prevent them all. And >> bloating one line of code into five or more lines to defend against >> can't-happen cases is a good way to end up with unreadable, >> unmaintainable software. >> >> regards, tom lane > > +1 > > -dg okay. BTW, my real interest here is the libpq hooks patch requires a lock/unlock for every conn-create, conn-destroy, result-create, result-destroy. Currently, it looks like only the ssl stuff uses mutexes. Adding more mutex use is a good case for a more optimized approach on windows. andrew
Andrew Chernow wrote: > Magnus Hagander wrote: > > >It changes the behavior when the pointer passed in is invalid from > >crash to silent working, right? > > Correct, it a Habit. I sub-consciously write code that checks > pointers. We can remove the pointer checks and let the thing dump > core if people prefer. Actually, if we can avoid it being a pointer at all, that'd be even better :-) Because if we don't send a pointer we have to allocate, then the critical section functions have the same properties as the pthread ones, namely they cannot fail. Any chance for that way instead? //Magnus
Andrew Chernow wrote: > Tom Lane wrote: > > Andrew Chernow <ac@esilo.com> writes: > >> The attached patch replaces the win32 mutex calls with critical > >> section calls. The change will not affect the behavior of the > >> windows pthread_xxx functions. > > > > Why have you defined the lock/unlock functions as willing to fall > > through silently if handed a null pointer? I think a crash in > > such a case is what we *want*. Silently not locking is surely > > not very safe. > > > > regards, tom lane > > > > Yeah, both naughty. > > These functions were not implemented to spec. These pthread > functions are all supposed to return an int (which is an errno > value). I was trying not to change the existing prototypes ... > should I? I can return EINVAL if something is NULL and ENOMEM if the > malloc fails ... or just dump core. > > If you like the return value idea, I can make all occurances of > pthread_xxx check the return value. Getting these emails in the wrong order for some reason :-( Yes, actually checking the return values from these functions in libpq seems like a good idea to me. ISTM that we already have the case where we can fall through silently when failing to lock, and that should be fixed. It looks like the internal API of pgthreadlock_t needs to be changed as well in this case, because it can currently only reaturn void. But - it also seems we are not actually *using* it anywhere. Perhaps that part of the API should simply be removed? //Magnus
Merlin Moncure wrote: > On Fri, Apr 11, 2008 at 2:49 PM, Magnus Hagander > <magnus@hagander.net> wrote: > > Andrew Chernow wrote: > > > I noticed several months ago, and came across it again today, > > > that libpq's pthread-win32.c implementation is using CreateMutex > > > rather than CRITICAL_SECTION. CreateMutex is like a semaphore > > > in that it is designed to be accessible via name system-wide. > > > Even when you don't give it a name, thus bound to process that > > > created it, it still carries significant overhead compared to > > > using win32 CRITICAL_SECTIONs. > > > > > > The attached patch replaces the win32 mutex calls with critical > > > section calls. The change will not affect the behavior of the > > > windows pthread_xxx functions. > > > > First of all, I like this in general :-) But a couple of comments. > > > > It changes the behavior when the pointer passed in is invalid from > > crash to silent working, right? This shouldn't actually matter, > > since these functions are only ever supposed to run from callers > > *inside libpq*, so it probalby doesn't matter... > > I noticed you conjured up a ecpg threading patch sometime around early > 2007. You used a mutex there deliberately because that's what libpq > did. Maybe that patch should be adjusted? Yes, I think it should. //Magnus
Andrew Chernow wrote: > Tom Lane wrote: > >Silently not locking is surely > > not very safe. > > > > Here is the dump code version of the patch. If anyone wants the > return value idea, let me know. Here's a version of this patch that doesn't use malloc - instead, I had to change the callers to it a bit. This makes a difference only on Vista+ really, because prior to Vista the function InitializeCriticalSection() *can* fail - it will throw an exception and not return any error code. Which really isn't that different from just crashing like this latest patch from Andrew would have us do (on out of memory). I'm leaning towards going with the simpler one, which is the patch from Andrew that crashes on out of memory. Comments/preferences? //Magnus
Attachment
Andrew Chernow wrote: > Tom Lane wrote: >> Silently not locking is surely >> not very safe. >> > > Here is the dump code version of the patch. If anyone wants the return > value idea, let me know. So is this a patch we want applied? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Andrew Chernow wrote: > > Tom Lane wrote: > >> Silently not locking is surely > >> not very safe. > >> > > > > Here is the dump code version of the patch. If anyone wants the > > return value idea, let me know. > > So is this a patch we want applied? Please see my other thread about libpq thread-locking which should be finished before this one, after which this patch will change. So no, this is not the version to be applied. //Magnus
Magnus Hagander wrote: > Alvaro Herrera wrote: > > Andrew Chernow wrote: > > > Tom Lane wrote: > > >> Silently not locking is surely > > >> not very safe. > > >> > > > > > > Here is the dump code version of the patch. If anyone wants the > > > return value idea, let me know. > > > > So is this a patch we want applied? > > Please see my other thread about libpq thread-locking which should be > finished before this one, after which this patch will change. So no, > this is not the version to be applied. Hmm, AFAICT on that other thread you state > I'm leaning towards going with the simpler one, which is the patch > from Andrew that crashes on out of memory. which would seem to mean this patch? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Magnus Hagander wrote: > Alvaro Herrera wrote: > > Andrew Chernow wrote: > > > Tom Lane wrote: > > >> Silently not locking is surely > > >> not very safe. > > >> > > > > > > Here is the dump code version of the patch. If anyone wants the > > > return value idea, let me know. > > > > So is this a patch we want applied? > > Please see my other thread about libpq thread-locking which should be > finished before this one, after which this patch will change. So no, > this is not the version to be applied. I've applied a version of this to make libpq use CRITICAL_SECTION on win32. //Magnus