classification
Title: Implement asyncio Future in C to improve performance
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: RemiCardona, giampaolo.rodola, gvanrossum, haypo, inada.naoki, mpaolini, ned.deily, python-dev, serhiy.storchaka, yselivanov
Priority: normal Keywords: patch

Created on 2016-01-11 17:23 by yselivanov, last changed 2016-11-09 00:06 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
futures.patch yselivanov, 2016-01-11 17:23 review
futures.patch inada.naoki, 2016-07-10 04:28 bugfix and implement remove_done_callback review
futures.patch inada.naoki, 2016-08-12 05:29 review
fastfuture.patch inada.naoki, 2016-09-10 15:01 merge master review
fastfuture.patch inada.naoki, 2016-09-11 03:48 recreate patch review
fastfuture.patch inada.naoki, 2016-09-11 07:51 _blocking -> _asyncio_future_blocking review
fastfuture.patch inada.naoki, 2016-09-15 04:15 review
fastfuture2.patch inada.naoki, 2016-10-07 03:54 review
fastfuture3-wip.patch inada.naoki, 2016-10-07 14:07 review
fastfuture3.patch inada.naoki, 2016-10-08 12:29 review
fastfuture4.patch inada.naoki, 2016-10-08 20:36 review
Messages (50)
msg257984 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-01-11 17:23
Some info on this: https://github.com/python/asyncio/issues/282#issuecomment-155957235  Long story short, Future implemented in C can speedup some asyncio code up to 25%.

I'm attaching a patch with a WIP implementation.  There are some failing assertions deep in GC, which I need to track down.  'Future.remove_done_callback' still needs to be properly implemented.
msg269773 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-07-03 20:18
We should really try to get this into 3.6.

--Guido (mobile)
msg270037 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-07-09 12:27
I'm working on this.  Some bugs are fixed, but doesn't pass tests for now.
https://github.com/methane/cpython/pull/5
msg270055 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-07-09 17:05
Passing all tests now.
Yury, could you explain what the comment "This isn't a Future class; it's a BaseFuture" means?

Should it be "_futures.Future" or "_futures.BaseFuture"?
msg270068 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-07-10 03:27
Should I send pull request to github.com/python/asyncio?
Or should I post patch here?
msg270069 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-07-10 03:31
> I'm working on this.  Some bugs are fixed, but doesn't pass tests for now.

Thanks a lot!  I couldn't find time to finish this myself.  I can definitely help you and review the patch once it's ready.

> Yury, could you explain what the comment "This isn't a Future class; it's a BaseFuture" means?

Unfortunately I don't remember :(

> Should I send pull request to github.com/python/asyncio?
Or should I post patch here?

Please post it here. AFAIK we haven't yet transitioned to the GitHub.
msg270070 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-07-10 04:28
OK.  Here is current version.
msg270175 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-07-11 12:49
In my patch, test_asyncio runs against C version Future.

I saw how test_json tests against C version and pure Python version.
But test_asyncio is more larger than test_json.

Before working on it, could someone give me idea to run whole test_asyncio
with and without C version Future easily?

And, which is master repository of asyncio? github? or hg.python.org?
If github, can I send separated pull request changing test_asyncio after
this patch is merged?
msg270183 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-07-11 14:32
> Before working on it, could someone give me idea to run whole test_asyncio
with and without C version Future easily?

asyncio uses loop.create_future() to create sockets.  I'd suggest you to create two base test classes: one that monkeypatches loop.create_future to return pure python Future in its setUp method; an another, that makes create_future to return a C version of the Future.

The derive some unittests from those base classes (which will effectively double the number of tests).

> And, which is master repository of asyncio? github? or hg.python.org?
If github, can I send separated pull request changing test_asyncio after
this patch is merged?

The master repo for asyncio is github, but since the C version won't be a part of asyncio (it will be checked in only in CPython source tree), I think it's fine to continue the work here, on bugs.python.org.
msg270221 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-07-12 02:27
Thanks.  I'll working on test_asyncio in next few days.
msg270232 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-07-12 08:50
> asyncio uses loop.create_future() to create sockets.  I'd suggest you to create two base test classes: one that monkeypatches loop.create_future to return pure python Future in its setUp method; an another, that makes create_future to return a C version of the Future.

windows_events.py has some classes extends futures.Future.
Task extends Future.
There are some `isinstance(future, futures.Future)`.

So monkeypatching `baseevent.create_future` seems not enough.
I want a way to completely reload asyncio and test_asyncio packages with and without C future.
msg270924 - (view) Author: Marco Paolini (mpaolini) Date: 2016-07-21 13:43
THe guys developing uvloop say their implementation is already quite fast [1]. Is it worth integrating it?

[1] https://github.com/MagicStack/uvloop
msg270928 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-07-21 14:19
Yes. Most people will use vanilla asyncio anyways.
msg272242 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-08-09 12:57
Yury, could you review this before 3.6a4?
msg272245 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-08-09 13:33
> Yury, could you review this before 3.6a4?

Left a couple of comments; the important one -- Future.__await__ (and Future.__iter__) should always return a *new* instance of a generator-like object (tied to the Future object).
msg272262 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-09 19:26
See also a discussion on Python-Dev about rewriting contextlib.contextmanager in C: https://mail.python.org/pipermail/python-dev/2016-August/thread.html#145786 .

What parts of Future are performance critical? Maybe it is worth to implement in C only the most critical code.
msg272278 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-08-09 23:30
> What parts of Future are performance critical? Maybe it is worth to implement in C only the most critical code.

Basically everything.  Contrary to @contextmanager, Futures are the building blocks of asyncio, so instantiation + awaiting on them + setting results must be fast.

To cover instantiation, I want to add a freelist for Futures, so this basically requires them to be implemented in C (and it's not a lot of C code actually).
msg272285 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-08-10 00:57
I'd also think about implementing asyncio.Handle in C (with a freelist).
msg272327 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-08-10 11:57
> Left a couple of comments; the important one -- Future.__await__ (and Future.__iter__) should always return a *new* instance of a generator-like object (tied to the Future object).

Implementing full behavior of generator seems difficult to me.
I'll implement minimum implementation in next patch.
msg272347 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-08-10 17:28
> Implementing full behavior of generator seems difficult to me.
I'll implement minimum implementation in next patch.

Sure, but you have to implement send() and throw().
msg272498 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-08-12 05:29
Implemented FutureIter
msg273802 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-08-28 02:48
There are only two weeks until 3.6 beta.
Yury, could you review this again?

Or should I implement freelist before review?
Implementing freelist may be easy, but measuring the effect of freelist from
realistic application is not easy.
msg275658 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-10 17:17
The actual _futures module appears missing from your latest patch -- what's up with that?
msg275729 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-09-11 03:48
Oh, I'm sorry.
I usually working on git, and convert git diff to hg diff when posting patch.

I've used `patch -p1` instead of `hg import --no-edit` to apply git patch into hg workdir.
I wonder if Rietveld accepts git diff format...
msg275734 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-11 04:54
Thanks! I can't review the whole thing, but I patched it in and tried running the asyncio/examples/crawl.py example, like so:

$ ~/src/cpython36/python.exe examples/crawl.py xkcd.com -q
Exception RuntimeError('yield was used instead of yield from in task <Task pending coro=<Crawler.fetch() running at examples/crawl.py:778>> with <Future pending cb=[_chain_future.<locals>._call_check_cancel() at /Users/guido/src/cpython36/Lib/asyncio/futures.py:472]>',) for ('xkcd.com', 80)
ERROR:asyncio:Task exception was never retrieved
future: <Task finished coro=<Crawler.fetch() done, defined at examples/crawl.py:769> exception=RuntimeError('yield was used instead of yield from in task <Task pending coro=<Crawler.fetch() running at examples/crawl.py:778>> with <Future pending cb=[_chain_future.<locals>._call_check_cancel() at /Users/guido/src/cpython36/Lib/asyncio/futures.py:472]>',)>
Traceback (most recent call last):
  File "/Users/guido/src/cpython36/Lib/asyncio/tasks.py", line 241, in _step
    result = coro.throw(exc)
  File "examples/crawl.py", line 778, in fetch
    yield from fetcher.fetch()  # Fetcher gonna fetch.
  File "examples/crawl.py", line 507, in fetch
    yield from self.request.connect()
  File "examples/crawl.py", line 315, in connect
    self.port, self.ssl)
  File "examples/crawl.py", line 143, in get_connection
    ipaddrs = yield from self.loop.getaddrinfo(host, port)
RuntimeError: yield was used instead of yield from in task <Task pending coro=<Crawler.fetch() running at examples/crawl.py:778>> with <Future pending cb=[_chain_future.<locals>._call_check_cancel() at /Users/guido/src/cpython36/Lib/asyncio/futures.py:472]>
*** Report ***
http://xkcd.com no response object
Finished 0 urls in 0.041 secs (max_tasks=100) (0.000 urls/sec/task)
Todo: 0
Busy: 1
Done: 0
Date: Sat Sep 10 21:50:08 2016 local time
Traceback (most recent call last):
  File "examples/crawl.py", line 864, in <module>
    main()
  File "examples/crawl.py", line 852, in main
    loop.run_until_complete(crawler.crawl())  # Crawler gonna crawl.
  File "/Users/guido/src/cpython36/Lib/asyncio/base_events.py", line 438, in run_until_complete
    return future.result()
  File "/Users/guido/src/cpython36/Lib/asyncio/tasks.py", line 241, in _step
    result = coro.throw(exc)
  File "examples/crawl.py", line 766, in crawl
    yield from self.termination.wait()
  File "/Users/guido/src/cpython36/Lib/asyncio/locks.py", line 326, in wait
    yield from fut
RuntimeError: yield was used instead of yield from in task <Task pending coro=<Crawler.crawl() running at examples/crawl.py:766> cb=[_run_until_complete_cb() at /Users/guido/src/cpython36/Lib/asyncio/base_events.py:164]> with <Future pending>

Without your diff, that works, and the output includes this line:

Finished 1786 urls in 7.105 secs (max_tasks=100) (2.514 urls/sec/task)
msg275740 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-09-11 07:51
Sorry, again. fixed.
Now this passes `./python -m test.test_asyncio`
msg275903 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-12 00:54
Yury: What do you think of the code? How solid is it? (The issue I found was due to my own very recent changes to _blocking.)

Ned: Is it better to do this in 3.6b1 or to wait for 3.6b2?
msg275904 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-12 01:07
> Yury: What do you think of the code? How solid is it? (The issue I found was due to my own very recent changes to _blocking.)

The code looks fine, I can fix the remaining nits myself.  I've left a couple of comments in review.

> Ned: Is it better to do this in 3.6b1 or to wait for 3.6b2?

TBH it would be way more convenient if we could push this into b2.  I can push this on Tuesday without rushing things, and we'll have plenty of time to watch buildbots etc.
msg275905 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-12 01:08
Yeah, let's do this in 3.6b2.
msg275906 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2016-09-12 01:11
This change touches a lot of files and affect both the unix* and Windows build processes so, yeah, I think it's too risky to go in to b1. Let's get it in as soon as possible after b1.
msg276494 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-14 22:37
INADA, would you be able to address my last review comments?  Also, I'm wondering what if we could implement __del__ and __repr__ in C too, so that we could drop BaseFuture class?
msg276504 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-09-15 03:14
I'm working on fixing points you commented.  Wait a minute.

Implementing __del__ and __repr__ in C is bit hard task to me.
I can't do it in this week. (maybe I can't do it in this month too.)

On Thu, Sep 15, 2016 at 7:37 AM, Yury Selivanov <report@bugs.python.org> wrote:
>
> Yury Selivanov added the comment:
>
> INADA, would you be able to address my last review comments?  Also, I'm wondering what if we could implement __del__ and __repr__ in C too, so that we could drop BaseFuture class?
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue26081>
> _______________________________________
msg276505 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-09-15 03:15
> Implementing __del__ and __repr__ in C is bit hard task to me.
> I can't do it in this week. (maybe I can't do it in this month too.)

NP.  I'll take a look myself after you upload the next iteration of the patch...
msg276513 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-09-15 04:15
This is the patch.
And git branch is here https://github.com/methane/cpython/pull/5
msg278204 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-06 19:07
The most recent patch segfaults... Will try to debug.
msg278206 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-06 19:14
INADA, would you be able to take a look?
msg278224 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-07 03:17
FutureIter_throw is wrong, maybe.
Removing FutureIter_send and FutureIter_throw from FutureIter_methods solves the segv and test passed.
msg278227 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-07 03:54
fixed
msg278243 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-07 14:07
fastfuture3-wip.patch is work in progress implementation of
implementing __repr__ and __del__ in C.
I post it to avoid duplicated works.

Known TODOs:

* Support overriding Future._repr_info()
* Fix __del__ is not called (Research how tp_del, tp_finalize, and tp_deallocate works)

I hope I have enough time to finish in next week, but I'm not sure.
msg278294 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-08 12:29
Fixed overriding Future._repr_info().
But I failed to implement overridable Future.__del__ in C yet.

(FYI, fastfuture2.patch passes tests by mix-in __del__ and __repr__)
msg278325 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-08 20:36
Now I understand tp_dealloc, tp_finalize and subtype_dealloc.
Attached patch passes tests.
msg278328 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-09 01:00
I quickly looked over the patch and I think it's good.  If anything we still have time to hunt down any bugs or even revert this before 3.6 final.

INADA, feel free to commit it before Monday to 3.6 and default branches.
msg278348 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-09 05:56
I've committed the patch with trivial fixes (adding curly braces to if statements).
And I'm sorry, I committed with wrong issue number.

https://hg.python.org/cpython/rev/678424183b38 (3.6)
https://hg.python.org/cpython/rev/f8815001a390 (default)

I fixed NEWS entry already.
msg278357 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-09 12:46
I close this issue for now.
Further improvements can be new issue.
msg278365 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-09 16:00
Thank you, INADA!  Next task -- optimize asyncio.Task in C in 3.7. Another 10-15% performance improvement.
msg278366 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-09 16:02
I mean another optimization possibility.
msg278368 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2016-10-09 16:16
How about changing module name?
_asyncio_speedup for example.
msg278370 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-09 16:19
Yes, I think it's a good idea.
msg280359 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-11-08 22:24
This patch introduced multiple refleaks in test_asyncgen.
msg280363 - (view) Author: Roundup Robot (python-dev) Date: 2016-11-09 00:05
New changeset 345904bd0456 by Yury Selivanov in branch '3.6':
Issue #26081: Fix refleak in _asyncio.Future.__iter__().throw.
https://hg.python.org/cpython/rev/345904bd0456

New changeset b977775aa07d by Yury Selivanov in branch 'default':
Merge 3.6 (issue #26081)
https://hg.python.org/cpython/rev/b977775aa07d
History
Date User Action Args
2016-11-09 00:06:18yselivanovsetpriority: release blocker -> normal
status: open -> closed
resolution: fixed
2016-11-09 00:05:50python-devsetnosy: + python-dev
messages: + msg280363
2016-11-08 22:24:00yselivanovsetpriority: normal -> release blocker
status: closed -> open
resolution: fixed -> (no value)
messages: + msg280359
2016-10-09 16:19:55yselivanovsetmessages: + msg278370
2016-10-09 16:16:30inada.naokisetmessages: + msg278368
2016-10-09 16:02:07yselivanovsetmessages: + msg278366
2016-10-09 16:00:43yselivanovsetmessages: + msg278365
2016-10-09 13:00:45berker.peksagsetstage: patch review -> resolved
versions: + Python 3.7
2016-10-09 12:46:52inada.naokisetstatus: open -> closed
resolution: fixed
messages: + msg278357
2016-10-09 05:56:58inada.naokisetmessages: + msg278348
2016-10-09 01:00:01yselivanovsetmessages: + msg278328
2016-10-08 20:36:24inada.naokisetfiles: + fastfuture4.patch

messages: + msg278325
2016-10-08 12:29:44inada.naokisetfiles: + fastfuture3.patch

messages: + msg278294
2016-10-07 14:07:17inada.naokisetfiles: + fastfuture3-wip.patch

messages: + msg278243
2016-10-07 03:54:52inada.naokisetfiles: + fastfuture2.patch

messages: + msg278227
2016-10-07 03:17:43inada.naokisetmessages: + msg278224
2016-10-06 19:14:48yselivanovsetmessages: + msg278206
2016-10-06 19:07:15yselivanovsetmessages: + msg278204
2016-09-16 16:44:51RemiCardonasetnosy: + RemiCardona
2016-09-15 04:16:03inada.naokisetfiles: + fastfuture.patch

messages: + msg276513
2016-09-15 03:15:40yselivanovsetmessages: + msg276505
2016-09-15 03:14:32inada.naokisetmessages: + msg276504
2016-09-14 22:37:51yselivanovsetmessages: + msg276494
2016-09-12 01:11:18ned.deilysetmessages: + msg275906
2016-09-12 01:08:24gvanrossumsetmessages: + msg275905
2016-09-12 01:07:37yselivanovsetmessages: + msg275904
2016-09-12 00:54:25gvanrossumsetnosy: + ned.deily
messages: + msg275903
2016-09-11 07:51:14inada.naokisetfiles: + fastfuture.patch

messages: + msg275740
2016-09-11 04:54:14gvanrossumsetmessages: + msg275734
2016-09-11 03:48:33inada.naokisetfiles: + fastfuture.patch

messages: + msg275729
2016-09-10 17:17:12gvanrossumsetmessages: + msg275658
2016-09-10 15:01:27inada.naokisetfiles: + fastfuture.patch
2016-08-28 02:48:35inada.naokisetmessages: + msg273802
2016-08-12 05:29:41inada.naokisetfiles: + futures.patch

messages: + msg272498
2016-08-10 17:28:33yselivanovsetmessages: + msg272347
2016-08-10 11:57:39inada.naokisetmessages: + msg272327
2016-08-10 00:57:17yselivanovsetmessages: + msg272285
2016-08-09 23:30:50yselivanovsetmessages: + msg272278
2016-08-09 19:26:15serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg272262
2016-08-09 13:33:18yselivanovsetmessages: + msg272245
2016-08-09 13:16:47berker.peksagsetstage: needs patch -> patch review
versions: - Python 3.5
2016-08-09 12:57:51inada.naokisetmessages: + msg272242
2016-07-21 14:19:20yselivanovsetmessages: + msg270928
2016-07-21 13:43:26mpaolinisetnosy: + mpaolini
messages: + msg270924
2016-07-12 08:50:03inada.naokisetmessages: + msg270232
2016-07-12 02:27:39inada.naokisetmessages: + msg270221
2016-07-11 14:32:51yselivanovsetmessages: + msg270183
2016-07-11 12:49:25inada.naokisetmessages: + msg270175
2016-07-10 04:28:30inada.naokisetfiles: + futures.patch

messages: + msg270070
2016-07-10 03:31:49yselivanovsetmessages: + msg270069
2016-07-10 03:27:06inada.naokisetmessages: + msg270068
2016-07-09 17:05:42inada.naokisetmessages: + msg270055
2016-07-09 12:27:41inada.naokisetnosy: + inada.naoki
messages: + msg270037
2016-07-03 20:18:20gvanrossumsetmessages: + msg269773
2016-07-03 19:43:45giampaolo.rodolasetnosy: + giampaolo.rodola
2016-01-11 17:23:42yselivanovcreate