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

Fix physical device surface querying regression #2026 by removing surface caching #2027

Merged
merged 6 commits into from
Oct 6, 2022
Merged

Fix physical device surface querying regression #2026 by removing surface caching #2027

merged 6 commits into from
Oct 6, 2022

Conversation

lilly-lizard
Copy link
Contributor

Fixes #2026

Fixed regression bug where new caching functionality introduced in #2006 caused surface queries to be out of date.

surface_capabilities, surface_formats, surface_present_modes and surface_support are no longer cached as it is proven in #2026 that surface properties can change at runtime for a given surface/physical device.

Also worth noting that requesting these structures doesn't happen too often so the performance impact will be minimal.

out of date. surface_capabilities, surface_formats,
surface_present_modes and surface_support are no longer cached as it is
proven that surface properties can change at runtime for a given
surface/physical device.
@marc0246
Copy link
Contributor

marc0246 commented Oct 6, 2022

Are you sure we need to ditch caching all-together? On my system, querying this info from the implementation is 800X slower. We could invalidate the caches where needed instead perhaps?

@lilly-lizard
Copy link
Contributor Author

lilly-lizard commented Oct 6, 2022

problem is I can't see a way to do that... the surface and physical device handles stay the same, but drivers seem happy to change the data returned by VK_KHR_surface commands depending on when you do the query e.g. VkSurfaceCapabilitiesKHR.min/maxImageExtent due to the window being resized.

you could assume that surface_formats, surface_present_modes and surface_support won't change at runtime, which is probably true but there's no guarantee of that. I doubt keeping those ones cached would cause any problems but seems like a weird risk to take given the infrequency of the calls imo.

but basically surface_capabilities is the only one that needs to not be cached to fix #2026 so I'd be happy to re-add caching for surface_formats, surface_present_modes and surface_support if that's what you guys want.

@marc0246
Copy link
Contributor

marc0246 commented Oct 6, 2022

I agree that caching the capabilities would be impractical, although possible. We would need to invalidate the cache on resize, which would pose no benefit as you likely only need that information when recreating the swapchain, so it would just overcomplicate things. About the other ones, I've looked through the specification repo but couldn't find anything specific about the lifetime of the info.

@lilly-lizard
Copy link
Contributor Author

After looking through the structs again, I really don't see any reason why surface_formats, surface_present_modes and surface_support would change during runtime. I'll add caching back for them, but keep surface_capabilities un-cached.

@marc0246
Copy link
Contributor

marc0246 commented Oct 6, 2022

Sounds good.

…present_modes and surface_support, because unlike surface_capabilities, it is almost certain they won't change at runtime.
@Rua
Copy link
Contributor

Rua commented Oct 6, 2022

Thank you for catching this one!

@Rua
Copy link
Contributor

Rua commented Oct 6, 2022

There's an error on MacOS, can you fix it?

@lilly-lizard
Copy link
Contributor Author

whoops missed that. pretty sure macos/ios will work now

@Rua Rua merged commit 2992f0d into vulkano-rs:master Oct 6, 2022
@lilly-lizard lilly-lizard deleted the physical-device-cache-bug branch October 6, 2022 18:27
fayalalebrun pushed a commit to VideowindoW/vulkano that referenced this pull request Oct 22, 2022
…moving surface caching (vulkano-rs#2027)

* removed cache check for PhysicalDevice::surface_capabilities_unchecked, fixing window resize bug

* Fixed bug where new caching functionality caused surface queries to be
out of date. surface_capabilities, surface_formats,
surface_present_modes and surface_support are no longer cached as it is
proven that surface properties can change at runtime for a given
surface/physical device.

* Re-added caching for VK_KHR_surface structs surface_formats, surface_present_modes and surface_support, because unlike surface_capabilities, it is almost certain they won't change at runtime.

* remove surface_capabilities cache from macos/ios to fix builds
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.

Surface-capabilities queries return out of date information due to caching
3 participants