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

asyncio: break some cycles #67398

Closed
Martiusweb opened this issue Jan 9, 2015 · 11 comments
Closed

asyncio: break some cycles #67398

Martiusweb opened this issue Jan 9, 2015 · 11 comments
Labels
performance Performance or resource usage topic-asyncio

Comments

@Martiusweb
Copy link
Member

BPO 23209
Nosy @gvanrossum, @vstinner, @1st1, @Martiusweb
Files
  • break-some-cycles.diff
  • break-selector-map-cycle.diff
  • 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 2015-01-13.09:07:44.146>
    created_at = <Date 2015-01-09.17:16:15.188>
    labels = ['expert-asyncio', 'performance']
    title = 'asyncio: break some cycles'
    updated_at = <Date 2015-01-15.08:24:41.377>
    user = 'https://github.com/Martiusweb'

    bugs.python.org fields:

    activity = <Date 2015-01-15.08:24:41.377>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-01-13.09:07:44.146>
    closer = 'vstinner'
    components = ['asyncio']
    creation = <Date 2015-01-09.17:16:15.188>
    creator = 'martius'
    dependencies = []
    files = ['37657', '37681']
    hgrepos = []
    issue_num = 23209
    keywords = ['patch']
    message_count = 11.0
    messages = ['233770', '233772', '233781', '233782', '233783', '233784', '233880', '233882', '233909', '233913', '234063']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'vstinner', 'python-dev', 'yselivanov', 'martius']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue23209'
    versions = ['Python 3.4']

    @Martiusweb
    Copy link
    Member Author

    Hi,

    I would like to submit 3 trivial modifications which break a cycle each. It is not much, but those three cycles caused a lot of objects to be garbage collected. They can now be freed using the reference counting mechanism, and therefore, reduce the latency that may be involved by the work of the garbage collector in a long living process.

    In asyncio/base_subprocess.py:
    WriteSubprocessPipeProto.proc is a reference to a BaseSubprocessTransport object, which holds a reference to the WriteSubprocessPipeProto in self._protocol.

    I break the cycle in the protocol at the end of connection_lost().

    In asyncio/futures.py:
    wrap_future() defines a lambda which uses a variable defined in the function, therefore creating a closure, referencing the wrap_future() function and creating a cycle.

    In the (really trivial) patch, the lambda uses the argument "future" instead of the "fut" variable defined in a closure. The closure is not needed anymore.

    This single cycle is very common, because caused when one uses getaddrinfo().

    In asyncio/selectors.py:
    _BaseSelectorImpl._map keeps a reference to the _SelectorMapping object, which also references the selector with _SelectorMapping._selector.

    The reference to the map in the selector is cleared once the selector is closed.

    @Martiusweb Martiusweb added topic-asyncio performance Performance or resource usage labels Jan 9, 2015
    @gvanrossum
    Copy link
    Member

    All three changes look good to me. The selectors.py fix should be applied to CPython first; the others to Tulip first.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 9, 2015

    New changeset 376c5398f28d by Victor Stinner in branch '3.4':
    Issue bpo-23209: Break some reference cycles in asyncio. Patch written by Martin
    https://hg.python.org/cpython/rev/376c5398f28d

    @vstinner
    Copy link
    Member

    vstinner commented Jan 9, 2015

    Hi Martin, thanks for the patch. It looks good to me. I applied it to Tulip, Python 3.4 and Python 3.5.

    @vstinner vstinner closed this as completed Jan 9, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 9, 2015

    New changeset 7438f2e30908 by Victor Stinner in branch '3.4':
    Issue bpo-23209: Revert change on selectors, test_selectors failed.
    https://hg.python.org/cpython/rev/7438f2e30908

    New changeset 27cbc877447b by Victor Stinner in branch 'default':
    (Merge 3.4) Issue bpo-23209: Revert change on selectors, test_selectors failed.
    https://hg.python.org/cpython/rev/27cbc877447b

    @vstinner
    Copy link
    Member

    vstinner commented Jan 9, 2015

    Ooops, test_selectors fails because get_key() raises "TypeError: 'NoneType' object is not subscriptable" when the selector is closed.

    A different fix should be written.

    I'm now using tox to run the Tulip test suite, I'm surprised that I didn't notice the failure. It looks like test_selectors is not executed. runtests.py searchs for test classes with a name ending with "Tests". I will fix that.

    @vstinner vstinner reopened this Jan 9, 2015
    @Martiusweb
    Copy link
    Member Author

    I updated the selector patch so BaseSelector.get_key() raises KeyError if the mapping is None. All the (non skipped) tests in test_selectors.py passed.

    Anyway, if there is an other problem with freeing the mapping object (I don't know, maybe "reopening" a loop may be considered?) this patch can probably be dropped. Since this cycle is broken when the loop is closed, the objects will likely be collected once the program terminates.

    @vstinner
    Copy link
    Member

    I opened the issue bpo-23225 "selectors: raise an exception if the selector is closed" which is a different approach (but it should also fix the reference cycle, I kept the "self._map = None" change).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 13, 2015

    New changeset 1544bdc409be by Victor Stinner in branch '3.4':
    Issue bpo-23209, bpo-23225: selectors.BaseSelector.close() now clears its internal
    https://hg.python.org/cpython/rev/1544bdc409be

    New changeset 6e7403bc906f by Victor Stinner in branch 'default':
    Issue bpo-23209, bpo-23225: selectors.BaseSelector.get_key() now raises a
    https://hg.python.org/cpython/rev/6e7403bc906f

    @vstinner
    Copy link
    Member

    I closed the issue bpo-23225, so I'm also closing this issue. Thanks again Martin.

    @vstinner
    Copy link
    Member

    I noticed that _ProactorBasePipeTransport doesn't clear its reference to the socket when it is closed. I changed this in the following commit (now merged in Python 3.4 & 3.5):
    https://code.google.com/p/tulip/source/detail?r=61ce7def97272ab1a6488545dc392160c2fbe316

    @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
    performance Performance or resource usage topic-asyncio
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants