-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
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] |
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. |
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] |
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... |
b8a2f0d
to
fb4a1fb
Compare
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:
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] |
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 |
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] |
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. |
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] |
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... |
No description provided.