[Impeller] simplify text shaders by moving UV computation to contents #39989

jonahwilliams · 2023-03-01T21:30:57Z

This should improve performance without dramatically changing the raster thread cost - additionally allows us to upload less data.

Changes found in shader flutter/impeller/entity/gles/glyph_atlas.frag.gles on core Mali-G78:
has_uniform_computation:
  True <- before
  False <- after
In variant Main:
  fp16_arithmetic: 45 <- before
  fp16_arithmetic: 100 <- after
longest_path_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.125000    0.125000    0.093750    0.000000    0.000000    0.875000    0.250000    varying
after   0.093750    0.031250    0.093750    0.000000    0.000000    0.375000    0.250000    varying

shortest_path_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.125000    0.125000    0.046875    0.000000    0.000000    0.875000    0.250000    varying
after   0.062500    0.031250    0.062500    0.000000    0.000000    0.375000    0.250000    varying

total_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.156250    0.156250    0.093750    0.000000    0.000000    0.875000    0.500000    varying
after   0.093750    0.031250    0.093750    0.000000    0.000000    0.375000    0.250000    varying


For a full report, run:
  $ malioc --fragment --core Mali-G78 /b/s/w/ir/cache/builder/src/out/android_debug_unopt/gen/flutter/impeller/entity/gles/glyph_atlas.frag.gles

Changes found in shader flutter/impeller/entity/gles/glyph_atlas.frag.gles on core Mali-T880:
longest_path_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  1.650000    2.000000    1.000000    load_store
after   1.000000    1.000000    1.000000    arithmetic,load_store,texture

shortest_path_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  1.650000    2.000000    1.000000    load_store
after   1.000000    1.000000    1.000000    arithmetic,load_store,texture

total_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  3.000000    2.000000    2.000000    arithmetic
after   1.333333    1.000000    1.000000    arithmetic

In variant Main:
  work_registers_used: 3 <- before
  work_registers_used: 2 <- after

For a full report, run:
  $ malioc --fragment --core Mali-T880 /b/s/w/ir/cache/builder/src/out/android_debug_unopt/gen/flutter/impeller/entity/gles/glyph_atlas.frag.gles

Changes found in shader flutter/impeller/entity/gles/glyph_atlas.vert.gles on core Mali-G78:
In variant Position:
  fp16_arithmetic: 0 <- before
  fp16_arithmetic: 88 <- after
longest_path_cycles in variant Position
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.296875    0.296875    0.000000    0.000000    4.000000    0.000000    load_store
after   0.140625    0.140625    0.046875    0.000000    2.000000    0.000000    load_store

shortest_path_cycles in variant Position
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.296875    0.296875    0.000000    0.000000    4.000000    0.000000    load_store
after   0.140625    0.140625    0.046875    0.000000    2.000000    0.000000    load_store

total_cycles in variant Position
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.296875    0.296875    0.000000    0.000000    4.000000    0.000000    load_store
after   0.140625    0.140625    0.046875    0.000000    2.000000    0.000000    load_store

In variant Position:
  uniform_registers_used: 18 <- before
  uniform_registers_used: 16 <- after
In variant Varying:
  fp16_arithmetic: 100 <- before
  fp16_arithmetic: None <- after
longest_path_cycles in variant Varying
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.078125    0.015625    0.078125    0.000000    7.000000    0.000000    load_store
after   0.031250    0.000000    0.031250    0.000000    4.000000    0.000000    load_store

shortest_path_cycles in variant Varying
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.078125    0.015625    0.078125    0.000000    7.000000    0.000000    load_store
after   0.031250    0.000000    0.031250    0.000000    4.000000    0.000000    load_store

total_cycles in variant Varying
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.078125    0.015625    0.078125    0.000000    7.000000    0.000000    load_store
after   0.031250    0.000000    0.031250    0.000000    4.000000    0.000000    load_store

In variant Varying:
  work_registers_used: 12 <- before
  work_registers_used: 9 <- after

For a full report, run:
  $ malioc --vertex --core Mali-G78 /b/s/w/ir/cache/builder/src/out/android_debug_unopt/gen/flutter/impeller/entity/gles/glyph_atlas.vert.gles

Changes found in shader flutter/impeller/entity/gles/glyph_atlas.vert.gles on core Mali-T880:
longest_path_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  3.960000    12.000000   0.000000    load_store
after   2.970000    7.000000    0.000000    load_store

shortest_path_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  3.960000    12.000000   0.000000    load_store
after   2.970000    7.000000    0.000000    load_store

total_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  4.000000    12.000000   0.000000    load_store
after   3.000000    7.000000    0.000000    load_store

In variant Main:
  work_registers_used: 3 <- before
  work_registers_used: 2 <- after

For a full report, run:
  $ malioc --vertex --core Mali-T880 /b/s/w/ir/cache/builder/src/out/android_debug_unopt/gen/flutter/impeller/entity/gles/glyph_atlas.vert.gles

