Optimize ShapeResult::ComputeGlyphBounds

Further optimize ShapeResult::ComputeGlyphBounds by moving population of
source glyphs vector to ComputeGlyphPositions thereby avoiding the extra
iteration over all glyphs in ComputeGlyphBounds. Furthermore by changing
SimpleFontData::BoundsForGlyphs to take a SkRect vector avoids one extra
copy and one extra iteration.

Bug: 591099
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Ib155ce1e069d5329c5fbc6a8850fc40736ee520b
Reviewed-on: https://chromium-review.googlesource.com/1157271Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579682}

third_party/blink/renderer/platform/fonts/shaping/shape_result.cc

@@ -876,6 +876,13 @@ void ShapeResult::ComputeGlyphPositions(ShapeResult::RunInfo* run,
   float total_advance = 0.0f;
   bool has_vertical_offsets = !is_horizontal_run;
 
+#if !defined(OS_MACOSX)
+  // Allocate and populate vector for BoundsForGlyphs call in ComputeGlyphBounds
+  // here to avoid having to iterate over all glyphs an extra time.
+  Vector<Glyph> glyphs(
+      std::min(num_glyphs, HarfBuzzRunGlyphData::kMaxCharacterIndex));
+#endif
+
   // HarfBuzz returns result in visual order, no need to flip for RTL.
   for (unsigned i = 0; i < num_glyphs; ++i) {
     uint16_t glyph = glyph_infos[start_glyph + i].codepoint;
@@ -906,49 +913,67 @@ void ShapeResult::ComputeGlyphPositions(ShapeResult::RunInfo* run,
         IsSafeToBreakBefore(glyph_infos + start_glyph, num_glyphs, i));
     total_advance += advance;
     has_vertical_offsets |= (offset.Height() != 0);
+
+#if !defined(OS_MACOSX)
+    // See comment in ShapeResult::ComputeGlyphBounds.
+    glyphs[i] = run->glyph_data_[i].glyph;
+#endif
   }
 
   run->width_ = std::max(0.0f, total_advance);
   has_vertical_offsets_ |= has_vertical_offsets;
-
+#if defined(OS_MACOSX)
   ComputeGlyphBounds<is_horizontal_run>(*run);
+#else
+  ComputeGlyphBounds<is_horizontal_run>(*run, glyphs);
+#endif
 }
 
+// TODO(kojii): MacOS does not benefit from batching the Skia request due to
+// https://bugs.chromium.org/p/skia/issues/detail?id=5328 , and the cost to
+// prepare batching, which is normally much less than the benefit of batching,
+// is not ignorable unfortunately.
+#if defined(OS_MACOSX)
 template <bool is_horizontal_run>
 void ShapeResult::ComputeGlyphBounds(const ShapeResult::RunInfo& run) {
-  // Skia runs much faster if we give a list of glyph ID rather than calling it
-  // on each glyph.
   const SimpleFontData& current_font_data = *run.font_data_;
-#if defined(OS_MACOSX)
-  // TODO(kojii): MacOS does not benefit from batching the Skia request due to
-  // https://bugs.chromium.org/p/skia/issues/detail?id=5328 , and the cost to
-  // prepare batching, which is normally much less than the benefit of batching,
-  // is not ignorable unfortunately.
+
   GlyphBoundsAccumulator bounds(width_);
   for (const HarfBuzzRunGlyphData& glyph_data : run.glyph_data_) {
     bounds.Unite<is_horizontal_run>(
         glyph_data, current_font_data.BoundsForGlyph(glyph_data.glyph));
     bounds.origin += glyph_data.advance;
   }
-#else
+
+  if (!is_horizontal_run)
+    bounds.ConvertVerticalRunToLogical(current_font_data.GetFontMetrics());
+  glyph_bounding_box_.Unite(bounds.bounds);
+}
+
+#else   // !OS_MACOSX
+template <bool is_horizontal_run>
+void ShapeResult::ComputeGlyphBounds(const ShapeResult::RunInfo& run,
+                                     const Vector<Glyph>& glyphs) {
+  const SimpleFontData& current_font_data = *run.font_data_;
+
+  // Skia runs much faster if we give a list of glyph ID rather than calling it
+  // on each glyph.
   unsigned num_glyphs = run.glyph_data_.size();
-  Vector<Glyph, 256> glyphs(num_glyphs);
-  for (unsigned i = 0; i < num_glyphs; i++)
-    glyphs[i] = run.glyph_data_[i].glyph;
-  Vector<FloatRect, 256> bounds_list(num_glyphs);
+  Vector<SkRect> bounds_list(num_glyphs);
   current_font_data.BoundsForGlyphs(glyphs, &bounds_list);
 
   GlyphBoundsAccumulator bounds(width_);
   for (unsigned i = 0; i < num_glyphs; i++) {
     const HarfBuzzRunGlyphData& glyph_data = run.glyph_data_[i];
-    bounds.Unite<is_horizontal_run>(glyph_data, bounds_list[i]);
+    bounds.Unite<is_horizontal_run>(glyph_data, FloatRect(bounds_list[i]));
     bounds.origin += glyph_data.advance;
   }
-#endif
+
   if (!is_horizontal_run)
     bounds.ConvertVerticalRunToLogical(current_font_data.GetFontMetrics());
   glyph_bounding_box_.Unite(bounds.bounds);
 }
