Try changing LockImpl to not have a spin count to see how it affects performance.

Try changing LockImpl to not have a spin count to see how it affects performance.

This was last change 7 years ago, and we have grown a lot of usages of locks since then. Since we don't use pthread_spin_lock on posix, I'm curious how this affects performance today on Windows.

Committed: https://crrev.com/00c9ca679badffc7584439fff00f18cb3977d5da
Cr-Commit-Position: refs/heads/master@{#348280}

[jam, 2015-09-10 20:59:36]

jam@chromium.org changed reviewers:
+ cpu@chromium.org

[jam, 2015-09-10 20:59:37]

Hey Carlos, I want to keep this in for a few hours to look at perf graphs, then
will revert.

[cpu_(ooo_6.6-7.5), 2015-09-10 21:51:57]

ok, but the call with the spincount has a secondary advantage if my memory does
not fail. It Creates the event of the spot instead of trying to create the event
when the lock is contended.

But for a bit of experimentation there is no harm. lgtm

[jam, 2015-09-10 22:09:57]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-09-10 22:10:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1333053002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1333053002/1

[jam, 2015-09-10 22:18:21]

On 2015/09/10 21:51:57, cpu wrote:
> ok, but the call with the spincount has a secondary advantage if my memory
does
> not fail. It Creates the event of the spot instead of trying to create the
event
> when the lock is contended.
> 
> But for a bit of experimentation there is no harm. lgtm

thanks for the background.

I want to see what the effects are on the perf bots, then depending on that we
can do finch experiments.

[commit-bot: I haz the power, 2015-09-10 23:22:50]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-09-10 23:24:11]

Patchset 1 (id:??) landed as
https://crrev.com/00c9ca679badffc7584439fff00f18cb3977d5da
Cr-Commit-Position: refs/heads/master@{#348280}

[brucedawson, 2015-09-10 23:36:06]

On 2015/09/10 23:22:50, commit-bot: I haz the power wrote:
> Committed patchset #1 (id:1)

I'm interested to see how the results look. We could also try a larger number.
Microsoft mentions in passing
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms683477(v=vs.85).aspx)
that the heap manager uses a spin count of 4,000. That is presumably ancient
documentation and an even higher count might be appropriate now. I'm on record
as opposing unbounded spins, but short spins can be pretty useful.

Windows 7 apparently added a flag that you could specify to get the OS to
dynamically adjust the spin count. The flag is only defined in RTL (i.e.; only
sort of exposed) form as RTL_CRITICAL_SECTION_FLAG_DYNAMIC_SPIN and is not
documented. I don't know how well it works.

Aside: I did a search for RTL_CRITICAL_SECTION_FLAG_DYNAMIC_SPIN and the top
hits are all me posting comments or questions about it. So that's not much use.

[jam, 2015-09-11 17:05:36]

On 2015/09/10 23:36:06, brucedawson wrote:
> On 2015/09/10 23:22:50, commit-bot: I haz the power wrote:
> > Committed patchset #1 (id:1)
> 
> I'm interested to see how the results look. We could also try a larger number.
> Microsoft mentions in passing
>
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms683477(v=vs.85).aspx)
> that the heap manager uses a spin count of 4,000. That is presumably ancient
> documentation and an even higher count might be appropriate now. I'm on record
> as opposing unbounded spins, but short spins can be pretty useful.
> 
> Windows 7 apparently added a flag that you could specify to get the OS to
> dynamically adjust the spin count. The flag is only defined in RTL (i.e.; only
> sort of exposed) form as RTL_CRITICAL_SECTION_FLAG_DYNAMIC_SPIN and is not
> documented. I don't know how well it works.
> 
> Aside: I did a search for RTL_CRITICAL_SECTION_FLAG_DYNAMIC_SPIN and the top
> hits are all me posting comments or questions about it. So that's not much
use.

btw I'll revert this as I don't see much movement either way on
https://chromeperf.appspot.com/group_report?rev=348280 (note some graphs are
worse, but then the equivalent ones on mac also get worse too so it's something
else)

Bruce: feel free to try out different values! the dynamic one sounds cool.

[jam, 2015-09-11 17:05:49]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1340653003/ by jam@chromium.org.

The reason for reverting is: didn't make any difference.

[commit-bot: I haz the power, 2015-09-23 12:16:12]

Patchset 1 (id:??) landed as
https://crrev.com/00c9ca679badffc7584439fff00f18cb3977d5da
Cr-Commit-Position: refs/heads/master@{#348280}