Add new loop.create_future() method. by 1st1 · Pull Request #290 · python/asyncio (original) (raw)
This repository was archived by the owner on Nov 23, 2017. It is now read-only.
Conversation25 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR adds a new API -- loop.create_future() (see discussion in #282 for details).
We don't want this committed before 3.5.1, right?
No, let's hold off on this one for a little bit.
Add loop.create_future() and start using it Add new loop.create_future() method.
@1st1: Python 3.5.1 has been out for a while now. You can safely merge this!
Hi Guido!
Sorry for the delayed reply, I was away from the office.
Before merging this PR, I'd like to reiterate on a few things:
- I'm working on an implementation of Future class in C. I'll create an issue on CPython bug tracker soon, and it would be great if we can ship 3.5.2 with it (would you be OK with that?).
- If we have a fast Future, then adding just
loop.create_futurewon't directly help projects like uvloop (since there are a lot of places with code likeif isinstance(fut, asyncio.Future): ...). - So this PR is mainly for streamlining creation of Futures for asyncio users. Since the
loopparameter in Future constructor is optional, it is often omitted.loop.create_futureis a less ambiguous API, and it mirrorsloop.create_task.
With all that in mind, are you still OK with merging this PR?
I forgot -- do you want this PR in 3.5.2 or the C-accelerated Future class?
Why wouldn't uvloop's Future class be able to inherit from the default Future class (accelerated or not)?
For most people leaving out the loop= parameter doesn't matter because there's only one loop they care about (or at least they're doing the Future instantiation when the right loop is current -- either way it doesn't matter).
I've noticed that create_task() has caused some confusion (see the recent issue #306 demanding that certain functions that currently return a Future be changed to be actual coroutines so they could be passed to create_task()). I worry that create_future() will cause more confusion (as yet unknown -- I hadn't anticipated the create_task() confusion either).
Anyway, do you really need this?
I forgot -- do you want this PR in 3.5.2 or the C-accelerated Future class?
The latter. If we have a fast Future implementation, we don't need loop.create_future just to further improve the performance. Sorry for the confusion.
We might need create_future to make Futures creation a little less error prone. You're right -- most users use only one event loop per process, but when users develop reusable libraries, they should pass the loop explicitly in their code. create_future would plug this specific hole. [1]
Another interesting benefit of create_future is that it'd allow one to instrument their code, say collect stats on how many Futures were created/cancelled/errored, how many callbacks were added/deleted. [2]
Anyway, do you really need this?
If you don't see enough benefits in having [1] and [2], then I propose to close this PR and focus on adding a C implementation of Future in CPython.
I've noticed that create_task() has caused some confusion (see the recent issue #306 demanding that certain functions that currently return a Future be changed to be actual coroutines so they could be passed to create_task()).
Oh, that's no good :( TBH, I always worried that we treat Futures and coroutines as they are interchangeable (in docs). Futures have add_done_callback(), cancel() etc, and some users will write code like loop.run_in_executable(...).add_done_callback(...). Maybe we should wrap asyncio functions that are documented as coroutines in actual coroutines?
On Fri, Jan 8, 2016 at 1:06 PM, Yury Selivanov notifications@github.com
wrote:
I forgot -- do you want this PR in 3.5.2 or the C-accelerated Future class?
The latter. If we have a fast Future implementation, we don't need
loop.create_future just to further improve the performance. Sorry for
the confusion.Honestly I'm skeptical about adding an accelerated Future class. That feels
like a risky thing to do in a bugfix release (even granting that asyncio is
still provisional).We might need create_future to make Futures creation a little less error
prone. You're right -- most users use only one event loop per process, but
when users develop reusable libraries, they should pass the loop explicitly
in their code. create_future would plug this specific hole. [1]Yeah, but they'll still have to change their code. It's not any easier to
rewrite every call to Future() to read loop.create_future() instead, and if
you're just going to call get_current_event_loop() it doesn't help at all
(since that's the default anyway).
So I don't believe this helps.
Another interesting benefit of create_future is that it'd allow one to
instrument their code, say collect stats on how many Futures were
created/cancelled/errored, how many callbacks were added/deleted. [2]And why couldn't you instrument Future.init directly? (Maybe using
mock.)Anyway, do you really need this?
If you don't see enough benefits in having [1] and [2], then I propose to
close this PR and focus on adding a C implementation of Future in CPython.Let's close this PR. But I'm not so sure about the latter.
I've noticed that create_task() has caused some confusion (see the recent
issue #306 #306 demanding that
certain functions that currently return a Future be changed to be actual
coroutines so they could be passed to create_task()).Oh, that's no good :( TBH, I always worried that we treat Futures and
coroutines as they are interchangeable (in docs). Futures have
add_done_callback(), cancel() etc, and some users will write code like
loop.run_in_executable(...).add_done_callback(...). Maybe we should wrap
asyncio functions that are documented as coroutines in actual
coroutines?No we should fix the docs.
--Guido van Rossum (python.org/~guido)
Hi Guido,
I continue to experiment with uvloop and create_future. I have an experimental branch of uvloop with create_future and a Future implemented in Cython.
The streams echo server benchmark becomes 25% faster, if I replace StreamReader._waiter with a fast uvloop Future implementation. My implementation is a subclass of asyncio.Future, so everything in asyncio supports it without any problem.
Future is the only core primitive in asyncio that I can't improve in uvloop. Even for tasks we have a create_task method.
Could you please re-consider merging this PR?
Did a quick futures benchmark here, the result is that uvloop's Futures are 1.8-2x faster.
Benchmarks for sock_recv/sock_sendall are >70% faster.
Benchmarks for asyncio streams are 25-30% faster.
I also think that loop.create_future() is a better API than asyncio.Future(loop=loop).
I am completely overwhelmed with stuff, can it wait a few days?
Absolutely. I only want this to make it to 3.5.2 if possible.
I don't think new APIs or new features are in scope of 3.5.2, my guess is this will have to wait for 3.6...
Actually they are, asyncio is still provisional in 3.5.
Actually they are, asyncio is still provisional in 3.5.
Yes, pretty please... There are some significant performance improvements here.
This PR is out of date, and I'm a little confused. But I think we should go ahead with something. Here are some questions and observations.
This PR seems to do a few things:
- Add metaclass=ABCMeta to Future (why?)
- Add create_future() (and a test for it)
- Use create_future() everywhere
So the last two bullets feel uncontroversial, but I'm curious about the addition of ABCMeta. (In mypy, we found that this significantly slows down some things.)
Also, in some previous comment you seemed to be claiming that you didn't (or at least might not) need create_future() after all, and in some other comment you seemed to be indicating that you really want an accelerated Future implementation in CPython. So which is it? Can you lay everything clearly on the table please? (We could open a new issue or use the python-tulip list, but we might as well continue the discussion here, unless you think you'd like to start a fresh PR.)
Thanks for reviewing!
Add metaclass=ABCMeta to Future (why?)
Having an ABC for Futures was something I wanted to do in the beginning, but now we don't need that. I'll update the PR. And yes, adding an ABCMeta will make isinstance calls significantly slower, this is something we cannot even consider.
Also, in some previous comment you seemed to be claiming that you didn't (or at least might not) need create_future() after all, and in some other comment you seemed to be indicating that you really want an accelerated Future implementation in CPython. So which is it?
Yes, I created an issue (http://bugs.python.org/issue26081) to re-implement the asyncio.Future in C. At that time I believed that it's something we can do relatively fast. However, we can only merge that in 3.6, it still requires a lot of work, and because of a few limitations of CPython C API and a very complex code dependancy graph the code looks very ugly. I'm honestly not sure if I will have time to finish that work, and there are somewhat more important things I wanted to do in 3.6.
OTOH, I tried to re-implement Future in Cython, and saw the same speedups as with my C Future from issue 26081. The performance benefits are very significant, and this is something I'd like people to start using asap.
I think that create_future is a nice solution, as it allows uvloop to speed-up things that are out of its control, for instance asyncio.streams. I was very surprised when I discovered that using an accelerated future for waiters in streams improves the performance by 25%.
What if some library or app mixes calling create_future() and instantiating
Future directly?
Also what about code that calls isinstance(x, Future)?
On Sunday, May 15, 2016, Yury Selivanov notifications@github.com wrote:
Thanks for reviewing!
Add metaclass=ABCMeta to Future (why?)
Having an ABC for Futures was something I wanted to do in the beginning,
but now we don't need that. I'll update the PR. And yes, adding an ABCMeta
will make isinstance calls significantly slower, this is something we
cannot even consider.Also, in some previous comment you seemed to be claiming that you didn't
(or at least might not) need create_future() after all, and in some other
comment you seemed to be indicating that you really want an accelerated
Future implementation in CPython. So which is it?Yes, I created an issue (http://bugs.python.org/issue26081) to
re-implement the asyncio.Future in C. At that time I believed that it's
something we can do relatively fast. However, we can only merge that in
3.6, it still requires a lot of work, and because of a few limitations of
CPython C API and a very complex code dependancy graph the code looks very
ugly. I'm honestly not sure if I will have time to finish that work, and
there are somewhat more important things I wanted to do in 3.6.OTOH, I tried to re-implement Future in Cython, and saw the same speedups
as with my C Future from issue 26081. The performance benefits are very
significant, and this is something I'd like people to start using asap.I think that create_future is a nice solution, as it allows uvloop to
speed-up things that are out of its control, for instance asyncio.streams.
I was very surprised when I discovered that using an accelerated future for
waiters in streams improves the performance by 25%.—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#290 (comment)
--Guido (mobile)
What if some library or app mixes calling create_future() and instantiating
Future directly?
That's totally fine. uvloop will recognize any Future that extends asyncio.Future. And uvloop.Future will work just fine with vanilla asyncio.
Also what about code that calls isinstance(x, Future)?
Here's some pseudocode illustrating how it's implemented in uvloop:
cdef class BaseFuture: """Optimized & statically typed Future implementation"""
class Future(BaseFuture, asyncio.Future): pass
Because uvloop.Future is inherited from asyncio.Future it works fine with isinstance checks, and since BaseFuture is the first parent it's faster.
Awesome. Let me know when the updated PR is ready! (Also, super thanks for all the PR merging and other stuff you've done recently to asyncio. As you might have guessed I'm totally consumed by typing stuff and I rarely have time to dive deep enough into asyncio. So you and Victor are basically it! Rest assured I am still watching the stream of notifications and will jump in when I see anything fishy happening. 3.5.2 will be a great release!)
Awesome. Let me know when the updated PR is ready!
It should be ready now. I can merge it myself if you don't have time.
Also, super thanks for all the PR merging and other stuff you've done
recently to asyncio. As you might have guessed I'm totally consumed by
typing stuff and I rarely have time to dive deep enough into asyncio.
NP! I can't wait to hear your typing talk on PyCon! And I have a few
ideas to discuss too ;)
(I'm not sure why github shows that the branch is out of date -- I've just rebased it on top of the master branch. Anyways, we can merge manually)
LGTM. Can you manually merge this? The GitHub warning feels scary, maybe it's because the branch is so old? Also you should probably make sure no new Future() calls have appeared since you wrote this.
Also you should probably make sure no new Future() calls have appeared since you wrote this.
Yep, I did that ;) -- Indeed we had a new one, where we parse ip addresses.
Thanks a lot, Guido, I'll merge it now.
Good luck! Remember to update the docs.