Implement Send/Sync for all vk structs

[Neo-Zhixing]

Oftentimes we want to populate vk::Create*Info and then send this struct to a worker thread for processing. This is extremely common for pipeline objects which generally takes longer than most other commands, necessitating async processing.

However, these vk::Create*Info structs oftentimes contain at least a p_next pointer which makes the struct non-Send and non-Sync. This makes sending those vk::Create*Info structs unnecessarily complicated.

As such, I propose that we implement "Send" and "Sync" by default for all the structs. Vulkan commands are unsafe anyways, so implementing "Send" and "Sync" shouldn't introduce additional unsafety.

[Ralith]

I think this is a justified change, though I haven't had time to review the specifics yet. As you say, every consumer is unsafe anyway, and ash doesn't generally consider it unsound for a pointer field to ever have an arbitrarily bad value in it. One note, though:

Oftentimes we want to populate vk::Create*Info and then send this struct to a worker thread for processing.

This pattern has limited use, since it's incompatible with any use of extensions on those structs, or even any other indirection like the many assorted arrays you'll typically pass when creating a pipeline. A more future-proof scheme is to always use your own message types in this case, even if they initially closely mirror Vulkan structs. Still, that's no reason not to make the change proposed here.

[Neo-Zhixing]

This pattern has limited use, since it's incompatible with any use of extensions on those structs

In my case this isn't exposed as part of the public API. It's simply the case that I found it more convenient to build the struct on the main thread and send the whole thing to the worker thread. Without "Send" and "Sync" I cannot do that even with the extension structs boxed.

[Neo-Zhixing]

Merged master but still seeing one CI issue:

error: package `half v2.4.0` cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.69.0
Either upgrade to rustc 1.70 or newer, or use
cargo update -p half@2.4.0 --precise ver
where `ver` is the latest version of `half` supporting rustc 1.69.0

Rust version in CI probably needs to be updated?

[Neo-Zhixing]

@Ralith The CI is now green

[Ralith]

Yep, thanks! LGTM, just leaving @MarijnS95 an opportunity to weigh in and/or merge.

[MarijnS95]

This PR does not seem to have had a compelling discussion to support this change besides @Neo-Zhixing needing this to make some shortcuts in their threading message passing implementation?

Users have already had lots of trouble with lifetime safety on their references, and I fear this is only going to make things worse when struct lifetimes start to protrude a stack - not to mention thread - boundary.
The only thing going for it is the "recent" addition of lifetimes on ash structs, effectively requiring all structs that pass a thread boundary to have the 'static lifetime to allow passage to another thread (barring the use of scoped threads which is not normally how worker-thread-processing is implemented unless going wide and blocking your application on startup to create pipelines). That hopefully voids this argument completely?

It sounds like @Neo-Zhixing might then also have to work around the new struct lifetimes introduced in the next version of ash to use "extension structs boxed" as proposed in #869 (comment), further diminishing the advantage of Send/Sync.

---

Secondly I do wonder if there are any types in Ash that are intentionally not Send/Sync, that are now shoved into a Send/Sync vessel and defeating that concept? (non) dispatchable handles are Send+Sync at least.

---

That is not to say that I haven't personally thought about making a similar change. Our own application has a SendSync wrapper around vk::AccelerationStructureGeometryKHR specifically for the pNext pointer (all other fields are GPU pointers that should be Send/Sync from a CPU perspective). The pNext pointer is null() and the whole struct hence of 'static lifetime.
Not to say that it wouldn't actually be easier for us to just rebuild struct contents from the information stored right next to that struct in the few instances that we need it again, without thread/lifetime/pointer woes.

[Friz64]

Won't has_lifetime always be true at this point? We know this struct has pointers, so why bother checking that flag?

[Neo-Zhixing]

Good point. That was non-obvious to me. Updated.

[Friz64]

That hopefully voids this argument completely?

