Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a vec4 instead of an array in canvas shader instance buffer #100028

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Dec 5, 2024

This avoids a pathological performance cliff on Adreno devices

I noticed a regression between dev2 and dev3 in 2D rendering performance. I finally took the time to investigate it last night. I found that on my Pixel 4 (Adreno 640) frame time roughly tripled (from 15 FPS down to 5 FPS) in the isometric 2D demo.

The regression was most likely caused by the move to batching which is very weird as batching barely affects the shader code.

I ran a profiler over the demo and found something very odd. The profiler showed shader processor memory bandwidth use go from 16mb/s to 6.5GB/s. I expected that this scene would be ALU bound due to the high number of lights.

After a lot of investigation, I accidentally stumbled on the cause. Adreno doesn't like having an array inside a struct inside of an array. We can easily fix that since vectors can be indexed like arrays. Simply by changing the lights member into a uvec4 we reclaim all the performance we lost. Non-Adreno devices don't care one way or the other, so this ends up being a very safe fix.

This avoids a pathological performance cliff on Adreno devices
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Nice find!

Copy link
Contributor

@Ansraer Ansraer left a comment

Choose a reason for hiding this comment

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

I hate that this works. Approved.

@Repiteo Repiteo merged commit a4c2e16 into godotengine:master Dec 5, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 5, 2024

Thanks!

@clayjohn clayjohn deleted the rd-lights-array branch December 18, 2024 21:20
@clayjohn clayjohn changed the title Use a vector instead of an array in canvas shader instance buffer Use a vec4 instead of an array in canvas shader instance buffer Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants