Cache varstore #3605
Merged
Cache varstore #3605
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I see 4% speedup in shaping of a simple variable font, and up to 32% speedup in shaping of RobotoFlex, which is a heavy variable font. Even for shaping of single words, 19% speedup. Non-variable font shaping performance is unaffected. These numbers are with hb font-funcs. Just looking at advance-width speedup, for simple variable font, 30% speedup, for RobotoFlex, 50%. |
Is faster. With this, I'm seeing 25 to 28% speedup in glyph_h_advances benchmark of benchmark-font for var/hb tests.
It's available so no need to use void*.
|
Thanks for doing this optimisation. |
drott
reviewed
May 24, 2022
| @@ -310,7 +311,7 @@ struct hmtxvmtx | |||
|
|
|||
| unsigned int default_advance; | |||
|
|
|||
| private: | |||
| public: | |||
There was a problem hiding this comment.
Why is visiblity changing here?
There was a problem hiding this comment.
var_table is now accessed from hb-ot-font to access the VarStore to create the cache.
| { | ||
| if (unlikely (region_index >= regionCount)) | ||
| return 0.; | ||
|
|
||
| float *cached = nullptr; |
There was a problem hiding this comment.
Nit: cached and cache as variable names in the same scope here read somewhat confusingly. Maybe cached_value?
| @@ -335,7 +340,7 @@ struct HVARVVAR | |||
|
|
|||
| bool has_side_bearing_deltas () const { return lsbMap && rsbMap; } | |||
|
|
|||
| protected: | |||
| public: | |||
There was a problem hiding this comment.
Similar question here, why does the visibility need to change - I didn't understand that.
There was a problem hiding this comment.
Same. Need to reach out to the varStore. Can add accessors. But these are safe to access. In those cases we just make them public.
behdad
added a commit
that referenced
this pull request
May 24, 2022
|
Hi @behdad - sorry to be so late responding to this, I've been out for a while (first PTO and then Covid)... trying to catch up on things. I tried a Firefox build with this applied, and can confirm that it appears to help. For a huge reflow (a 40MB text block) using the Apple system font, which is variable (but not very complex), it reduced the time spent under VariationDevice during GPOS pair-pos processing from around 500ms to 300ms. Obviously the specific gains will be depend on characteristics of the font being used, but in any case it's a nice win - thanks! |
|
Thanks @jfkthame |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issue originally raised in #2878 (comment)
Hi Jonathan,
Can you test this in Firefox and see if it helps with variable font performance?
Thanks