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

Add __slots__ to all classes #744

Closed
pxeger opened this issue Apr 16, 2021 · 11 comments · Fixed by #6005
Closed

Add __slots__ to all classes #744

pxeger opened this issue Apr 16, 2021 · 11 comments · Fixed by #6005
Assignees
Labels
enhancement New feature or request pr_wanted Will eventually be fixed/implemented upstream, PRs will happily be accepted.

Comments

@pxeger
Copy link

pxeger commented Apr 16, 2021

Is your feature request related to a problem? Please describe.
High memory usage

Describe the solution you'd like
Add the special __slots__ class variable to all or most classes.

Describe alternatives you've considered
Using an alternative Python interpreter such as PyPy can also reduce memory usage as PyPy does some optimisations like this already. However, PyPy lacks some features of CPython (especially the C API).

Additional context
If you're not familiar with __slots__, see here.

I'm working on a PR already, but I thought I'd create an issue first to track progress.

@pxeger pxeger added the enhancement New feature or request label Apr 16, 2021
@BeryJu
Copy link
Member

BeryJu commented Apr 16, 2021

Hey, I wasn't aware that __slots__ makes this much of a difference, I am looking forward to seeing what the PR will bring!

@BeryJu BeryJu self-assigned this Apr 16, 2021
@pxeger
Copy link
Author

pxeger commented Apr 16, 2021

I don't know how much of a difference it will make, but the codebase is pretty class-heavy so I'm optimistic

@traverseda
Copy link

PyPy does some optimisations like this already.

PyPy uses JIT, which generally uses a lot more memory, has slower startup times, but works faster. I'd expect most code running in PyPy to use more memory, unless you're getting to the hundred's of MB range. I'd be pretty surprised is slots helped that much either, definitely I don't think you should apply it to all classes but if there's some class that's getting instantiated thousands of times it would be a good thing to look at optimizing.

Is this project really instantiating thousands and thousands of classes?

@BeryJu
Copy link
Member

BeryJu commented Apr 17, 2021

There are some classes that are instantiated a lot (especially for the PolicyEngine), but a lot of the other classes use a lot of inheritance.

Most things in this projects are class based, simply just because of django and drf.

@traverseda
Copy link

Well it will be interesting to see if this has any significant impact

@pxeger
Copy link
Author

pxeger commented Apr 17, 2021

The PyPy comment was based on this page about __slots__ which says PyPy "does these optimisations by default", which I just blindly trusted. Maybe that's totally wrong, I don't know

@james-d-elliott
Copy link

james-d-elliott commented Apr 21, 2021

How __slots__ works is by telling the python interpreter to only expect objects to only contain certain attributes which makes the objects not use dicts to store them in memory. The reason why this has an effect on memory consumption is how the data is structured in memory. It also has an additional benefit of improving the time it takes to get/set the attributes. PyPy does indeed do this by default, it using more memory on average is not indicative of how it will change a project that doesn't use it as PyPy drastically alters the way python operates in other ways (JIT instead of interpreted).

The only downside is you cannot get/set attributes that do not exist in this array and ALL objects in the inheritance chain MAY NOT use dicts for memory storage, i.e. they MUST also define __slots__. One other piece of information is that __slots__ is inherited also. Technically it's better to allow __slots__ to be inherited, i.e. don't define an attribute twice; however you can do so if you must/desire, it will just use a bit more memory.

@stale
Copy link

stale bot commented Jun 20, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/wontfix This will not be worked on label Jun 20, 2021
@traverseda
Copy link

traverseda commented Jun 20, 2021

I hate stale closing bot. It's the worst bot. I mean don't get my wrong, I don't think adding __slots__ to all classes makes sense, but stale-close bot it still the worst.

@stale stale bot removed the status/wontfix This will not be worked on label Jun 20, 2021
@pxeger
Copy link
Author

pxeger commented Jun 20, 2021

I got bored of working on this, so it's fine for stale bot to close it if it wants.

@pxeger pxeger closed this as completed Jun 20, 2021
@BeryJu BeryJu added the pr_wanted Will eventually be fixed/implemented upstream, PRs will happily be accepted. label Jun 20, 2021
@BeryJu BeryJu reopened this Jun 20, 2021
@BeryJu
Copy link
Member

BeryJu commented Jun 20, 2021

@traverseda My bad, I didn't fully think through configuring stale bot when I initially added it.

I re-opened this issue with a pr_wanted label, which won't be closed by stale bot, and essentially means its a nice feature/addition, that I don't (currently) plan on adding myself, but I am open to PRs for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pr_wanted Will eventually be fixed/implemented upstream, PRs will happily be accepted.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants