Skip to content

release GIL while doing I/O operations in the mmap module #44098

Closed

Description

@luks
mannequin
BPO 1572968
Nosy @pitrou, @briancurtin, @jnwatson, @ZackerySpytz, @isidentical
PRs
  • gh-44098: Release the GIL while calling mmap() in the mmap module on Windows #14114
  • 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 2006-10-08.00:26:31.000>
    labels = ['extension-modules', '3.9', 'performance']
    title = 'release GIL while doing I/O operations in the mmap module'
    updated_at = <Date 2019-06-15.15:48:30.691>
    user = 'https://bugs.python.org/luks'

    bugs.python.org fields:

    activity = <Date 2019-06-15.15:48:30.691>
    actor = 'ZackerySpytz'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Extension Modules']
    creation = <Date 2006-10-08.00:26:31.000>
    creator = 'luks'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 1572968
    keywords = ['patch']
    message_count = 8.0
    messages = ['61261', '61262', '84542', '102453', '102465', '102466', '342561', '343989']
    nosy_count = 8.0
    nosy_names = ['pitrou', 'luks', 'brian.curtin', 'neologix', 'adiroiban', 'jnwatson', 'ZackerySpytz', 'BTaskaya']
    pr_nums = ['14114']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue1572968'
    versions = ['Python 3.9']

    Activity

    1. luks commented on Oct 8, 2006

      luksmannequin
      MannequinAuthor

      There is a few system I/O calls in the mmap module that
      can take some time. Would be be possible to put
      Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS around
      these to release the GIL and allow other Python threads
      to do their work?

    2. added
      type-featureA feature request or enhancement
      stdlibStandard Library Python modules in the Lib/ directory
      on Oct 8, 2006
    3. josiahcarlson commented on Oct 16, 2006

      josiahcarlsonmannequin
      Mannequin

      Logged In: YES
      user_id=341410

      Can you come up with the listing of code blocks where it
      would make sense to allow threads, or even provide a patch?

    4. pitrou commented on Mar 30, 2009

      @pitrou
      Member

      I don't know mmap myself but patches are welcome.

    5. added
      performancePerformance or resource usage
      and removed
      type-featureA feature request or enhancement
      on Mar 30, 2009
    6. neologix commented on Apr 6, 2010

      neologixmannequin
      Mannequin

      As soon as you're dealing with files (not anonymous mapping), you can get the same type of latency than when using open/read/write...
      While it's probably not worth the trouble to release the GIL for every operation involving mmaped-files, there are a couple places where it should be considered, for example:

      Modules/mmapmodule.c:new_mmap_object
      #ifdef HAVE_FSTAT
      #  ifdef __VMS
      	/* on OpenVMS we must ensure that all bytes are written to the file */
      	if (fd != -1) {
      	        fsync(fd);
      	}
      #  endif
      

      fsync() can take up to a couple seconds, so we should probably allow threads here (this is done only on VMS).
      Another place worth looking at is when using msync(), like in mmap_object_dealloc():

      	if (m_obj->data!=NULL) {
      		msync(m_obj->data, m_obj->size, MS_SYNC);
      		munmap(m_obj->data, m_obj->size);
      	}
      

      or in mmap_flush_method():

      	if (-1 == msync(self->data + offset, size, MS_SYNC)) {
      		PyErr_SetFromErrno(mmap_module_error);
      		return NULL;
      	}
      

      msycn too can take quite a long time to complete.

      I can write a patch if someone's willing to review it.

    7. pitrou commented on Apr 6, 2010

      @pitrou
      Member

      This shouldn't really care about VMS here. As for the rest, feel free to propose a patch :)
      Please note that most non-trivial system calls, such as ftruncate(), mremap(), even mmap() itself, deserve to be enclosed in Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS pairs.

      However, you'll have to be careful that the internal state of the mmap object remains consistent, such that using it from several threads doesn't crash the interpreter.
      (you can even add multi-threaded unit tests if you want to be sure of this)

    8. pitrou commented on Apr 6, 2010

      @pitrou
      Member

      This shouldn't really care about VMS here.

      Correction: /we/ shouldn't really care about VMS here.
      And the reason being, of course, that we have neither developers nor
      known testers under VMS.
      (we don't even know if the current trunk builds there or not)

    9. isidentical commented on May 15, 2019

      @isidentical
      SponsorMember

      Any news? If a patch is not ready, i can work on a patch too.

    10. 4 remaining items

    11. added and removed
      stdlibStandard Library Python modules in the Lib/ directory
      on Jun 15, 2019
    12. transferred this issue fromon Apr 10, 2022
    13. gvanrossum commented on Sep 3, 2022

      @gvanrossum
      Member

      This seems stuck, and it’s really old (2006!).

      @isidentical Could I interest you in providing that patch you mentioned?

    14. added
      pendingThe issue will be closed if no feedback is provided
      on Sep 3, 2022
    15. added
      3.12only security fixes
      and removed on Sep 6, 2022
    16. added 2 commits that reference this issue on Oct 10, 2022
    17. removed
      pendingThe issue will be closed if no feedback is provided
      on Oct 10, 2022
    18. gvanrossum commented on Oct 10, 2022

      @gvanrossum
      Member

      Once the Unix PR lands, should we keep this open hoping someone will do it for Windows, or close it?

    19. added a commit that references this issue on Oct 10, 2022
    20. hauntsaninja commented on Dec 24, 2022

      @hauntsaninja
      Contributor

      The Unix PR has landed and I don't intend to make one for Windows (don't have access to a machine or a use case). This has been open since 2006, so I vote we close it.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.12only security fixesextension-modulesC modules in the Modules dirperformancePerformance or resource usage

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions