-
Notifications
You must be signed in to change notification settings - Fork 9
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
Replace maxdict with lru_cache #52
base: main
Are you sure you want to change the base?
Conversation
Requires an update to matplotlib-pyodide before it will work: pyodide/matplotlib-pyodide#52
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
======================================
Coverage ? 0.00%
======================================
Files ? 4
Lines ? 648
Branches ? 0
======================================
Hits ? 0
Misses ? 648
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
self.mathtext_parser = MathTextParser("bitmap") | ||
self._get_font_helper = lru_cache(maxsize=50)(self._get_font_helper) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- 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.
- The cache holds a strong reference to the arguments of the function including the
self
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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!
Matplotlib removed maxdict:
https://matplotlib.org/stable/api/prev_api_changes/api_changes_3.6.0.html#miscellaneous-internals
I asked about this on the matplotlib discourse:
https://discourse.matplotlib.org/t/matplotlib-pyodide-font-caching-with-maxdict-and-lru-cache/24260