Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] migrate gaussian to half precision on Metal #40788

Closed
jonahwilliams wants to merge 2 commits into from
Closed

[Impeller] migrate gaussian to half precision on Metal #40788

jonahwilliams wants to merge 2 commits into from

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Mar 30, 2023 •
edited
Loading

A redo of #40752.

This only migrates the shader and the interpolants and not gaussian.glsl to half precision. Locally this doubles the frame throughput on the ios backdropfilter benchmark.

Local engine before:

Task result:
{
  "success": true,
  "data": {
    "average_frame_build_time_millis": 0.30974999999999997,
    "90th_percentile_frame_build_time_millis": 0.409,
    "99th_percentile_frame_build_time_millis": 0.459,
    "worst_frame_build_time_millis": 0.459,
    "missed_frame_build_budget_count": 0,
    "average_frame_rasterizer_time_millis": 251.06689743589746,
    "90th_percentile_frame_rasterizer_time_millis": 251.512,
    "99th_percentile_frame_rasterizer_time_millis": 252.051,
    "worst_frame_rasterizer_time_millis": 252.051,
    "missed_frame_rasterizer_budget_count": 39,
    "frame_count": 40,
    "frame_rasterizer_count": 39,
    "new_gen_gc_count": 0,
    "old_gen_gc_count": 0,

After

{
 "success": true,
 "data": {
   "average_frame_build_time_millis": 0.3017714285714285,
   "90th_percentile_frame_build_time_millis": 0.37,
   "99th_percentile_frame_build_time_millis": 0.476,
   "worst_frame_build_time_millis": 0.476,
   "missed_frame_build_budget_count": 0,
   "average_frame_rasterizer_time_millis": 286.54435294117644,
   "90th_percentile_frame_rasterizer_time_millis": 286.778,
   "99th_percentile_frame_rasterizer_time_millis": 287.042,
   "worst_frame_rasterizer_time_millis": 287.042,
   "missed_frame_rasterizer_budget_count": 34,
   "frame_count": 35,
   "frame_rasterizer_count": 34,
   "new_gen_gc_count": 0,
   "old_gen_gc_count": 0,
   "frame_build_times": [

@chinmaygarde
Copy link
Contributor

chinmaygarde commented Mar 30, 2023

"this doubles the frame throughput"

Dang

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 30, 2023
dnfield
dnfield reviewed Mar 30, 2023
View reviewed changes
impeller/compiler/shader_lib/impeller/texture.glsl
Comment on lines +148 to +151
if (any(lessThan(coords, f16vec2(0.0hf))) ||
any(greaterThanEqual(coords, f16vec2(1.0hf)))) {
return f16vec4(0.0hf);
}
Copy link
Contributor

@dnfield dnfield Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clamp?

Why are we truncating to zero always here?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is how decal works...

Copy link
Contributor

@dnfield dnfield Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I missed that

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 30, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 30, 2023

auto label is removed for flutter/engine, pr: 40788, due to - The status or check suite Linux Unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Mar 30, 2023

oops, this is faster beacuse I broke it LOL

jonahwilliams
jonahwilliams commented Mar 30, 2023
View reviewed changes
impeller/entity/shaders/gaussian_blur/gaussian_blur.glsl

for (float i = -blur_info.blur_radius; i <= blur_info.blur_radius; i++) {
float gaussian = IPGaussian(i, blur_info.blur_sigma);
for (float i = blur_info.blur_radius; i <= blur_info.blur_radius; i++) {
Copy link
Contributor Author

@jonahwilliams jonahwilliams Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My b, I forgot the - 😆

@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Mar 30, 2023

This is still a regression and I need to investigate

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@zanderso zanderso Awaiting requested review from zanderso

2 more reviewers

@dnfield dnfield dnfield left review comments

@chinmaygarde chinmaygarde chinmaygarde approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jonahwilliams jonahwilliams

Labels

e: impeller

Projects

No open projects
Impeller
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jonahwilliams @chinmaygarde @dnfield