Shape plan caching

[laurmaedje]

Shape plan caching was dropped during the port. However, in some experiments I did a while ago, creating the shape plan was a nontrivial amount of the work done. Looking at the definition of fn shape, it seems to be trivial to offer an fn shape_with_plan API and expose ShapePlan::new. What do you think?

[RazrFalcon]

Sure. The only thing stoping me is time.
It was dropped due to time constrains as well, not because of technical difficulties. But harfbuzz does use tricky, implicit caching which I'm not sure we would be able to implement. Therefore exposing the plan is our only option.
Note that we have to somehow make sure that the plan matches the font somehow.

[laurmaedje]

I think exposing it and letting consumers handle the cache is good. I'll look into how we can check whether the font matches and make a PR next week.

[vorporeal]

Hey @laurmaedje - are you working on a PR? In a project of mine, ShapePlan::new is 62% of the CPU time based on profiling data. If you're not actively working on it, I could put together a PR.

In terms of handling mismatches between the plan and the face provided as an argument to shape(), I can think of a couple options:

1. Change the return type to a Result and return an error if there's a mismatch between the provided plan and the face/features passed to shape_with_plan.
2. Keep the return type the same, and ignore the provided plan if there's a mismatch.
3. Narrow the function signature to fn shape_with_plan(plan: &ShapePlan, buffer: UnicodeBuffer) -> GlyphBuffer and utilize the face and features that are passed to ShapePlan::new, guaranteeing that there won't be a mismatch. This would probably require defining a ReusableShapePlan wrapper around ShapePlan, to explicitly hold onto the provided face and feature list.

Thoughts?

[RazrFalcon]

@vorporeal 1 and 2 probably would not work, since we have to figure out how to differentiate fonts to begin with. Which is kinda impossible at the moment.
I guess ShapePlan should simply store the Face. And then we can make ShapePlan public and pass it to the new shape_with_plan function which would have to only check direction, script and language.

[laurmaedje]

@vorporeal Feel free to, I didn't really get to it.

@RazrFalcon Storing Face in ShapePlan means that the currently 'static shape plan gains a lifetime. This wouldn't be the end of the world, but 'static is definitely nicer for caching, which is the point of exposing it in the first place.

What's the main concern here? That rustybuzz will panic or that it will shape garbage? If it's just the latter, I think it's not a big deal and user error.

[vorporeal]

Agreed that if it's just a matter of garbage in, garbage out, then it's reasonable to push the responsibility of avoiding mismatch issues onto the caller and clearly document that in the API.

[RazrFalcon]

If it's just the latter, I think it's not a big deal and user error.

I honestly don't know. Garbage is the best case scenario. Crash in the worst. Hard to tell. Someone has to test it.

Moving responsibility to the user is unfortunate, but I have no idea how to handle it otherwise. We cannot really guarantee font uniqueness otherwise.

[behdad]

What we do in HB, which is probably not feasible in Rust, is that the shape_plan keeps a pointer (but no reference) to the face, and just double-checks that they two match. It's a heuristic only.

[behdad]

I think in HB it won't crash if you use the wrong shape-plan, just garbage. But I'm not sure.

[vorporeal]

I can do some manual testing today and see whether I can produce any panics.

[vorporeal]

Ran a quick test using forked versions of rustybuzz and cosmic-text to see what would happen when incorrectly reusing a ShapePlan. They have a test case that renders a line of text in both English and Arabic, with different fonts used for the different languages (Noto Sans and Noto Sans Arabic, repsectively).

Correct rendering:

Rendering with a single ShapePlan:

At least for my two simple tests (I ran another with English+Hebrew), improper ShapePlan reuse leads to incorrect rendering but no panics. This by no means was a comprehensive test; if someone has ideas for a lowish-effort way to test this more robustly, I'd be happy to give it a go.

---

In terms of implementation, I landed on this:

pub fn shape_with_plan(
    face: &Face,
    features: &[Feature],
    plan: &mut Option<ShapePlan>,
    buffer: UnicodeBuffer
) -> GlyphBuffer {
    ...
}

plan functions as an in-out argument. If you don't have a cached ShapePlan already for the given face and feature set, you pass in a &mut None and it will be replaced by an actual ShapePlan, which you can then reuse later.

For example:

let shape_plan = match shape_plan_cache.entry(font.id()) {
    Entry::Occupied(occ) => occ.into_mut(),
    Entry::Vacant(vac) => vac.insert(None),
};
let glyph_buffer = rustybuzz::shape_with_plan(font, &[], shape_plan, buffer);

It's a bit unusual for Rust, but I felt it was better than exposing a ShapePlan constructor and relying on the caller to properly construct one (which is more error-prone, given the need to call guess_segment_properties() on the Buffer, than simply requiring that they use the appropriate cache keys for their shape plan cache).

[laurmaedje]

This requires mutable access to the shape plan. Personally, I would prefer it to take a manually constructed &ShapePlan because then one can use an immutable cache and access the same shape plan from multiple threads. It's not that difficult to use correctly.

[vorporeal]

Yeah, good point.

In terms of cache keys - the ShapePlan makes use of things like direction and language; does the cache key used by the caller need to include these (in addition to the font face and any user features)?

[RazrFalcon]

@vorporeal I would suggest simply exposing the current ShapePlan constructor. And then simply:

pub fn shape_with_plan(
    face: &Face,
    plan: &ShapePlan,
    buffer: UnicodeBuffer
) -> GlyphBuffer {
    ...
}

[vorporeal]

Sounds good. Is there a reason you're not including the features argument? Seems to be needed to construct a ShapeContext (and I can't find any clear way to extract it back out of a ShapePlan).

[RazrFalcon]

I guess we have to store them in ShapePlan then. Simply clone into an internal Vec.

[vorporeal]

#88 is the PR I made based on the above discussion.

[LaurenzV]

I suppose this issue can be closed then?

[RazrFalcon]

Yep!