BLF: Mutex Lock Glyph Cache Per Font, Not Global
ClosedPublic
Actions

Authored by Harley Acheson (harley) on Aug 8 2022, 8:24 PM.

Details

Reviewers

Brecht Van Lommel (brecht)

Commits

rBd2b1e4712dc1: BLF: Mutex Lock Glyph Cache Per Font, Not Global

Summary

Only lock access to our glyph caches per-font, rather than globally.
Also upgrade from spinlocks to mutexes.

Each FontBLF has a glyph cache that is a resource that must be protected with locks to ensure that one thread does not delete/change while another reads. However, we are locking globally, rather than per-font. This means we cannot access the cache of a font and then do so for different font before releasing that first lock.

This global locking was necessary for early versions of FreeType, but that library is now perfectly threadsafe if a mutex lock is used around uses of FT_New_Face, FT_New_Memory_Face, and FT_Done_Face (which we do). https://freetype.org/freetype2/docs/reference/ft2-base_interface.html#ft_library

This patch also changes to the lock types from spinlocks to a mutexes as mutex is a better fit for this use and is also considered better in almost all cases.

Diff Detail

Repository: rB Blender

Event Timeline

Harley Acheson (harley) requested review of this revision.Aug 8 2022, 8:24 PM

Harley Acheson (harley) created this revision.

Harley Acheson (harley) mentioned this in D15645: BLF: Remove blf_mono_font_render.Aug 8 2022, 9:27 PM

There's also GPU operations happening within these locks, but that should be ok as well since acquiring the GPU context already involves holding a lock.

This revision is now accepted and ready to land.Aug 9 2022, 5:44 PM

This will need changes after the cache removal. I think mostly just locking around ft_face_done.

Might as well look at updating the library spinlock to a mutex while doing so.

Without the cache (at least for now)...

A ThreadMutex, instead of a SpinLock, for the FT_Library object. Init in blf_font_init, end in blf_font_exit. Locking only around FT_New_Face, FT_New_Memory_Face, and FT_Done_Face, as per https://freetype.org/freetype2/docs/reference/ft2-base_interface.html#ft_library

A ThreadMutex, instead of a SpinLock, for each FontBLF. Init in blf_font_new_ex, end in blf_font_free. Lock in blf_glyph_cache_acquire, unlock in blf_glyph_cache_release. Lock/Unlock in blf_glyph_cache_clear.

This revision is now accepted and ready to land.Aug 11 2022, 3:56 AM

Brecht Van Lommel (brecht) accepted this revision.Aug 11 2022, 3:46 PM

+1 for the mutex, while i don't know what "GPU Operations" would entail, it does sound a lot longer than one should be holding a spinlock for

Only adding a single comment.

Closed by commit rBd2b1e4712dc1: BLF: Mutex Lock Glyph Cache Per Font, Not Global (authored by Harley Acheson (harley)). · Explain WhyAug 11 2022, 9:53 PM

This revision was automatically updated to reflect the committed changes.

Harley Acheson (harley) added a commit: rBd2b1e4712dc1: BLF: Mutex Lock Glyph Cache Per Font, Not Global.

Harley Acheson (harley) mentioned this in T94030: Font Changes (Parent Task).Aug 14 2022, 6:02 PM