Skip to content
This repository

use class level rather than instance level caching of fontd #798

Merged
merged 3 commits into from about 2 years ago

4 participants

John Hunter Michael Droettboom cgohlke ruidc
John Hunter
Owner

windows users are reporting too many open file handles exceptions which appear to be triggered by too many font files being opened and parsed. I think we may be able to dramatically reduce these for normal use cases by caching the agg font dictionary at the renderer class level rather than instance level.

This needs testing and confirmation by a windows user before merging.

Michael Droettboom
Owner

Ah. Good solution.

John Hunter
Owner

Because this problem seems to clearly stem from too many font file openings, I am surprised that the call to FT_Done_Face in the FT2Font destructor is not enough to take care of this (in conjunction with garbage collection). This makes me wonder if we need to be triggering garbage collecting during the tests. When pyplot.close is called on a figure, it triggers a gc collect via _pylab_helpers.Gcf.destroy, and the CleanupTest does call close('all') on every test, so we should be getting a gc.collect on every test. So i don't understand why these font openings are creating too many open handles during the test. This leads me to one other possibility: we have been building and packaging freetype with the mpl windows release since before the dawn of time. If we haven't updated our version of freetype in a long time that we a re bundling, could we be seeing a bug in that version. @cgohlke , what version are you compiling mpl against, and if it's ancient could you consider upgrading? It would interesting to know if this fixes the problem even w/o this PR.

Interestingly, backend_ps and backend_pdf already do the caching at the renderer class level. Unsure, why agg was doing it at the renderer level, I went to check git blame to see if maybe it had been moved in and we could get some insight into why. Unfortunately, I found a commit by me entitled "moved agg caches to instance namespace for thread safety" 175e3ec so this was done for a reason. So we probably can't just merge this as is: the question is, can we get a smarter class level font cache that implements some sort of thread safe locking? I am a thread novice, so don't feel like I am the best person to address this.

@cgohlke , because there is a problem with this PR and thread safety, it doubles my interest in the question above: can we fix this by upgrading freetype? And does anyone else have any ideas why these handles are not getting freed? To rehash the above, in v1.1.x the font file handles are in the FT2Font instances, these font objects are in a Renderer instance level dict associated with the figure, the figure is getting closed by the test cleanup, this triggers gc.collect, and this should trigger the destruction of the figure which should lead to the call to the FT2Font destructor which in turn calls FT_Done_Face. In that pipeline, the file handle should be freed. So it looks like the logic is right on the mpl end.

cgohlke

I am using freetype 2.4.9, the latest version available.

John Hunter
Owner

OK, scratch the second commit. I think the right approach here is to use a threading Lock right before drawing and release it right after drawing. Now we have a single class level cache that is locked during a single renderer draw.

ruidc
ruidc commented March 26, 2012

OK, scratch the second commit.
I'd like to test but am unclear as to what to test given the above line.

John Hunter
Owner

@mdboom the PR is good to test now. I had tried something more involved in the second commit, and ended up pulling it all for the simpler lock in the third commit. I had also written a long comment after the second commit which I subsequently deleted. So "scratch the second" commit is just me talking to myself and anyone reading the history to say that that line wasn't fruitful. I think what we have in there now is good.

ruidc
ruidc commented March 26, 2012

Thanks, replacing the rc version of backend_agg.py with the one in a00e510 resolves this issue for me on win32 and 64 and passes all the tests.

cgohlke

Works for me on win-amd64-py2.7

Michael Droettboom
Owner

This looks good to me, also. I say we merge. Still agreed?

John Hunter
Owner

Yep, go ahead. Sorry I've been out of the loop recently. Just getting back from vacation and will have some time to close loose ends and finish this release

Michael Droettboom mdboom merged commit fe1e48a into from April 13, 2012
Michael Droettboom mdboom closed this April 13, 2012
Michael Droettboom mdboom referenced this pull request May 14, 2012
Closed

File closing #795

hankhank hankhank referenced this pull request November 08, 2012
Closed

Too many open files #1466

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

Showing 3 unique commits by 1 author.

Mar 23, 2012
John Hunter use class level rather than instance level caching of fonts to delay …
…the too many open file handles windows bug
aa4bb8a
Mar 24, 2012
John Hunter slurp down a class level cache into the instance level at the start o…
…f draw, and push it back up when done
cc628ce
John Hunter use threading Lock to protect class level font cache a00e510
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 32 additions and 8 deletions. Show diff stats Hide diff stats

  1. 40  lib/matplotlib/backends/backend_agg.py
40  lib/matplotlib/backends/backend_agg.py
@@ -21,7 +21,7 @@
21 21
   * integrate screen dpi w/ ppi and text
22 22
 """
23 23
 from __future__ import division
24  
-
  24
+import threading
25 25
 import numpy as np
26 26
 
27 27
 from matplotlib import verbose, rcParams
@@ -46,11 +46,24 @@ class RendererAgg(RendererBase):
46 46
     context instance that controls the colors/styles
47 47
     """
48 48
     debug=1
  49
+
  50
+    # we want to cache the fonts at the class level so that when
  51
+    # multiple figures are created we can reuse them.  This helps with
  52
+    # a bug on windows where the creation of too many figures leads to
  53
+    # too many open file handles.  However, storing them at the class
  54
+    # level is not thread safe.  The solution here is to let the
  55
+    # FigureCanvas acquire a lock on the fontd at the start of the
  56
+    # draw, and release it when it is done.  This allows multiple
  57
+    # renderers to share the cached fonts, but only one figure can
  58
+    # draw at at time and so the font cache is used by only one
  59
+    # renderer at a time
  60
+
  61
+    lock = threading.Lock()
  62
+    _fontd = maxdict(50)
49 63
     def __init__(self, width, height, dpi):
50 64
         if __debug__: verbose.report('RendererAgg.__init__', 'debug-annoying')
51 65
         RendererBase.__init__(self)
52 66
         self.texd = maxdict(50)  # a cache of tex image rasters
53  
-        self._fontd = maxdict(50)
54 67
 
55 68
         self.dpi = dpi
56 69
         self.width = width
@@ -69,6 +82,7 @@ def __init__(self, width, height, dpi):
69 82
         if __debug__: verbose.report('RendererAgg.__init__ done',
70 83
                                      'debug-annoying')
71 84
 
  85
+
72 86
     def _get_hinting_flag(self):
73 87
         if rcParams['text.hinting']:
74 88
             return LOAD_FORCE_AUTOHINT
@@ -82,7 +96,7 @@ def draw_markers(self, *kl, **kw):
82 96
 
83 97
     def draw_path_collection(self, *kl, **kw):
84 98
         return self._renderer.draw_path_collection(*kl, **kw)
85  
-        
  99
+
86 100
     def _update_methods(self):
87 101
         #self.draw_path = self._renderer.draw_path  # see below
88 102
         #self.draw_markers = self._renderer.draw_markers
@@ -215,15 +229,16 @@ def _get_agg_font(self, prop):
215 229
                                      'debug-annoying')
216 230
 
217 231
         key = hash(prop)
218  
-        font = self._fontd.get(key)
  232
+        font = RendererAgg._fontd.get(key)
219 233
 
220 234
         if font is None:
221 235
             fname = findfont(prop)
222  
-            font = self._fontd.get(fname)
  236
+            font = RendererAgg._fontd.get(fname)
223 237
             if font is None:
224 238
                 font = FT2Font(str(fname))
225  
-                self._fontd[fname] = font
226  
-            self._fontd[key] = font
  239
+                RendererAgg._fontd[fname] = font
  240
+
  241
+            RendererAgg._fontd[key] = font
227 242
 
228 243
         font.clear()
229 244
         size = prop.get_size_in_points()
@@ -358,6 +373,7 @@ def post_processing(image, dpi):
358 373
                                       image)
359 374
 
360 375
 
  376
+
361 377
 def new_figure_manager(num, *args, **kwargs):
362 378
     """
363 379
     Create a new figure manager instance
@@ -398,7 +414,15 @@ def draw(self):
398 414
         if __debug__: verbose.report('FigureCanvasAgg.draw', 'debug-annoying')
399 415
 
400 416
         self.renderer = self.get_renderer()
401  
-        self.figure.draw(self.renderer)
  417
+        # acquire a lock on the shared font cache
  418
+        RendererAgg.lock.acquire()
  419
+
  420
+        try:
  421
+            self.figure.draw(self.renderer)
  422
+        finally:
  423
+            RendererAgg.lock.release()
  424
+
  425
+
402 426
 
403 427
     def get_renderer(self):
404 428
         l, b, w, h = self.figure.bbox.bounds
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.