classification
Title: Implement asyncio.Task in C
Type: performance Stage: resolved
Components: asyncio Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: yselivanov Nosy List: Elvis.Pranskevichus, asvetlov, brett.cannon, gvanrossum, haypo, inada.naoki, ned.deily, python-dev, yselivanov
Priority: normal Keywords: patch

Created on 2016-10-27 16:33 by yselivanov, last changed 2016-10-31 17:00 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
ctask.patch yselivanov, 2016-10-27 16:32 review
ctask2.patch yselivanov, 2016-10-27 21:16 review
ctask3.patch yselivanov, 2016-10-28 04:14 review
Messages (25)
msg279546 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-27 16:32
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.
msg279549 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-27 16:46
Also, with this patch uvloop becomes ~3-5% faster too.
msg279553 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-27 17:38
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.
msg279554 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-27 17:40
> Yury, would you like to merge this before 3.6b3?

Yes!

> I'll look this as soon as possible.

Thanks a lot!
msg279560 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-10-27 18:35
If it's OK with Guido, it's OK with me for 360b3. (That's Monday.)
msg279564 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2016-10-27 19:18
Very impressive.
I've left a couple comments in rietveld though.
msg279568 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-10-27 20:39
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.
msg279569 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-10-27 20:48
I also trust Yury to do the right thing.
msg279570 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-27 21:16
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.
msg279577 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-28 04:14
Uploading a new patch: sorted imports; added a unittest that exceptions in loop.call_soon aren't breaking Task.__init__.
msg279578 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-28 04:19
Why isfuture() is moved, and asyncio.coroutine uses
base_futures.isfuture() instead of futures.isfuture()?
msg279579 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-28 04:23
> 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.
msg279580 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-28 04:27
> - 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.
msg279613 - (view) Author: Roundup Robot (python-dev) Date: 2016-10-28 16:53
New changeset db5ae4f6df8a by Yury Selivanov in branch '3.6':
Issue #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 #28544)
https://hg.python.org/cpython/rev/8059289b55c1
msg279614 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-28 17:00
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] https://github.com/1st1/cpython/commit/99adc35c94d76d78b8e666381436016c3c74a5ab

[2] https://github.com/1st1/cpython/commit/e294491c4cc7c92bc94ff12b4796c8ab12c40435

[3] https://github.com/1st1/cpython/commit/4d1c50c8987af124ce0bcccaea6bde82a60b59ee

[4] https://github.com/1st1/cpython/commit/efec03cbb4f81e78defd989d72c9bdae1122f50d
msg279618 - (view) Author: Roundup Robot (python-dev) Date: 2016-10-28 17:17
New changeset fb7c439103b9 by Victor Stinner in branch '3.6':
Issue #28544: Fix _asynciomodule.c on Windows
https://hg.python.org/cpython/rev/fb7c439103b9
msg279629 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-10-28 21:38
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?
msg279630 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-28 21:41
> Can you please do it Yury?

Yes, Elvis and I will be the editors of What's New this year anyways ;)
msg279632 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-10-28 22:05
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]
msg279635 - (view) Author: Roundup Robot (python-dev) Date: 2016-10-28 22:49
New changeset 635f5854cb3e by Yury Selivanov in branch '3.6':
Issue #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 #28544)
https://hg.python.org/cpython/rev/9d95510dc203
msg279636 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-28 22:52
> Python 3.6 & default don't compile on Windows because of the change:

Pushed a fix for that.
msg279638 - (view) Author: Roundup Robot (python-dev) Date: 2016-10-28 23:01
New changeset db7bcd92cf85 by Yury Selivanov in branch '3.6':
Issue #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 #28544)
https://hg.python.org/cpython/rev/60b6e820abe5
msg279645 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-29 02:17
Looks like that Windows buildbot is green now. Closing.
msg279657 - (view) Author: Roundup Robot (python-dev) Date: 2016-10-29 07:12
New changeset 950fbd75223b by Victor Stinner in branch '3.6':
Issue #28544: Fix inefficient call to _PyObject_CallMethodId()
https://hg.python.org/cpython/rev/950fbd75223b
msg279805 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-10-31 17:00
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 .
History
Date User Action Args
2016-10-31 17:00:26brett.cannonsetnosy: + brett.cannon
messages: + msg279805
2016-10-29 07:12:38python-devsetmessages: + msg279657
2016-10-29 02:17:28yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg279645
2016-10-28 23:01:53python-devsetmessages: + msg279638
2016-10-28 22:52:23yselivanovsetmessages: + msg279636
2016-10-28 22:49:17python-devsetmessages: + msg279635
2016-10-28 22:05:35hayposetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg279632
2016-10-28 21:41:41yselivanovsetmessages: + msg279630
2016-10-28 21:38:42hayposetmessages: + msg279629
2016-10-28 17:17:23python-devsetmessages: + msg279618
2016-10-28 17:00:43yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg279614

stage: patch review -> resolved
2016-10-28 16:53:37python-devsetnosy: + python-dev
messages: + msg279613
2016-10-28 04:27:46yselivanovsetmessages: + msg279580
2016-10-28 04:23:10yselivanovsetmessages: + msg279579
2016-10-28 04:19:59inada.naokisetmessages: + msg279578
2016-10-28 04:14:11yselivanovsetfiles: + ctask3.patch

messages: + msg279577
2016-10-27 21:16:37yselivanovsetfiles: + ctask2.patch

messages: + msg279570
2016-10-27 20:48:07ned.deilysetmessages: + msg279569
2016-10-27 20:39:58gvanrossumsetmessages: + msg279568
2016-10-27 19:18:33asvetlovsetmessages: + msg279564
2016-10-27 18:35:16ned.deilysetmessages: + msg279560
2016-10-27 17:40:51yselivanovsetmessages: + msg279554
2016-10-27 17:38:58inada.naokisetmessages: + msg279553
2016-10-27 16:46:59yselivanovsetmessages: + msg279549
2016-10-27 16:35:54Elvis.Pranskevichussetnosy: + Elvis.Pranskevichus
2016-10-27 16:33:11yselivanovcreate