-
Notifications
You must be signed in to change notification settings - Fork 424
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
Add disk cache for shader compilations #5829
Conversation
Awesome stuff. For the record, if a user was to change their hardware, could this cause an issue? Put another way, is there already a pathway to recovery if a cached shader file is corrupt in some way? |
The compilations are hardware-agnostic. We could even distribute these files ourselves if it came to it. Both data and a checksum of the data is written. If there's hardware-induced/OS-induced corruption, the file is abandoned and recompilation takes place, with the result of that then replacing the corrupt files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source looks good except for a few parts I'm not getting. I'll give this a test spin on the platforms I have access to later.
return compilation; | ||
} | ||
|
||
public ComputeProgramCompilation CompileCompute(string programText, CrossCompileTarget target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks unused - is this intended for future consumption somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking it could be used for #5757 for example.
@@ -123,10 +121,10 @@ private string loadFile(byte[] bytes, bool mainFile) | |||
|
|||
if (!string.IsNullOrEmpty(backbufferCode)) | |||
{ | |||
string realMainName = "real_main_" + Guid.NewGuid().ToString("N"); | |||
const string real_main_name = "__internal_real_main"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this main name change significant in any way? I'm not sure what the guid was doing, and blame doesn't help, since it was added in, of all things, b9406dd, which just barely predates me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's significant by keeping the shader code deterministic. The guid was only present to avoid naming collisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, good point. I'm assuming the naming collisions you mention are no longer relevant. Makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__internal_
should be as effective as guid, and if not then we could suffix an incrementing value like __internal_real_main_1
, but I doubt that'll happen.
One case I can realistically think of is if we end up wanting multiple main
trampolines, but there's no support for that right now.
Tested on Windows and Android. Can confirm sizeable gains in startup time on both. I don't have profiles to show from Android because Android but it does look significantly snappier when launching. This is pretty much good to go as far as I'm concerned, but I won't push the button yet pending potentially an iOS check and maybe a decision as to when we want to get this out, I guess. |
The previous output was always implying that a compilation happened, even when it wasn't. I also removed the pre-compilation log output, as that was mostly there to gauge performance of shader compiles, which is less of an issue now. If we still want performance metrics in logs, I'd instead consider adding a `Stopwatch` and including the elapsed time in the "loaded" log line. But this is probably unnecessary.
Tested to work well here. I've updated the logging around shader compilation to read better. I think this is fine to be merged, but will leave it approved for someone to check my latest commit. |
Shader compilation dominates our startup time, so they're now cached to disk (spirv + cross-compile + reflection) at
cache/shaders/
.On Vulkan, the refactoring means only one compilation occurs now:
The results are also significant on the D3D and Metal surfaces which have to compile twice - once to GLSL for reflection and a second time to the destination platform. I haven't added profiling results for these but they can be imagined by doubling every duration from the first table.