[cairo] The glyph cache (was Re: [cairo-commit] 10 commits )

[Behdad Esfahbod]

On Thu, 2008-05-08 at 11:26 +0100, Chris Wilson wrote:
> On Thu, 2008-05-08 at 11:53 -0400, Behdad Esfahbod wrote:
> > On Wed, 2008-05-07 at 00:22 -0700, Chris Wilson wrote:
> > > 
> > > +       to_free = glyphset_info->pending_free_glyphs;
> > > +       if (to_free != NULL &&
> > > +           to_free->glyph_count == ARRAY_LENGTH
> > > (to_free->glyph_indices))
> > > +       {
> > > +           status = _cairo_xlib_display_queue_work
> > > (font_private->display,
> > >                     (cairo_xlib_notify_func)
> > > _cairo_xlib_render_free_glyphs,
> > > -                   arg,
> > > +                   to_free,
> > >                     free);
> > > -           if (status) {
> > > -               /* XXX cannot propagate failure */
> > > -               free (arg);
> > > +           /* XXX cannot propagate failure */
> > > +           if (status)
> > > +               free (to_free);
> > > +
> > > +           to_free = glyphset_info->pending_free_glyphs = NULL;
> > > +       }
> > > +
> > > +       if (to_free == NULL) {
> > > +           to_free = malloc (sizeof
> > > (cairo_xlib_font_glyphset_free_glyphs_t));
> > > +           if (to_free == NULL) {
> > > +               _cairo_error_throw (CAIRO_STATUS_NO_MEMORY);
> > > +               return; /* XXX cannot propagate failure */
> > >             }
> > 
> > Here you can avoid freeing the to_free array and allocating it again...
> > 
> > Also this definitely doesn't look right:
> > 
> > 
> > > +           if (status)
> > > +               free (to_free);
> > > +
> > > +           to_free = glyphset_info->pending_free_glyphs = NULL;
> > 
> > Aren't you leaking to_free in case of success??
> 
> Here we are trying to append the array of expired glyphs to the work
> queue and in doing so transfer ownership of the array to the work queue
> - which will call our destructor (free) when it has completed. So if
> that fails, we free the array and forget about all the glyphs we wanted
> to release. Fortunately, this is just a minor bookkeeping error in our
> monitoring of server-side resources and not a gargantuan glyph leak!

Ah I see.  Right.

I was also wondering how good an idea what you are doing here is.
Basically we have a cache of 256 glyphs per scaled-font, then you are
essentially keeping a cache of an extra 128 glyphs on the server side.
I understand the motivation behind this, but sounds weird.

What about making cairo's glyph cache go beyond the 256, up to say 384
before removing anything from the cache?  Then remove down to the 256
again.  That still doesn't fix your problem as the glyphs will be freed
one at a time, at least you get 128 glyph free operations in a row, and
if you choose your free buffer to be the same size (as it is now), you
fill it straight, and then you can send it over and free the list.  That
is, move the check for 128 to after appending a glyph to it.


Also makes me wonder if we can/should change the glyph cache to do LRU.
Doing LRU then makes it easier to use a dynamic cache size instead of
hardcoding 256.  We know that for East-Asian languages our current cache
performs really bad.

Just some ideas...


-- 
behdad
http://behdad.org/

"Those who would give up Essential Liberty to purchase a little
 Temporary Safety, deserve neither Liberty nor Safety."
        -- Benjamin Franklin, 1759