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.

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

Replace maxdict with lru_cache #52

Merged
merged 1 commit into from
Oct 22, 2024
+15 −15
Merged

Replace maxdict with lru_cache #52

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions matplotlib_pyodide/html5_canvas_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import base64
import io
import math
from functools import lru_cache

import numpy as np
from matplotlib import __version__, interactive
Expand All @@ -10,7 +11,6 @@
RendererBase,
_Backend,
)
from matplotlib.cbook import maxdict
from matplotlib.colors import colorConverter, rgb2hex
from matplotlib.font_manager import findfont
from matplotlib.ft2font import LOAD_NO_HINTING, FT2Font
Expand Down Expand Up @@ -204,8 +204,8 @@ def __init__(self, ctx, width, height, dpi, fig):
self.ctx.width = self.width
self.ctx.height = self.height
self.dpi = dpi
self.fontd = maxdict(50)
self.mathtext_parser = MathTextParser("bitmap")
self._get_font_helper = lru_cache(maxsize=50)(self._get_font_helper)
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between adding a lru_cache decorator on top of self._get_font_helper, or re-defining a variable like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two things:

  1. Decorating the method makes one shared cache across all class instances, this makes a per-instance cache. It actually might make more sense to have a shared cache here but I don't understand what's going on well enough to know.
  2. The cache holds a strong reference to the arguments of the function including theself argument so if you decorate the method directly then instances can be kept alive by the cache. This instead makes a reference loop but the gc will collect it.

It would probably be better to shift this to a top level or static method and use a shared cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the main concern is that the font objects appear to be mutable. I wonder if there is a good way to copy them so we can avoid mutating cache entries...

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks for the explanation!


# Keep the state of fontfaces that are loading
self.fonts_loading = {}
Expand Down Expand Up @@ -309,22 +309,22 @@ def draw_image(self, gc, x, y, im, transform=None):
pixels_proxy.destroy()
pixels_buf.release()

def _get_font_helper(self, prop):
"""Cached font lookup

We wrap this in an lru-cache in the constructor.
"""
fname = findfont(prop)
font = FT2Font(str(fname))
font_file_name = fname.rpartition("/")[-1]
return (font, font_file_name)

def _get_font(self, prop):
key = hash(prop)
font_value = self.fontd.get(key)
if font_value is None:
fname = findfont(prop)
font_value = self.fontd.get(fname)
if font_value is None:
font = FT2Font(str(fname))
font_file_name = fname[fname.rfind("/") + 1 :]
font_value = font, font_file_name
self.fontd[fname] = font_value
self.fontd[key] = font_value
font, font_file_name = font_value
result = self._get_font_helper(prop)
font = result[0]
font.clear()
font.set_size(prop.get_size_in_points(), self.dpi)
return font, font_file_name
return result

def get_text_width_height_descent(self, s, prop, ismath):
w: float
Expand Down
Loading