[iOS] remove clear color hack.

[jonahwilliams]

Fixes flutter/flutter#125640

This doesn't actually do anything, instead it forces opaque to false. If developers want opaque to be false, they can just set it directly.

[jmagman]

This doesn't actually do anything, instead it forces opaque to false. If developers want opaque to be false, they can just set it directly.

I recall this hack fixing a whole lot of measurable jank issues listed in #39359, and we had confirmation from customers who could reliably reproduce jank that it fixed their issue (flutter/flutter#103847). I would prefer us not to remove it speculatively though indeed it looks unnecessary and crazy.

[jonahwilliams]

We should absolutely remove it as 1) we were never able to repro anything in our devicelab and 2) developers can opt into the existing mode by setting opaque = false.

All this change did was break direct2display.

[chinmaygarde]

Breaking direct-to-display is pretty severe and if we can't measure the improvement in any benchmark, I think we should go ahead with this.

[jmagman]

we were never able to repro anything in our devicelab

It was pretty reliably manually reproducible (check out the video) #39359. I agree it would be better if we had benchmark that definitely showed hitching went down, and removing it made hitching go up. But given the issue was widely seen in the wild, and this weird hack was widely confirmed to fix it, I'd value that over our incomplete benchmarks.

I agree though we don't need to keep weird hacks around forever as superstition, but given the number of 👍 in the issues this closed, I think it's worth investigating whether it's still needed.

cc @luckysmg who found the hack, as well as some investigated some alternatives.

[jmagman]

developers can opt into the existing mode by setting opaque = false.

Can we opt them into it? The weird hack improved the hitching by a lot, without needing to change anything themselves.

[luckysmg]

I m not sure whether the junk will regress if this hack removed now. Engine code has change a lot since I last seen haha
I think we need to test this change on local device. Especially using high fps device like iPhone15Pro

[jonahwilliams]

Absent specific evidence that something is an improvement, we should follow apple guidelines which specifically call out this property in the metal developer tools:

https://user-images.githubusercontent.com/8975114/234942519-fba0982a-e2c3-4bee-b838-c564ca1cc9b8.png

but given the number of 👍 in the issues this closed, I think it's worth investigating whether it's still needed.

People will thumbs up anything that claims to make performance faster, that doesn't mean that many people individually verified it.

[jonahwilliams]

Now if I land this and a benchmark regresses (which It might), then I at least have something to investigate.

[jmagman]

I think we need to test this change on local device. Especially using high fps device like iPhone15Pro

@jonahwilliams would you be able to run through the manual test @luckysmg reproduced this with on a high fps device and confirm this doesn't seriously regress hitching? I'm not sure if there is a benchmark test that would fail, but it was really obvious irl with manual testing.

we should follow apple guidelines which specifically call out this property in the metal developer tools:

Since the egregious color hack essentially "fixed" the performance issues without app devs needing to do anything, is there something more explicit we can do that doesn't require app devs needing to know they should set the opaque property? Can we default it to something with similar performance characteristics?

[chinmaygarde]

@jmagman @luckysmg What is the manual test?

[jmagman]

@jmagman @luckysmg What is the manual test?

I think it was just scrolling a list view on a Pro device like this?
flutter/flutter#103847 (comment)

Additional data, here's an internal bug that reported performance improving when the color hack rolled in b/263817100#comment17

[chinmaygarde]

What are the next steps here? Do we need a reproduce this on devices we have? Perhaps @jmagman and @jonahwilliams should chat?

[jonahwilliams]

I think this change is valid as is and I'm waiting for actionable feedback.

[jmagman]

I started trying to reproduce this to see if I can profile a simple scrolling list clearly with and without the color hack, but I was hitting the dreaded Can't load kernel binary (I gclient syncd, my flutter engine SHA matches where I branched this change from, I'm run --profileing, can't think what I'm missing here). I'm going to try with clean out directories.

Once I get this working:
If I can't reproduce it locally, I think we should land this and see what the benchmarks do.
If I can reproduce it locally then presumably this weird hack is still doing something. I think we should still land this to see what the benchmarks do, then immediately revert it. If the benchmarks moved, then yay benchmarks and we move on. If nothing moved, then we should add a benchmark that would have caught the regression.

What do you think, @jonahwilliams?

[jonahwilliams]

SGTM!

[jtmcdole]

Any updates?

[jmagman]

Any updates?

Yes, I have the repro case running at home, but I left my promotion display phone at work and I wanted to test at a higher frame rate. I already have a note to myself to bring it home next time I'm in the office.

[chinmaygarde]

@jonahwilliams has a high-framerate device and can confirm this patch makes no difference to performance. We would like to move ahead with this unless there are any objections.

[jmagman]

@jonahwilliams has a high-framerate device and can confirm this patch makes no difference to performance. We would like to move ahead with this unless there are any objections.

Sounds good, let's land it then and see how the benchmarks move. I had bought a new iPhone 16 Pro that came on Friday to test but I hadn't had a change to profile. Thanks for testing it, Jonah!

[jonahwilliams]

This needs an approval to land.

[jmagman]

This needs an approval to land.

Sorry, done.

[jonahwilliams]

Thanks!