Skip to content
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

Cache rustybuzz shape plans for vastly improved performance. #207

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

vorporeal
Copy link
Contributor

I added support to rustybuzz to cache shape plans (harfbuzz/rustybuzz#88), which has been released in rustybuzz v0.12.0.

This PR updates cosmic-text to make use of the new rustybuzz functionality by adding a ShapePlanCache, storing shape plans (with an appropriate cache key), and passing the cached shape plan to rustybuzz::shape_with_plan (instead of rustybuzz::shape) to avoid re-paying the cost of computing the shape plan.

A couple notes:

  • I renamed cache.rs to glyph_cache.rs now that there's a shape_plan_cache.rs, but kept the names of symbols within it generic (e.g.: Cache) as they're public outside of the crate.
  • The shape plan cache does not have a bounded size. I believe this is ok, due to the fact that:
    1. Nothing sets a language on the shaping buffer,
    2. I would expect there to only be one direction per script value (not to mention the fact that the max multiplication factor is 2), and
    3. I would expect there to usually be only one script value per font (e.g.: Latin/Hebrew/Chinese).
  • Exposing the ShapePlanCache via FontFallbackIter is nasty, but there isn't a great alternative given the fact that ShapePlanCache is held by FontSystem and FontFallbackIter holds a &mut FontSystem and we need a &mut ShapePlanCache in shape_fallback. That said, this nastiness is narrowly scoped and entirely internal to this crate.

All tests in cosmic-text still pass with this change.

For some anecdotal evidence on performance improvements - all font shaping/layout performance issues we were seeing in our app (https://github.com/warpdotdev/Warp) have disappeared with this patch.

@jackpot51 jackpot51 merged commit 73acfb0 into pop-os:main Dec 20, 2023
0 of 2 checks passed
@jackpot51
Copy link
Member

Thanks!

@jackpot51
Copy link
Member

If cosmic-text is being used by warp please consider mentioning this in #177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants