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

Vulkan: Don't warn about invalid pipelines cache if missing #89180

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Mar 5, 2024

It used to warn when opening a new project because no cache pre-exists,
which isn't particularly helpful.

See #81150 (comment)

Also include the rendering method in the cache filename, as it differs
between Forward+ and Mobile for a same GPU.

Full set of pipelines caches on my laptop with integrated AMD and discrete AMD GPUs, editing both Forward+ and Mobile versions of a blank Vulkan project.

$ ls -l ~/.local/share/godot/app_userdata/TestVulkan/vulkan/
total 1800
-rw-rw-rw-. 1 akien akien 722500 Mar  5 14:23 'pipelines.forward_plus.amd_radeon_graphics_(radv_gfx1103_r1).editor.cache'
-rw-rw-rw-. 1 akien akien 722500 Mar  5 14:24 'pipelines.forward_plus.amd_radeon_rx_7600m_xt_(radv_navi33).editor.cache'
-rw-rw-rw-. 1 akien akien 195624 Mar  5 14:21 'pipelines.mobile.amd_radeon_graphics_(radv_gfx1103_r1).editor.cache'
-rw-rw-rw-. 1 akien akien 195624 Mar  5 14:21 'pipelines.mobile.amd_radeon_rx_7600m_xt_(radv_navi33).editor.cache'

$ md5sum ~/.local/share/godot/app_userdata/TestVulkan/vulkan/*
2ecc1c142a674127d8417136d43162d4  /home/akien/.local/share/godot/app_userdata/TestVulkan/vulkan/pipelines.forward_plus.amd_radeon_graphics_(radv_gfx1103_r1).editor.cache
19fb5379fb8254e22f4f04144970e82a  /home/akien/.local/share/godot/app_userdata/TestVulkan/vulkan/pipelines.forward_plus.amd_radeon_rx_7600m_xt_(radv_navi33).editor.cache
690d563e72ef52973ac18370e5ff5183  /home/akien/.local/share/godot/app_userdata/TestVulkan/vulkan/pipelines.mobile.amd_radeon_graphics_(radv_gfx1103_r1).editor.cache
088f86d4dc5dfc06a04de1b9b87b2bc1  /home/akien/.local/share/godot/app_userdata/TestVulkan/vulkan/pipelines.mobile.amd_radeon_rx_7600m_xt_(radv_navi33).editor.cache

@akien-mga akien-mga added enhancement topic:rendering cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 5, 2024
@akien-mga akien-mga added this to the 4.3 milestone Mar 5, 2024
@akien-mga akien-mga requested a review from a team as a code owner March 5, 2024 13:24
@@ -3828,8 +3828,8 @@ bool RenderingDeviceDriverVulkan::pipeline_cache_create(const Vector<uint8_t> &p

// Parse.
{
if (p_data.size() <= (int)sizeof(PipelineCacheHeader)) {
WARN_PRINT("Invalid/corrupt pipelines cache.");
if (p_data.size() != (int)sizeof(PipelineCacheHeader)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I can change it back to <= if it was indeed intentional to treat a bigger buffer as supported and attempt parsing it.

Copy link
Contributor

@DarioSamo DarioSamo Mar 5, 2024

Choose a reason for hiding this comment

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

That sounds like it'd make it fail any time the data is not exactly the size of the header. The condition is for ensuring it has enough bytes to parse at least the header. This effectively makes the pipeline cache never work.

Copy link
Member Author

Choose a reason for hiding this comment

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

At first I only singled out the size() == 0 case, so that it doesn't warn when there's no data (which is the case currently, it just loads the file and sends the data to the function without further check).

But then I still got warnings after adding a 3D node to my project, presumably because the data was now bigger than the previous pipelines cache generated from an empty scene? Might have been with my != change though, so I'll test again with <=.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can say in isolation the code doing the <= check is the intended approach at least. I believe there's other reasons why it keeps complaining it's corrupted, but this condition can't be it: it's just checking if there's enough bytes to parse the header.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted that change and now only handle the size() == 0 case specifically.

That solves the warning when editing a project without a pre-existing cache, and doesn't seem to trigger warnings when modifying the project (from a quick test). So the warnings I saw earlier were likely from my changing the check to !=.

@DarioSamo
Copy link
Contributor

DarioSamo commented Mar 5, 2024

and it warns when opening a project after having done any kind of rendering adjacent edit, since the cache will need to change.

In theory it should not do that. There seems to be an unrelated error as to why it thinks the pipeline cache is corrupted every time it starts. The only kind of change that should lead to invalidating it is a driver version change or some source-level change in Godot itself that invalidates everything by changing the version number.

That said the pipeline cache is a bit of a discussion point and I'm not sure I've seen much benefit of using it myself. I think the current belief is that Mobile might rely more on it, as on desktop it's pretty much guaranteed the driver will handle it on its own as well.

It'll certainly make things a bit harder going forward to provide parallel PSO compilation, as synchronizing one cache is a multi-thread contention point there.

@Calinou
Copy link
Member

Calinou commented Mar 5, 2024

See #80232 which makes the messages more descriptive without removing them entirely.

@akien-mga
Copy link
Member Author

akien-mga commented Mar 5, 2024

See #80232 which makes the messages more descriptive without removing them entirely.

IMO given how spammy those are, I wouldn't make them even more verbose and give users the impression their game will suffer from it.

Edit: After my PR is merged, I guess those additions could be ok, as those warnings should become rare. But those warnings being rare and engine dev oriented is also an incentive not to make them too verbose and user-oriented.

@akien-mga akien-mga force-pushed the vulkan-pipelines-cache-silence-wrong-warning branch from e00c56a to 2cb0ba2 Compare March 5, 2024 14:27
@akien-mga akien-mga changed the title Vulkan: Don't warn about invalid pipelines cache if missing or outdated Vulkan: Don't warn about invalid pipelines cache if missing Mar 5, 2024
@akien-mga akien-mga requested a review from DarioSamo March 5, 2024 14:29
It used to warn when opening a new project because no cache pre-exists,
which isn't particularly helpful.

Also include the rendering method in the cache filename, as it differs
between Forward+ and Mobile for a same GPU.
@akien-mga akien-mga force-pushed the vulkan-pipelines-cache-silence-wrong-warning branch from 2cb0ba2 to e74f4ea Compare March 5, 2024 15:39
@akien-mga akien-mga merged commit bee7e12 into godotengine:master Mar 5, 2024
16 checks passed
@akien-mga akien-mga deleted the vulkan-pipelines-cache-silence-wrong-warning branch March 5, 2024 16:12
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
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.

4 participants