~~Please look at each of the first commit (option 1) and then the combination of both commits (net to option 2). Once we pick which one we like better I'll drop the other commit and fix / add tests.~~

We settled on option 2 in the discussion so I squashed option 1 out of existence.

Fixes#28827fixes#30400

tacaswell added this to thev3.10.0 milestone

Sep 17, 2024

github-actionsbot added the topic: text label

Sep 17, 2024

Copy link

Member

timhoffm commentedSep 17, 2024•
edited
Loading

Please look at each of the commits separately.

This tripped me up. "Please look separately at (i) commit 1, (ii) commit 1+2". is the way to go.

timhoffm reviewed

Sep 17, 2024

View reviewed changes

Copy link

Member

timhoffm left a comment

Choose a reason for hiding this comment

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

Trying to sum up the differences:

API: option 1 takes a weak ref, option 2 takes the renderer itself. - API-wise 2 is slightly more comfortable. The user does not have to care for weak refs. Also, the weakref-handling is better contained.
Behavior: The renderer cache in option 2 is unbound. Can that be an issue for long-running processes?
Both compared to the original implementation: We create separate caches per renderer. Before we had one large cache for renderer+settings. I assume this does not make a relevant difference

lib/matplotlib/text.py Outdated

		@functools.lru_cache(4096)
		def_text_metrics(text,fontprop,ismath,dpi):
		# dpi is unused, but participates in cache invalidation (via the renderer).
		returnrenderer_ref().get_text_width_height_descent(text,fontprop,ismath)

Copy link

Member

timhoffmSep 17, 2024

Choose a reason for hiding this comment

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

In option 2: don't we have to raise a RuntimeError (as in option 1) if the weakref does not resolve (renderer_ref() is None)?

Copy link

MemberAuthor

tacaswellSep 17, 2024

Choose a reason for hiding this comment

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

I thought about that, but if we are being called then we have (on behalf of the user) recently called_get_text_metrics_with_cache with a hard-ref so we are sure it is alive. That said, I'll put it back in.

This helper should probably be made__ private as well?

Copy link

MemberAuthor

tacaswell commentedSep 17, 2024

This tripped me up.

🤦🏻 yeah, that makes sense, the second diff in going to be weird. I thought putting them in the same PR would be simpler than two PRs and easier to deal with diffs in the comments of the issue.

Currently the only place we call the_impl function is

matplotlib/lib/matplotlib/text.py

Lines 65 to 70 in64d45cb

	def_get_text_metrics_with_cache(renderer,text,fontprop,ismath,dpi):
	"""Call ``renderer.get_text_width_height_descent``, caching the results."""
	# Cached based on a copy of fontprop so that later in-place mutations of
	# the passed-in argument do not mess up the cache.
	return_get_text_metrics_with_cache_impl(
	weakref.ref(renderer),text,fontprop.copy(),ismath,dpi)

and the only place we call_get_text_metrics_with_cache is

matplotlib/lib/matplotlib/text.py

Lines 372 to 384 in64d45cb

	# Full vertical extent of font, including ascenders and descenders:
	_,lp_h,lp_d=_get_text_metrics_with_cache(
	renderer,"lp",self._fontproperties,
	ismath="TeX"ifself.get_usetex()elseFalse,
	dpi=self.get_figure(root=True).dpi)
	min_dy= (lp_h-lp_d)*self._linespacing

	fori,lineinenumerate(lines):
	clean_line,ismath=self._preprocess_math(line)
	ifclean_line:
	w,h,d=_get_text_metrics_with_cache(
	renderer,clean_line,self._fontproperties,
	ismath=ismath,dpi=self.get_figure(root=True).dpi)

so I'm not too worried about API considerations.

option 2 is unbounded, but it is tied to the lifetime of the renderers it should not grow past the number of renderers the user implicitly keeps alive by the number ofFigures they keep alive.

In option 1 we keep some number of renderers alive (25) which is either going to cause incorrect misses for someone who has more than 25 figures alive (good idea or not, someone might really want this) and keep the cache too long in cases where the user is churning through figures one at a time.

I think the pro of option 1 is that it is "simpler" by using layered LRU cache, but option 2 is more complex but technically better.

In either case the two-tiered cache is the main "fix".

tacaswell force-pushed thefix/better_fm_cache branch frome93968c tob2f73a6Compare

September 17, 2024 14:32

Copy link

Member

timhoffm commentedSep 17, 2024

option 2 is unbounded, but it is tied to the lifetime of the renderers it should not grow past the number of renderers the user implicitly keeps alive by the number ofFigures they keep alive.

That's clever. 👍 I hadn't realized that. In this case I'm for option 2 plus a good comment why we do this and how it works 😃.

anntzer reviewed

Sep 17, 2024

View reviewed changes

lib/matplotlib/text.py OutdatedShow resolvedHide resolved

anntzer reviewed

Sep 17, 2024

View reviewed changes

lib/matplotlib/text.py Outdated

		# use mutable default to carry hidden global state
		def_get_text_metrics_function(inp_renderer,_cache=weakref.WeakKeyDictionary()):
		if (_text_metrics:=_cache.get(inp_renderer,None))isNone:
		renderer_ref=weakref.ref(inp_renderer)

Copy link

Contributor

anntzerSep 17, 2024

Choose a reason for hiding this comment

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

Do you actually need to explicitly create a weakref here? Doesn't using a WeakKeyDictionary basically do this for you for free already (i.e. aren't you having two layers of weakref'ing here?)? (not sure...)

Copy link

MemberAuthor

tacaswellSep 17, 2024

Choose a reason for hiding this comment

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

yes, if we do not the closure holds a hard reference mediated in the value side of the weakkey dict and they become immortal.

Copy link

Contributor

anntzerSep 17, 2024

Choose a reason for hiding this comment

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

Ah yes, I missed the fact that you are caching a closure.

Copy link

Contributor

anntzerSep 17, 2024

Choose a reason for hiding this comment

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

Perhaps the closure can be made more explicit, something like (untested)

cache=WeakKeyDictionary()# Can be hidden as an attribute or a private parameter...def_get_text_metrics_with_cache(renderer,text,fp,ismath,dpi):ifrenderernotincache:cache[renderer]=functools.lru_cache(4096)(functools.partial(_weak_text_metrics,weakref.ref(renderer)))returncache[renderer](text,fp.copy(),ismath,dpi)def_weak_text_metrics(renderer_ref,text,fp,ismath,dpi):returnrenderer_ref().get_text_width_height_descent(text,fp,ismath)

Copy link

MemberAuthor

tacaswellSep 18, 2024

Choose a reason for hiding this comment

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

I added a some comments. I think that the closure is marginally clearer than the use ofpartial.

anntzer reviewed

Sep 17, 2024

View reviewed changes

lib/matplotlib/text.py Outdated



		# use mutable default to carry hidden global state
		def_get_text_metrics_function(inp_renderer,_cache=weakref.WeakKeyDictionary()):

Copy link

Contributor

anntzerSep 17, 2024

Choose a reason for hiding this comment

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

Hiding the cache in a hidden mutable kwarg seems a bit weird to me (but sure, why not); I would rather have defined it as a custom attribute on the function (_get_text_metrics_function._cache = WeakKeyDictionary()). Just a stylistic choice, though.

Copy link

Member

timhoffmSep 17, 2024

Choose a reason for hiding this comment

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

When put into a keyword, it's easier to access it inside the function. - But needs documentation 😄

QuLogic reviewed

Sep 18, 2024

View reviewed changes

lib/matplotlib/text.py Outdated

		@functools.lru_cache(4096)
		def_text_metrics(text,fontprop,ismath,dpi):
		# dpi is unused, but participates in cache invalidation (via the renderer).
		if (lcl_renderer:=renderer_ref())isNone:

Copy link

Member

QuLogicSep 18, 2024

Choose a reason for hiding this comment

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

And I assumelcl here islocal as well? Probably can fit the two letters here too...

tacaswell force-pushed thefix/better_fm_cache branch fromdb121ec toa0e0809Compare

September 18, 2024 14:07

tacaswell changed the title~~Two proposals to improve the cache when getting font metrics~~Improve the cache when getting font metrics

Sep 18, 2024

tacaswell marked this pull request as ready for review

September 18, 2024 14:09

timhoffm reviewed

Sep 18, 2024

View reviewed changes

lib/matplotlib/text.py OutdatedShow resolvedHide resolved

timhoffm reviewed

Sep 18, 2024

View reviewed changes

Copy link

Member

timhoffm left a comment

Choose a reason for hiding this comment

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

Very nice documentation 👍

timhoffm approved these changes

Sep 18, 2024

View reviewed changes

anntzer reviewed

Sep 18, 2024

View reviewed changes

lib/matplotlib/text.pyShow resolvedHide resolved

QuLogic reviewed

Sep 19, 2024

View reviewed changes

lib/matplotlib/tests/test_text.py Outdated



		deftest_metrics_cache2():
		plt.close('all')

Copy link

Member

QuLogicSep 19, 2024

Choose a reason for hiding this comment

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

All tests should start with no figure open due to the test fixture, so this should be unnecessary?

Copy link

MemberAuthor

tacaswellSep 19, 2024

Choose a reason for hiding this comment

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

This was a long shot that something was getting not cleared because the assert that is failing is before we do anything in this test.

Copy link

Contributor

anntzerAug 26, 2025

Choose a reason for hiding this comment

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

Agreed the close("all") should likely go.

tacaswell modified the milestones:v3.10.0,v3.11.0

Oct 2, 2024

tacaswell force-pushed thefix/better_fm_cache branch from1e50b6f tob047764Compare

August 8, 2025 02:12

tacaswell mentioned this pull request

Aug 8, 2025

[Bug]: Megabyte-level memory leak when using imshow() in a loop#30400

Closed

Copy link

MemberAuthor

tacaswell commentedAug 8, 2025

We have now had 2 nearly identical reports tied to this.

@QuLogic Does this conflict with all of your font work?

Copy link

Member

QuLogic commentedAug 8, 2025•
edited
Loading

@QuLogic Does this conflict with all of your font work?

I don't believe I've made any changes here, and though I would like to replace it with FreeType's caching mechanism, I haven't really done any work on that.

anntzer reviewed

Aug 26, 2025

View reviewed changes

lib/matplotlib/text.py OutdatedShow resolvedHide resolved

anntzer reviewed

Aug 26, 2025

View reviewed changes

lib/matplotlib/text.pyShow resolvedHide resolved

anntzer reviewed

Aug 26, 2025

View reviewed changes

lib/matplotlib/tests/test_text.pyShow resolvedHide resolved

tacaswell mentioned this pull request

Sep 11, 2025

[Bug]: memory baking figure is not freed when figure is closed#29743

Open

tacaswell force-pushed thefix/better_fm_cache branch fromb047764 toae5af7fCompare

September 11, 2025 15:28

tacaswelland others added2 commits

October 16, 2025 14:24

MNT: improve how we manage the cache for font metrics

11fc120

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

TST: clear renderer cache in tests

94d6fc8

Only on windows test_metrics_cache2 was starting with 2 renderersin the cache.Given that his is only happening on windows (and not osx/linux) on CI goingwith the assumption that these is something being held alive longer than weexpect someplace else in the tests.

tacaswell force-pushed thefix/better_fm_cache branch from5c18c09 to94d6fc8Compare

October 16, 2025 18:24