Re: 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/

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(at)hagander(dot)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(at)esilo(dot)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(at)esilo(dot)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/

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(at)esilo(dot)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(at)esilo(dot)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(at)sonic(dot)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(at)esilo(dot)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(at)esilo(dot)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(at)hagander(dot)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

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