Skip to content

Conversation

@valadaptive
Copy link
Contributor

As discussed in #509, we should be able to render multiple things (fill, stroke, underlines) from a single GlyphRunBuilder.

This PR introduces the GlyphRunRenderer, which is what the GlyphRunBuilder now actually builds. It can perform multiple rendering operations without being consumed, allowing a lot of work to be reused.

On my machine, this shows around a 5% improvement on all the end-to-end drawing benchmarks. For some reason, this includes the non-underline ones.

The snapshot tests appear different because I forgot to disable hinting when rendering underlines, which results in very subtle changes to their appearance. Since the GlyphRunRenderer comes from a single GlyphRunBuilder, it must use the same hinting setting for both the glyphs and underlines, so we need to make them match. I fixed the hinting setting and regenerated the snapshots in a separate commit--the optimization itself does not affect the snapshots at all.

};

for glyph in glyphs {
for glyph in self.glyph_iterator.clone() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this API means that the consumer needs to become implementation aware to avoid an expensive clone. It feels like a bit of a footgun.

Wouldn't passing Vec<Glyph>.into_iter() here cause an expensive clone of the underlying data?

My feeling is that we should nudge the consumer into an API that requires only a single iteration of the glyphs. Extracting their outlines, doing the hashmap lookup, etc are things that we want to minimise.

I.e., instead of creating an API that nudges consumption into re-running this loop for any decoration/fill/stroke glyph, just ask the consumer up-front what they want to render and then render that. Since this is already borrowed data, I can't help but think that the most common usage pattern will be:

let run_renderer = /* build run renderer */
run_renderer.fill_glyphs(...);
if has_underline {
   run_renderer.render_decoration(...)
}

Whereas, the "speed of light" implementation/API looks like:

let run_renderer = /* build with or without decoration */
run_renderer.render()

Since we can ensure only 1 hashmap lookup occurs per glyph

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