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

improve performance of _get_free_channel_id, fix channel max bug #385

Merged
merged 2 commits into from
Dec 22, 2021

Conversation

pawl
Copy link
Contributor

@pawl pawl commented Dec 21, 2021

This PR fixes a performance issue introduced by #377.

I noticed while running tests that _get_free_channel_id is way slower than I expected.

Surprisingly, this code that's similar to _get_free_channel_id (when all the channel ids are used) takes almost 40 seconds to run:

import time
from array import array

channel_max = 65535
used_channel_ids = array('H', range(1, channel_max + 1))
start = time.time()
for channel_id in range(1, channel_max + 1):
    if channel_id not in used_channel_ids:
        assert False
print(f'finished {time.time() - start:0.3f}')

This PR casts _used_channel_ids to a set before the loop, and it only takes ~0.004 seconds to run the loop after this change. It also allows keeping the memory usage improvements by keeping the _used_channel_ids stored as an array on the class.

I also learned that there's a bug preventing _get_free_channel_id from reaching the channel_max (the limit is currently channel_max - 1), and I fixed that too.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you also adjust some integration tests as well?

@pawl
Copy link
Contributor Author

pawl commented Dec 21, 2021

@auvipy Is this what you were thinking?: 3db6df1

@auvipy
Copy link
Member

auvipy commented Dec 21, 2021

yes

@auvipy auvipy merged commit 935a06b into celery:master Dec 22, 2021
@skinnyh
Copy link

skinnyh commented Feb 2, 2022

Hi, can we get a version bump for this fix? Thank you!

@auvipy
Copy link
Member

auvipy commented Feb 3, 2022

Yes of course, this weekend for sure

@skinnyh
Copy link

skinnyh commented Feb 14, 2022

Yes of course, this weekend for sure

Hi, any update on it?

@GuardianRain
Copy link

Hi, which version has already include this fix? Thank you!

@auvipy
Copy link
Member

auvipy commented Apr 24, 2022

5.1.1 please

tanaypf9 pushed a commit to tanaypf9/pf9-requirements that referenced this pull request May 20, 2024
amqp v5.1.0 solved a performance issue of the previous version[1].

[1] celery/py-amqp#385

Change-Id: If9dbbb0e681d6f1c7dca61fe892b10f81f0abb1c
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