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

corechecks: Add setting to enable fine-grained locking #3646

Merged

Conversation

jeremyg-lunarg
Copy link
Contributor

@jeremyg-lunarg jeremyg-lunarg commented Jan 3, 2022

WIP! This is one of the last changes I intend to merge for the locking improvements, but it needs sooner review because it involves user-visible names that need to end up in documentation in several different places.

Add a setting to make calls to ValidationObject::ReadLock() and
WriteLock() be no-ops, because locking in other parts of the code
has made these global locks unnecessary.

Currently this only affects Core Validation, but eventually it should
also change the behavior of Best Practices and Synchronization
Validation.

The setting defaults to off for now and there is logging when fine grained
locking is enabled, since this is an experimental feature which
make cause stability problems or incorrect errors to be reported.
Once this stabilizes, the setting will default to on and the log message
will report when it is turned off, because that will be slow.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 9085.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5808 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 9095.

@KarenGhavam-lunarG
Copy link
Contributor

@christophe-lunarg Ping. We will want these settings exposed by vkconfig.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5809 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5809 failed.

Copy link
Contributor

@christophe-lunarg christophe-lunarg left a comment

Choose a reason for hiding this comment

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

The manifest file is not correct (default is a boolean), here is a fix with some clean up:

            "settings": [
                {
                    "key": "fine_grained_locking",
                    "env": "VK_LAYER_FINE_GRAINED_LOCKING",
                    "label": "Fine Grained Locking",
                    "description": "Enable fine grained locking for Core Validation, which should improve performance in multithreaded applications. This is an experimental feature which may cause stability problems or incorrect errors to be reported.",
                    "status": "BETA",
                    "type": "BOOL",
                    "default": false,
                    "platforms": [ "WINDOWS", "LINUX", "MACOS", "ANDROID" ]
                },

There is no "experimental" status for settings only "alpha", "beta", "stable" and "deprecated".

I would add this setting first in the list for where it will be exposed in Vulkan Configurator UI

I only made a little change in Vulkan Configurator to display the status next to the label :
image

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 9837.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 9852.

Copy link
Contributor

@mark-lunarg mark-lunarg left a comment

Choose a reason for hiding this comment

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

This LGTM. One of the things on my (previous life) cleanup list was to redo the settings stuff -- you can see the work it currently takes to add even a simple setting. It should be a new setting object that does everything automagically, someday.

@@ -563,6 +563,16 @@
}
],
"default": []
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christophe-lunarg , I think I pulled in your fix. Not sure if I got it right though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great!

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5828 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5828 passed.

Copy link
Contributor

@ncesario-lunarg ncesario-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM!

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 16739.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5952 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5952 passed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 18081.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5961 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5961 passed.

Add a setting to make calls to ValidationObject::ReadLock() and
WriteLock() be no-ops, because locking in other parts of the code
has made these global locks unnecessary.

Currently this only affects Core Validation, but eventually it should
also change the behavior of Best Practices and Synchronization
Validation.

The setting defaults to off for now and there is logging when fine grained
locking is enabled, since this is an experimental feature which
make cause stability problems or incorrect errors to be reported.
Once this stabilizes, the setting will default to on and the log message
will report when it is turned off, because that will be slow.
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 19506.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5980 running.

@jeremyg-lunarg jeremyg-lunarg merged commit 50edce6 into KhronosGroup:master Jan 19, 2022
@jeremyg-lunarg jeremyg-lunarg deleted the jeremyg-lock-setting branch January 19, 2022 18:05
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.

6 participants