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.

Sign up for GitHub

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

Jump to bottom

layers: Don't pre-query surface attributes during creation #3452

Merged
merged 5 commits into from
Oct 25, 2021
Merged

layers: Don't pre-query surface attributes during creation #3452

merged 5 commits into from
Oct 25, 2021

Conversation

jeremyg-lunarg
Copy link
Contributor

@jeremyg-lunarg jeremyg-lunarg commented Oct 18, 2021 •
edited
Loading

Until commit c7a834a, each PHYSICAL_DEVICE_STATE had a cache of supported surface formats which was used in validating swapchain creation.

Some of the calls to get surface capabilities, formats or presentation modes are very slow in some implementations. Revert back to caching results made by the application, but store them in SURFACE_STATE rather than PHYSICAL_DEVICE_STATE. The original code assumed that the same results would apply to all surfaces, which doesn't appear to be true.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 2893.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5237 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5237 passed.

@jeremyg-lunarg jeremyg-lunarg mentioned this pull request Oct 18, 2021
Closed
@y-novikov
Copy link
Contributor

Thanks for working on this!
Unfortunately, the slowdown is even worse with this patch.
E.g. before the regression
[7222/13627] AtomicCounterBufferTest.AtomicCounterBufferBindings/ES3_Vulkan (738.9 ms)
after the regression
[7783/13627] AtomicCounterBufferTest.AtomicCounterBufferBindings/ES3_Vulkan (1509.1 ms)
with this patch
[8840/13632] AtomicCounterBufferTest.AtomicCounterBufferBindings/ES3_Vulkan (2791.9 ms)

@jeremyg-lunarg
Copy link
Contributor Author

😕 Ok, I'll take a look at those test cases on different hw and see if I can figure out what to do instead. The original caching wasn't 100% correct so I can't just go back to it, unfortunately.

@jeremyg-lunarg jeremyg-lunarg marked this pull request as draft October 19, 2021 14:22
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 4118.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5270 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5270 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 4220.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5274 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5274 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 4271.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5277 running.

@jeremyg-lunarg jeremyg-lunarg changed the title layers: Cache surface formats in SURFACE_STATE layers: Don't pre-query surface attributes during creation Oct 20, 2021
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5277 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 4343.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5280 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5280 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 4914.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5297 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5297 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5300 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5300 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 5362.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5303 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5303 passed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 5639.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5314 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5314 passed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 6760.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5329 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5329 passed.

@jeremyg-lunarg jeremyg-lunarg marked this pull request as ready for review October 25, 2021 15:39
ncesario-lunarg
ncesario-lunarg approved these changes Oct 25, 2021
View reviewed changes
layers/image_state.h Outdated Show resolved Hide resolved
@jeremyg-lunarg
tests: Check for surface support first in InitSwapchain()
c9b45a5 
Before calling InitSwapchainInfo(), make sure the physical
device and surface are supported. The vulkan calls in
InitSwapchainInfo() require this to be true or they are invalid
to make.
@jeremyg-lunarg
tests: Fix test for VUID 01270
4797913 
VUID-VkSwapchainCreateInfoKHR-surface-01270 requires that the device
support the surface, it doesn't require that you actually call
vkGetPhysicalDeviceSurfaceSupportKHR(). (You should, but that's a best
practice...)

Change the test to create a device that doesn't support surface present,
and then use it to try to create a swapchain.
@jeremyg-lunarg
tests: Only get requested queues in Device::init_queues()
bb63d68 
Make sure it only calls vkGetDeviceQueue() on the queues specified
in the device create info. This allows creating devices with a limited
set of queues rather than everything the phyiscal device supports.
@jeremyg-lunarg
layers: Fix IMAGE_STATE::Unlink() for swapchain images
3be7454 
Since these images are only destroyed when the swapchain is
destroyed, calling RemoveParent on it is unnecessary and can
cause the parent_nodes_ set to be corrupted.
@jeremyg-lunarg
layers: Don't pre-query surface attributes during creation
60eacee 
Some of the calls to get surface capabilities, formats or
presentation modes are very slow in some implementations.
Revert back to caching results made by the application, but
store them in SURFACE_STATE rather than PHYSICAL_DEVICE_STATE.
The original code assumed that the same results would apply
to all surfaces, which doesn't appear to be true.
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 6939.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5334 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5334 passed.

@jeremyg-lunarg jeremyg-lunarg merged commit 87b2e6b into KhronosGroup:master Oct 25, 2021
@jeremyg-lunarg jeremyg-lunarg deleted the jeremyg-issue-3449 branch October 25, 2021 20:57
FrankEnderman pushed a commit to FrankEnderman/cursemium that referenced this pull request Oct 27, 2021
@y-novikov
Revert "Increase angle_end2end_tests batch timeout on Win7 NVIDIA"
97445d4 
This reverts commit a8e2d2c.

Reason for revert: performance regression fixed in
KhronosGroup/Vulkan-ValidationLayers#3452,
rolled into Chromium in crrev.com/934959.

Original change's description:
> Increase angle_end2end_tests batch timeout on Win7 NVIDIA
>
> To unblock rolling VVL into Chromium,
> which have a performance regression on this platform.
>
> Bug: 1249209
> Change-Id: Ic66a1309b0482c41d8057bfea13303f2a306923d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3227076
> Auto-Submit: Yuly Novikov <ynovikov@chromium.org>
> Commit-Queue: Jamie Madill <jmadill@chromium.org>
> Reviewed-by: Jamie Madill <jmadill@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#932483}

Bug: 1249209
Change-Id: I80c579110027037a786ca863de423c3628c45fc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3246093
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Yuly Novikov <ynovikov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#935243}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncesario-lunarg ncesario-lunarg ncesario-lunarg approved these changes

@ziga-lunarg ziga-lunarg Awaiting requested review from ziga-lunarg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeremyg-lunarg @ci-tester-lunarg @y-novikov @ncesario-lunarg @ziga-lunarg