[Impeller] faster glyph atlas generation by removing data copies #41290

gaaclarke · 2023-04-18T05:07:10Z

This is the best I could come up with easily. It doesn't give as good of a result as I'd hoped. I measured it with a simple microbenchmark derived from one of the unit tests and it gave a 6% decrease in time ( 241.314us vs 257.626us) on macos release builds (run with rosetta).

Improvements:

Removed the copying of the std::set to an std::vector
Uses references instead of copying FontGlyphPairs out of collections in a few places
Holds new glyphs as a vector of references to the set instead of copying all the FontGlyphPairs
Deletes more lines of code than it adds

the benchmark

diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc
index 01a11d494c..1b99afa699 100644
--- a/impeller/typographer/typographer_unittests.cc
+++ b/impeller/typographer/typographer_unittests.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <chrono>
 #include "flutter/testing/testing.h"
 #include "impeller/playground/playground_test.h"
 #include "impeller/typographer/backends/skia/text_frame_skia.h"
@@ -149,23 +150,29 @@ TEST_P(TypographerTest, GlyphAtlasWithLotsOfdUniqueGlyphSize) {
       sk_font);
   ASSERT_TRUE(blob);
 
-  TextFrame frame;
-  size_t count = 0;
-  TextRenderContext::FrameIterator iterator = [&]() -> const TextFrame* {
-    if (count < 8) {
-      count++;
-      frame = TextFrameFromTextBlob(blob, 0.6 * count);
-      return &frame;
-    }
-    return nullptr;
-  };
-  auto atlas = context->CreateGlyphAtlas(GlyphAtlas::Type::kAlphaBitmap,
-                                         atlas_context, iterator);
-  ASSERT_NE(atlas, nullptr);
-  ASSERT_NE(atlas->GetTexture(), nullptr);
-
-  ASSERT_EQ(atlas->GetTexture()->GetSize().width * 2,
-            atlas->GetTexture()->GetSize().height);
+  auto beg = std::chrono::high_resolution_clock::now();
+  int count = 10000;
+  for (int i = 0; i < count; ++i) {
+    TextFrame frame;
+    size_t count = 0;
+    TextRenderContext::FrameIterator iterator = [&]() -> const TextFrame* {
+      if (count < 8) {
+        count++;
+        frame = TextFrameFromTextBlob(blob, 0.6 * count);
+        return &frame;
+      }
+      return nullptr;
+    };
+    auto atlas = context->CreateGlyphAtlas(GlyphAtlas::Type::kAlphaBitmap,
+                                           atlas_context, iterator);
+    ASSERT_NE(atlas, nullptr);
+    ASSERT_NE(atlas->GetTexture(), nullptr);
+    ASSERT_EQ(atlas->GetTexture()->GetSize().width * 2,
+              atlas->GetTexture()->GetSize().height);
+  }
+  auto end = std::chrono::high_resolution_clock::now();
+  auto duration = std::chrono::duration_cast<std::chrono::microseconds>(end - beg);
+  FML_LOG(ERROR) << "Elapsed Time: " << static_cast<double>(duration.count())/count << "us";
 }

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…ions of fonts. - updated the existing test to be a proper unit test

This reverts commit 383d200.

jonahwilliams · 2023-04-18T18:01:59Z

impeller/typographer/backends/skia/text_render_context_skia.cc


-static ISize OptimumAtlasSizeForFontGlyphPairs(
-    const FontGlyphPair::Vector& pairs,
+namespace {


what does putting in a namespace like this do?

It should have no functional difference in this case. I was playing around with making that function templated and looks like I left that change in there by happenstance.

impeller/typographer/font.h

jonahwilliams · 2023-04-18T19:00:07Z

impeller/typographer/backends/skia/text_render_context_skia.cc

+  size_t i = 0;
+  for (auto it = pairs.begin(); it != pairs.end(); ++i, ++it) {
+    const auto& pair = *it;


I don't really love this. Does it make a significant improvement on the generated code?

It was required because you can't index into std::unordered_set. We could use a foreach loop but it isn't really cleaner imo.

Previously we were copying all of the data in the std::unordered_set to make a std::vector just so we could cleanly index into the data. But since the order of iteration of an std::unordered_set is deterministic, we don't really need to copy the data just to index into it.

makes sense

jonahwilliams

LGTM with nits

…opies (flutter/engine#41290)

…125069) flutter/engine@6d263ea...5fcc7b7 2023-04-18 [email protected] [Impeller] Gpu model information to Skia gold (flutter/engine#41216) 2023-04-18 [email protected] [Impeller] faster glyph atlas generation by removing data copies (flutter/engine#41290) 2023-04-18 [email protected] Migrate android AOT to engine_v2. (flutter/engine#41229) 2023-04-18 [email protected] Roll Skia from 5bd4bdc0d8e2 to f80ee1088861 (8 revisions) (flutter/engine#41302) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

gaaclarke changed the title ~~Faster impeller glyphs~~ [Impeller] faster glyph atlas generation by removing data copies Apr 18, 2023

gaaclarke requested a review from jonahwilliams April 18, 2023 05:07

gaaclarke added 12 commits April 17, 2023 22:10

[impeller] Made the glyph atlas generation faster by reducing allocat…

b420060

…ions of fonts. - updated the existing test to be a proper unit test

removed one font copy, remoted some auto usage

22eef89

started moving the glyph pairs out of the set

5350c78

removed more copies

d00236d

moved the font to a shared_ptr

7d76f7e

Revert "moved the font to a shared_ptr"

14a3f2e

This reverts commit 383d200.

removed another extra copy

91f0bd5

made the font destructor virtual

396fd2b

made GrabNotPresentPairs const

8e8fc44

removed copy of the set to a vec

12aa60c

removed copying the fonts when detecting new glyphs

12e9294

format

c3238c1

gaaclarke force-pushed the faster-impeller-glyphs branch from 72c2f6a to c3238c1 Compare April 18, 2023 05:10

jonahwilliams reviewed Apr 18, 2023

View reviewed changes

impeller/typographer/font.h Outdated Show resolved Hide resolved

removed virtual destructor

355e213

gaaclarke requested a review from jonahwilliams April 18, 2023 18:50

jonahwilliams reviewed Apr 18, 2023

View reviewed changes

jonahwilliams approved these changes Apr 18, 2023

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 18, 2023

auto-submit bot merged commit b71dd7e into flutter:main Apr 18, 2023

engine-flutter-autoroll mentioned this pull request Apr 18, 2023

Roll Flutter Engine from 6d263ea56a62 to 5fcc7b719029 (4 revisions) flutter/flutter#125069

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 18, 2023

b71dd7ef5 [Impeller] faster glyph atlas generation by removing data c…

d6c4a69

…opies (flutter/engine#41290)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] faster glyph atlas generation by removing data copies #41290

[Impeller] faster glyph atlas generation by removing data copies #41290

Uh oh!

gaaclarke commented Apr 18, 2023

Uh oh!

jonahwilliams Apr 18, 2023

Uh oh!

gaaclarke Apr 18, 2023

Uh oh!

Uh oh!

jonahwilliams Apr 18, 2023

Uh oh!

gaaclarke Apr 18, 2023

Uh oh!

jonahwilliams Apr 18, 2023

Uh oh!

jonahwilliams left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[Impeller] faster glyph atlas generation by removing data copies #41290

[Impeller] faster glyph atlas generation by removing data copies #41290

Uh oh!

Conversation

gaaclarke commented Apr 18, 2023

the benchmark

Pre-launch Checklist

Uh oh!

jonahwilliams Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

gaaclarke Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

gaaclarke Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants