[Impeller] faster glyph atlas generation by removing data copies

[gaaclarke]

fixes flutter/flutter#124996

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:

1. Removed the copying of the std::set to an std::vector
2. Uses references instead of copying FontGlyphPairs out of collections in a few places
3. Holds new glyphs as a vector of references to the set instead of copying all the FontGlyphPairs
4. 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.

[jonahwilliams]

what does putting in a namespace like this do?

[gaaclarke]

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.

[jonahwilliams]

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

[gaaclarke]

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.

[jonahwilliams]

makes sense

[jonahwilliams]

LGTM with nits