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

Simplify disk-cache-load on GLES as well, for the same reasons as #18216 #18217

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

hrydgard
Copy link
Owner

No description provided.

@hrydgard hrydgard added this to the v1.17.0 milestone Sep 24, 2023
@unknownbrackets
Copy link
Collaborator

I don't really agree with this one. It is nice that the loading screen and spinner show while the shaders are building. I don't like that it'll make the UI lock up and cause ANRs?

-[Unknown]

@hrydgard
Copy link
Owner Author

Hm, the thing is, we don't do much work on the main thread, the creation happens asynchronously on the render thread anyway - so the spinning isn't actually waiting for shader creation? Anyway, I didn't merge this now because I'm less sure of this one than the Vulkan one, I will test this and make sure I didn't fool myself again.

@unknownbrackets
Copy link
Collaborator

Well, even if the LoadCache func returns quickly, it seems like it's a problem if the spinner drops off before the shaders are actually ready. Maybe this is an existing problem then, though. If everything has to run in init steps and there are a bunch of shaders, you'd think that might cause some frame drops? Previously that was the purpose of all this pending stuff, to allow for frames to render while the cached shaders were built.

-[Unknown]

@hrydgard
Copy link
Owner Author

hrydgard commented Sep 30, 2023

It is indeed a problem and it's been happening for some time - what happens is that since creation of the shader objects from the cache is instantaneous, we just immediately move on, and the GPU thread will stall compiling instead. We'd need a way to tell it to limit itself to X number of tagged-as-from-cache shaders compiled per frame, and also a way to check the status, if we wanted the spinner to work for this. Hard to notice on PC though since it's so fast. If you see it spinning now, it's not because of shader compiles...

@unknownbrackets
Copy link
Collaborator

Well, I'm worried about the load time and locking up on weaker GL devices. But I guess it'll need to be changed ultimately in the render manager indeed.

In theory, we could have maybe the best of both worlds by:

  • Introducing a "global" steps queue just for shaders/programs (kinda like Vulkan has its background thing, but we can't do GL on a separate thread really...)
  • Add a flag to shaders/programs to indiciate if they are preloading.
  • When any program is used otherwise, mark its preloading flag false.
  • Introduce a budget and pass through the global queue each frame twice: first processing any marked with preloading=false, then processing any remaining if there's budgeted time left.

In theory, as long as a game didn't immediately need all the shaders, we could even skip the spinner still this way. That said, it'd still introduce a penalty if auto-loading a save state...

-[Unknown]

@hrydgard
Copy link
Owner Author

hrydgard commented Oct 1, 2023

Yeah, like, this PR doesn't actually break anything - it already was broken, and we need a separate mechanism, like the one you propose. So I think we can get this in, and then do something better on the RenderThread side later.

Unfortunately, not even that will lead to completely smooth shader caching on GL, since a lot of state that affect shader compilation is not baked into glProgram objects, unlike in Vulkan. So if a blend state is different, a lot of drivers will do a recompile anyway at glDrawArrays time. Fixing this problem is why Vulkan pipeline carry so much state...

@hrydgard hrydgard merged commit bd760b9 into master Oct 3, 2023
18 checks passed
@hrydgard hrydgard deleted the gles-simplify-disk-cache branch October 3, 2023 08:39
@unknownbrackets
Copy link
Collaborator

Well, this just piles a lot of them in the first frame. If we accept the premise that precompiling shaders doesn't reduce stutter at all, isn't it only making things worse here, then? If the game uses 30 combinations of shaders that get cached, and then stalls for a long while, it might appear to crash on some devices.

I mean, we know these can take hundreds of milliseconds on some drivers each, so the app may sit unresponsive for multiple seconds possibly. And even if it doesn't ANR from that, as you say, it'll later stutter anyway.

-[Unknown]

@hrydgard
Copy link
Owner Author

hrydgard commented Oct 8, 2023

It's indeed that bad, but this PR didn't make it worse.

As previously mentioned, a more sophisticated mechanism can spread the compiles out in time indeed, but in OpenGL there are no guarantees anyway due to the possible at-draw-call compiles.

That said, the longest precompile stalls I've seen in testing are on the order of 5-8 seconds, which, while bad, aren't catastrophic, and devices that slow are on the way out.

@unknownbrackets
Copy link
Collaborator

Well, it did used to work, it was made worse at some point obviously. But if it's not worth fixing and not really helping that much anyway, but causing potentially seconds of stall and potentially ANRs, wouldn't it be better at this point to just rip out the GLES precompile?

-[Unknown]

@hrydgard
Copy link
Owner Author

hrydgard commented Oct 8, 2023

It might still be somewhat better than not having it, but also it's kinda hard to know what's best globally given all the combinations of hardware and drivers. Not enough information to make a decision either way without tons of data gathering... easier to just simplify the code a little and leave it be, and focus on Vulkan...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants