Skip to content

Fix CallbackRegistry memory leak #19480

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Feb 19, 2021
Merged

Fix CallbackRegistry memory leak #19480

merged 11 commits into from
Feb 19, 2021

Conversation

vallsv
Copy link
Contributor

@vallsv vallsv commented Feb 8, 2021 •
edited
Loading

PR Summary

This PR fixes the issue #19474, and probably the issue #19345

  • Plus rewrite some code for readability (like full name instead of letter, intricate loop, try-catch potentially caching many things)
  • Plus coverage for the method disconnect

A much shorter change count be done to only fix the issue, if you like.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@vallsv vallsv force-pushed the fix-callback-registry-mem-leak branch from e6de4db to c64b4c7 Compare February 8, 2021 13:58
@vallsv vallsv mentioned this pull request Feb 8, 2021
Closed
@vallsv
Copy link
Contributor Author

vallsv commented Feb 9, 2021

Do you really expect a coverage of 100%? The warn from codecov/project/tests is most probably a dead code. So i don't know what to do with it. Safer to let it there / cleaner to remove it.

Another point is the _is_finilizing=sys.is_finilizing. I am pretty sure this argument is not needed, else sys.is_finilizing became pointless. What do you thing about removing this argument? It's the same argue as my commit 12aa238, it is not needed.

@anntzer
Copy link
Contributor

anntzer commented Feb 12, 2021

We prefer full coverage but allow ourselves some human judgment when pieces of code are very hard to test.

tacaswell
tacaswell reviewed Feb 15, 2021
View reviewed changes
lib/matplotlib/cbook/__init__.py
del self.callbacks[signal]
del self._func_cid_map[signal]
for signal, proxy_to_cid in list(self._func_cid_map.items()):
cid = proxy_to_cid.pop(proxy, None)
Copy link
Member

Choose a reason for hiding this comment

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

To check my understanding, The change on L221 and L241 are the critical changes in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. And i have just apply the same pattern to the disconnect.

@vallsv vallsv force-pushed the fix-callback-registry-mem-leak branch from c64b4c7 to ab10d45 Compare February 15, 2021 12:59
@vallsv @timhoffm
Update lib/matplotlib/cbook/__init__.py
9cfbab6 
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@tacaswell tacaswell added this to the v3.4.0 milestone Feb 18, 2021
tacaswell
tacaswell approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Modulo the change to the docsting.

Thanks @vallsv !

@vallsv @tacaswell
Fix signal docstring
f1d3f16 
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
@vallsv
Copy link
Contributor Author

vallsv commented Feb 18, 2021

Let me know if you prefer better commit history.
Come can be merged and message can be fixed.

@tacaswell
Copy link
Member

If you don't mind squashing that would be appreciated. We can also squash-merge on our side.

@jklymak
Copy link
Member

jklymak commented Feb 18, 2021

@anntzer did you have time to take a quick look at this? Though it could prob be bumped to 3.4.1 as well if folks prefer.

@tacaswell
Copy link
Member

I would also be comfortable merging this between an rc and a final release.

@anntzer
Copy link
Contributor

anntzer commented Feb 18, 2021

Can you also remove cid from pickled_cids, as suggested at the end of #19345 (comment)?

@tacaswell
Copy link
Member

@vallsv I took the liberty of pushing commits to this branch to address the remaining memory leak.

@QuLogic QuLogic merged commit 9184787 into matplotlib:master Feb 19, 2021
@QuLogic QuLogic mentioned this pull request Feb 19, 2021
Closed
@vallsv
Copy link
Contributor Author

vallsv commented Feb 19, 2021

Nice. Thanks for the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@jklymak jklymak jklymak left review comments

@timhoffm timhoffm timhoffm left review comments

@tacaswell tacaswell tacaswell approved these changes

@anntzer anntzer anntzer approved these changes

Assignees
No one assigned
Labels
Performance
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@vallsv @anntzer @tacaswell @jklymak @QuLogic @timhoffm