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

reduce memory usage of Connection #377

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

pawl
Copy link
Contributor

@pawl pawl commented Dec 14, 2021

While looking into celery/celery#4843 (comment), I noticed Connection._avail_channel_ids seems to be the thing that uses the most memory on a Connection:

Currently _avail_channel_ids is defined on Connection like this:

self._avail_channel_ids = array('H', range(self.channel_max, 0, -1))

It creates an array of all possible channel ids and removes them as they're used.

This PR reduces the memory usage of the Connection by reversing that logic and turning _avail_channel_ids into _used_channel_ids. This will only store the channel ids that are currently used, which is almost always going to be a much smaller array.

You can see the empty arrays use a lot less memory:

>>> from array import array
>>> import sys
>>> sys.getsizeof(array('H'))
64
>>> sys.getsizeof(array('H', range(65535, 0, -1)))
133524

There's downside though: It's going to take a little bit longer to _get_free_channel_id, because it's now going to start counting from the 1 to channel_max and return the first number that doesn't exist in _used_channel_ids.

However, this could potentially reduce the severity of memory leaks.

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.

thanks for your patch, you were involved in https://github.com/celery/py-amqp/pull/374/files aswell right?

@auvipy
Copy link
Member

auvipy commented Dec 14, 2021

Integrations tests are failing though so need some adjustment. Also I am almost done with #368 can you guys test it on your setup

@pawl
Copy link
Contributor Author

pawl commented Dec 14, 2021

thanks for your patch, you were involved in https://github.com/celery/py-amqp/pull/374/files aswell right?

Yeah, I helped reproduce the issue.

Integrations tests are failing though so need some adjustment.

I'm thinking this should fix it: 64f4900

Also I am almost done with #368 can you guys test it on your setup

I can try running our unit tests with it. Our last release of the year is tomorrow, so we couldn't put that into production for a while. However, we will be sending the fix for #374 to production tomorrow.

@auvipy
Copy link
Member

auvipy commented Dec 14, 2021

OK thank you. you can try the slot related code in your local env as well.

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

Successfully merging this pull request may close these issues.

None yet

2 participants