Not completely I think, as some users may, for whatever reason (maybe they don't know better), directly manipulate the pointers through field access, therefore bypassing the struct lifetime. But then they'd be shooting themselves in the foot in much more ways than just passing a pointer over a thread boundary that they shouldn't have. I digress, as that's a more abstract discussion to be had, less related to this PR.

Anyways, either way, I don't see much reason not to do this.

[Neo-Zhixing]

@MarijnS95 Thanks for your response!

This PR does not seem to have had a compelling discussion to support this change

Yeah so I suppose this PR would be the perfect place for discussions!

Users have already had lots of trouble with lifetime safety on their references, and I fear this is only going to make things worse when struct lifetimes start to protrude a stack - not to mention thread - boundary.

That is true, but safeguarding users and making sure that their API calls are thread-safe is explicitly a non-goal for ash.
Ash is supposed to be "A true Vulkan API without compromises" and "Convenience features without limiting functionality".

effectively requiring all structs that pass a thread boundary to have the 'static lifetime to allow passage to another thread

Like @Friz64 said, the users are always allowed to manipulate raw pointers directly. The difference between this and the recent lifetime changes is that one is a quality-of-life improvement, while the other is limiting what the user can do. And ash users are generally expected to be able to "fk around and find out".

Secondly I do wonder if there are any types in Ash that are intentionally not Send/Sync, that are now shoved into a Send/Sync vessel and defeating that concept? (non) dispatchable handles are Send+Sync at least.

I don't think that's the case. In Vulkan, everything is assumed to be thread-safe unless the spec says "Host access to ** must be externally synchronized". But once again, ash users are expected to respect Vulkan specification, and it's not our job to check that they do.

Our own application has a SendSync wrapper around vk::AccelerationStructureGeometryKHR specifically for the pNext pointer (all other fields are GPU pointers that should be Send/Sync from a CPU perspective). The pNext pointer is null() and the whole struct hence of 'static lifetime.

Having to wrap everything in SendSync wrapper is a lot of ceremony for not a lot of safety. Things like thread safety should probably be left for higher level wrappers and engines.

Not to say that it wouldn't actually be easier for us to just rebuild struct contents from the information stored right next to that struct in the few instances that we need it again, without thread/lifetime/pointer woes.

In a lot of cases this might be true, but in other cases it's easier to build the struct first and then send them across thread boundary. I do think that there's value for both cases to be supported.

[MarijnS95]

That hopefully voids this argument completely?

Not completely I think, as some users may, for whatever reason (maybe they don't know better), directly manipulate the pointers through field access, therefore bypassing the struct lifetime.

@Friz64 Yeah, that is covered in the paragraph after the one you quoted. It's good to have discussed and know that this feature will only support 'static (and otherwise thread::scope borrowed) structs crossing a thread boundary.

---

Yeah so I suppose this PR would be the perfect place for discussions!

Hence all these points that were brought up 😉

Users have already had lots of trouble with lifetime safety on their references, and I fear this is only going to make things worse when struct lifetimes start to protrude a stack - not to mention thread - boundary.

That is true, but safeguarding users and making sure that their API calls are thread-safe is explicitly a non-goal for ash. Ash is supposed to be "A true Vulkan API without compromises" and "Convenience features without limiting functionality".

That doesn't mean to say that it is a goal of ash to merge any and all user contributions for convenience without first considering their surface area and impact. Raw pointer structs are and will always be a footgun and, while there are plans to improve that one way or another, anything that further exposes their impact should be weighed carefully.
As such I'm still of the opinion that you're better off defining a higher-level message structure to convey your Vulkan pipeline messages, without sending raw pointer structs.

As a sanity-check regarding "extension structs boxed", how much extra effort is your code putting on finding non-null pointers and Box::into_raw()'ing them again to clean up?

For the specific example of pipeline creation, we're doing that off-thread as well but without any "sending Vulkan pointer structs across a thread boundary" issue. I can look up how we do it, but I don't remember it being particularly clever or involved: not many (pointer) arguments are needed to be serialized in the first place.

Alas, let's continue discussing the change at hand rather me rambling about how I don't see it being particularly useful to solve the proposed case.

---

effectively requiring all structs that pass a thread boundary to have the 'static lifetime to allow passage to another thread

Like @Friz64 said, the users are always allowed to manipulate raw pointers directly. The difference between this and the recent lifetime changes is that one is a quality-of-life improvement, while the other is limiting what the user can do. And ash users are generally expected to be able to "fk around and find out".

Yeah, @Friz64 confirmed what I said in the next paragraph about the lifetimes no longer serving their purpose (intentionally, to support your case) when manually storing pointers to pinned boxed structs.

Secondly I do wonder if there are any types in Ash that are intentionally not Send/Sync, that are now shoved into a Send/Sync vessel and defeating that concept? (non) dispatchable handles are Send+Sync at least.

I don't think that's the case. In Vulkan, everything is assumed to be thread-safe unless the spec says "Host access to ** must be externally synchronized". But once again, ash users are expected to respect Vulkan specification, and it's not our job to check that they do.

I agree, but it is again desired to first discuss and agree that this is the case. As said all handles are Send+Sync because synchronization is defined at the call level. Please see https://registry.khronos.org/vulkan/specs/1.3-khr-extensions/html/chap3.html#fundamentals-threadingbehavior.

Our own application has a SendSync wrapper around vk::AccelerationStructureGeometryKHR specifically for the pNext pointer (all other fields are GPU pointers that should be Send/Sync from a CPU perspective). The pNext pointer is null() and the whole struct hence of 'static lifetime.

Having to wrap everything in SendSync wrapper is a lot of ceremony for not a lot of safety. Things like thread safety should probably be left for higher level wrappers and engines.

This was a point in favour of your PR 😉. We'd be able to drop that SendSync wrapper while still taking advantage of 'static lifetiming when this PR is accepted and merged.

(A SendSync wrapper is needed because consumer-crates are not allowed to impl external/core/std traits on types from external crates, like ash)

[MarijnS95]

All in all, since the other maintainers seem to be in favour of this change, and it doesn't further expose any kind of safety issues, let's clean this up and merge this, and focus on the bigger breaking tasks at hand.

[Neo-Zhixing]

As such I'm still of the opinion that you're better off defining a higher-level message structure to convey your Vulkan pipeline messages, without sending raw pointer structs.

That is true, but it's a decision better left to the developer of higher-level abstractions.

There's one additional point I forgot to mention - DeferredHostOperation. It's probably not uncommon for people to "going wide and blocking" when using API calls that take DeferredHostOperation. And you're supposed to keep those command parameter structs alive until the DeferredHostOperation completes, because the implementation may access those structs at any time and presumably from any thread. I think this sorta implies that the info structs are implicitly Send/Sync in Vulkan anyways?

But yeah, overall it sounds like we can all agree that this change is justified. I do appreciate that we're taking our time to carefully weigh it against alternatives and making sure that the discussions are fully documented. I've rebased the PR with master and someone can click the merge button whenever.

[Ralith]

I've rebased the PR with master

FYI you merged in master, which is different from a rebase, though that doesn't matter here since we'll squash it down into one commit anyway.