[read-fonts] gvar: improve delta decoding perf #1235
Conversation
Determining the delta count and offset to the y-deltas requires processing the full stream, but we really only need to read the control byte for each run to do so. This gives a roughly 30%(!) boost in loading outlines from variable TrueType fonts.
read-fonts/src/tables/variations.rs
Outdated
| // if no items remain in this run, start the next one. | ||
| // NOTE: we use `while` so we can sanely handle the case where some | ||
| // run in the middle of the data has an explicit zero length | ||
| //TODO: create a font with data of this shape and go crash some font parsers | ||
| while self.remaining_in_run == 0 { | ||
| if self.remaining_in_run == 0 { | ||
| let control: u8 = self.cursor.read().ok()?; | ||
| self.value_type = DeltaRunType::new(control); | ||
| self.remaining_in_run = (control & DELTA_RUN_COUNT_MASK) + 1; | ||
| } |
There was a problem hiding this comment.
Given the + 1, it looks like encoding an explicit zero length run is impossible, so I removed this loop and comment.
|
Turns out my new code had slightly different behavior in that it made an assumption that the boundary between x and y coordinate deltas always aligned with a packed run boundary. This seems to be the case in all fonts I tested. write-fonts specifically encodes the coordinates separately so this is always true and the same probably applies to fontmake. But the spec notably does not make this guarantee so I didn't want this to bite us later. Fixed the code to handle this case and added a test for it. |
Can this be specified in terms of a reproducible test? Run x on main, get result y, run x on my branch, get result less-than-y? |
| @@ -473,6 +482,41 @@ impl<'a> DeltaRunIter<'a> { | |||
| while self.next().is_some() {} | |||
| self.cursor | |||
| } | |||
|
|
|||
| /// Skips `n` deltas without reading the actual delta values. | |||
There was a problem hiding this comment.
Seems like it would be faster still to have precomputed start of y deltas. Spec opportunity that we could support with measurable perf impact?
There was a problem hiding this comment.
I imagine it would improve performance but the size increase might be substantial. I was actually thinking the opposite— that teaching write-fonts to pack the x/y streams together could shave off a nice chunk of bytes in a large CJK variable font.
There was a problem hiding this comment.
While it's not specified in the spec, I don't think anyone in their sane mind would get the idea that they can be encoded together. HB doesn't support decoding them as such. I don't think FreeType does either.
There was a problem hiding this comment.
It would be an interesting data point to know what the impact on perf would be if we could jump rathern than read-to-skip.
read-fonts/src/tables/variations.rs
Outdated
| @@ -806,7 +856,7 @@ pub struct TupleDeltaIter<'a, T> { | |||
| points: Option<PackedPointNumbersIter<'a>>, | |||
| next_point: usize, | |||
| x_iter: DeltaRunIter<'a>, | |||
| y_iter: Option<Skip<DeltaRunIter<'a>>>, | |||
| y_iter: Option<DeltaRunIter<'a>>, | |||
There was a problem hiding this comment.
Optional, feels a little weird that only y is an option, could it just be an iter that's empty instead of None?
There was a problem hiding this comment.
The iterator would need to produce an infinite stream of zeroes to fit into the current logic and I think that complication might lead to bugs.
I’ll replace this with an enum that holds two iters for points and one for scalars which should make this more clear.
There was a problem hiding this comment.
That rationale would make a fine comment :)
Agreed something like this would be nice. Filed #1247 to investigate doing some automated performance regression testing. |
Determining the delta count and offset to the y-deltas requires processing the full stream, but we really only need to read the control byte for each run to do so.
This gives a roughly 30%(!) boost in loading outlines from variable TrueType fonts.