[Impeller] hash even less stuff per frame. #55092

jonahwilliams · 2024-09-10T23:19:29Z

Follow up to #55060

Currently we have multiple stages of hashing while font rendering, which is relatively expensive for the actualy required workload. First, we hash the contents of all text frames to compute the unique set of glyphs per frame. Then we diff this hash set against the hashmap of glyphs within the atlas. Finally we hash and lookup the final rendered bounds for each glyph.

We can simplify this to 2. hash lookups for glyphs not yet in the atlas and 1. hash lookup for glyphs that are in the atlas. This is done by combing the step where we uniquely compute glyphs per frame with the diff against the current atlas. When this lookup is performed, we also store the glyph position (if found) in the text_frame itself - which allows text contents to skip the last hash, as long as the glyph has already been rendered.

Before

After

Using this handy dandy test app:

import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {

  Widget build(context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(
          title: Text('Platform View'),
        ),
        body: SafeArea(child: Stack(children: [
          SizedBox(
            width: 380,
            height: 380,
            child: LinearProgressIndicator(),
          ),
          Stack(
            children: List<Widget>.generate(1000, (index) {
              // The problem already happens with a small amount of widgets.
              // Using an excessive amount of widgets is just to make the problem more evident.
              return Text("Lots of Texts represent a Widget with complex components.");
            }),
          ),

          Align(
            alignment: Alignment.bottomCenter,
            child:
            TextButton(
              child: Text("Button"),
              onPressed: () {
                print("Tap ${DateTime.now()}");
              },
            ),
          ),
        ],
        ),
        ),
      ),
    );
  }
}

jonahwilliams · 2024-09-11T23:23:20Z

impeller/typographer/text_frame.h

@@ -16,6 +16,8 @@ namespace impeller {
 ///             This object is typically the entrypoint in the Impeller type
 ///             rendering subsystem.
 ///
+/// A text frame should not be reused in multiple places within a single frame,


If this behavior is a problem, we could instead use a different object as a cache, but I think that would be substantially more refactoring for little to no gain, as this condition is always true for us.

Sounds good and thanks for the clarification.

jonahwilliams · 2024-09-12T00:19:20Z

impeller/typographer/backends/skia/typographer_context_skia.cc

+    std::vector<FontGlyphPair>& new_glyphs,
+    std::vector<Rect>& glyph_sizes) {
+  for (const auto& frame : text_frames) {
+    // TODO(jonahwilliams): unless we destroy the atlas (which we know about),


Leaving this as the next optimizations. With a little plumbing when we recreate the atlas we can actually avoid any hashing on frames with no new text.

flutter-dashboard · 2024-09-12T03:10:18Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #55092 at sha c87ab8b

jtmcdole

lgtm from me; but you asked for Chinmay

jtmcdole · 2024-09-13T19:50:00Z

impeller/typographer/text_frame.h

  std::vector<TextRun> runs_;
  Rect bounds_;
  bool has_color_;
+
+  // per frame data.


super nit for clarity: "per frame data" -> "per text frame data"

jonahwilliams · 2024-09-26T23:30:46Z

FYI @chinmaygarde this is still waiting on a review from you.

chinmaygarde

this is still waiting on a review from you

Sorry about the delay. All nits.

chinmaygarde · 2024-09-26T23:33:12Z

impeller/typographer/text_frame.h

+  std::vector<FrameBounds> bound_values_;
+  Scalar scale_;
+  Point offset_;
+  std::optional<GlyphProperties> properties_ = std::nullopt;


= std::nullopt is redundant.

chinmaygarde · 2024-09-26T23:33:36Z

impeller/typographer/text_frame.h

+
+  // per frame data.
+  std::vector<FrameBounds> bound_values_;
+  Scalar scale_;


Zero out the scalar.

chinmaygarde · 2024-09-26T23:34:26Z

impeller/typographer/text_frame.h

@@ -16,6 +16,8 @@ namespace impeller {
 ///             This object is typically the entrypoint in the Impeller type
 ///             rendering subsystem.
 ///
+/// A text frame should not be reused in multiple places within a single frame,


Sounds good and thanks for the clarification.

chinmaygarde · 2024-09-26T23:35:51Z

impeller/typographer/text_frame.cc

+}
+
+FrameBounds TextFrame::GetFrameBounds(size_t index) {
+  return bound_values_[index];


If you use .at(index) you get bounds checking in debug modes for free.

GetFrameBounds can be const.

chinmaygarde · 2024-09-26T23:38:16Z

impeller/typographer/glyph_atlas.h

+  Rect glyph_bounds;
+  /// Whether [atlas_bounds] are still a placeholder and have
+  /// not yet been computed.
+  bool placeholder;


Zero out the scalar.

Name this is_placeholder instead? While reading this later, I thought placeholder returned a placeholder itself.

chinmaygarde · 2024-09-26T23:39:39Z

impeller/typographer/glyph_atlas.h

                     SubpixelGlyph::Hash,
                     SubpixelGlyph::Equal>
      positions_;

  FontGlyphAtlas(const FontGlyphAtlas&) = delete;

-  FontGlyphAtlas& operator=(const FontGlyphAtlas&) = delete;
+  // FontGlyphAtlas& operator=(const FontGlyphAtlas&) = delete;


Did you mean to comment this out? Couldn't find where you don't want to copy but do want to make it copy assigned. At least the rule of 5 thing doesn't track.

I forgot about this one. I wasn't sure if we should try to design around this. Basically with this I can't do:

font_atlas_map_[scaled_font] = FontGlyphAtlas(); return &font_atlas_map_[scaled_font];

and I'm not sure what the alternative is.

I think the alternative is insert. You provide a pair and it returns an iterator.

chinmaygarde · 2024-09-26T23:43:24Z

impeller/typographer/backends/stb/typographer_context_stb.cc

-      }
-    }
-  }
+  CollectNewGlyphs(last_atlas, text_frames, new_glyphs, new_sizes);


DCHECK that the sizes of the two out vectors are the same?

I'm slightly partial to returning a pair of vectors instead of out variables because it makes it clear that existing values in the vectors will be not be disregarded. I didn't see a place where you didn't have to clear the vectors before calling CollectNewGlyphs. Just one less thing you don't have to remember to do. Then you can use a structured binding to do something like:

auto [new_glyphs, new_sizes] = CollectNewGlyphs(last_atlas, text_frames);

Done. I think I one point we were incrementally adding to these but that is long since gone

chinmaygarde · 2024-09-26T23:50:46Z

impeller/typographer/backends/skia/typographer_context_skia.cc

+    std::vector<FontGlyphPair>& new_glyphs,
+    std::vector<Rect>& glyph_sizes) {
+  for (const auto& frame : text_frames) {
+    // TODO(jonahwilliams): unless we destroy the atlas (which we know about),


jonahwilliams · 2024-09-27T00:06:33Z

Thank you! :)

…55092)

jonahwilliams · 2024-09-27T17:04:00Z

reason for revert: framework golden failures.

This reverts commit d283bc8.

Reverts: #55092 Initiated by: jonahwilliams Reason for reverting: framework golden failures. Original PR Author: jonahwilliams Reviewed By: {chinmaygarde, jtmcdole} This change reverts the following previous change: Follow up to #55060 Currently we have multiple stages of hashing while font rendering, which is relatively expensive for the actualy required workload. First, we hash the contents of all text frames to compute the unique set of glyphs per frame. Then we diff this hash set against the hashmap of glyphs within the atlas. Finally we hash and lookup the final rendered bounds for each glyph. We can simplify this to 2. hash lookups for glyphs not yet in the atlas and 1. hash lookup for glyphs that are in the atlas. This is done by combing the step where we uniquely compute glyphs per frame with the diff against the current atlas. When this lookup is performed, we also store the glyph position (if found) in the text_frame itself - which allows text contents to skip the last hash, as long as the glyph has already been rendered. ### Before ![flutter_03](https://github.com/user-attachments/assets/be9c4459-f0c8-426c-b152-38861acb207f) ### After ![flutter_04](https://github.com/user-attachments/assets/1aa7cbd1-6af7-4020-8966-7e3baaef102b) Using this handy dandy test app: ```dart import 'package:flutter/material.dart'; void main() { runApp(MyApp()); } class MyApp extends StatelessWidget { Widget build(context) { return MaterialApp( home: Scaffold( appBar: AppBar( title: Text('Platform View'), ), body: SafeArea(child: Stack(children: [ SizedBox( width: 380, height: 380, child: LinearProgressIndicator(), ), Stack( children: List<Widget>.generate(1000, (index) { // The problem already happens with a small amount of widgets. // Using an excessive amount of widgets is just to make the problem more evident. return Text("Lots of Texts represent a Widget with complex components."); }), ), Align( alignment: Alignment.bottomCenter, child: TextButton( child: Text("Button"), onPressed: () { print("Tap ${DateTime.now()}"); }, ), ), ], ), ), ), ); } } ```

…55092)

…155843) flutter/engine@7c603de...f21f2b2 2024-09-27 [email protected] [docs] Fix broken links in docs/ (flutter/engine#55350) 2024-09-27 [email protected] Roll Skia from e77818421e91 to 7efc11f2ea9e (6 revisions) (flutter/engine#55489) 2024-09-27 [email protected] Listen for uncaught exceptions during loading of a web test suite in Chrome (flutter/engine#55166) 2024-09-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] hash even less stuff per frame. (#55092)" (flutter/engine#55491) 2024-09-27 [email protected] [web] Update builder json generator to reflect recent changes (flutter/engine#55307) 2024-09-27 [email protected] [Impeller] hash even less stuff per frame. (flutter/engine#55092) 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] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#155843) flutter/engine@7c603de...f21f2b2 2024-09-27 [email protected] [docs] Fix broken links in docs/ (flutter/engine#55350) 2024-09-27 [email protected] Roll Skia from e77818421e91 to 7efc11f2ea9e (6 revisions) (flutter/engine#55489) 2024-09-27 [email protected] Listen for uncaught exceptions during loading of a web test suite in Chrome (flutter/engine#55166) 2024-09-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] hash even less stuff per frame. (flutter#55092)" (flutter/engine#55491) 2024-09-27 [email protected] [web] Update builder json generator to reflect recent changes (flutter/engine#55307) 2024-09-27 [email protected] [Impeller] hash even less stuff per frame. (flutter/engine#55092) 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] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Reverted due to missing glyph when rolling into the framework. Problem was that we should be always using the unrounded rounded scale to compute subpixel position but we sometimes used the rounded scale. --- Currently we have multiple stages of hashing while font rendering, which is relatively expensive for the actualy required workload. First, we hash the contents of all text frames to compute the unique set of glyphs per frame. Then we diff this hash set against the hashmap of glyphs within the atlas. Finally we hash and lookup the final rendered bounds for each glyph. We can simplify this to 2. hash lookups for glyphs not yet in the atlas and 1. hash lookup for glyphs that are in the atlas. This is done by combing the step where we uniquely compute glyphs per frame with the diff against the current atlas. When this lookup is performed, we also store the glyph position (if found) in the text_frame itself - which allows text contents to skip the last hash, as long as the glyph has already been rendered. See #55092

Reverted due to missing glyph when rolling into the framework. Problem was that we should be always using the unrounded rounded scale to compute subpixel position but we sometimes used the rounded scale. --- Currently we have multiple stages of hashing while font rendering, which is relatively expensive for the actualy required workload. First, we hash the contents of all text frames to compute the unique set of glyphs per frame. Then we diff this hash set against the hashmap of glyphs within the atlas. Finally we hash and lookup the final rendered bounds for each glyph. We can simplify this to 2. hash lookups for glyphs not yet in the atlas and 1. hash lookup for glyphs that are in the atlas. This is done by combing the step where we uniquely compute glyphs per frame with the diff against the current atlas. When this lookup is performed, we also store the glyph position (if found) in the text_frame itself - which allows text contents to skip the last hash, as long as the glyph has already been rendered. See flutter/engine#55092

jonahwilliams added 2 commits September 10, 2024 13:48

Only hash once

f1c8ca3

make some things private.

7b10275

github-actions bot added the e: impeller label Sep 10, 2024

jonahwilliams added 3 commits September 11, 2024 11:23

Merge branch 'main' of github.com:flutter/engine into collector

43344c5

updates for pt 1

b590a17

hmm

fa9bde1

github-actions bot added the platform-ios label Sep 11, 2024

++

6ae31ee

jonahwilliams marked this pull request as ready for review September 11, 2024 20:48

jonahwilliams added 2 commits September 11, 2024 15:39

++

57c2f8f

++

52ce07f

jonahwilliams commented Sep 11, 2024

View reviewed changes

jonahwilliams requested a review from chinmaygarde September 11, 2024 23:24

++

b618ced

jonahwilliams commented Sep 12, 2024

View reviewed changes

jonahwilliams added 3 commits September 11, 2024 17:27

++

207be7d

oops

38ada16

oops 2

c87ab8b

flutter-dashboard bot added the will affect goldens label Sep 12, 2024

Jonah Williams added 2 commits September 11, 2024 21:24

Update text_contents.cc

80bd4c8

Update text_contents.cc

6498872

jtmcdole approved these changes Sep 13, 2024

View reviewed changes

Merge branch 'main' into collector

84af683

chinmaygarde approved these changes Sep 26, 2024

View reviewed changes

jonahwilliams added 3 commits September 26, 2024 17:31

Merge branch 'main' of github.com:flutter/engine into collector

beac737

++

d55bed7

++

31b7bdc

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 27, 2024

auto-submit bot merged commit d283bc8 into flutter:main Sep 27, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Sep 27, 2024

Roll Flutter Engine from 7c603de2dca7 to d283bc8d7e60 (1 revision) flutter/flutter#155830

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2024

d283bc8d7 [Impeller] hash even less stuff per frame. (flutter/engine#…

af9f75f

…55092)

jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Sep 27, 2024

auto-submit bot pushed a commit that referenced this pull request Sep 27, 2024

Revert "[Impeller] hash even less stuff per frame. (#55092)"

a254c5e

This reverts commit d283bc8.

auto-submit bot mentioned this pull request Sep 27, 2024

Reverts "[Impeller] hash even less stuff per frame. (#55092)" #55491

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Sep 27, 2024

engine-flutter-autoroll mentioned this pull request Sep 27, 2024

Roll Flutter Engine from 7c603de2dca7 to 852dd3af66e9 (3 revisions) flutter/flutter#155834

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2024

d283bc8d7 [Impeller] hash even less stuff per frame. (flutter/engine#…

b6f87df

…55092)

engine-flutter-autoroll mentioned this pull request Sep 27, 2024

Roll Flutter Engine from 7c603de2dca7 to 570643cb6355 (5 revisions) flutter/flutter#155838

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2024

d283bc8d7 [Impeller] hash even less stuff per frame. (flutter/engine#…

013a171

…55092)

engine-flutter-autoroll mentioned this pull request Sep 27, 2024

Roll Flutter Engine from 7c603de2dca7 to f21f2b232b8a (6 revisions) flutter/flutter#155843

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2024

d283bc8d7 [Impeller] hash even less stuff per frame. (flutter/engine#…

c1446e7

…55092)

jonahwilliams mentioned this pull request Oct 24, 2024

[Impeller] Reland: hash less stuff per frame. #55742

Merged

[Impeller] hash even less stuff per frame. #55092

[Impeller] hash even less stuff per frame. #55092

Uh oh!

Conversation

jonahwilliams commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Sep 12, 2024

Uh oh!

jtmcdole left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Sep 26, 2024

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Sep 27, 2024

Uh oh!

Uh oh!

jonahwilliams commented Sep 27, 2024

Uh oh!

Uh oh!

jonahwilliams commented Sep 10, 2024 •

edited

Loading