Title: Enable optimized locks on Windows
Type: performance Stage:
Components: Interpreter Core, Windows Versions: Python 3.8
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: izbyshev,, josh.r, kristjan.jonsson, paul.moore, paulie4, steve.dower, tim.golden, zach.ware
Priority: normal Keywords:

Created on 2017-03-22 01:33 by josh.r, last changed 2019-02-22 13:48 by vstinner.

Pull Requests
URL Status Linked Edit
PR 756 open josh.r, 2017-03-22 01:48
Messages (9)
msg289955 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2017-03-22 01:33
Kristjan wrote improved locking primitives in #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 #15038, we should expect to see a significant improvement in speed.
msg289958 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2017-03-22 01:57
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:

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".
msg289966 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2017-03-22 03:46
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.
msg289979 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2017-03-22 09:11
Hi there.
Looking at the API docs today
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)
2) 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.

msg294365 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-05-24 17:13
I updated the PR to be mergeable and let the AppVeyor run work -

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.
msg310235 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-01-18 11:20
FYI I proposed a PR which removes the emulation of condition variable:

This PR removes code specific to Windows Vista: see bpo-32592.
msg333726 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-01-15 18:33
On issue 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.
msg333731 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-01-15 19:27
I assume you meant #35662 (based on the superseder note in the history).
msg336264 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-21 21:47
> I assume you meant #35662

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

Meanwhile, over on #35662, Jeff has a fix for at least one of the regressions.
Date User Action Args
2019-02-22 13:48:09vstinnersetnosy: - vstinner
2019-02-22 04:23:14paulie4setnosy: + paulie4
2019-02-21 21:47:47steve.dowersetmessages: + msg336264
versions: + Python 3.8, - Python 3.7
2019-01-15 19:27:52josh.rsetmessages: + msg333731
2019-01-15 18:33:26steve.dowersetnosy: +
messages: + msg333726
2019-01-15 18:31:20steve.dowerlinkissue35662 superseder
2018-03-08 17:46:30izbyshevsetnosy: + izbyshev
2018-01-18 11:20:28vstinnersetnosy: + vstinner
messages: + msg310235
2017-05-24 17:13:12steve.dowersetmessages: + msg294365
2017-03-22 09:11:16kristjan.jonssonsetmessages: + msg289979
2017-03-22 03:46:17josh.rsetmessages: + msg289966
2017-03-22 01:57:37josh.rsetmessages: + msg289958
2017-03-22 01:48:26josh.rsetpull_requests: + pull_request670
2017-03-22 01:33:16josh.rcreate