Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QueueIterator: speedup via __anext_impl w/wo timeout in __init__ #627

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gglluukk
Copy link

Hi!

During investigation of optimization RabbitMQ's reader I've faced up performance gap between aio-pika 6.6.1 and 6.7.0 (benchmarks for 10,000 reads/seconds):

aio-pika 6.8.2: 4.484
aio-pika 6.8.1: 4.374
aio-pika 6.8.0: 4.309
aio-pika 6.7.1: 4.279
aio-pika 6.7.0: 4.436 <--- gap
aio-pika 6.6.1: 4.013
aio-pika 6.6.0: 4.083
aio-pika 6.5.3: 4.098

More: https://github.com/gglluukk/rmq2psql

The performance gap arose due to the addition of timeout support in queue.iterator(), introduced in version 6.7.0:

# diff aio_pika-6.6.1/queue.py aio_pika-6.7.0/queue.py 
439c439,442
<             return await self._queue.get()
---
>             return await asyncio.wait_for(
>                 self._queue.get(),
>                 timeout=self._consume_kwargs.get('timeout')
>             )

This change caused a decrease in performance compared to version 6.6.1 and subsequent versions inherited this gap.

PR can hopefully resolve this.

Regards!

@@ -484,6 +484,11 @@ def __init__(self, queue: Queue, **kwargs: Any):
self._queue = asyncio.Queue()
self._consume_kwargs = kwargs

if kwargs.get("timeout"):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... that's should be handled by wait_for implementation: https://github.com/python/cpython/blob/main/Lib/asyncio/tasks.py#L494-L507

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep... and asyncio.wait_for calls:
https://github.com/python/cpython/blob/main/Lib/asyncio/timeouts.py#L145-L162 and so on which gives ~5% overhead even if we don't use timeout= argument.
my idea is to eliminate this calls' chain if we don't need it

@Darsstar
Copy link
Contributor

How does #615 do in this regard?

@gglluukk
Copy link
Author

@Darsstar my custom task was to speed up the particular code (cancellation/connection check there is done by its own logic). during that i've researched the speed degradation and proposed patch. decision will it be accepted or not is out of my responsibility.
btw i've benchmarked your patch and it turns to be ~10% slower:

before your patch:
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #1: 4.905
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #2: 4.503
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #3: 4.809
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #4: 4.870
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #5: 4.741
Average: 4.766

after your patch:
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #1: 5.481
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #2: 5.123
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #3: 5.397
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #4: 5.437
rmq2psql.py --loop-type queue_iteration_with_timeout --max-bulks 100: #5: 5.128
Average: 5.313

@coveralls
Copy link

Coverage Status

coverage: 87.694% (-0.4%) from 88.125%
when pulling 4a89ff0 on gglluukk:master
into 848c025 on mosquito:master.

@Darsstar
Copy link
Contributor

Darsstar commented Nov 21, 2024

9.5.0 now contains a refactored version of the branch you benchmarked. I have incorperated the same optimizations you did, just differently. And an extra one as well. (queue.get_nowait())
Please give it a spin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants