From 6d390bfaed8a316896be0b34011c31e43f1ef167 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Mon, 2 Dec 2024 19:04:17 -0800 Subject: [PATCH] Moved font glyph atlas to flat_hash_map (flutter/engine#56847) In https://github.com/flutter/engine/pull/56844 we saw a 15% increase in performance by switching to absl::flat_hash_map. I suspect we say see even better results ([source](https://martin.ankerl.com/2022/08/27/hashmap-bench-01)). The absl guidance is to default to using this. test exempt: no intentional functional change, just performance [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../src/flutter/impeller/typographer/BUILD.gn | 5 ++ .../impeller/typographer/font_glyph_pair.h | 32 ++++----- .../impeller/typographer/glyph_atlas.cc | 13 ++-- .../impeller/typographer/glyph_atlas.h | 71 ++++++++++++++++--- 4 files changed, 86 insertions(+), 35 deletions(-) diff --git a/engine/src/flutter/impeller/typographer/BUILD.gn b/engine/src/flutter/impeller/typographer/BUILD.gn index acdbeb80a8..3470c3d0be 100644 --- a/engine/src/flutter/impeller/typographer/BUILD.gn +++ b/engine/src/flutter/impeller/typographer/BUILD.gn @@ -34,6 +34,11 @@ impeller_component("typographer") { "../renderer", ] + if (!is_fuchsia) { + public_deps += + [ "//flutter/third_party/abseil-cpp/absl/container:flat_hash_map" ] + } + deps = [ "//flutter/fml" ] } diff --git a/engine/src/flutter/impeller/typographer/font_glyph_pair.h b/engine/src/flutter/impeller/typographer/font_glyph_pair.h index 87ea16b004..d5b8906de8 100644 --- a/engine/src/flutter/impeller/typographer/font_glyph_pair.h +++ b/engine/src/flutter/impeller/typographer/font_glyph_pair.h @@ -41,11 +41,10 @@ struct ScaledFont { Font font; Scalar scale; - struct Hash { - constexpr std::size_t operator()(const impeller::ScaledFont& sf) const { - return fml::HashCombine(sf.font.GetHash(), sf.scale); - } - }; + template + friend H AbslHashValue(H h, const ScaledFont& sf) { + return H::combine(std::move(h), sf.font.GetHash(), sf.scale); + } struct Equal { constexpr bool operator()(const impeller::ScaledFont& lhs, @@ -70,19 +69,18 @@ struct SubpixelGlyph { subpixel_offset(p_subpixel_offset), properties(p_properties) {} - struct Hash { - constexpr std::size_t operator()(const impeller::SubpixelGlyph& sg) const { - if (!sg.properties.has_value()) { - return fml::HashCombine(sg.glyph.index, sg.subpixel_offset.x, - sg.subpixel_offset.y); - } - return fml::HashCombine( - sg.glyph.index, sg.subpixel_offset.x, sg.subpixel_offset.y, - sg.properties->color.ToARGB(), sg.properties->stroke, - sg.properties->stroke_cap, sg.properties->stroke_join, - sg.properties->stroke_miter, sg.properties->stroke_width); + template + friend H AbslHashValue(H h, const SubpixelGlyph& sg) { + if (!sg.properties.has_value()) { + return H::combine(std::move(h), sg.glyph.index, sg.subpixel_offset.x, + sg.subpixel_offset.y); } - }; + return H::combine(std::move(h), sg.glyph.index, sg.subpixel_offset.x, + sg.subpixel_offset.y, sg.properties->color.ToARGB(), + sg.properties->stroke, sg.properties->stroke_cap, + sg.properties->stroke_join, sg.properties->stroke_miter, + sg.properties->stroke_width); + } struct Equal { constexpr bool operator()(const impeller::SubpixelGlyph& lhs, diff --git a/engine/src/flutter/impeller/typographer/glyph_atlas.cc b/engine/src/flutter/impeller/typographer/glyph_atlas.cc index 98038668fb..9d75d62710 100644 --- a/engine/src/flutter/impeller/typographer/glyph_atlas.cc +++ b/engine/src/flutter/impeller/typographer/glyph_atlas.cc @@ -78,7 +78,9 @@ void GlyphAtlas::SetAtlasGeneration(size_t generation) { void GlyphAtlas::AddTypefaceGlyphPositionAndBounds(const FontGlyphPair& pair, Rect position, Rect bounds) { - font_atlas_map_[pair.scaled_font].positions_[pair.glyph] = + FontAtlasMap::iterator it = font_atlas_map_.find(pair.scaled_font); + FML_DCHECK(it != font_atlas_map_.end()); + it->second.positions_[pair.glyph] = FrameBounds{position, bounds, /*is_placeholder=*/false}; } @@ -93,12 +95,9 @@ std::optional GlyphAtlas::FindFontGlyphBounds( FontGlyphAtlas* GlyphAtlas::GetOrCreateFontGlyphAtlas( const ScaledFont& scaled_font) { - const auto& found = font_atlas_map_.find(scaled_font); - if (found != font_atlas_map_.end()) { - return &found->second; - } - font_atlas_map_[scaled_font] = FontGlyphAtlas(); - return &font_atlas_map_[scaled_font]; + auto [iter, inserted] = + font_atlas_map_.try_emplace(scaled_font, FontGlyphAtlas()); + return &iter->second; } size_t GlyphAtlas::GetGlyphCount() const { diff --git a/engine/src/flutter/impeller/typographer/glyph_atlas.h b/engine/src/flutter/impeller/typographer/glyph_atlas.h index bf11ff056a..de158bae31 100644 --- a/engine/src/flutter/impeller/typographer/glyph_atlas.h +++ b/engine/src/flutter/impeller/typographer/glyph_atlas.h @@ -8,7 +8,16 @@ #include #include #include -#include + +#include "flutter/fml/build_config.h" + +#if defined(OS_FUCHSIA) +// TODO(gaaclarke): Migrate to use absl. I couldn't get it working since absl +// has special logic in its GN files for Fuchsia that I couldn't sort out. +#define IMPELLER_TYPOGRAPHER_USE_STD_HASH +#else +#include "flutter/third_party/abseil-cpp/absl/container/flat_hash_map.h" +#endif #include "impeller/core/texture.h" #include "impeller/geometry/rect.h" @@ -19,6 +28,30 @@ namespace impeller { class FontGlyphAtlas; +/// Helper for AbslHashAdapter. Tallies a hash value with fml::HashCombine. +template +struct AbslHashAdapterCombiner { + std::size_t value = 0; + + template + static AbslHashAdapterCombiner combine(AbslHashAdapterCombiner combiner, + const Args&... args) { + combiner.value = fml::HashCombine(combiner.value, args...); + return combiner; + } +}; + +/// Adapts AbslHashValue functions to be used with std::unordered_map and the +/// fml hash functions. +template +struct AbslHashAdapter { + constexpr std::size_t operator()(const T& element) const { + AbslHashAdapterCombiner combiner; + combiner = AbslHashValue(std::move(combiner), element); + return combiner.value; + } +}; + struct FrameBounds { /// The bounds of the glyph within the glyph atlas. Rect atlas_bounds; @@ -160,11 +193,19 @@ class GlyphAtlas { std::shared_ptr texture_; size_t generation_ = 0; - std::unordered_map - font_atlas_map_; +#if defined(IMPELLER_TYPOGRAPHER_USE_STD_HASH) + using FontAtlasMap = std::unordered_map, + ScaledFont::Equal>; +#else + using FontAtlasMap = absl::flat_hash_map, + ScaledFont::Equal>; +#endif + + FontAtlasMap font_atlas_map_; GlyphAtlas(const GlyphAtlas&) = delete; @@ -228,6 +269,7 @@ class GlyphAtlasContext { class FontGlyphAtlas { public: FontGlyphAtlas() = default; + FontGlyphAtlas(FontGlyphAtlas&&) = default; //---------------------------------------------------------------------------- /// @brief Find the location of a glyph in the atlas. @@ -249,12 +291,19 @@ class FontGlyphAtlas { private: friend class GlyphAtlas; - std::unordered_map - positions_; +#if defined(IMPELLER_TYPOGRAPHER_USE_STD_HASH) + using PositionsMap = std::unordered_map, + SubpixelGlyph::Equal>; +#else + using PositionsMap = absl::flat_hash_map, + SubpixelGlyph::Equal>; +#endif + PositionsMap positions_; FontGlyphAtlas(const FontGlyphAtlas&) = delete; };