Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't release the GIL if we can acquire a multiprocessing semaphore immediately #75834

Closed
DanielColascione mannequin opened this issue Oct 1, 2017 · 4 comments
Closed
Labels
3.7 (EOL) end of life easy extension-modules C modules in the Modules dir performance Performance or resource usage

Comments

@DanielColascione
Copy link
Mannequin

DanielColascione mannequin commented Oct 1, 2017

BPO 31653
Nosy @pitrou, @vstinner, @serhiy-storchaka, @applio
PRs
  • bpo-31653: Don't release the GIL if we can acquire a multiprocessing semaphore immediately #4078
  • bpo-31653: Remove deadcode in semlock_acquire() #4091
  • 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 = <Date 2017-10-22.11:11:08.000>
    created_at = <Date 2017-10-01.01:43:01.894>
    labels = ['extension-modules', 'easy', '3.7', 'performance']
    title = "Don't release the GIL if we can acquire a multiprocessing semaphore immediately"
    updated_at = <Date 2017-10-23.20:57:57.879>
    user = 'https://bugs.python.org/DanielColascione'

    bugs.python.org fields:

    activity = <Date 2017-10-23.20:57:57.879>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-10-22.11:11:08.000>
    closer = 'pitrou'
    components = ['Extension Modules']
    creation = <Date 2017-10-01.01:43:01.894>
    creator = 'Daniel Colascione'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31653
    keywords = ['patch', 'easy (C)']
    message_count = 4.0
    messages = ['303441', '303449', '304742', '304840']
    nosy_count = 5.0
    nosy_names = ['pitrou', 'vstinner', 'serhiy.storchaka', 'davin', 'Daniel Colascione']
    pr_nums = ['4078', '4091']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue31653'
    versions = ['Python 3.7']

    @DanielColascione
    Copy link
    Mannequin Author

    DanielColascione mannequin commented Oct 1, 2017

    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.

    @DanielColascione DanielColascione mannequin added stdlib Python modules in the Lib dir 3.8 only security fixes 3.7 (EOL) end of life labels Oct 1, 2017
    @serhiy-storchaka
    Copy link
    Member

    Looks reasonable to me. Do you want to provide a patch Daniel?

    @serhiy-storchaka serhiy-storchaka added easy extension-modules C modules in the Modules dir type-feature A feature request or enhancement and removed stdlib Python modules in the Lib dir 3.8 only security fixes labels Oct 1, 2017
    @pitrou pitrou added performance Performance or resource usage and removed type-feature A feature request or enhancement labels Oct 1, 2017
    @pitrou
    Copy link
    Member

    pitrou commented Oct 22, 2017

    New changeset c872d39 by Antoine Pitrou in branch 'master':
    bpo-31653: Don't release the GIL if we can acquire a multiprocessing semaphore immediately (bpo-4078)
    c872d39

    @pitrou pitrou closed this as completed Oct 22, 2017
    @vstinner
    Copy link
    Member

    New changeset 828ca59 by Victor Stinner in branch 'master':
    bpo-31653: Remove deadcode in semlock_acquire() (bpo-4091)
    828ca59

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life easy extension-modules C modules in the Modules dir performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants