bpo-32204: Optimize asyncio.Future _schedule_callbacks

[LiraNuna]

Summary

While migrating our code to asyncio, we've noticed that performance was not up to our standards. We have been performing profiles to determine what are some quick wins we could contribute to python in order to increase asyncio's performance.

While investigating, we have noticed that the method set_result of asyncio.Future is very slow when a lot of callbacks are waiting for the future. When digging, we noticed that a full copy of all callbacks is made.

This PR corrects this issue and reduces the amount of time set_result takes on our benchmarks from 17% down to 2.5%

Test plan

In addition to passing all unit tests, I performed a before and after profiles using cProfile:

Before:

After:

https://bugs.python.org/issue32204

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

[Mariatta]

This requires an issue in the bug tracker. Please create one and reference the issue number here in the title. Thanks.

[LiraNuna]

@Mariatta comments addressed.

[serhiy-storchaka]

This changes the intended behavior. If callbacks add other callbacks the latter will be lost.

[LiraNuna]

self._loop.call_soon does not execute the callbacks (docs, impl) just adds them to be called later, when control returns to the loop. I don't think the scenario you describe is possible.

Can you construct an example case that shows this scenario?

[asvetlov]

The change is safe from my point of view.
On other hand I doubt if the patch will boost overall performance significantly. Micro betchmark test time of callback rescheduling but from my experience it is not the bottleneck. At least I see almost no difference on aiohttp becnchmarks.

@1st1 could you take a look?

[serhiy-storchaka]

Why the code was written in this way at first time? While we don't know the reasons it would be safer to not touch it.

I don't see any difference in benchmarks. And it shouldn't be, since seems this code is not executed in normal circumstances.

[LiraNuna]

@asvetlov The PR improves performance by ~3-5% which is noticeable at larger scales. I found the issue while benchmarking and figured the python project would benefit from it.

Why the code was written in this way at first time?

I was trying to to find the original reason but the code is from around 2015, when asyncio was in a separate repository. I personally believe it was an oversight. I personally believe it would be safer to assume that since tests pass, this PR should be considered safe, but I do not know the breadth of asyncio's tests.

The original commit is big and do not dive into implementation detail.

[1st1]

The Python version of asyncio/futures.py is not even being executed in your benchmark. Starting with Python 3.6 both Task and Furure are written in C and their real implementations are in _asynciomodule.c. In my opinion this PR should be closed without merging as it can't really affect anything.

[1st1]

This kind of optimization is unsafe, as it will change the behaviour when Future's callbacks add new callbacks to that Future. Take a look at the C implementation which has an optimized version of this code.

I'm sorry, but I have to close this PR. I'll take a look at your benchmark of async/await later.

[1st1]

FWIW we can further optimize _asynciomodule.c:future_schedule_callbacks by implementing a linked list of callbacks + a freelist for linked list nodes. That would allow us to stop creating a list instance on Future completion. Future._callbacks getter can then build a list dynamically on request, but we better to just remove it in 3.7 along with the Future._schedule_callbacks() method.

[1st1]

self._loop.call_soon does not execute the callbacks (docs, impl) just adds them to be called later, when control returns to the loop. I don't think the scenario you describe is possible.

This is actually true. I suggest we leave Python code as is. C code in asynciomodule.c can be optimized to use just one list object. Please open a new PR to do that (and a new issue on b.p.o)