Fix TkAgg memory leaks and test for memory growth regressions #22002
Conversation
|
Hopefully adding a test dependency in a patch release will not be too troublesome for the packagers. Assume the escape hatch to patch out the test. |
|
@richardsheridan Thank you for tracking this down and 👍🏻 on adding that test! |
|
@tacaswell should I XFAIL the test for now or just let it fail entirely? |
|
Can you uses |
|
This PR is blocked by the hidpi leak that maybe only @QuLogic can debug. Otherwise we could adjust the memory leak threshold higher so it passes for now. That way the hidpi leak can be fixed later and th threshold can be lowered again at that time. Thoughts? |
af9d83f to
b59a89e
Compare
|
I don't have Windows to check the HiDPI code right now. But I'm not sure how it could be leaking? The only HiDPI variable is attached to the window (the figure manager does |
|
I suppose there's also the additional |
|
If I run $ git diff
diff --git a/tools/memleak.py b/tools/memleak.py
index 9b9da912b7..d61d0b57c2 100755
--- a/tools/memleak.py
+++ b/tools/memleak.py
@@ -79,10 +79,10 @@ def run_memleak_test(bench, iterations, report):
ax3.plot(open_files_arr)
ax3.set_ylabel('open file handles')
- if not report.endswith('.pdf'):
- report = report + '.pdf'
+ if not report.endswith('.png'):
+ report = report + '.png'
fig.tight_layout()
- fig.savefig(report, format='pdf')
+ fig.savefig(report, format='png')
class MemleakTest:
@@ -115,7 +115,7 @@ class MemleakTest:
ax.pcolor(10 * np.random.rand(50, 50))
fig.savefig(BytesIO(), dpi=75)
- fig.canvas.flush_events()
+ # fig.canvas.flush_events()
plt.close(1)
Main branch
this branch
This clearly shows that this branch fixes something, but that the warm up time is in the hundreds of figures which is not practical for the test suite. |
|
I took the test and put it in a standalone script, and ran it through scalene. Unfortunately, it seems to break Tk threading with explicit That being said, @tacaswell noticed that we explicitly deleted the image created from the photo on the canvas during resize: matplotlib/lib/matplotlib/backends/_backend_tk.py Lines 235 to 239 in 0e46ff2 So I wonder if we're somehow missing deletion of that image when destroying the figure? |
tkinter variables get cleaned up with normal `destroy` and `gc` semantics but tkinter's implementation of trace is effectively global and keeps the callback object alive until the trace is removed.
we know we have created a bunch of cycles with heavyweight GUI objects, but they won't be in gen 1 yet
|
I made a bit of progress on this thanks to learning about I say mostly, but actually all were gone but one; the I manually unregister the trace in a3016af, and accellerate the garbage collection in e6e4cad. This gave me flat graphs for memory and object behavior in I updated the test to flush events after both creating and closing the figure and adjusted the thresholds. It passes on my machine and we'll see if CI accepts it. |
|
If I change $ git diff
diff --git a/tools/memleak.py b/tools/memleak.py
index 9b9da912b7..6dbc9dc505 100755
--- a/tools/memleak.py
+++ b/tools/memleak.py
@@ -115,8 +121,9 @@ class MemleakTest:
ax.pcolor(10 * np.random.rand(50, 50))
fig.savefig(BytesIO(), dpi=75)
- fig.canvas.flush_events()
+ plt.pause(.1)
plt.close(1)
if __name__ == '__main__':I see continued growth in RSS in the tkagg backend, but constant object count which is disturbing.
Nevertheless, the growth is muted compared to before the patches. |
This reverts commit 2829677
|
I cannot reproduce the error in CI that has to do with a missing |
based on suggestion from code review
Based on suggestion from code review
Without it, checking the 'external' memory may indicate a leak even if we've gotten rid of all our objects.
|
I pushed a couple of updates to the tests that should hopefully fix them. At least, they appear fine locally. |
As apparently dropping this was needed for Tk.
|
thanks @QuLogic I'm happy with your changes fwiw. this one is a definite squash merge when the time comes |
|
For reference, using the example from #20490 (comment) (+ a |
|
Thank you @richardsheridan and @QuLogic ! |
|
@meeseeksdev backport to v3.5.x |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
…ory growth regressions FIX: TkAgg memory leaks and test for memory growth regressions (matplotlib#22002) tkinter variables get cleaned up with normal `destroy` and `gc` semantics but tkinter's implementation of trace is effectively global and keeps the callback object alive until the trace is removed. Additionally extend and clean up the tests. Closes matplotlib#20490 Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> (cherry picked from commit 1a016f0)
…ory growth regressions FIX: TkAgg memory leaks and test for memory growth regressions (matplotlib#22002) tkinter variables get cleaned up with normal `destroy` and `gc` semantics but tkinter's implementation of trace is effectively global and keeps the callback object alive until the trace is removed. Additionally extend and clean up the tests. Closes matplotlib#20490 Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> (cherry picked from commit 1a016f0)
…-v3.5.x Backport PR #22002: Fix TkAgg memory leaks and test for memory growth regressions
PR Summary
This PR is an attempt to fix #20490. Basically it allows a normally-forbidden
self.window.update()if we detect that thetkintermainloop is not running. The new test currently still fails though because of some additional leak related to the HiDPI stuff in #19167.I took the liberty of introducing a new dependency in the test suite. Let me know if I should insert it into any other files or if you would rather find a different way to observe the leak.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
New features are documented, with examples if plot related.New features have an entry indoc/users/next_whats_new/(follow instructions in README.rst there).API changes documented indoc/api/next_api_changes/(follow instructions in README.rst there).Documentation is sphinx and numpydoc compliant (the docs should build without error).