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

adding experimental __slots__ to some classes #368

Merged
merged 5 commits into from Feb 8, 2022
Merged

adding experimental __slots__ to some classes #368

merged 5 commits into from Feb 8, 2022

Conversation

auvipy
Copy link
Member

@auvipy auvipy commented Nov 18, 2021

will add more slots and fix the issues which break the CI

related celery/vine#67

amqp/basic_message.py Outdated Show resolved Hide resolved
@auvipy
Copy link
Member Author

auvipy commented Nov 18, 2021

good to see tests are passing

@auvipy
Copy link
Member Author

auvipy commented Dec 13, 2021

I want to include it on 5.1 release, may be after christmas or on the new years

@auvipy
Copy link
Member Author

auvipy commented Jan 1, 2022

image
load time without this patch/patches

@auvipy auvipy added this to the v5.1.0 milestone Jan 1, 2022
@auvipy
Copy link
Member Author

auvipy commented Jan 1, 2022

@4383 as far i can recall you use this package on old openstack versions right? would love to have you try this changes

@auvipy auvipy requested a review from matusvalo January 8, 2022 08:05
@auvipy auvipy mentioned this pull request Jan 10, 2022
Copy link
Member

@matusvalo matusvalo left a comment

Choose a reason for hiding this comment

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

I don't see any benefit having __slots__ for classes except Message class. __slots__ is mainly performance related and the only class that is created on bigger scales is Message class.

@auvipy
Copy link
Member Author

auvipy commented Jan 12, 2022

IMHO, other classes when used in production, could be also benefited as well?

Copy link
Member Author

@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.

I am merging this as I can see most of the classes are used by kombu and celery, so in bigger system tiny resource saving would be beneficial, IMHO

@auvipy auvipy merged commit d0ab95e into master Feb 8, 2022
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