classification
Title: Don't release the GIL if we can acquire a multiprocessing semaphore immediately
Type: performance Stage: resolved
Components: Extension Modules Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Daniel Colascione, davin, pitrou, serhiy.storchaka, vstinner
Priority: normal Keywords: easy (C), patch

Created on 2017-10-01 01:43 by Daniel Colascione, last changed 2017-10-23 20:57 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4078 merged pitrou, 2017-10-22 10:45
PR 4091 merged vstinner, 2017-10-23 17:34
Messages (4)
msg303441 - (view) Author: Daniel Colascione (Daniel Colascione) Date: 2017-10-01 01:43
Right now, the main loop in semlock_acquire looks like this:

    do {
        Py_BEGIN_ALLOW_THREADS
        if (blocking && timeout_obj == Py_None)
            res = sem_wait(self->handle);
        else if (!blocking)
            res = sem_trywait(self->handle);
        else
            res = sem_timedwait(self->handle, &deadline);
        Py_END_ALLOW_THREADS
        err = errno;
        if (res == MP_EXCEPTION_HAS_BEEN_SET)
            break;
    } while (res < 0 && errno == EINTR && !PyErr_CheckSignals());

Here, we unconditionally release the GIL even we could acquire the mutex without blocking! As a result, we could end up switching to another thread in the process and greatly increasing the latency of operations that lock and release multiple shared data structures.

Instead, we should unconditionally try sem_trywait, and only then, if we want to block, release the GIL and try sem_wait or sem_timedwait. This way, we'll churn only when we need to.

Note that threading.Lock works the way I propose.
msg303449 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-01 06:11
Looks reasonable to me. Do you want to provide a patch Daniel?
msg304742 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-10-22 11:10
New changeset c872d39d324cd6f1a71b73e10406bbaed192d35f by Antoine Pitrou in branch 'master':
bpo-31653: Don't release the GIL if we can acquire a multiprocessing semaphore immediately (#4078)
https://github.com/python/cpython/commit/c872d39d324cd6f1a71b73e10406bbaed192d35f
msg304840 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-23 20:57
New changeset 828ca59208af0b1b52a328676c5cc0c5e2e999b0 by Victor Stinner in branch 'master':
bpo-31653: Remove deadcode in semlock_acquire() (#4091)
https://github.com/python/cpython/commit/828ca59208af0b1b52a328676c5cc0c5e2e999b0
History
Date User Action Args
2017-10-23 20:57:57vstinnersetnosy: + vstinner
messages: + msg304840
2017-10-23 17:34:11vstinnersetpull_requests: + pull_request4061
2017-10-22 11:11:08pitrousetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-10-22 11:10:50pitrousetmessages: + msg304742
2017-10-22 10:45:33pitrousetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request4048
2017-10-01 09:49:11pitrousettype: enhancement -> performance
2017-10-01 06:11:04serhiy.storchakasettype: enhancement
components: + Extension Modules, - Library (Lib)
versions: - Python 3.4, Python 3.5, Python 3.6, Python 3.8
keywords: + easy (C)
nosy: + serhiy.storchaka, pitrou, davin

messages: + msg303449
stage: needs patch
2017-10-01 01:43:01Daniel Colascionecreate