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

platform.win32_ver() leaks in 2.7.12 #72119

Closed
OkkoWilleboordse mannequin opened this issue Sep 1, 2016 · 13 comments
Closed

platform.win32_ver() leaks in 2.7.12 #72119

OkkoWilleboordse mannequin opened this issue Sep 1, 2016 · 13 comments
Assignees
Labels
3.7 OS-windows performance Performance or resource usage

Comments

@OkkoWilleboordse
Copy link
Mannequin

OkkoWilleboordse mannequin commented Sep 1, 2016

BPO 27932
Nosy @pfmoore, @vstinner, @tjguk, @zware, @eryksun, @zooba
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Files
  • leaked_objects.txt: List of leaked objects
  • 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 = 'https://github.com/zooba'
    closed_at = <Date 2016-09-21.16:08:56.448>
    created_at = <Date 2016-09-01.15:34:31.099>
    labels = ['3.7', 'OS-windows', 'performance']
    title = 'platform.win32_ver() leaks in 2.7.12'
    updated_at = <Date 2017-03-31.16:36:14.564>
    user = 'https://bugs.python.org/OkkoWilleboordse'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:14.564>
    actor = 'dstufft'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2016-09-21.16:08:56.448>
    closer = 'steve.dower'
    components = ['Windows']
    creation = <Date 2016-09-01.15:34:31.099>
    creator = 'Okko.Willeboordse'
    dependencies = []
    files = ['44324']
    hgrepos = []
    issue_num = 27932
    keywords = []
    message_count = 13.0
    messages = ['274144', '274145', '274146', '275520', '275521', '275672', '275673', '275715', '275744', '275753', '276859', '276861', '276862']
    nosy_count = 8.0
    nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'python-dev', 'Okko.Willeboordse', 'zach.ware', 'eryksun', 'steve.dower']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue27932'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

    @OkkoWilleboordse
    Copy link
    Mannequin Author

    OkkoWilleboordse mannequin commented Sep 1, 2016

    Running;
    Python 2.7.12 (v2.7.12:d33e0cf91556, Jun 27 2016, 15:19:22) [MSC v.1500 32 bit (Intel)] on win32

    platform.win32_ver() returns;
    ('10', '10.0.10586', '', u'Multiprocessor Free')

    @OkkoWilleboordse OkkoWilleboordse mannequin added OS-windows performance Performance or resource usage labels Sep 1, 2016
    @vstinner
    Copy link
    Member

    vstinner commented Sep 1, 2016

    Hum, can you please elaborate the issue? What is leaked_objects.txt?

    @OkkoWilleboordse
    Copy link
    Mannequin Author

    OkkoWilleboordse mannequin commented Sep 1, 2016

    I did a gc.get_objects() before and after platform.win32_ver() and printed
    objects that were not present before calling platform.win32_ver().

    If I run platform.win32_ver() in a loop I see the memory increasing.

    On 1 September 2016 at 17:35, STINNER Victor <report@bugs.python.org> wrote:

    STINNER Victor added the comment:

    Hum, can you please elaborate the issue? What is leaked_objects.txt?

    ----------
    nosy: +haypo


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue27932\>


    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 10, 2016

    New changeset 384c178cf823 by Steve Dower in branch '3.5':
    Issue bpo-27932: Fixes memory leak in platform.win32_ver()
    https://hg.python.org/cpython/rev/384c178cf823

    New changeset 31b7eaff5588 by Steve Dower in branch 'default':
    Issue bpo-27932: Fixes memory leak in platform.win32_ver()
    https://hg.python.org/cpython/rev/31b7eaff5588

    @zooba
    Copy link
    Member

    zooba commented Sep 10, 2016

    I fixed in 3.5 and 3.6, but I'm not set up for 2.7 so someone will have to backport it.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 10, 2016

    New changeset dbfcb3b9c3c1 by Steve Dower in branch '3.5':
    Issue bpo-27932: Backs out change
    https://hg.python.org/cpython/rev/dbfcb3b9c3c1

    @zooba
    Copy link
    Member

    zooba commented Sep 10, 2016

    Backed out because of bpo-28059. I'll rewrite this to properly use native code.

    @zooba zooba self-assigned this Sep 10, 2016
    @eryksun
    Copy link
    Contributor

    eryksun commented Sep 10, 2016

    The leak is due the pointer-type cache that ctypes.POINTER uses. The type the pointer refers to is used as the key. In this case, VS_FIXEDFILEINFO is created each time win32_ver is called, so the pointer-type cache grows without bound. Example leak:

        >>> import psutil, platform
        >>> proc = psutil.Process()
        >>> proc.memory_info().rss
        14704640
        >>> for i in range(10000):
        ...     v = platform.win32_ver()
        ...
        >>> proc.memory_info().rss
        92704768
        >>> for i in range(10000):
        ...     v = platform.win32_ver()
        ...
        >>> proc.memory_info().rss
        168861696

    Clearing the cache followed by a collect() reclaims the leaked memory for the most part:

        >>> import gc, ctypes
        >>> gc.collect()
        333
        >>> proc.memory_info().rss
        168849408
        >>> ctypes._pointer_type_cache.clear()
        >>> gc.collect()
        740000
        >>> proc.memory_info().rss
        20303872

    It's a moot point, since Steve plans to re-implement this check in C, but the minimal change to fix this leak is to bypass the pointer-type cache by manually subclassing ctypes._Pointer:

        class PVS_FIXEDFILEINFO(_Pointer):
            _type_ = VS_FIXEDFILEINFO
    
        pvi = PVS_FIXEDFILEINFO()

    There's no more leak after this change:

        >>> import psutil, platform
        >>> proc = psutil.Process()
        >>> proc.memory_info().rss
        15450112
        >>> for i in range(10000):
        ...     v = platform.win32_ver()
        ...
        >>> proc.memory_info().rss
        16592896
        >>> for i in range(10000):
        ...     v = platform.win32_ver()
        ...
        >>> proc.memory_info().rss
        16601088

    @vstinner
    Copy link
    Member

    vstinner commented Sep 11, 2016

    pointer-type cache grows without bound

    Maybe we should also fix ctypes to use an LRU cache like
    @functools.lru_cache? A cache without limit doesn't seem like a good
    idea.

    @eryksun
    Copy link
    Contributor

    eryksun commented Sep 11, 2016

    Limiting the pointer-type cache could be a problem. It's expected that POINTER() returns a reference to an existing pointer type. ctypes uses Python types to ensure compatible C data types. For example:

        LP_c_int = ctypes.POINTER(ctypes.c_int)
    
        class LP_c_int_uncached(ctypes._Pointer):
            _type_ = ctypes.c_int
        >>> arr = (LP_c_int * 2)()
        >>> ctypes.POINTER(ctypes.c_int) is LP_c_int
        True
        >>> arr[0] = ctypes.POINTER(ctypes.c_int)()
        >>> arr[1] = LP_c_int_uncached()
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        TypeError: incompatible types, LP_c_int_uncached instance
        instead of LP_c_int instance

    The docs could warn users that the cache used by ctypes.POINTER(), and implicitly by ctypes.pointer(), will leak memory if it's called with a type that's created at function scope instead of at module or class-body scope.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 17, 2016

    New changeset 9b0d661a16de by Steve Dower in branch '2.7':
    Issue bpo-27932: Prevent memory leak in win32_ver().
    https://hg.python.org/cpython/rev/9b0d661a16de

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 18, 2016

    New changeset db4b254f5df4 by Steve Dower in branch '3.5':
    Issue bpo-27932: Prevent memory leak in win32_ver().
    https://hg.python.org/cpython/rev/db4b254f5df4

    New changeset 14fca2c8eb36 by Steve Dower in branch '3.6':
    Issue bpo-27932: Prevent memory leak in win32_ver().
    https://hg.python.org/cpython/rev/14fca2c8eb36

    New changeset 19ceb5034fe5 by Steve Dower in branch 'default':
    Issue bpo-27932: Prevent memory leak in win32_ver().
    https://hg.python.org/cpython/rev/19ceb5034fe5

    @zooba
    Copy link
    Member

    zooba commented Sep 18, 2016

    Phew. Doing that in a reasonable way for all the active versions was a pain, but I think I managed it.

    In short:

    • 2.7 gets the fix proposed by Eryk Sun
    • 3.5 gets a private sys.getwindowsversion()._platform_version member
    • 3.6+ gets a public/documented sys.getwindowsversion().platform_version member

    AFAICT the implementation is solid, but please shout if you spot anything weird/wrong.

    @zooba zooba added the 3.7 label Sep 18, 2016
    @zooba zooba closed this as completed Sep 21, 2016
    @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 OS-windows performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants