bpo-32355: Optimize asyncio.gather() #4913
Conversation
| for arg in coros_or_futures: | ||
| if arg not in arg_to_fut: | ||
| fut = ensure_future(arg, loop=loop) | ||
| if loop is None: |
There was a problem hiding this comment.
It seems like the check for multiple loops (see line 622) has been removed. Is this intentional?
If it is, you may want to add a test for that.
There was a problem hiding this comment.
ensure_future does it. No need to do it twice.
There was a problem hiding this comment.
ensure_future validates that the passed in loop argument matches that of the future passed in. If I were to pass 3 futures each having their own different loop, a new _GatheringFuture will be created tied to the loop that was passed to gather (which can be a separate loop as well).
There was a problem hiding this comment.
I think I understand what you mean by that, ensure_future will make sure that the future/coro passed is bound to the same loop - either current or what was passed in. Thanks for explaining.
Lib/asyncio/tasks.py
Outdated
| for fut in children: | ||
| if fut.cancelled(): | ||
| res = futures.CancelledError() | ||
| elif fut._exception is not None: |
There was a problem hiding this comment.
_exception and _result is not a part of Future public API
There was a problem hiding this comment.
well, old code used it, i'm not adding anything new
There was a problem hiding this comment.
I thought about this for a while. Let's use only public Future API in asyncio. I've updated this PR and will make a new one to fix all remaining places in asyncio.
There was a problem hiding this comment.
Actually, it's just one other place where we access Future._exception or Future._result in asyncio -- it's base_events.py. I've fixed it as part of this PR.
| if not return_exceptions: | ||
| outer.set_exception(res) | ||
| if not return_exceptions: | ||
| if fut.cancelled(): |
There was a problem hiding this comment.
Both checks can be merged:
exc = fut.exception()
if exc is not None:
outer.set_exception(exc)
it handles CancelledError too.
There was a problem hiding this comment.
no, it will raise CancelledError, not return it. We can't merge these checks.
| # Future created specifically for 'arg'. Since the caller | ||
| # can't control it, disable the "destroy pending task" | ||
| # warning. | ||
| fut._log_destroy_pending = False |
There was a problem hiding this comment.
Unrelated question: maybe add log_destroy_pending keyword-only parameter?
Often I want to see cancelled coroutines in logs.
5 participants
I also believe the this PR makes
asyncio.gather()code simpler (besides also making it 10-15% faster).https://bugs.python.org/issue32355