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.

Sign up for GitHub

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

Jump to bottom

asyncio: Optimize loop.call_soon #72786

Closed
1st1 opened this issue Nov 3, 2016 · 12 comments
Closed

asyncio: Optimize loop.call_soon #72786

1st1 opened this issue Nov 3, 2016 · 12 comments
Assignees
@1st1
Labels
3.7 (EOL) end of life performance Performance or resource usage topic-asyncio

Comments

@1st1
Copy link
Member

1st1 commented Nov 3, 2016

BPO 28600
Nosy @gvanrossum, @vstinner, @ned-deily, @asvetlov, @methane, @socketpair, @1st1
Files
  • call_soon.patch
  • call_soon2.patch: moved another isinstance check from Handle to _check_callback
  • call_soon3.patch: address Guido's review
    		•

    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/1st1'
closed_at = <Date 2016-11-03.22:12:50.207>
created_at = <Date 2016-11-03.18:29:01.566>
labels = ['expert-asyncio', '3.7', 'performance']
title = 'asyncio: Optimize loop.call_soon'
updated_at = <Date 2017-07-28.03:34:23.426>
user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2017-07-28.03:34:23.426>
actor = 'chris.jerdonek'
assignee = 'yselivanov'
closed = True
closed_date = <Date 2016-11-03.22:12:50.207>
closer = 'yselivanov'
components = ['asyncio']
creation = <Date 2016-11-03.18:29:01.566>
creator = 'yselivanov'
dependencies = []
files = ['45339', '45340', '45341']
hgrepos = []
issue_num = 28600
keywords = ['patch']
message_count = 12.0
messages = ['280002', '280004', '280006', '280013', '280016', '280017', '280018', '280019', '280026', '280027', '280030', '280031']
nosy_count = 8.0
nosy_names = ['gvanrossum', 'vstinner', 'ned.deily', 'asvetlov', 'methane', 'socketpair', 'python-dev', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue28600'
versions = ['Python 3.6', 'Python 3.7']
    @1st1
    Copy link
    Member Author

    1st1 commented Nov 3, 2016

    loop.call_soon is the central function of asyncio. Everything goes through it.

    Current design of asyncio.loop.call_soon makes the following checks:

    1. [debug mode] check that the loop is not closed
    2. [debug mode] check that we are calling call_soon from the proper thread
    3. [always] check that callback is not a coroutine or a coroutine function

    Check #3 is very expensive, because it uses an 'isinstance' check. Moreover, isinstance checks against an ABC, which makes the call even slower.

    Attached patch makes check #3 optional and run only in debug mode. This is a very safe patch to merge.

    This makes asyncio another 17% (sic!) faster. In fact it becomes as fast as curio for realistic streams benchmarks.

    @1st1 1st1 added the 3.7 (EOL) end of life label Nov 3, 2016
    @1st1 1st1 self-assigned this Nov 3, 2016
    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 3, 2016

    The patch looks good.
    IIRC haypo added the check because people called .call_later() with coroutine instead of callback very often.

    But checking in debug mode looks very reasonable to me if it is so expensive.

    @1st1
    Copy link
    Member Author

    1st1 commented Nov 3, 2016

    IIRC haypo added the check because people called .call_later() with coroutine instead of callback very often.

    We'll update asyncio docs in 3.6 with a tutorial to focus on coroutines (not on low-level event loop). This should leave the loop API to advanced users.

    But checking in debug mode looks very reasonable to me if it is so expensive.

    Exactly.

    @vstinner
    Copy link
    Member

    vstinner commented Nov 3, 2016

    I didn't review the patch, but I agree with the overall approach: expensive
    checks can be made only in debug mode.

    If people are concerned by the removal of the check by default, we should
    repeat everywhere in the doc that async programming is hard and that the
    debug mode saves a lot of time ;-)

    @gvanrossum
    Copy link
    Member

    Patch 3 LGTM.

    @vstinner
    Copy link
    Member

    vstinner commented Nov 3, 2016

    call_soon3.patch: LGTM. It enhances error messages (fix the method name) and should make asyncio faster.

    @socketpair
    Copy link
    Mannequin

    socketpair mannequin commented Nov 3, 2016

    haypo added the check because people called .call_later() with coroutine instead of callback very often

    maybe make dirty hack and check hasattr(callback, 'send') ?

    @1st1
    Copy link
    Member Author

    1st1 commented Nov 3, 2016

    LGTM

    Will commit this soon.

    maybe make dirty hack and check hasattr(callback, 'send') ?

    If you schedule a coroutine object it will fail anyways, because it's not callable.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 3, 2016

    New changeset 128ffe3c3eb9 by Yury Selivanov in branch '3.5':
    Issue bpo-28600: Optimize loop.call_soon().
    https://hg.python.org/cpython/rev/128ffe3c3eb9

    New changeset 4f570a612aec by Yury Selivanov in branch '3.6':
    Merge 3.5 (issue bpo-28600)
    https://hg.python.org/cpython/rev/4f570a612aec

    New changeset 46c3eede41a6 by Yury Selivanov in branch 'default':
    Merge 3.6 (issue bpo-28600)
    https://hg.python.org/cpython/rev/46c3eede41a6

    @1st1
    Copy link
    Member Author

    1st1 commented Nov 3, 2016

    Guido, Andrew, thanks for reviews!

    I've fixed some unittests before committing the patch.

    @1st1 1st1 closed this as completed Nov 3, 2016
    @1st1 1st1 added the performance Performance or resource usage label Nov 3, 2016
    @gvanrossum
    Copy link
    Member

    PS. I noticed there are a few lines different between the "upstream" repo
    and the 3.5 stdlib:

    + # Wake up the loop if the finalizer was called from
    + # a different thread.
    + self._write_to_self()

    On Thu, Nov 3, 2016 at 3:12 PM, Yury Selivanov <report@bugs.python.org>
    wrote:

    Changes by Yury Selivanov <yselivanov@gmail.com>:

    ----------
    resolution: -> fixed
    stage: commit review -> resolved
    status: open -> closed
    type: -> performance

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

    @1st1
    Copy link
    Member Author

    1st1 commented Nov 3, 2016

    + # Wake up the loop if the finalizer was called from
    + # a different thread.
    + self._write_to_self()

    Yeah, looks like shutdown_asyncgens somehow ended up in 3.5 branch (there's no harm in it being there). I'll sync the branches.

    @cjerdonek cjerdonek changed the title asyncio: Optimize loop.call_soon shutdown_asyncgens Jul 28, 2017
    @cjerdonek cjerdonek changed the title shutdown_asyncgens asyncio: Optimize loop.call_soon Jul 28, 2017
    @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
    Assignees

    @1st1 1st1

    Labels
    3.7 (EOL) end of life performance Performance or resource usage topic-asyncio
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @1st1 @asvetlov @gvanrossum