Optimize Task._step and Task._wakeup #289
Conversation
| @@ -247,7 +247,8 @@ def _step(self, value=None, exc=None): | |||
| self.set_exception(exc) | |||
| raise | |||
| else: | |||
| if isinstance(result, futures.Future): | |||
| # Use duck-typing here. `isinstance(result, Future)` is slow. | |||
| if hasattr(result, 'add_done_callback'): | |||
There was a problem hiding this comment.
I would recommend checking for a method that concurrent.futures.Future does not have. Or add a new class attribute specifically for the purpose of this test.
(I'm also still unclear on what makes isistance() slow. Do you have a microbenchmark for that?)
There was a problem hiding this comment.
On a macro benchmark the difference is about 4%. Let me come up with a micro benchmark...
There was a problem hiding this comment.
On a micro benchmark the difference is about 4% too.
After giving this some thought, I think we can keep this isinstance, since, quoting you, "[...] checking for a method that concurrent.futures.Future does not have" I don't like -- feels too hacky.
Can I merge the second change _step(value, None) -> _step(), though?
There was a problem hiding this comment.
|
@gvanrossum So what do you think about this PR? |
|
OK, you can commit the change to the |
|
Thanks a lot for the review! Closing the PR. |
See python/asyncio#289 for details. --HG-- branch : 3.4
See python/asyncio#289 After this optimization to asyncio, my "yieldable()" wrapper no longer returns the Future's result, so I have to retrieve the result in a separate step.
I found a way to monkey-patch
asyncio.Futureand make it compatible with uvloop (see #282 for details).This PR will significantly improve futures performance in uvloop (~10-15%) while we wait till 3.5.2. In 3.5.2, instead of adding
AbstractFuture, or usingABCMeta, I propose to directly incorporate my C accelerator in asyncio -- that's probably the simplest solution.This change is 100% backwards compatible -- it's a pure optimization. Please consider accepting this before 3.5.1.