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

layers: Only support shared_ptr Get<>() methods #3575

Merged

Conversation

jeremyg-lunarg
Copy link
Contributor

@jeremyg-lunarg jeremyg-lunarg commented Nov 29, 2021

In order to make it feasible to use reduced locking in the validation
layers, it will be necessary for the validation code to hold a
reference count on any state objects that are being used.

Copying shared_ptr's involves atomic operations and is more expensive
than copying raw pointers, so it should be avoided if possible.

Some guidelines:

-Avoid repeated Get<> calls whenever possible. These are a hash lookup,
shared_ptr<> copy, and (eventually) map locking operations.

-Functions that use a state object but don't change ownership should
pass by reference (preferred) or raw pointer.
void func(Foo &); or void func(Foo *);

-Functions that share ownership (caller keeps reference and callee takes
an addiional reference) should pass a shared_ptr lvalue reference
void func(shared_ptr<Foo>&). This avoids a useless shared pointer
copy for the function argument.

-Functions that transfer ownership (caller gives up reference and callee
takes it) should pass a shared_ptr rvalue reference
void func2(shared_ptr<Foo> &&); This allows std::move() to be used
to avoid shared_ptr copies.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 7421.

@jeremyg-lunarg jeremyg-lunarg marked this pull request as draft November 29, 2021 21:55
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5608 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5608 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 7893.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5622 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5622 passed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 8135.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5629 running.

@jeremyg-lunarg jeremyg-lunarg marked this pull request as ready for review November 30, 2021 22:13
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5629 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! One general question/comment about potentially doing a "reference refactor" first, but that's mainly me trying to sneak in some additional cleanup "for free" ;)

@@ -3584,18 +3589,18 @@ bool CoreChecks::ValidateCmdResolveImage(VkCommandBuffer commandBuffer, VkImage
bool skip = false;
if (cb_node && src_image_state && dst_image_state) {
vuid = is_2khr ? "VUID-VkResolveImageInfo2KHR-srcImage-00256" : "VUID-vkCmdResolveImage-srcImage-00256";
skip |= ValidateMemoryIsBoundToImage(src_image_state, func_name, vuid);
skip |= ValidateMemoryIsBoundToImage(src_image_state.get(), func_name, vuid);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to first refactor functions like this to take a reference? Seems like that might make this change a lot smaller, and the use of operator* could avoid future churn between smart-pointer <-> pointer refactors in the future.

That could also be done as a post-refactor, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I tried to start doing that and quickly realized it would make this patch 2x - 3x bigger so I stopped. for now...

In order to make it feasible to use reduced locking in the validation
layers, it will be necessary for the validation code to hold a
reference count on any state objects that are being used.

Copying shared_ptr's involves atomic operations and is more expensive
than copying raw pointers, so it should be avoided if possible.

Some guidelines:

-Avoid repeated Get<> calls whenever possible. These are a hash lookup,
shared_ptr<> copy, and (eventually) map locking operations.

-Functions that use a state object but don't change ownership should
pass by reference (preferred) or raw pointer.
'void func(Foo &);' or 'void func(Foo *);'

-Functions that share ownership (caller keeps reference and callee takes
an addiional reference) should pass a shared_ptr lvalue reference
`void func(shared_ptr<Foo>&).` This avoids a useless shared pointer
copy for the function argument.

-Functions that transfer ownership (caller gives up reference and callee
takes it) should pass a shared_ptr rvalue reference
`void func2(shared_ptr<Foo> &&);` This allows std::move() to be used
to avoid shared_ptr copies.
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 12874.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5679 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 5679 passed.

Copy link
Contributor

@TonyBarbour TonyBarbour left a comment

Choose a reason for hiding this comment

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

LGTM

@jeremyg-lunarg jeremyg-lunarg merged commit 9f53710 into KhronosGroup:master Dec 10, 2021
@jeremyg-lunarg jeremyg-lunarg deleted the jeremyg-get-shared branch December 10, 2021 18:19
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.

None yet

4 participants