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

Implement asyncio.Task in C #72730

Closed
1st1 opened this issue Oct 27, 2016 · 25 comments
Closed

Implement asyncio.Task in C #72730

1st1 opened this issue Oct 27, 2016 · 25 comments
Assignees
Labels
3.7 expert-asyncio performance Performance or resource usage

Comments

@1st1
Copy link
Member

1st1 commented Oct 27, 2016

BPO 28544
Nosy @gvanrossum, @brettcannon, @vstinner, @ned-deily, @asvetlov, @methane, @elprans, @1st1
Files
  • ctask.patch
  • ctask2.patch
  • ctask3.patch
  • 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-10-29.02:17:28.753>
    created_at = <Date 2016-10-27.16:33:11.278>
    labels = ['expert-asyncio', '3.7', 'performance']
    title = 'Implement asyncio.Task in C'
    updated_at = <Date 2016-10-31.17:00:26.349>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2016-10-31.17:00:26.349>
    actor = 'brett.cannon'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2016-10-29.02:17:28.753>
    closer = 'yselivanov'
    components = ['asyncio']
    creation = <Date 2016-10-27.16:33:11.278>
    creator = 'yselivanov'
    dependencies = []
    files = ['45242', '45245', '45248']
    hgrepos = []
    issue_num = 28544
    keywords = ['patch']
    message_count = 25.0
    messages = ['279546', '279549', '279553', '279554', '279560', '279564', '279568', '279569', '279570', '279577', '279578', '279579', '279580', '279613', '279614', '279618', '279629', '279630', '279632', '279635', '279636', '279638', '279645', '279657', '279805']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'brett.cannon', 'vstinner', 'ned.deily', 'asvetlov', 'methane', 'Elvis.Pranskevichus', 'python-dev', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue28544'
    versions = ['Python 3.6', 'Python 3.7']

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 27, 2016

    The attached patch implements asyncio.Task in C. Besides that, it also implements Argument Clinic for C Future.

    Performance improvement on a simple echo server implementation using asyncio.streams:

    Python Future & Task | C Future & Py Task | C Future & C Task
    23K req/s | 26K | 30K
    | ~10-15% boost | ~15%

    Both Task and Future implemented in C, make asyncio programs up to 25-30% faster.

    The patch is 100% backwards compatible. I modified asyncio tests to cross test Tasks and Futures implementations, i.e. to run Task+Future, Task+CFuture, CTask+Future, CTask+CFuture tests. No refleaks or other bugs. All uvloop functional tests pass without any problem.

    Ned, Guido, are you OK if I merge this in 3.6 before beta 3? I'm confident that the patch is stable and even if something comes up we have time to fix it or even retract the patch. The performance boost is very impressive, and I can also make uvloop simpler.

    @1st1 1st1 added the 3.7 label Oct 27, 2016
    @1st1 1st1 self-assigned this Oct 27, 2016
    @1st1 1st1 added expert-asyncio performance Performance or resource usage labels Oct 27, 2016
    @1st1
    Copy link
    Member Author

    1st1 commented Oct 27, 2016

    Also, with this patch uvloop becomes ~3-5% faster too.

    @methane
    Copy link
    Member

    methane commented Oct 27, 2016

    Wow! Great Job!
    Yury, would you like to merge this before 3.6b3?
    I'll look this as soon as possible.

    (nit fix) Some modules doesn't sort imports in lexicography.

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 27, 2016

    Yury, would you like to merge this before 3.6b3?

    Yes!

    I'll look this as soon as possible.

    Thanks a lot!

    @ned-deily
    Copy link
    Member

    ned-deily commented Oct 27, 2016

    If it's OK with Guido, it's OK with me for 360b3. (That's Monday.)

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 27, 2016

    Very impressive.
    I've left a couple comments in rietveld though.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Oct 27, 2016

    I don't want to be the decider here. I don't have time to review the
    code. I trust you all to do the right thing.

    @ned-deily
    Copy link
    Member

    ned-deily commented Oct 27, 2016

    I also trust Yury to do the right thing.

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 27, 2016

    Guido, Ned, thanks! Andrew already glanced through the code, let's see what Inada-san says.

    I'm uploading an updated patch addressing Andrew's review.

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 28, 2016

    Uploading a new patch: sorted imports; added a unittest that exceptions in loop.call_soon aren't breaking Task.__init__.

    @methane
    Copy link
    Member

    methane commented Oct 28, 2016

    Why isfuture() is moved, and asyncio.coroutine uses
    base_futures.isfuture() instead of futures.isfuture()?

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 28, 2016

    Why isfuture() is moved, and asyncio.coroutine uses
    base_futures.isfuture() instead of futures.isfuture()?

    Import cycles:

    • _asyncio module is now being imported from futures.py
    • and _asyncio is now imported from tasks.py;
    • and tasks.py imports futures.py to have isfuture.

    Long story short, I don't see a way of keeping isfuture in futures.py. The easiest option is to have it in a separate module.

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 28, 2016

    • and tasks.py imports futures.py to have isfuture.

    And tasks.py also imports coroutines.py (which was importing futures.py), making the cycle even worse. Anyways, I don't see a problem in moving a function or two that everybody uses into a separate module.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 28, 2016

    New changeset db5ae4f6df8a by Yury Selivanov in branch '3.6':
    Issue bpo-28544: Implement asyncio.Task in C.
    https://hg.python.org/cpython/rev/db5ae4f6df8a

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

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 28, 2016

    I've committed the patch with a few touchups:

    • Applied code-review feedback.

    • We now use AC for all methods, including Task._repr_info, Task._step, etc. See [1].

    • I fixed the Task to be fully subclassable; users can override Task._step and Task._wakeup now. There are tests for ensuring that subclassing works the same for both Python and C implementations. See [2] and [3].

    • I made it possible to override Future._schedule_subclass, as it was in Python 3.5. See [4].

    Andrew, Inada-san, thank you for reviewing the patch! Please take a look at the patch updates referenced below. Feel free to re-open the issue if you see anything suspicious.

    [1] 1st1@99adc35

    [2] 1st1@e294491

    [3] 1st1@4d1c50c

    [4] 1st1@efec03c

    @1st1 1st1 closed this as completed Oct 28, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 28, 2016

    New changeset fb7c439103b9 by Victor Stinner in branch '3.6':
    Issue bpo-28544: Fix _asynciomodule.c on Windows
    https://hg.python.org/cpython/rev/fb7c439103b9

    @vstinner
    Copy link
    Member

    vstinner commented Oct 28, 2016

    Since asyncio becomes popular, IMO it is worth to mention this optimization at:
    https://docs.python.org/dev/whatsnew/3.6.html#optimizations

    Can you please do it Yury?

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 28, 2016

    Can you please do it Yury?

    Yes, Elvis and I will be the editors of What's New this year anyways ;)

    @vstinner
    Copy link
    Member

    vstinner commented Oct 28, 2016

    Python 3.6 & default don't compile on Windows because of the change:

    http://buildbot.python.org/all/builders/AMD64%20Windows8%203.x/builds/2795/steps/compile/logs/stdio

       "D:\buildarea\3.x.bolen-windows8\build\PCbuild\_asyncio.vcxproj" (Build target) (15) ->
       (Link target) -> 
         _asynciomodule.obj : error LNK2019: unresolved external symbol _PyDict_Pop referenced in function task_step [D:\buildarea\3.x.bolen-windows8\build\PCbuild\_asyncio.vcxproj]
         _asynciomodule.obj : error LNK2019: unresolved external symbol _PyGen_Send referenced in function task_step_impl [D:\buildarea\3.x.bolen-windows8\build\PCbuild\_asyncio.vcxproj]
         D:\buildarea\3.x.bolen-windows8\build\PCBuild\amd64\_asyncio_d.pyd : fatal error LNK1120: 2 unresolved externals [D:\buildarea\3.x.bolen-windows8\build\PCbuild\_asyncio.vcxproj]
    

    @vstinner vstinner reopened this Oct 28, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 28, 2016

    New changeset 635f5854cb3e by Yury Selivanov in branch '3.6':
    Issue bpo-28544: Fix compilation of _asynciomodule.c on Windows
    https://hg.python.org/cpython/rev/635f5854cb3e

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

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 28, 2016

    Python 3.6 & default don't compile on Windows because of the change:

    Pushed a fix for that.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 28, 2016

    New changeset db7bcd92cf85 by Yury Selivanov in branch '3.6':
    Issue bpo-28544: Pass PyObject* to _PyDict_Pop, not PyDictObject*
    https://hg.python.org/cpython/rev/db7bcd92cf85

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

    @1st1
    Copy link
    Member Author

    1st1 commented Oct 29, 2016

    Looks like that Windows buildbot is green now. Closing.

    @1st1 1st1 closed this as completed Oct 29, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 29, 2016

    New changeset 950fbd75223b by Victor Stinner in branch '3.6':
    Issue bpo-28544: Fix inefficient call to _PyObject_CallMethodId()
    https://hg.python.org/cpython/rev/950fbd75223b

    @brettcannon
    Copy link
    Member

    brettcannon commented Oct 31, 2016

    Just an FYI, for testing code that has both pure Python and C versions you can follow the advice in https://www.python.org/dev/peps/pep-0399/#details . And if you want to really simplify things you start down the road of what test_importlib uses: https://github.com/python/cpython/blob/master/Lib/test/test_importlib/util.py#L81 .

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

    No branches or pull requests

    7 participants