Changes found in shader flutter/impeller/entity/gles/glyph_atlas_sdf.frag.gles on core Mali-G78:
has_uniform_computation:
  True <- before
  False <- after
In variant Main:
  fp16_arithmetic: 60 <- before
  fp16_arithmetic: 77 <- after
longest_path_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.406250    0.406250    0.046875    0.312500    0.000000    0.750000    0.250000    varying
after   0.312500    0.312500    0.046875    0.312500    0.000000    0.250000    0.250000    arith_total,arith_fma,arith_sfu

shortest_path_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.406250    0.406250    0.015625    0.312500    0.000000    0.750000    0.250000    varying
after   0.312500    0.312500    0.015625    0.312500    0.000000    0.250000    0.250000    arith_total,arith_fma,arith_sfu

total_cycles in variant Main
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  varying     texture     bound       
before  0.406250    0.406250    0.046875    0.312500    0.000000    0.750000    0.250000    varying
after   0.312500    0.312500    0.046875    0.312500    0.000000    0.250000    0.250000    arith_total,arith_fma,arith_sfu

In variant Main:
  uniform_registers_used: 10 <- before
  uniform_registers_used: 8 <- after
In variant Main:
  work_registers_used: 22 <- before
  work_registers_used: 20 <- after

For a full report, run:
  $ malioc --fragment --core Mali-G78 /b/s/w/ir/cache/builder/src/out/android_debug_unopt/gen/flutter/impeller/entity/gles/glyph_atlas_sdf.frag.gles

Changes found in shader flutter/impeller/entity/gles/glyph_atlas_sdf.frag.gles on core Mali-T880:
longest_path_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  4.620000    2.000000    3.000000    arithmetic
after   3.300000    1.000000    3.000000    arithmetic

shortest_path_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  4.620000    2.000000    3.000000    arithmetic
after   3.300000    1.000000    3.000000    arithmetic

total_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  5.000000    2.000000    3.000000    arithmetic
after   3.666667    1.000000    3.000000    arithmetic

In variant Main:
  uniform_registers_used: 2 <- before
  uniform_registers_used: 1 <- after

For a full report, run:
  $ malioc --fragment --core Mali-T880 /b/s/w/ir/cache/builder/src/out/android_debug_unopt/gen/flutter/impeller/entity/gles/glyph_atlas_sdf.frag.gles

Changes found in shader flutter/impeller/entity/gles/glyph_atlas_sdf.vert.gles on core Mali-G78:
In variant Position:
  fp16_arithmetic: 90 <- before
  fp16_arithmetic: 88 <- after
longest_path_cycles in variant Position
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.171875    0.171875    0.046875    0.000000    4.000000    0.000000    load_store
after   0.140625    0.140625    0.046875    0.000000    2.000000    0.000000    load_store

shortest_path_cycles in variant Position
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.171875    0.171875    0.046875    0.000000    4.000000    0.000000    load_store
after   0.140625    0.140625    0.046875    0.000000    2.000000    0.000000    load_store

total_cycles in variant Position
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.171875    0.171875    0.046875    0.000000    4.000000    0.000000    load_store
after   0.140625    0.140625    0.046875    0.000000    2.000000    0.000000    load_store

In variant Position:
  uniform_registers_used: 14 <- before
  uniform_registers_used: 16 <- after
longest_path_cycles in variant Varying
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.000000    0.000000    0.000000    0.000000    5.000000    0.000000    load_store
after   0.000000    0.000000    0.000000    0.000000    3.000000    0.000000    load_store

shortest_path_cycles in variant Varying
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.000000    0.000000    0.000000    0.000000    5.000000    0.000000    load_store
after   0.000000    0.000000    0.000000    0.000000    3.000000    0.000000    load_store

total_cycles in variant Varying
        arith_total arith_fma   arith_cvt   arith_sfu   load_store  texture     bound       
before  0.000000    0.000000    0.000000    0.000000    5.000000    0.000000    load_store
after   0.000000    0.000000    0.000000    0.000000    3.000000    0.000000    load_store

In variant Varying:
  work_registers_used: 8 <- before
  work_registers_used: 6 <- after

For a full report, run:
  $ malioc --vertex --core Mali-G78 /b/s/w/ir/cache/builder/src/out/android_debug_unopt/gen/flutter/impeller/entity/gles/glyph_atlas_sdf.vert.gles

Changes found in shader flutter/impeller/entity/gles/glyph_atlas_sdf.vert.gles on core Mali-T880:
longest_path_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  3.300000    10.000000   0.000000    load_store
after   2.970000    5.000000    0.000000    load_store

shortest_path_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  3.300000    10.000000   0.000000    load_store
after   2.970000    5.000000    0.000000    load_store

total_cycles in variant Main
        arithmetic  load_store  texture     bound       
before  3.333333    10.000000   0.000000    load_store
after   3.000000    5.000000    0.000000    load_store


