From 690af7aa69b05db6a925bcdaeac4ea0d7efba5da Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Fri, 30 Jun 2023 10:36:01 -0600 Subject: [PATCH 1/4] [GPOS] Sanitize Device tables lazily This speeds up face loading for variable fonts by 80%! Comparing before to after Benchmark Time CPU Time Old Time New CPU Old CPU New --------------------------------------------------------------------------------------------------------------------------------------------------------------- BM_Font/load_face_and_shape/Roboto-Regular.ttf/hb -0.0368 -0.0366 20 20 20 19 BM_Font/load_face_and_shape/RobotoFlex-Variable.ttf/hb -0.7149 -0.7162 77 22 77 22 BM_Font/load_face_and_shape/RobotoFlex-Variable.ttf/var/hb -0.7241 -0.7255 80 22 79 22 BM_Font/load_face_and_shape/SourceSansPro-Regular.otf/hb -0.1441 -0.1445 28 24 28 24 BM_Font/load_face_and_shape/AdobeVFPrototype.otf/hb -0.7893 -0.7910 66 14 66 14 BM_Font/load_face_and_shape/AdobeVFPrototype.otf/var/hb -0.7865 -0.7882 67 14 66 14 BM_Font/load_face_and_shape/SourceSerifVariable-Roman.ttf/hb -0.8895 -0.8900 227 25 226 25 BM_Font/load_face_and_shape/SourceSerifVariable-Roman.ttf/var/hb -0.8895 -0.8900 226 25 225 25 BM_Font/load_face_and_shape/Comfortaa-Regular-new.ttf/hb -0.5512 -0.5531 42 19 42 19 BM_Font/load_face_and_shape/NotoNastaliqUrdu-Regular.ttf/hb -0.1511 -0.1510 227 192 225 191 BM_Font/load_face_and_shape/NotoSerifMyanmar-Regular.otf/hb -0.1494 -0.1498 41 35 40 34 OVERALL_GEOMEAN -0.6443 -0.6456 0 0 0 0 --- src/OT/Layout/GPOS/AnchorFormat3.hh | 11 ++++-- src/OT/Layout/GPOS/AnchorMatrix.hh | 4 ++ src/OT/Layout/GPOS/SinglePosFormat1.hh | 3 +- src/OT/Layout/GPOS/SinglePosFormat2.hh | 3 +- src/OT/Layout/GPOS/ValueFormat.hh | 52 +++++++++++++++++++------- src/hb-kern.hh | 2 +- src/hb-ot-layout-gsubgpos.hh | 12 +++++- src/hb-ot-layout.cc | 5 ++- src/hb-ot-shaper-arabic-fallback.hh | 2 +- src/hb-sanitize.hh | 18 ++++++++- 10 files changed, 87 insertions(+), 25 deletions(-) diff --git a/src/OT/Layout/GPOS/AnchorFormat3.hh b/src/OT/Layout/GPOS/AnchorFormat3.hh index 644d63d8a..703d126b2 100644 --- a/src/OT/Layout/GPOS/AnchorFormat3.hh +++ b/src/OT/Layout/GPOS/AnchorFormat3.hh @@ -25,7 +25,12 @@ struct AnchorFormat3 bool sanitize (hb_sanitize_context_t *c) const { TRACE_SANITIZE (this); - return_trace (c->check_struct (this) && xDeviceTable.sanitize (c, this) && yDeviceTable.sanitize (c, this)); + if (unlikely (!c->check_struct (this))) return_trace (false); + + if (c->lazy_gpos_devices) + return_trace (true); + + return_trace (xDeviceTable.sanitize (c, this) && yDeviceTable.sanitize (c, this)); } void get_anchor (hb_ot_apply_context_t *c, hb_codepoint_t glyph_id HB_UNUSED, @@ -35,9 +40,9 @@ struct AnchorFormat3 *x = font->em_fscale_x (xCoordinate); *y = font->em_fscale_y (yCoordinate); - if (font->x_ppem || font->num_coords) + if ((font->x_ppem || font->num_coords) && xDeviceTable.sanitize (&c->sanitizer, this)) *x += (this+xDeviceTable).get_x_delta (font, c->var_store, c->var_store_cache); - if (font->y_ppem || font->num_coords) + if ((font->y_ppem || font->num_coords) && yDeviceTable.sanitize (&c->sanitizer, this)) *y += (this+yDeviceTable).get_y_delta (font, c->var_store, c->var_store_cache); } diff --git a/src/OT/Layout/GPOS/AnchorMatrix.hh b/src/OT/Layout/GPOS/AnchorMatrix.hh index c442efa1e..23eb0c5e8 100644 --- a/src/OT/Layout/GPOS/AnchorMatrix.hh +++ b/src/OT/Layout/GPOS/AnchorMatrix.hh @@ -21,6 +21,10 @@ struct AnchorMatrix if (unlikely (hb_unsigned_mul_overflows (rows, cols))) return_trace (false); unsigned int count = rows * cols; if (!c->check_array (matrixZ.arrayZ, count)) return_trace (false); + + if (c->lazy_gpos_devices) + return_trace (true); + for (unsigned int i = 0; i < count; i++) if (!matrixZ[i].sanitize (c, this)) return_trace (false); return_trace (true); diff --git a/src/OT/Layout/GPOS/SinglePosFormat1.hh b/src/OT/Layout/GPOS/SinglePosFormat1.hh index 623e4e66b..dff1f7316 100644 --- a/src/OT/Layout/GPOS/SinglePosFormat1.hh +++ b/src/OT/Layout/GPOS/SinglePosFormat1.hh @@ -90,6 +90,7 @@ struct SinglePosFormat1 bool position_single (hb_font_t *font, + hb_blob_t *table_blob, hb_direction_t direction, hb_codepoint_t gid, hb_glyph_position_t &pos) const @@ -100,7 +101,7 @@ struct SinglePosFormat1 /* This is ugly... */ hb_buffer_t buffer; buffer.props.direction = direction; - OT::hb_ot_apply_context_t c (1, font, &buffer); + OT::hb_ot_apply_context_t c (1, font, &buffer, table_blob); valueFormat.apply_value (&c, this, values, pos); return true; diff --git a/src/OT/Layout/GPOS/SinglePosFormat2.hh b/src/OT/Layout/GPOS/SinglePosFormat2.hh index e8f2d7c2c..168ad3bb8 100644 --- a/src/OT/Layout/GPOS/SinglePosFormat2.hh +++ b/src/OT/Layout/GPOS/SinglePosFormat2.hh @@ -94,6 +94,7 @@ struct SinglePosFormat2 bool position_single (hb_font_t *font, + hb_blob_t *table_blob, hb_direction_t direction, hb_codepoint_t gid, hb_glyph_position_t &pos) const @@ -105,7 +106,7 @@ struct SinglePosFormat2 /* This is ugly... */ hb_buffer_t buffer; buffer.props.direction = direction; - OT::hb_ot_apply_context_t c (1, font, &buffer); + OT::hb_ot_apply_context_t c (1, font, &buffer, table_blob); valueFormat.apply_value (&c, this, &values[index * valueFormat.get_len ()], diff --git a/src/OT/Layout/GPOS/ValueFormat.hh b/src/OT/Layout/GPOS/ValueFormat.hh index a6d5eb632..55a9cea8b 100644 --- a/src/OT/Layout/GPOS/ValueFormat.hh +++ b/src/OT/Layout/GPOS/ValueFormat.hh @@ -118,21 +118,25 @@ struct ValueFormat : HBUINT16 auto *cache = c->var_store_cache; /* pixel -> fractional pixel */ - if (format & xPlaDevice) { - if (use_x_device) glyph_pos.x_offset += (base + get_device (values, &ret)).get_x_delta (font, store, cache); + if (format & xPlaDevice) + { + if (use_x_device) glyph_pos.x_offset += get_device (values, &ret, base, c->sanitizer).get_x_delta (font, store, cache); values++; } - if (format & yPlaDevice) { - if (use_y_device) glyph_pos.y_offset += (base + get_device (values, &ret)).get_y_delta (font, store, cache); + if (format & yPlaDevice) + { + if (use_y_device) glyph_pos.y_offset += get_device (values, &ret, base, c->sanitizer).get_y_delta (font, store, cache); values++; } - if (format & xAdvDevice) { - if (horizontal && use_x_device) glyph_pos.x_advance += (base + get_device (values, &ret)).get_x_delta (font, store, cache); + if (format & xAdvDevice) + { + if (horizontal && use_x_device) glyph_pos.x_advance += get_device (values, &ret, base, c->sanitizer).get_x_delta (font, store, cache); values++; } - if (format & yAdvDevice) { + if (format & yAdvDevice) + { /* y_advance values grow downward but font-space grows upward, hence negation */ - if (!horizontal && use_y_device) glyph_pos.y_advance -= (base + get_device (values, &ret)).get_y_delta (font, store, cache); + if (!horizontal && use_y_device) glyph_pos.y_advance -= get_device (values, &ret, base, c->sanitizer).get_y_delta (font, store, cache); values++; } return ret; @@ -236,14 +240,12 @@ struct ValueFormat : HBUINT16 if (format & ValueFormat::xAdvDevice) { - (base + get_device (&(values[i]))).collect_variation_indices (c); i++; } if (format & ValueFormat::yAdvDevice) { - (base + get_device (&(values[i]))).collect_variation_indices (c); i++; } @@ -280,10 +282,22 @@ struct ValueFormat : HBUINT16 { return *static_cast *> (value); } - static inline const Offset16To& get_device (const Value* value, bool *worked=nullptr) + static inline const Offset16To& get_device (const Value* value) + { + return *static_cast *> (value); + } + static inline const Device& get_device (const Value* value, + bool *worked, + const void *base, + hb_sanitize_context_t &c) { if (worked) *worked |= bool (*value); - return *static_cast *> (value); + auto &offset = *static_cast *> (value); + + if (unlikely (!offset.sanitize (&c, base))) + return Null(Device); + + return base + offset; } void add_delta_to_value (HBINT16 *value, @@ -343,7 +357,13 @@ struct ValueFormat : HBUINT16 bool sanitize_value (hb_sanitize_context_t *c, const void *base, const Value *values) const { TRACE_SANITIZE (this); - return_trace (c->check_range (values, get_size ()) && (!has_device () || sanitize_value_devices (c, base, values))); + + if (unlikely (!c->check_range (values, get_size ()))) return_trace (false); + + if (c->lazy_gpos_devices) + return_trace (true); + + return_trace (!has_device () || sanitize_value_devices (c, base, values)); } bool sanitize_values (hb_sanitize_context_t *c, const void *base, const Value *values, unsigned int count) const @@ -353,6 +373,9 @@ struct ValueFormat : HBUINT16 if (!c->check_range (values, count, size)) return_trace (false); + if (c->lazy_gpos_devices) + return_trace (true); + return_trace (sanitize_values_stride_unsafe (c, base, values, count, size)); } @@ -361,6 +384,9 @@ struct ValueFormat : HBUINT16 { TRACE_SANITIZE (this); + if (c->lazy_gpos_devices) + return_trace (true); + if (!has_device ()) return_trace (true); for (unsigned int i = 0; i < count; i++) { diff --git a/src/hb-kern.hh b/src/hb-kern.hh index 9ea945cae..9ac744c9d 100644 --- a/src/hb-kern.hh +++ b/src/hb-kern.hh @@ -53,7 +53,7 @@ struct hb_kern_machine_t return; buffer->unsafe_to_concat (); - OT::hb_ot_apply_context_t c (1, font, buffer); + OT::hb_ot_apply_context_t c (1, font, buffer, hb_blob_get_empty ()); c.set_lookup_mask (kern_mask); c.set_lookup_props (OT::LookupFlag::IgnoreMarks); auto &skippy_iter = c.iter_input; diff --git a/src/hb-ot-layout-gsubgpos.hh b/src/hb-ot-layout-gsubgpos.hh index 4b9c3cc48..713f015f8 100644 --- a/src/hb-ot-layout-gsubgpos.hh +++ b/src/hb-ot-layout-gsubgpos.hh @@ -706,6 +706,7 @@ struct hb_ot_apply_context_t : hb_font_t *font; hb_face_t *face; hb_buffer_t *buffer; + hb_sanitize_context_t sanitizer; recurse_func_t recurse_func = nullptr; const GDEF &gdef; const GDEF::accelerator_t &gdef_accel; @@ -732,9 +733,11 @@ struct hb_ot_apply_context_t : hb_ot_apply_context_t (unsigned int table_index_, hb_font_t *font_, - hb_buffer_t *buffer_) : + hb_buffer_t *buffer_, + hb_blob_t *table_blob_) : table_index (table_index_), font (font_), face (font->face), buffer (buffer_), + sanitizer (table_blob_), gdef ( #ifndef HB_NO_OT_LAYOUT *face->table.GDEF->table @@ -4509,7 +4512,10 @@ struct GSUBGPOS { accelerator_t (hb_face_t *face) { - this->table = hb_sanitize_context_t ().reference_table (face); + hb_sanitize_context_t sc; + sc.lazy_gpos_devices = true; + this->table = sc.reference_table (face); + if (unlikely (this->table->is_blocklisted (this->table.get_blob (), face))) { hb_blob_destroy (this->table.get_blob ()); @@ -4534,6 +4540,8 @@ struct GSUBGPOS this->table.destroy (); } + hb_blob_t *get_blob () const { return table.get_blob (); } + hb_ot_layout_lookup_accelerator_t *get_accel (unsigned lookup_index) const { if (unlikely (lookup_index >= lookup_count)) return nullptr; diff --git a/src/hb-ot-layout.cc b/src/hb-ot-layout.cc index c3417e4ea..1e392a7d1 100644 --- a/src/hb-ot-layout.cc +++ b/src/hb-ot-layout.cc @@ -1952,7 +1952,7 @@ inline void hb_ot_map_t::apply (const Proxy &proxy, { const unsigned int table_index = proxy.table_index; unsigned int i = 0; - OT::hb_ot_apply_context_t c (table_index, font, buffer); + OT::hb_ot_apply_context_t c (table_index, font, buffer, proxy.accel.get_blob ()); c.set_recurse_func (Proxy::Lookup::template dispatch_recurse_func); for (unsigned int stage_index = 0; stage_index < stages[table_index].length; stage_index++) @@ -2627,9 +2627,10 @@ hb_ot_layout_lookup_get_optical_bound (hb_font_t *font, hb_codepoint_t glyph) { const OT::PosLookup &lookup = font->face->table.GPOS->table->get_lookup (lookup_index); + hb_blob_t *blob = font->face->table.GPOS->get_blob (); hb_glyph_position_t pos = {0}; hb_position_single_dispatch_t c; - lookup.dispatch (&c, font, direction, glyph, pos); + lookup.dispatch (&c, font, blob, direction, glyph, pos); hb_position_t ret = 0; switch (direction) { diff --git a/src/hb-ot-shaper-arabic-fallback.hh b/src/hb-ot-shaper-arabic-fallback.hh index e7a69008b..66a8bfbd2 100644 --- a/src/hb-ot-shaper-arabic-fallback.hh +++ b/src/hb-ot-shaper-arabic-fallback.hh @@ -368,7 +368,7 @@ arabic_fallback_plan_shape (arabic_fallback_plan_t *fallback_plan, hb_font_t *font, hb_buffer_t *buffer) { - OT::hb_ot_apply_context_t c (0, font, buffer); + OT::hb_ot_apply_context_t c (0, font, buffer, hb_blob_get_empty ()); for (unsigned int i = 0; i < fallback_plan->num_lookups; i++) if (fallback_plan->lookup_array[i]) { c.set_lookup_mask (fallback_plan->mask_array[i]); diff --git a/src/hb-sanitize.hh b/src/hb-sanitize.hh index 5259891b7..0f905bd99 100644 --- a/src/hb-sanitize.hh +++ b/src/hb-sanitize.hh @@ -127,7 +127,8 @@ struct hb_sanitize_context_t : writable (false), edit_count (0), blob (nullptr), num_glyphs (65536), - num_glyphs_set (false) {} + num_glyphs_set (false), + lazy_gpos_devices (false) {} const char *get_name () { return "SANITIZE"; } template @@ -155,6 +156,19 @@ struct hb_sanitize_context_t : dispatch (const T &obj, Ts&&... ds) HB_AUTO_RETURN ( _dispatch (obj, hb_prioritize, std::forward (ds)...) ) + hb_sanitize_context_t (hb_blob_t *b) : hb_sanitize_context_t () + { + init (b); + + if (blob) + start_processing (); + } + + ~hb_sanitize_context_t () + { + if (blob) + end_processing (); + } void init (hb_blob_t *b) { @@ -424,6 +438,8 @@ struct hb_sanitize_context_t : hb_blob_t *blob; unsigned int num_glyphs; bool num_glyphs_set; + public: + bool lazy_gpos_devices; }; struct hb_sanitize_with_object_t From 83eb744e09bb8ff7a9c78c79569100f560f129fb Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Fri, 30 Jun 2023 11:33:39 -0600 Subject: [PATCH 2/4] [sanizie] Inline check_range if not OPTIMIZE_SIZE BM_Font/load_face_and_shape/NotoNastaliqUrdu-Regular.ttf/hb -0.1046 -0.1051 194 173 193 172 BM_Font/load_face_and_shape/NotoSerifMyanmar-Regular.otf/hb -0.2401 -0.2412 36 27 36 27 --- src/hb-sanitize.hh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/hb-sanitize.hh b/src/hb-sanitize.hh index 0f905bd99..c28582997 100644 --- a/src/hb-sanitize.hh +++ b/src/hb-sanitize.hh @@ -254,6 +254,9 @@ struct hb_sanitize_context_t : return (this->max_ops -= (int) count) > 0; } +#ifndef HB_OPTIMIZE_SIZE + HB_ALWAYS_INLINE +#endif bool check_range (const void *base, unsigned int len) const { From 0887382cdf64f9c590eb9c086f7f5622c482e32c Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Fri, 30 Jun 2023 11:39:46 -0600 Subject: [PATCH 3/4] [GPOS] Fix sanitize --- src/OT/Layout/GPOS/AnchorMatrix.hh | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/OT/Layout/GPOS/AnchorMatrix.hh b/src/OT/Layout/GPOS/AnchorMatrix.hh index 23eb0c5e8..e62e64f3a 100644 --- a/src/OT/Layout/GPOS/AnchorMatrix.hh +++ b/src/OT/Layout/GPOS/AnchorMatrix.hh @@ -22,9 +22,6 @@ struct AnchorMatrix unsigned int count = rows * cols; if (!c->check_array (matrixZ.arrayZ, count)) return_trace (false); - if (c->lazy_gpos_devices) - return_trace (true); - for (unsigned int i = 0; i < count; i++) if (!matrixZ[i].sanitize (c, this)) return_trace (false); return_trace (true); From 2d6091fc42c81ba68fe6710de42d313cfda7a309 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Fri, 30 Jun 2023 11:48:56 -0600 Subject: [PATCH 4/4] [GPOS] Make AnchorMatrix sanitize lazy again Was reverted in the previous commit, because it was incomplete. --- src/OT/Layout/GPOS/AnchorFormat3.hh | 3 --- src/OT/Layout/GPOS/AnchorMatrix.hh | 14 ++++++++++---- src/OT/Layout/GPOS/MarkArray.hh | 2 +- src/OT/Layout/GPOS/ValueFormat.hh | 6 +++--- src/hb-ot-layout-gsubgpos.hh | 2 +- src/hb-sanitize.hh | 4 ++-- 6 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/OT/Layout/GPOS/AnchorFormat3.hh b/src/OT/Layout/GPOS/AnchorFormat3.hh index 703d126b2..8684f60ca 100644 --- a/src/OT/Layout/GPOS/AnchorFormat3.hh +++ b/src/OT/Layout/GPOS/AnchorFormat3.hh @@ -27,9 +27,6 @@ struct AnchorFormat3 TRACE_SANITIZE (this); if (unlikely (!c->check_struct (this))) return_trace (false); - if (c->lazy_gpos_devices) - return_trace (true); - return_trace (xDeviceTable.sanitize (c, this) && yDeviceTable.sanitize (c, this)); } diff --git a/src/OT/Layout/GPOS/AnchorMatrix.hh b/src/OT/Layout/GPOS/AnchorMatrix.hh index e62e64f3a..bd9b18973 100644 --- a/src/OT/Layout/GPOS/AnchorMatrix.hh +++ b/src/OT/Layout/GPOS/AnchorMatrix.hh @@ -22,18 +22,24 @@ struct AnchorMatrix unsigned int count = rows * cols; if (!c->check_array (matrixZ.arrayZ, count)) return_trace (false); + if (c->lazy_some_gpos) + return_trace (true); + for (unsigned int i = 0; i < count; i++) if (!matrixZ[i].sanitize (c, this)) return_trace (false); return_trace (true); } - const Anchor& get_anchor (unsigned int row, unsigned int col, - unsigned int cols, bool *found) const + const Anchor& get_anchor (hb_ot_apply_context_t *c, + unsigned int row, unsigned int col, + unsigned int cols, bool *found) const { *found = false; if (unlikely (row >= rows || col >= cols)) return Null (Anchor); - *found = !matrixZ[row * cols + col].is_null (); - return this+matrixZ[row * cols + col]; + auto &offset = matrixZ[row * cols + col]; + if (unlikely (!offset.sanitize (&c->sanitizer, this))) return Null (Anchor); + *found = !offset.is_null (); + return this+offset; } template /* Array of MarkRecords--in Cove const Anchor& mark_anchor = this + record.markAnchor; bool found; - const Anchor& glyph_anchor = anchors.get_anchor (glyph_index, mark_class, class_count, &found); + const Anchor& glyph_anchor = anchors.get_anchor (c, glyph_index, mark_class, class_count, &found); /* If this subtable doesn't have an anchor for this base and this class, * return false such that the subsequent subtables have a chance at it. */ if (unlikely (!found)) return_trace (false); diff --git a/src/OT/Layout/GPOS/ValueFormat.hh b/src/OT/Layout/GPOS/ValueFormat.hh index 55a9cea8b..46b69bcfe 100644 --- a/src/OT/Layout/GPOS/ValueFormat.hh +++ b/src/OT/Layout/GPOS/ValueFormat.hh @@ -360,7 +360,7 @@ struct ValueFormat : HBUINT16 if (unlikely (!c->check_range (values, get_size ()))) return_trace (false); - if (c->lazy_gpos_devices) + if (c->lazy_some_gpos) return_trace (true); return_trace (!has_device () || sanitize_value_devices (c, base, values)); @@ -373,7 +373,7 @@ struct ValueFormat : HBUINT16 if (!c->check_range (values, count, size)) return_trace (false); - if (c->lazy_gpos_devices) + if (c->lazy_some_gpos) return_trace (true); return_trace (sanitize_values_stride_unsafe (c, base, values, count, size)); @@ -384,7 +384,7 @@ struct ValueFormat : HBUINT16 { TRACE_SANITIZE (this); - if (c->lazy_gpos_devices) + if (c->lazy_some_gpos) return_trace (true); if (!has_device ()) return_trace (true); diff --git a/src/hb-ot-layout-gsubgpos.hh b/src/hb-ot-layout-gsubgpos.hh index 713f015f8..67984331b 100644 --- a/src/hb-ot-layout-gsubgpos.hh +++ b/src/hb-ot-layout-gsubgpos.hh @@ -4513,7 +4513,7 @@ struct GSUBGPOS accelerator_t (hb_face_t *face) { hb_sanitize_context_t sc; - sc.lazy_gpos_devices = true; + sc.lazy_some_gpos = true; this->table = sc.reference_table (face); if (unlikely (this->table->is_blocklisted (this->table.get_blob (), face))) diff --git a/src/hb-sanitize.hh b/src/hb-sanitize.hh index c28582997..0dcff61d6 100644 --- a/src/hb-sanitize.hh +++ b/src/hb-sanitize.hh @@ -128,7 +128,7 @@ struct hb_sanitize_context_t : blob (nullptr), num_glyphs (65536), num_glyphs_set (false), - lazy_gpos_devices (false) {} + lazy_some_gpos (false) {} const char *get_name () { return "SANITIZE"; } template @@ -442,7 +442,7 @@ struct hb_sanitize_context_t : unsigned int num_glyphs; bool num_glyphs_set; public: - bool lazy_gpos_devices; + bool lazy_some_gpos; }; struct hb_sanitize_with_object_t