+#endif  // !OS_MACOSX
 
 void ShapeResult::InsertRun(std::unique_ptr<ShapeResult::RunInfo> run_to_insert,
                             unsigned start_glyph,

third_party/blink/renderer/platform/fonts/shaping/shape_result.h

@@ -32,7 +32,9 @@
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_FONTS_SHAPING_SHAPE_RESULT_H_
 
 #include <memory>
+#include "build/build_config.h"
 #include "third_party/blink/renderer/platform/fonts/canvas_rotation_in_vertical.h"
+#include "third_party/blink/renderer/platform/fonts/glyph.h"
 #include "third_party/blink/renderer/platform/geometry/float_rect.h"
 #include "third_party/blink/renderer/platform/layout_unit.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
@@ -321,8 +323,15 @@ class PLATFORM_EXPORT ShapeResult : public RefCounted<ShapeResult> {
                              unsigned start_glyph,
                              unsigned num_glyphs,
                              hb_buffer_t*);
+
+#if defined(OS_MACOSX)
   template <bool is_horizontal_run>
   void ComputeGlyphBounds(const ShapeResult::RunInfo&);
+#else
+  template <bool is_horizontal_run>
+  void ComputeGlyphBounds(const ShapeResult::RunInfo&, const Vector<Glyph>&);
+#endif
+
   void InsertRun(std::unique_ptr<ShapeResult::RunInfo>,
                  unsigned start_glyph,
                  unsigned num_glyphs,

third_party/blink/renderer/platform/fonts/simple_font_data.cc

@@ -354,18 +354,14 @@ FloatRect SimpleFontData::PlatformBoundsForGlyph(Glyph glyph) const {
   return FloatRect(bounds);
 }
 
-void SimpleFontData::BoundsForGlyphs(const Vector<Glyph, 256> glyphs,
-                                     Vector<FloatRect, 256>* bounds) const {
+void SimpleFontData::BoundsForGlyphs(const Vector<Glyph> glyphs,
+                                     Vector<SkRect>* bounds) const {
   DCHECK_EQ(glyphs.size(), bounds->size());
 
   if (!platform_data_.size())
     return;
 
-  Vector<SkRect, 256> skia_bounds(glyphs.size());
-  SkiaTextMetrics(&paint_).GetSkiaBoundsForGlyphs(glyphs, skia_bounds.data());
-
-  for (unsigned i = 0; i < skia_bounds.size(); i++)
-    (*bounds)[i] = FloatRect(skia_bounds[i]);
+  SkiaTextMetrics(&paint_).GetSkiaBoundsForGlyphs(glyphs, bounds->data());
 }
 
 float SimpleFontData::PlatformWidthForGlyph(Glyph glyph) const {

third_party/blink/renderer/platform/fonts/simple_font_data.h

@@ -110,7 +110,7 @@ class PLATFORM_EXPORT SimpleFontData : public FontData {
   }
 
   FloatRect BoundsForGlyph(Glyph) const;
-  void BoundsForGlyphs(const Vector<Glyph, 256>, Vector<FloatRect, 256>*) const;
+  void BoundsForGlyphs(const Vector<Glyph>, Vector<SkRect>*) const;
   FloatRect PlatformBoundsForGlyph(Glyph) const;
   float WidthForGlyph(Glyph) const;
   float PlatformWidthForGlyph(Glyph) const;

third_party/blink/renderer/platform/fonts/skia/skia_text_metrics.cc

@@ -72,7 +72,7 @@ void SkiaTextMetrics::GetSkiaBoundsForGlyph(Glyph glyph, SkRect* bounds) {
   }
 }
 
-void SkiaTextMetrics::GetSkiaBoundsForGlyphs(const Vector<Glyph, 256> glyphs,
+void SkiaTextMetrics::GetSkiaBoundsForGlyphs(const Vector<Glyph> glyphs,
                                              SkRect* bounds) {
 #if defined(OS_MACOSX)
   for (unsigned i = 0; i < glyphs.size(); i++) {

third_party/blink/renderer/platform/fonts/skia/skia_text_metrics.h

@@ -21,7 +21,7 @@ class SkiaTextMetrics final {
   void GetGlyphExtentsForHarfBuzz(hb_codepoint_t, hb_glyph_extents_t*);
 
   void GetSkiaBoundsForGlyph(Glyph, SkRect* bounds);
-  void GetSkiaBoundsForGlyphs(const Vector<Glyph, 256>, SkRect*);
+  void GetSkiaBoundsForGlyphs(const Vector<Glyph>, SkRect*);
   float GetSkiaWidthForGlyph(Glyph);
 
   static hb_position_t SkiaScalarToHarfBuzzPosition(SkScalar value);