For a full report, run:
  $ malioc --vertex --core Mali-T880 /b/s/w/ir/cache/builder/src/out/android_debug_unopt/gen/flutter/impeller/entity/gles/glyph_atlas_sdf.vert.gles

Part of flutter/flutter#121732

flutter-dashboard · 2023-03-01T21:30:59Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jonahwilliams · 2023-03-01T21:33:32Z

impeller/entity/shaders/glyph_atlas.frag

-            .aaaa *
-        frag_info.text_color;
+  vec4 value = texture(glyph_atlas_sampler, v_uv);
+  if (v_has_color != 1.0) {


need to get rid of this too

Perhaps this wouldn't be an issue if we just had separate atlases. Leaving for another day.

jonahwilliams · 2023-03-01T23:07:56Z

This shader is also almost what I need for flutter/flutter#121650

bdero

LGTM! Super minor comments.

bdero · 2023-03-02T00:10:27Z

impeller/entity/contents/text_contents.cc

+        auto position = PositionForGlyphPosition(
+            translation, point, glyph_position.glyph.bounds.size);
+        vtx.uv = point * uv_scaler_a + uv_scaler_b;
+        vtx.position = position;


I like that we're not passing as much needlessly interpolated data to the fragment shader anymore (🤢), but I wonder if the 16 byte savings per vertex is worth the extra per character CPU work. My guess is that it's worth it.

you also have to pay the CPU cost of copying that extra data too. Its probably slightly more expensive on the raster side of things but hard to tell exactly.

bdero · 2023-03-02T00:12:09Z

impeller/entity/contents/text_contents.cc

+          Matrix(1, 0, 0, 0,  //
+                 0, 1, 0, 0,  //
+                 0, 0, 1, 0,  //
+                 offset_glyph_position.x, offset_glyph_position.y, 0, 1);


Matrix::MakeTranslation?

bdero · 2023-03-02T00:14:56Z

impeller/entity/contents/text_contents.cc

+                                        Size destination_size) {
+  return translation * Vector4(unit_position.x * destination_size.width,
+                               unit_position.y * destination_size.height, 0.0,
+                               1.0);


Doesn't translation * (unit_position * destination_size) work fine for this, or are we missing an overload somewhere? Size should get coerced to Point.

…ne into simplify-text-shaders

…o contents (flutter/engine#39989)

[Impeller] simplify text shaders

cf7e6fc

flutter-dashboard bot added the needs tests label Mar 1, 2023

jonahwilliams commented Mar 1, 2023

View reviewed changes

jonahwilliams added 2 commits March 1, 2023 15:38

update malioc.json

b92b7e8

Update glyph_atlas_sdf.frag

dc7a238

jonahwilliams requested review from dnfield and bdero March 1, 2023 23:39

bdero approved these changes Mar 2, 2023

View reviewed changes

chinmaygarde added the e: impeller label Mar 2, 2023

chinmaygarde assigned jonahwilliams Mar 2, 2023

jonahwilliams added 3 commits March 3, 2023 11:16

bdero review

386c861

Merge branch 'simplify-text-shaders' of github.com:jonahwilliams/engi…

5e68461

…ne into simplify-text-shaders

Merge branch 'main' into simplify-text-shaders

e54afc5

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 3, 2023

auto-submit bot merged commit 7912b23 into flutter:main Mar 3, 2023

engine-flutter-autoroll mentioned this pull request Mar 3, 2023

Roll Flutter Engine from f31988115453 to 9a6b6d85f180 (5 revisions) flutter/flutter#121911

Closed

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

7912b23b3 [Impeller] simplify text shaders by moving UV computation t…

f778a38

…o contents (flutter/engine#39989)

engine-flutter-autoroll mentioned this pull request Mar 3, 2023

Roll Flutter Engine from f31988115453 to 249e78dfb025 (8 revisions) flutter/flutter#121913

Merged

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

7912b23b3 [Impeller] simplify text shaders by moving UV computation t…

3cdcfda

…o contents (flutter/engine#39989)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] simplify text shaders by moving UV computation to contents #39989

[Impeller] simplify text shaders by moving UV computation to contents #39989

jonahwilliams commented Mar 1, 2023 •

edited

flutter-dashboard bot commented Mar 1, 2023

jonahwilliams Mar 1, 2023

jonahwilliams Mar 1, 2023

jonahwilliams commented Mar 1, 2023

bdero left a comment

bdero Mar 2, 2023

jonahwilliams Mar 3, 2023

bdero Mar 2, 2023

jonahwilliams Mar 3, 2023

bdero Mar 2, 2023

jonahwilliams Mar 3, 2023

[Impeller] simplify text shaders by moving UV computation to contents #39989

[Impeller] simplify text shaders by moving UV computation to contents #39989

Conversation

jonahwilliams commented Mar 1, 2023 • edited

flutter-dashboard bot commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Mar 1, 2023

bdero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Mar 1, 2023 •

edited