Skip to content

MNT: optionally collect gc in memleak.py #25500

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

Closed

Conversation

richardsheridan
Copy link
Contributor

PR Summary

Follow up to #23712. It's quite enlightening to see the effect of collecting or not collecting on this test script.

@richardsheridan richardsheridan changed the title optionally collect gc in memleak.py MNT: optionally collect gc in memleak.py Mar 19, 2023
@QuLogic
Copy link
Member

QuLogic commented Mar 20, 2023

It was previously on always, but this now disables it by default. Do we want to change that? The release manager guide probably needs an update as well.

@richardsheridan
Copy link
Contributor Author

I figure the script should reflect the way memory grows in matplotlib, and subsequent to #23712 that means not interfering with gc heuristics.

I left the gc option available since it now takes a while to observe the long term trend and someone may want to smooth out the gc bumps for a short term measurement.

@ksunden
Copy link
Member

ksunden commented Mar 21, 2023

For context, the output is actually quite qualitatively different (I did a shorter 100 test rather than the standard from the release docs which is 1000, but still)

Without explicit gc.collect:

agg

With explicit gc.collect:
agg-gc

So, without calling gc.collect, memory usage is higher and pymalloc/total objects oscillate with a period of ~4.
While calling gc.collect, memory usage and number of objects grow fairly linearly. (iirc they do plateau off further in, but takes longer to get to that plateau at least for rss)

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