Spin Lock sleep resolution

Jeff Janes <jeff.janes@gmail.com> writes:

The problem is that the state is maintained only to an integer number of
milliseconds starting at 1, so it can take a number of attempts for the
random increment to jump from 1 to 2, and then from 2 to 3.

Hm ... fair point, if you assume that the underlying OS has a sleep
resolution finer than 1ms. Otherwise it would not matter.

The attached patch changes the resolution of the state variable to
microseconds, but keeps the starting value at 1msec, i.e. 1000 usec.

No patch seen here ...

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Apr 2, 2013 at 1:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Jeff Janes <jeff.janes@gmail.com> writes:

The problem is that the state is maintained only to an integer number of
milliseconds starting at 1, so it can take a number of attempts for the
random increment to jump from 1 to 2, and then from 2 to 3.

Hm ... fair point, if you assume that the underlying OS has a sleep
resolution finer than 1ms. Otherwise it would not matter.

I would guess it does matter for the cumulative error when re-sleeping
several times.

In any case, the resolution is limited on tick-based kernels, which
are few nowadays. However, I've found evidence[0]http://lists.freebsd.org/pipermail/freebsd-arch/2012-March/012423.html that FreeBSD is
still on that boat.

[0]: http://lists.freebsd.org/pipermail/freebsd-arch/2012-March/012423.html

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Monday, April 1, 2013, Tom Lane wrote:

Jeff Janes <jeff.janes@gmail.com> writes:

The problem is that the state is maintained only to an integer number of
milliseconds starting at 1, so it can take a number of attempts for the
random increment to jump from 1 to 2, and then from 2 to 3.

Hm ... fair point, if you assume that the underlying OS has a sleep
resolution finer than 1ms. Otherwise it would not matter.

Let's say you get a long stretch of increments that are all a ratio of <1.5
fold, for simplicity let's say they are all 1.3 fold. When you do
intermediate truncations of the state variable, it never progresses at all.

perl -le '$foo=1; foreach (1..10) {$foo*=1.3; print int $foo}'

perl -le '$foo=1; foreach (1..10) {$foo*=1.3; $foo=int $foo; print int
$foo}'

Obviously the true stochastic case is not quite that stark.

No patch seen here ...

Sorry. I triple checked that the patch was there, but it seems like if you
save a draft with an attachment, when you come back later to finish and
send it, the attachment may not be there anymore. The Gmail Offline teams
still has a ways to go. Hopefully it is actually there this time.

Cheers,

Jeff

On Tue, 2 Apr 2013 09:01:36 -0700
Jeff Janes <jeff.janes@gmail.com> wrote:

Sorry. I triple checked that the patch was there, but it seems like if you
save a draft with an attachment, when you come back later to finish and
send it, the attachment may not be there anymore. The Gmail Offline teams
still has a ways to go. Hopefully it is actually there this time.

I'll give the patch a try, I have a workload that is impacted by spinlocks
fairly heavily sometimes and this might help or at least give me more
information. Thanks!

-dg

--
David Gould daveg@sonic.net
If simplicity worked, the world would be overrun with insects.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi David,

On 02.04.2013 22:58, David Gould wrote:

On Tue, 2 Apr 2013 09:01:36 -0700
Jeff Janes<jeff.janes@gmail.com> wrote:

Sorry. I triple checked that the patch was there, but it seems like if you
save a draft with an attachment, when you come back later to finish and
send it, the attachment may not be there anymore. The Gmail Offline teams
still has a ways to go. Hopefully it is actually there this time.

I'll give the patch a try, I have a workload that is impacted by spinlocks
fairly heavily sometimes and this might help or at least give me more
information. Thanks!

Did you ever get around to test this?

I repeated these pgbench tests I did earlier:

/messages/by-id/5190E17B.9060804@vmware.com

I concluded in that thread that on this platform, the TAS_SPIN macro
really needs a non-locked test before the locked one. That fixes the big
fall in performance with more than 28 clients. So I repeated that test
with four versions:

master - no patch
spin-delay-ms - Jeff's patch
nonlocked-test - master with the non-locked test added to TAS_SPIN
spin-delay-ms-nonlocked-test - both patches

Jeff's patch seems to somewhat alleviate the huge fall in performance
I'm otherwise seeing without the nonlocked-test patch. With the
nonlocked-test patch, if you squint you can see a miniscule benefit.

I wasn't expecting much of a gain from this, just wanted to verify that
it's not making things worse. So looks good to me.

- Heikki

On Tue, 18 Jun 2013 10:09:55 +0300
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

On 02.04.2013 22:58, David Gould wrote:

I'll give the patch a try, I have a workload that is impacted by spinlocks
fairly heavily sometimes and this might help or at least give me more
information. Thanks!

Did you ever get around to test this?

I repeated these pgbench tests I did earlier:

/messages/by-id/5190E17B.9060804@vmware.com

I concluded in that thread that on this platform, the TAS_SPIN macro
really needs a non-locked test before the locked one. That fixes the big
fall in performance with more than 28 clients.

...

I wasn't expecting much of a gain from this, just wanted to verify that
it's not making things worse. So looks good to me.

Thanks for the followup, and I really like your graph, it looks exactly
like what we were hitting. My client ended up configuring around it
and adding more hosts so the urgency to run more tests sort of declined,
although I think we still hit it from time to time.

If you would like to point me at or send me the latest flavor of the patch
it may be timely for me to test again. Especially if this is a more or less
finished version, we are about to roll out a new build to all these hosts
and I'd be happy to try to incorporate this patch and get some production
experience with it on 80 core hosts.

-dg

--
David Gould 510 282 0869 daveg@sonic.net
If simplicity worked, the world would be overrun with insects.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 18.06.2013 10:52, David Gould wrote:

On Tue, 18 Jun 2013 10:09:55 +0300
Heikki Linnakangas<hlinnakangas@vmware.com> wrote:

I repeated these pgbench tests I did earlier:

/messages/by-id/5190E17B.9060804@vmware.com

I concluded in that thread that on this platform, the TAS_SPIN macro
really needs a non-locked test before the locked one. That fixes the big
fall in performance with more than 28 clients.

...

I wasn't expecting much of a gain from this, just wanted to verify that
it's not making things worse. So looks good to me.

Thanks for the followup, and I really like your graph, it looks exactly
like what we were hitting. My client ended up configuring around it
and adding more hosts so the urgency to run more tests sort of declined,
although I think we still hit it from time to time.

Oh, interesting. What kind of hardware are you running on? To be honest,
I'm not sure what my test hardware is, it's managed by another team
across the world, but /proc/cpuinfo says:

model name : Intel(R) Xeon(R) CPU E5-4640 0 @ 2.40GHz

And it's running in a virtual machine on VMware; that might also be a
factor.

It would be good to test the TAS_SPIN nonlocked patch on a variety of
systems. The comments in s_lock.h say that on Opteron, the non-locked
test is a huge loss. In particular, would be good to re-test that on a
modern AMD system.

If you would like to point me at or send me the latest flavor of the patch
it may be timely for me to test again. Especially if this is a more or less
finished version, we are about to roll out a new build to all these hosts
and I'd be happy to try to incorporate this patch and get some production
experience with it on 80 core hosts.

Jeff's patch is here:
/messages/by-id/CAMkU=1xtP+3UWL6dg10rDRtMA28zy-9ujrOxNuST_r3zOdpNxg@mail.gmail.com.
My non-locked TAS_SPIN patch is the one-liner here:
/messages/by-id/519A938A.1070903@vmware.com

Thanks!

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, 18 Jun 2013 11:41:06 +0300
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

Oh, interesting. What kind of hardware are you running on? To be honest,
I'm not sure what my test hardware is, it's managed by another team
across the world, but /proc/cpuinfo says:

model name : Intel(R) Xeon(R) CPU E5-4640 0 @ 2.40GHz

It claims to have 80 of these:

model name : Intel(R) Xeon(R) CPU E7-L8867 @2.13GHz

Postgres is on ramfs on these with unlogged tables.

And it's running in a virtual machine on VMware; that might also be a
factor.

I'm not a fan of virtualization. It makes performance even harder to
reason about.

It would be good to test the TAS_SPIN nonlocked patch on a variety of
systems. The comments in s_lock.h say that on Opteron, the non-locked
test is a huge loss. In particular, would be good to re-test that on a
modern AMD system.

I'll see what I can do. However I don't have acces to any large modern AMD
systems.

-dg

--
David Gould 510 282 0869 daveg@sonic.net
If simplicity worked, the world would be overrun with insects.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jun 18, 2013 at 12:09 AM, Heikki Linnakangas <
hlinnakangas@vmware.com> wrote:

Jeff's patch seems to somewhat alleviate the huge fall in performance I'm
otherwise seeing without the nonlocked-test patch. With the nonlocked-test
patch, if you squint you can see a miniscule benefit.

I wasn't expecting much of a gain from this, just wanted to verify that
it's not making things worse. So looks good to me.

Hi Heikki,

Thanks for trying out the patch.

I see in the commitfest app it is set to "Waiting on Author" (but I don't
know who "maiku41" is).

Based on the comments so far, I don't know what I should be doing on it at
the moment, and I thought perhaps your comment above meant it should be
"ready for committer".

If we think the patch has a risk of introducing subtle errors, then it
probably can't be justified based on the small performance gains you saw.

But if we think it has little risk, then I think it is justified simply
based on cleaner code, and less confusion for people who are tracing a
problematic process. If we want it to do a random escalation, it should
probably look like a random escalation to the interested observer.

Thanks,

Jeff

Jeff Janes escribi�:

I see in the commitfest app it is set to "Waiting on Author" (but I don't
know who "maiku41" is).

Yeah, that guy is misterious. I'm guessing the Mike Blackwell person
Josh mentioned in his "week 1" report.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/26/2013 02:49 PM, Jeff Janes wrote:

I see in the commitfest app it is set to "Waiting on Author" (but I don't
know who "maiku41" is).

Mike Blackwell, who's helping track patches for the CommitFest.

It's been our practice since the 9.3 cycle that patches which are still
under contentious discussion which blocks commit are "waiting on author"
rather than some other status. It was not clear to me (or to Mike) that
the dicussion on this patch was concluded favorably. Is it?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jun 26, 2013 at 5:49 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

On Tue, Jun 18, 2013 at 12:09 AM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

Jeff's patch seems to somewhat alleviate the huge fall in performance I'm
otherwise seeing without the nonlocked-test patch. With the nonlocked-test
patch, if you squint you can see a miniscule benefit.

I wasn't expecting much of a gain from this, just wanted to verify that
it's not making things worse. So looks good to me.

Hi Heikki,

Thanks for trying out the patch.

I see in the commitfest app it is set to "Waiting on Author" (but I don't
know who "maiku41" is).

Based on the comments so far, I don't know what I should be doing on it at
the moment, and I thought perhaps your comment above meant it should be
"ready for committer".

If we think the patch has a risk of introducing subtle errors, then it
probably can't be justified based on the small performance gains you saw.

But if we think it has little risk, then I think it is justified simply
based on cleaner code, and less confusion for people who are tracing a
problematic process. If we want it to do a random escalation, it should
probably look like a random escalation to the interested observer.

I think it has little risk. If it turns out to be worse for
performance, we can always revert it, but I expect it'll be better or
a wash, and the results so far seem to bear that out. Another
interesting question is whether we should fool with the actual values
for minimum and maximum delays, but that's a separate and much harder
question, so I think we should just do this for now and call it good.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 27.06.2013 17:30, Robert Haas wrote:

On Wed, Jun 26, 2013 at 5:49 PM, Jeff Janes<jeff.janes@gmail.com> wrote:

If we think the patch has a risk of introducing subtle errors, then it
probably can't be justified based on the small performance gains you saw.

But if we think it has little risk, then I think it is justified simply
based on cleaner code, and less confusion for people who are tracing a
problematic process. If we want it to do a random escalation, it should
probably look like a random escalation to the interested observer.

I think it has little risk. If it turns out to be worse for
performance, we can always revert it, but I expect it'll be better or
a wash, and the results so far seem to bear that out. Another
interesting question is whether we should fool with the actual values
for minimum and maximum delays, but that's a separate and much harder
question, so I think we should just do this for now and call it good.

My thoughts exactly. I wanted to see if David Gould would like to do
some testing with it, but there's realy no need to hold off committing
for that, I'm not expecting any surprises there. Committed.

Jeff, in the patch you changed the datatype of cur_delay variable from
int to long. AFAICS that makes no difference, so I kept it as int. Let
me know if there was a reason for that change.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jun 28, 2013 at 2:46 AM, Heikki Linnakangas <hlinnakangas@vmware.com

wrote:

On 27.06.2013 17:30, Robert Haas wrote:

On Wed, Jun 26, 2013 at 5:49 PM, Jeff Janes<jeff.janes@gmail.com> wrote:

If we think the patch has a risk of introducing subtle errors, then it

probably can't be justified based on the small performance gains you saw.

But if we think it has little risk, then I think it is justified simply
based on cleaner code, and less confusion for people who are tracing a
problematic process. If we want it to do a random escalation, it should
probably look like a random escalation to the interested observer.

I think it has little risk. If it turns out to be worse for
performance, we can always revert it, but I expect it'll be better or
a wash, and the results so far seem to bear that out. Another
interesting question is whether we should fool with the actual values
for minimum and maximum delays, but that's a separate and much harder
question, so I think we should just do this for now and call it good.

My thoughts exactly. I wanted to see if David Gould would like to do some
testing with it, but there's realy no need to hold off committing for that,
I'm not expecting any surprises there. Committed.

Thanks.

Jeff, in the patch you changed the datatype of cur_delay variable from int
to long. AFAICS that makes no difference, so I kept it as int. Let me know
if there was a reason for that change.

I think my thought process at the time was that since the old code
multiplied by "1000L", not "1000" in the expression argument to pg_usleep,
I wanted to keep the spirit of the L in place. It seemed safer than trying
to prove to myself that it was not necessary.

If int suffices, should the L come out of the defines for MIN_DELAY_USEC
and MAX_DELAY_USEC ?

Cheers,

Jeff