Page MenuHome

BLF: make library glyph cache handling thread-safe
ClosedPublic

Authored by Richard Antalik (ISS) on Fri, Sep 6, 12:07 AM.

Details

Summary

Functions that utilize glyph cache should lock and unlock cache by
calling blf_glyph_cache_acquire() and blf_glyph_cache_release().
Function blf_glyph_cache_acquire() will create glyph cache, if it doesn't exist.
Locking mutex is global and shared by all fonts.

Diff Detail

Repository
rB Blender

Event Timeline

  • Add lock for BLF API function which failed with VSE prefething. This wasn't immediately obvious before testing.
Brecht Van Lommel (brecht) requested changes to this revision.Fri, Sep 6, 12:26 PM

Be sure to test the performance impact of this on regular drawing. You can do this by creating a window with lots of text, and then operator search > Redraw Timer.

source/blender/blenfont/intern/blf.c
611

if (font) goes before blf_glyph_cache_acquire(font)

This should not use font->glyph_cache. Rather it should get the glyph cache pointer from blf_glyph_cache_acquire() and pass it to all functions that use it.

Otherwise it's hard to guarantee that all functions are properly covered by acquire/releases.

This revision now requires changes to proceed.Fri, Sep 6, 12:26 PM

Be sure to test the performance impact of this on regular drawing. You can do this by creating a window with lots of text, and then operator search > Redraw Timer.

I will test this.
This patch should not affect performance unless multiple threads are drawing fonts or clearing cache. Doing so does have measurable impact on font drawing performance.

source/blender/blenfont/intern/blf.c
611

I am aware, that this is not ideal. but then I would have to understand the function of whole library to do this properly, obviously...

I didn't see the scope, so I feard that I will loose too much time there.
But it's quite simple library, so I think I can do this properly.

So I won't lock API functions, but move few levels deeper, where glyph cache is actually being used. I think that passing cache in dependency-injection style is not quite appropriate either.

  • Lock glyph cache only in functions, where glyph cache is actually needed
  • fix errors and warnings
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Fri, Sep 13, 1:20 AM
Richard Antalik (ISS) updated this revision to Diff 18184.

I ended up passing glyph cache around, because of callbacks and few other functions. But it's nothing wild, like I feared it could be.

I think that blf_font_size() could be a bit nicer, or I could directly use spin lock here.
Other than that all access to font->glyph_cache should be protected.
Not sure if it makes sense to protect blf_font_fill().

Also ft_lib_mutex could be removed at this point I think. Technically it has different function, but the only function that uses it is protected by glyph cache mutex.

font->glyph_cache is still visible, but that is by design necessary at this point, I left comment there to use acquire / release functions.

I havent measured performance impact yet, but to me it seems unchanged.
Sorry for whitespace - OCD. I will clean it that would be better.

Richard Antalik (ISS) marked an inline comment as done.Fri, Sep 13, 1:20 AM

Compiler warnings:

/home/brecht/dev/worktree/source/blender/blenfont/intern/blf_font.c: In function ‘blf_font_width_to_strlen’:
/home/brecht/dev/worktree/source/blender/blenfont/intern/blf_font.c:733:9: warning: ‘g’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (blf_font_width_to_strlen_glyph_process(
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             font, has_kerning, kern_mode, c_prev, c, g_prev, g, &pen_x, width_i)) {
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brecht/dev/worktree/source/blender/blenfont/intern/blf_font.c: In function ‘blf_font_width_to_rstrlen’:
/home/brecht/dev/worktree/source/blender/blenfont/intern/blf_font.c:785:9: warning: ‘c_prev’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (blf_font_width_to_strlen_glyph_process(
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             font, has_kerning, kern_mode, c_prev, c, g_prev, g, &pen_x, width_i)) {
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brecht/dev/worktree/source/blender/blenfont/intern/blf_font.c:785:9: warning: ‘g’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Actually those warnings are not caused by this change, they were already there. I will fix them.

The patch itself looks good to me. It's quite straightforward, and I measured no performance regression.

This revision is now accepted and ready to land.Fri, Sep 13, 3:24 PM