I did a fairly high-level review of this. I did notice a number of details as well (e.g. "The output format of a compiled shader." as documentation for enum vkd3d_shader_cache_flags), but have largely left those out for brevity.
- We shouldn't add this to include/vkd3d.h. We can start implementing the shader cache functionality inside libvkd3d, but we shouldn't expose it as a public libvkd3d API at any point. !732 should unblock moving the implementation to libvkd3d-shader.
- Do we really need vkd3d_shader_cache_enumerate(), and do we really need enumeration to happen through a callback? Could we e.g. return a list of keys instead? As the documentation says, the callback runs while holding the cache mutex, and that seems like asking for trouble.
- vkd3d_shader_cache_put() has a "flags" parameter, but vkd3d_shader_cache_get() doesn't. Are we sure we'll never need it?
- Presumably "mem_size" and "disk_size" in struct vkd3d_shader_cache_info are specified in bytes. Being uint32_t's that would make their maximum size about 4 GiB. And while you can store a fair amount of data in 4 GiB, it's perhaps not entirely inconceivable to go over that limit these days. Should we use size_t for these?
- vkd3d_shader_cache_open() gets most of its parameters through struct vkd3d_shader_cache_info, but not "name". Seems arbitrary.
- By convention, vkd3d_shader_cache_open() would be called vkd3d_shader_open_cache(). (We also have e.g. vkd3d_create_instance(), vkd3d_create_device(), vkd3d_shader_free_shader_code(), and so on.)
- Do we want to give the application control over when the cache should be flushed to disk? Probably not a requirement for the initial version, but it seems useful to have options other than "the cache get serialised to disk when the cache is closed" and "any shader cache API call may cause the cache to get serialised to disk".
- Suppose I want to run two instances of e.g. vkd3d-gears simultaneously. How should that work?
- How should names get resolved for on-disk caches? Should "cache" and "./cache" be considered the same cache?
- vkd3d_shader_cache_compare_key() should use vkd3d_u32_compare(), and introduce vkd3d_u64_compare().
- > ```diff > +/* As the name implies this is taken from moltenvk. */ > +#define MVKHASH_SEED 5381 > +static inline uint64_t mvkHash64(const uint64_t *pVals, size_t count, uint64_t seed) > +{ > + uint64_t hash = seed; > + for (size_t i = 0; i < count; ++i) > + hash = ((hash << 5) + hash) ^ pVals[i]; > + > + return hash; > +} > ```
Please don't do that; MoltenVK has an incompatible license. There's also no need; this essentially the djb2a hash, although modified in a questionable way to use a 64-bit "pVals". (Incidentally, note that djb2a is equivalent to FNV-1 with "offset" 5381 and "prime" 33.)
- > ```diff > - struct vkd3d_render_pass_cache render_pass_cache; > + struct vkd3d_shader_cache *render_pass_cache; > ```
I think we'd like to instead define struct vkd3d_render_pass_cache like this: ```c struct vkd3d_render_pass_cache { struct vkd3d_shader_cache cache; }; ``` and retain some type checking.
- Presumably we'd like to be able to evict entries at some point. How would that work with e.g. the render pass cache? Would an eviction just vkDestroyRenderPass() an entry that may still be in use by libvkd3d? Also, how would the cache know how to destroy the evicted entry?
- Serialising the rbtree in depth-first order, and then inserting one node at a time when reading it back doesn't seem optimal.
- vkd3d_shader_cache_write() doesn't seem very robust. Generally, when rewriting a file like that, you'd first write the new version to a temporary file, sync, and then replace the old file with the name one. Ideally we'd simply avoid completely rewriting the cache each time though.
- We shouldn't pollute the current working directory with shader cache files. These should go into XDG_CACHE_HOME/vkd3d unless otherwise specified, and the Windows equivalent on Windows.
- > ```diff > +HRESULT vkd3d_render_pass_cache_find(struct vkd3d_shader_cache *cache, struct d3d12_device *device, > + const struct vkd3d_render_pass_key *key, VkRenderPass *vk_render_pass); > +struct vkd3d_shader_cache *vkd3d_root_signature_cache_init(struct d3d12_device *device); > +void vkd3d_root_signature_cache_cleanup(struct vkd3d_shader_cache *cache, struct d3d12_device *device); > HRESULT vkd3d_render_pass_cache_find(struct vkd3d_shader_cache *cache, struct d3d12_device *device, > const struct vkd3d_render_pass_key *key, VkRenderPass *vk_render_pass); > ```
That doesn't look right...
- Instead of introducing d3d12_cache_hr_from_vkd3d(), I think we can just add the extra cases to the existing hresult_from_vkd3d_result().
As for next steps: - I don't think there's much point in carrying around the ID3D12Device9 patch in this MR. You should just upstream that, ideally sooner rather than later. - That's largely true for the ID3D12ShaderCacheSession tests as well. - You can then start implementing ID3D12ShaderCacheSession, and make those tests pass, using the vkd3d_shader_cache API as a purely internal API. - Once that's in place the rest of libvkd3d can start using this for the renderpass cache, the Vulkan pipeline cache, and so on. - Once we have those working well, we can introduce this as a public API in libvkd3d-shader, for external users like wined3d.