gh-122881: Reduce asyncio heapq scheduling overhead

[bdraco]

Wrap the TimerHandle in a tuple with the when value at the front to avoid having to call TimerHandle.__lt__ for heapq operations.

• Issue: Reduce asyncio heapq overhead #122881

[picnixz]

Some nitpicks.

[picnixz]

If you have numbers, you could also add them to the NEWS entry, saying that you improved the performances by a factor X for instance, otherwise I'm good (still requires a core dev for the final acceptance).

[bdraco]

If you have numbers, you could also add them to the NEWS entry, saying that you improved the performances by a factor X for instance, otherwise I'm good (still requires a core dev for the final acceptance).

It works out to a

• ~10.2% speed up to in _run_once on my production Home Assistant instance but its already well optimized to reduce scheduling as much as possible so I'd expect other use cases have more substantial speed ups
• ~9.5% speed up scheduling and running timer handles

[bdraco]

Added the number for the TimerHandle improvement. _run_once is going to vary greatly based on the use case since some use cases will schedule far more than others. My production use case was previously an 18% speed up with _run_once before aiohttp was optimized to remove a lot of unneeded TimerHandle creation.

[bdraco]

Thanks!

[itamaro]

I have a few questions

1. could you please split out all the attribute caching optimizations to a separate PR (and benchmark them separately)? it's not clear that they improve things in a measurable way, and it hurts readability.
2. speaking of benchmarking - the microbenchmarks look good, but is there a measurable impact on the asyncio pyperformance benchmarks?
3. what is the memory impact? iiuc, this would use strictly more memory (extra tuple and when per heapq item), but I have no idea whether the increase is meaningful or negligible.

[bdraco]

I have a few questions

1. could you please split out all the attribute caching optimizations to a separate PR (and benchmark them separately)? it's not clear that they improve things in a measurable way, and it hurts readability.

This has already been discussed above. Is this strictly required?

2. speaking of benchmarking - the microbenchmarks look good, but is there a measurable impact on the asyncio pyperformance benchmarks?

I'm not sure the pyperformance would be a better benchmark here as I think the real-world use case impact is more important.

3. what is the memory impact? iiuc, this would use strictly more memory (extra tuple and when per heapq item), but I have no idea whether the increase is meaningful or negligible.

>>> (1.12,object()).__sizeof__()
40

The memory impact should be negligible compared to the cost of TimerHandle

[bdraco]

In recent versions of Python, attribute caching of bound methods usually hurts performance. So yes, let's remove it, unless we can demonstrate meaningful improvement in a benchmark

@hauntsaninja I pushed the commit I had original held off on

a5b6647 but than I realized you deleted your comment

[hauntsaninja]

Ah, I deleted my comment because only the ready_popleft was a pessimisation and I didn't really want to get into it :-) That said, I do like having self if it's cheap, I find it easier to reason about state. Thanks for the change!

[bdraco]

Ah, I deleted my comment because only the ready_popleft was a pessimisation and I didn't really want to get into it :-) That said, I do like having self if it's cheap, I find it easier to reason about state. Thanks for the change!

The self changes made a tiny difference compared to avoiding the __lt__ overhead so it not worthing getting hung up on anyways.

[hauntsaninja]

Thanks again!

[mdboom]

I ran this PR against the pyperformance suite on our benchmarking hardware. The results overall are basically "no change", within the noise. The vast majority of the benchmarks there don't use async, however, we do have the async benchmarks broken out separately, and even there the results are kind of inconclusive. The async benchmarks in pyperformance are actually known to have a great deal of inherent variability, so, honestly it's hard to conclude too much from them.

Results on async benchmarks only

Platform	Speedup
Linux aarch64	1% slower
Linux x86_64	0% slower
Windows x86_64	1% faster
Windows x86	2% faster
macOS arm	1% slower

From these results, I'd say there's no obvious win or obvious red flag to merging this. The microbenchmark in #122881 shows a significant improvement, and if the additional code complexity is acceptable to others, this is probably fine to merge.

[bdraco]

I took a look at https://github.com/python/pyperformance/tree/main/pyperformance and I don't see any asyncio benchmarks that will generate a significant amount of TimerHandles so I wouldn't expect you would see any significant change in the benchmarks.

[bdraco]

For context, I've been working on addressing some performance complaints from aiohttp users. What finally pushed me to submit this upstream was how much more time is spent in aiohttp.web_ws.WebSocketResponse.receive() and aiohttp.client.ClientSession.get() with a timeout.

[1st1]

In my opinion these sort of micro-optimizations aren't worth it as they mostly just harm readability. The effect of this optimization will be barely detectable. If you want performance - use uvloop.

-1.

[willingc]

For context, I've been working on addressing some performance complaints from aiohttp users. What finally pushed me to submit this upstream was how much more time is spent in aiohttp.web_ws.WebSocketResponse.receive() and aiohttp.client.ClientSession.get() with a timeout.

Can you link to the issues re: aiohttp?

[willingc]

I would like to see the aiohttp performance issues before I weigh in on the need for this change.

[Dreamsorcerer]

I would like to see the aiohttp performance issues before I weigh in on the need for this change.

bdraco was analysing performance issues, but some of the discussion around that is in aio-libs/aiohttp#8608 and the thread: aio-libs/aiohttp#8608 (comment)

I think one concern here is that .reschedule is a public, high-level API: https://docs.python.org/3/library/asyncio-task.html#asyncio.Timeout.reschedule
And this can result in this heapq rescheduling, so if a user does use that a lot it could have a significant performance impact.

[willingc]

I think one concern here is that .reschedule is a public, high-level API: https://docs.python.org/3/library/asyncio-task.html#asyncio.Timeout.reschedule
And this can result in this heapq rescheduling, so if a user does use that a lot it could have a significant performance impact.

Thanks @Dreamsorcerer for additional information.

I'm at -0 in my view on this change at present. Personally, I would need to see more evidence of real world impact for me to support this optimization. I will remove the "Do Not Merge" label in case another core team member feels strongly to merge this.

Thanks @bdraco for the PR too and the detailed explanations.

[vstinner]

#122881 (comment) microbenchmark doesn't use modified code: it doesn't use the asyncio event loop :-( It's a benchmark on heapq.heappush() and heapq.heappop().

Can you write a benchmark using the asyncio event loop? For example, schedule 1000 callbacks with call_at().

[bdraco]

#122881 (comment) microbenchmark doesn't use modified code: it doesn't use the asyncio event loop :-( It's a benchmark on heapq.heappush() and heapq.heappop().

Can you write a benchmark using the asyncio event loop? For example, schedule 1000 callbacks with call_at().

#122881 (comment)

Is this what you are looking for?

[itamaro]

As a benchmarking data point - I applied a minimally modified version (ported to cinder 3.10) of this PR to Instagram Server, and there was no measurable perf impact.

[1st1]

As a benchmarking data point - I applied a minimally modified version (ported to cinder 3.10) of this PR to Instagram Server, and there was no measurable perf impact.

Yeah, basically what I expected. I think let's close it, unless the author comes up with a more significant and provable optimization.

[bdraco]

I did link to a benchmark with call_at above #122881 (comment)

If the instagram server isn't doing a lot of call_ats, I wouldn't expect any performance difference