Enable optimized locks on Windows

[MojoVampire]

BPO	29871
Nosy	@pfmoore, @kristjanvalur, @tjguk, @paulie4, @zware, @zooba, @MojoVampire, @izbyshev
PRs	bpo-29871: Replace old condition variable emulation on Windows with o… #756

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2017-03-22.01:33:16.583>
labels = ['interpreter-core', '3.8', 'OS-windows', 'performance']
title = 'Enable optimized locks on Windows'
updated_at = <Date 2019-02-22.13:48:09.891>
user = 'https://github.com/MojoVampire'

bugs.python.org fields:

activity = <Date 2019-02-22.13:48:09.891>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core', 'Windows']
creation = <Date 2017-03-22.01:33:16.583>
creator = 'josh.r'
dependencies = []
files = []
hgrepos = []
issue_num = 29871
keywords = []
message_count = 9.0
messages = ['289955', '289958', '289966', '289979', '294365', '310235', '333726', '333731', '336264']
nosy_count = 9.0
nosy_names = ['paul.moore', 'kristjan.jonsson', 'tim.golden', 'paulie4', 'zach.ware', 'jeffr@livedata.com', 'steve.dower', 'josh.r', 'izbyshev']
pr_nums = ['756']
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue29871'
versions = ['Python 3.8']

[MojoVampire]

Kristjan wrote improved locking primitives in bpo-15038 that use the new (in Vista) SRWLock and Condition Variable APIs. SRWLocks (used in exclusive mode only) replace Critical Sections, which is slower than SRWLock and provides no features we use that might justify it. Condition Variables replace Semaphores, where the former is user mode (cheap) and the latter kernel mode (expensive).

These changes remain disabled by default.

Given that CPython dropped support for pre-Vista OSes in 3.5, I propose enabling the faster locking primitives by default. The PR I'll submit leaves the condition variable emulation code in the source so it's available to people who might try to build XP/WS03 compatible code, it just tweaks the define so it defaults to using the Vista+ APIs.

Based on the numbers from bpo-15038, we should expect to see a significant improvement in speed.

[MojoVampire]

Note: Beyond turning on the new primitives by default, I also made a change to PyCOND_TIMEDWAIT. The original code looked wrong, in that:

1. It assumed that when SleepConditionVariableSRW returned non-zero, you didn't know if the wait had timed out or not, so it returned 2 to indicate "Might have timed out, act as if it timed out"
2. It assumed that when SleepConditionVariableSRW returned zero, it had suffered a critical failure, and returned -1 to indicate that.

Neither of these things is true AFAICT. SleepConditionVariableSRW returns non-zero when it's alerted prior to timing out. Otherwise, it returns 0, and you have to check GetLastError to distinguish timeout from critical failures. Documentation is here: https://msdn.microsoft.com/en-us/library/windows/desktop/ms686304(v=vs.85).aspx

It's subject to "spurious" wake-ups, but so is pthread_cond_timedwait and the pthreads code doesn't use an "all timedwaits time out".

[MojoVampire]

Hmm... I was only running the threading related tests originally, and they passed, but it looks like this causes problems with multiprocessing (test_multiprocessing_spawn and test_concurrent_futures both stall out forever with this change, with or without my fix to the PyCOND_TIMEDWAIT function). I'll investigate further, don't bother to review yet.

[kristjanvalur]

Hi there.
Looking at the API docs today
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms686304(v=vs.85).aspx)
it appears that the timeout case is documented. I'm fairly sure that it wasn't when I implemented it. There was a good reason for the "2" return code. The idea was: Either there was an error (-1) or we woke up. Since there are spurious wakeups and stolen wakeups, the predicate must be tested again anyway. The '2' return code would mean that the timeout condition should be tested by looking at some external clock.

Now, the api documentation is bad. Return value is BOOL.
Nonzero means "success" (whatever that means)
Failure means ´zero´
Timeout means FALSE and GetLastError() == ERROR_TIMEOUT

If memory serves, FALSE == 0 on windows....

Anyway, I've been out of this part of the code for sufficient time for details to be blurry.
My advise:

1. Check that the API of the function is indeed correct.
2. If there is no bulletproof way of distinguishing timeout from normal return, just consider all returns normal (remember, non-error return means that we woke up, not that _we_ were signaled)
3. Verify that the code that is failing can indeed support spurious wakeups/stolen wakeups. It used to be that the python condition variables didn't have this property, because of the way they were implemented. This may have made people lazy.

K

[zooba]

I updated the PR to be mergeable and let the AppVeyor run work - https://ci.appveyor.com/project/python/cpython/build/3.7.0a0.2452

Unfortunately, there appear to be a number of regressions due to this. I'm not going to have time right now to work through them myself, but if someone else can it would be great to resolve these and get the change in.

[vstinner]

FYI I proposed a PR which removes the emulation of condition variable:
#5231

This PR removes code specific to Windows Vista: see bpo-32592.

[zooba]

On bpo-35562 Jeff posted a deeper analysis of the issue in TIMEDWAIT. That will need fixing along with the other regressions before we can enable these.

[MojoVampire]

I assume you meant bpo-35662 (based on the superseder note in the history).

[zooba]

I assume you meant bpo-35662

Yes indeed. I am apparently massively dyslexic when it comes to copying issue numbers into the bpo comment field :)

Meanwhile, over on bpo-35662, Jeff has a fix for at least one of the regressions.