Timeline semaphore support #75

MoritzRoth · 2023-03-30T18:06:27Z

Adds basic timeline semaphore support. Related issue: cg-tuwien/Auto-Vk-Toolkit#45

Changes:
Timeline semaphores functionality has been added to the existing class avk::semaphore_t, is well documented and nicely abstracted.

Note: I added comments to some parts I would like feedback on.

johannesugb

Good job so far, but please mind the comments and update!

include/avk/avk.hpp

johannesugb · 2023-04-03T18:31:27Z

include/avk/commands.hpp

+	inline semaphore_wait_info&& operator| (uint64_t aValue, semaphore_wait_info&& aInfo)
+	{
+		aInfo.mValue = aValue;
+		return std::move(aInfo);
+	}
+
+	inline semaphore_wait_info& operator| (uint64_t aValue, semaphore_wait_info& aInfo)
+	{
+		aInfo.mValue = aValue;
+		return aInfo;
+	}


Hmm, I'm not so sure if we should really abuse operator| for that (because generally, Auto-Vk's API already abuses operators quite a lot).

Should we maybe rather, instead of aSrcSignalStage >> sem | aSignalValue aim for something like aSrcSignalStage >> sem.at_value(aSignalValue)?
What do you think?

Fine by me. This seems like it would definitely be more understandable to read.

Since the semaphore type to construct the semaphore_wait_info is avk::resource_argument<avk::semaphore_t> I was hesitant to add an explicit template specialization for avk::semaphore_t to avk::resource_argument, adding the at_value / to_value functions.

A return type for such a function would also need to be either a std::pair, or some additional incomplete_semaphore_wait_info struct type, which both seem weird to me.

My current solution is to just add the at_value / to_value functions to the wait / signal info structs. Constructing the info structs would thus look like this (aSrcSignalStage >> sem).at_value(aSignalValue). If I'm missing something obvious or you can think of a better solution please let me know.

johannesugb · 2023-04-03T18:33:17Z

include/avk/commands.hpp

+	inline semaphore_signal_info&& operator| (semaphore_signal_info&& aInfo, uint64_t aValue)
+	{
+		aInfo.mValue = aValue;
+		return std::move(aInfo);
+	}
+
+	inline semaphore_signal_info& operator| (semaphore_signal_info& aInfo, uint64_t aValue)
+	{
+		aInfo.mValue = aValue;
+		return aInfo;


Same here -- maybe better go for sem.to_value(aValue)?

johannesugb · 2023-04-03T18:33:59Z

include/avk/semaphore.hpp

+		// returns the current value of the timeline semaphore
+		const uint64_t query_current_value() const;
+		// sets the timeline semaphore to the specified value


Please consistently use /** ... */ comments for all the public methods!

Please also use new lines to separate two methods.

include/avk/semaphore.hpp

src/avk.cpp

johannesugb

Let's rework this once again. See also the changes that I'm going to push! And please don't see them as a final version, but improve them until everything looks and feels great!

johannesugb · 2023-08-01T08:40:58Z

include/avk/commands.hpp

+		semaphore_wait_info& at_value(uint64_t aValue) {
+			mValue = aValue;
+			return *this;
+		}


Hmm, I don't like this approach too much, because it leads to the impression that stage >> semaphore is a unit, whereas semaphore + value should be a unit.

My most recent commit addresses this issue and semaphore_wait_info / semaphore_signal_info can now be created via

semaphore_signal_info foo = pipelineStageFlags >> (signaledSemaphore, newValue); semaphore_wait_info bar = (waitSemaphore, waitValue) >> pipelineStageFlags;

However, to get this to work I had to overload operator,(avk::resource_argument<avk::semaphore_t>, uint64_t). While I currently don't see any unwanted side effects this might have, it doesn't strike me as clean coding practice.

Uff, operator, sounds very adventurous. And what's more, it even requires the parens around it, right? So it actually isn't really of any benefit over constructing semaphore_value_info directly via semaphore_value_info{signaledSemaphore, newValue} or semaphore_value_info{waitSemaphore, waitValue}.

One more thing comes to mind which might be cooler: Overloading opertator= and operator==. What do you think?

semaphore_signal_info foo = pipelineStageFlags >> signaledSemaphore = newValue;

semaphore_wait_info bar = waitSemaphore == waitValue >> pipelineStageFlags;

Is that expressive and well understandable and would that compile without parens?

When you're investigating operator= and operator==, please make sure that you handle both cases:

pipelineStageFlags >> signaledSemaphore = newValue and pipelineStageFlags >> (signaledSemaphore = newValue)

waitSemaphore == waitValue >> pipelineStageFlags and (waitSemaphore == waitValue) >> pipelineStageFlags

You might have to create different overloads for operator= and operator== to handle both cases. But this is fine.

Finally got to it and implemented the suggested change.

semaphore_signal_info foo = pipelineStageFlags >> signaledSemaphore = newValue;

semaphore_wait_info bar = waitSemaphore >= waitValue >> pipelineStageFlags;

semaphore_value_info baz = semaphore = value;

semaphore_value_info quz = semaphore >= value;

Are all valid expressions now. Note that I used operator>= instead of operator== since this is the technically more correct way to specify it.

I again had to make some less desireable changes to get this to compile. I will highlight those in seperate threads so we can discuss potential workarounds (1, 2, 3)

johannesugb · 2023-08-01T08:42:13Z

include/avk/commands.hpp

+		semaphore_signal_info& to_value(uint64_t aValue) {
+			mValue = aValue;
+			return *this;
+		}


...same here, but I have to agree that the at_value/to_value is not easy to get into class semaphore_t.

So, I guess we should rather go back to operator| or use operator>> once again (as in the source code that I'm about to push).

As mentioned above I now use operator, to create semaphore_value_info in a concise way, which can then be used together with operator>> to create semaphore_wait_info and semaphore_signal_info.

johannesugb · 2023-08-01T08:42:58Z

include/avk/semaphore.hpp

+		/** @brief Waits on host until the condition specified with the parameters is met.
+		*   @param aSemaphores Vector of timeline semaphores that should be waited on. All semaphores are required to be owned by the same logical device.
+		*   @param aTimestamps Vector of payload values to wait on. Is required to have the same size as aSemaphores. The n-th value in aTimestamps corresponds to the n-th entry in aSemaphores.
+		*   @param aWaitOnAll (optional) If true, waits until ALL semaphores have reached their target timestamps. If false, waits until ANY semaphore has reached its target timestamp.
+		*   @param aTimeout (optional) Defines a timeout (in nanoseconds) after which the function returns regardless of the semaphore state.
+		*   @return Value of type vk::Result containing information about whether the wait operation succeeded, or the timeout has been triggered.
+		*/
+		static vk::Result wait_until_signaled(const std::span<std::reference_wrapper<const semaphore_t>> aSemaphores, const std::span<uint64_t> &aTimestamps, bool aWaitOnAll = true, std::optional<uint64_t> aTimeout = {});


This is somewhat out of place here. Better turn it into a free function and move it somewhere into class root!

Usage of std::span is wrong here. std::span should never be passed as const&.

johannesugb · 2023-08-01T08:43:42Z

include/avk/semaphore.hpp

+		/** @brief Waits on host until the condition specified with the parameters is met.
+		*   @param aDevice The logical device owning all referenced timeline semaphores.
+		*   @param aInfo Struct containing all relevant information about the wait operation.
+		*   @param aTimeout (optional) Defines a timeout (in nanoseconds) after which the function returns regardless of the semaphore state.
+		*   @return Value of type vk::Result containing information about whether the wait operation succeeded, or the timeout has been triggered.
+		*/
+		static vk::Result wait_until_signaled(const vk::Device &aDevice, const vk::SemaphoreWaitInfo &aInfo, std::optional<uint64_t> aTimeout = {});


...same applies here. Additionally, I just don't really know if there's value in having such a raw-Vulkan-version overload of this.

johannesugb · 2023-08-01T08:47:18Z

src/avk.cpp

+
+	vk::Result semaphore_t::wait_until_signaled(uint64_t aRequiredValue, std::optional<uint64_t> aTimeout) const {
+		std::vector<std::reference_wrapper<const semaphore_t>> semaphores{ *this };
+		return wait_until_signaled(semaphores, std::span<uint64_t>(&aRequiredValue, 1), true, aTimeout);


Construction of std::span looks veeeery odd here. It actually defies the purpose of a std::span. I.e, I would say, the point is to never explicitly create a std::span, but use it to refer to just elements in contiguous memory (wherever they might be stored, e.g., std::array or std::vector, ...)

johannesugb · 2023-08-01T08:48:28Z

src/avk.cpp

+		return wait_until_signaled(semaphores, std::span<uint64_t>(&aRequiredValue, 1), true, aTimeout);
+	}
+
+	vk::Result semaphore_t::wait_until_signaled(const std::span<std::reference_wrapper<const semaphore_t>> aSemaphores, const std::span<uint64_t>& aTimestamps, bool aWaitOnAll, std::optional<uint64_t> aTimeout) {


Having to pass two collections which must be aligned is very inconvenient and error-prone. This should be turned into a std::tuple or a small helper struct instead -- then pass only one collection of that!

johannesugb

Please mind the comments and once again, rebase on development! That should fix the workflows which are failing.

johannesugb · 2023-10-10T10:06:24Z

include/avk/commands.hpp

+		avk::resource_argument<avk::semaphore_t> mSemaphore;
+		uint64_t mValue;
+	};
+	inline semaphore_value_info operator,(avk::resource_argument<avk::semaphore_t> aSemaphore, uint64_t aValue) {


Please add exactly one newline between each function/method!
Also make sure to add comments!

johannesugb · 2023-10-10T10:07:24Z

src/avk.cpp

+		vk::SemaphoreWaitInfo info{
+	        vk::SemaphoreWaitFlags{},
+	        1u,
+	        handle_addr(),
+	        &aSignalValue
+		};
+		mSemaphore.getOwner().waitSemaphores(info, aTimeout.value_or(UINT64_MAX));


Bad code formatting.

MoritzRoth · 2023-12-09T17:33:33Z

include/avk/semaphore.hpp

+	/**
+	 * @brief This is an explicit template specialization of owning_resource as defined in cpp_utils.hpp
+	 * It is a complete duplicate of the class that contains an additional overload for operator= (defined at the bottom).
+	 *
+	 * The definition of an explicit template specialization is required since operator= overloads cannot be defined as non-member functions.
+	 * (see https://en.cppreference.com/w/cpp/language/operators)
+	 * As far as I can tell, defining an explicit template specialization for a class requires duplicating the entire class even if only minor changes are made.
+	 * 
+	 *
+	 * The overload allows instantiating semaphore_value_info structs with the expression `sem = val`.
+	 * Where the type of sem is `owning_resource<semaphore_t>` and the type of val is `uint64_t`.
+	 */
+	template<>
+	class owning_resource<semaphore_t> : public std::variant<std::monostate, semaphore_t, std::shared_ptr<semaphore_t>>


Here I am defining an explicit template specialization for owning_resource<semaphore_t> (and resource_argument<semaphore_t> below) in order to add an overload for the = operator. I can't simply use non-member operator overloads (like for >=) since operator= explicitly cannot be declared as non-member function.

This results in a LOT of duplicated code, since the remaining functions need to be redefined as well. As far as I am aware it isn't possible to define new member functions ( / operator overloads) for a specific template configuration without redefining the entire class, though I still hope I've missed something in the documentation.

MoritzRoth · 2023-12-09T17:46:23Z

include/avk/commands.hpp

+	/**
+	 *  Allows `waitSemaphore >= waitValue >> pipelineStageFlags` without parentheses around `>=`
+	 *  Requires `operator>>(uint64_t, avk::stage::pipeline_stage_flags) -> semaphore_wait_info` to work
+	 *
+	 *  Unwanted side effect: `waitValue >> pipelineStageFlags` compiles but produces an invalid semaphore_wait_info
+	 */
+	inline auto operator>=(avk::owning_resource<avk::semaphore_t> aSemaphore, semaphore_wait_info aSemWaitInfo)->semaphore_wait_info {
+		aSemWaitInfo.mWaitSemaphore = std::move(aSemaphore);
+		return aSemWaitInfo;
+	}
+	} // namespace avk
+
+	/**
+	 *  Allows `waitSemaphore >= waitValue >> pipelineStageFlags` without parentheses around `>=`
+	 *  Requires `operator>=(avk::resource_argument<avk::semaphore_t>, semaphore_wait_info) -> semaphore_wait_info` to work
+	 * 
+	 *  Unwanted side effect: `waitValue >> pipelineStageFlags` compiles but produces an invalid semaphore_wait_info
+	 *  @note This operator overload is defined in global scope because it weirdly wasn't found by auto_vk_toolkit applications otherwise.
+	 */
+	inline auto operator>>(uint64_t aValue, avk::stage::pipeline_stage_flags aStageFlags) -> avk::semaphore_wait_info {
+		return avk::semaphore_wait_info{ avk::owning_resource<avk::semaphore_t>(), aStageFlags, aValue};
+	}


For the expression below, the fixed operator precedence of C++ leads to an undesireable evaluation order:
sem >= val >> flags
sem >= (val >> flags)
Because of this I had to define operator>>(uint64_t, avk::stage::pipeline_stage_flags) which results in an incomplete semaphore_wait_info that has to be completed with operator>=.

MoritzRoth · 2023-12-09T18:30:29Z

include/avk/commands.hpp

+	} // namespace avk
+
+	/**
+	 *  Allows `waitSemaphore >= waitValue >> pipelineStageFlags` without parentheses around `>=`
+	 *  Requires `operator>=(avk::resource_argument<avk::semaphore_t>, semaphore_wait_info) -> semaphore_wait_info` to work
+	 * 
+	 *  Unwanted side effect: `waitValue >> pipelineStageFlags` compiles but produces an invalid semaphore_wait_info
+	 *  @note This operator overload is defined in global scope because it weirdly wasn't found by auto_vk_toolkit applications otherwise.
+	 */
+	inline auto operator>>(uint64_t aValue, avk::stage::pipeline_stage_flags aStageFlags) -> avk::semaphore_wait_info {
+		return avk::semaphore_wait_info{ avk::owning_resource<avk::semaphore_t>(), aStageFlags, aValue};
+	}

+	namespace avk {


When trying to use this operator overload from another solution (i.e. model_loader) the compiler ran into issues finding the function. ~~This strangely does not happen when I define the function in global scope.~~ Since argument-dependent lookup cannot consider the return type during name lookup and the function definition isn't inside the namespaces of uint64_t or avk::stage::pipeline_stage_flags the overload can't be found. A cleaner fix probably just defines the overload in namespace avk::stage.

…il_signalled

add function create_timeline_semaphore allow specifying wait/signal values for device side sync

return vk::Result in semaphore_t::wait_until_signalled

change semaphore_wait/signal_info construction for timeline semaphores

…their current form. Consider moving the static functions out of semaphore_t and into class root!

And besides, NULL is C-esque. Use nullptr instead!

…>> value Not sure if it is prettier than operator|, but better than ().to_value(value)! Don't hesitate to introduce new small helper structs!

remove wait_unitl_signal overload that requires manual construction of vk::SemaphoreWaitInfo

achieved by adding operator,() overload allow creation of semaphore_signal_info via semaphore_value_info new semaphore_signal_info / semaphore_wait_info syntax: pipelineStageFlags >> (signalSemaphore, newValue) (waitSemaphore, waitValue) >> pipelineStageFlags

in some places tabs were mixed with 4 spaces for indentation note: only fixed cases related to timeline semaphore code to avoid merge conflicts

…esources which are no longer needed convert semaphore_t::mLifetimeHandledResources to a forward_list of owning resources together with a timestamp describing when they can be discarded choose forward_list due to constant insert at front and linear remove with predicate

MoritzRoth self-assigned this Mar 30, 2023

johannesugb requested changes Apr 3, 2023

View reviewed changes

MoritzRoth force-pushed the timeline-semaphores branch from 61fa537 to 9c0f0b5 Compare July 5, 2023 18:27

MoritzRoth changed the base branch from master to development July 5, 2023 18:56

MoritzRoth requested a review from johannesugb July 25, 2023 15:56

johannesugb requested changes Aug 1, 2023

View reviewed changes

MoritzRoth requested a review from johannesugb October 8, 2023 17:06

johannesugb requested changes Oct 10, 2023

View reviewed changes

MoritzRoth force-pushed the timeline-semaphores branch from 2d1439f to 1e605e6 Compare October 14, 2023 09:39

MoritzRoth commented Dec 9, 2023

View reviewed changes

MoritzRoth requested a review from johannesugb December 9, 2023 18:42

MoritzRoth and others added 14 commits February 27, 2024 13:29

add timline semaphore host side function stubs and implement wait_unt…

a598186

…il_signalled

implement remaining host side functions

f7924e7

add function create_timeline_semaphore allow specifying wait/signal values for device side sync

fix issue in semaphore_t::wait_until_signal

ec44689

add documentation

1fdce85

return vk::Result in semaphore_t::wait_until_signalled

fix typo, adapt documentation for consistency

e42259e

fix bugged semaphore_wait_until_signaled impl

f574c8e

change semaphore_wait/signal_info construction for timeline semaphores

add missing include

1ccece9

Didn't really like the wait_until_signaled methods of semaphore_t in …

e7c0aee

…their current form. Consider moving the static functions out of semaphore_t and into class root!

Use Vulkan-Hpp powers! No need to set sType and pNext!

cde0731

And besides, NULL is C-esque. Use nullptr instead!

Instead of (stage >> sem).to_value(value) this enables: stage >> sem …

a985b35

…>> value Not sure if it is prettier than operator|, but better than ().to_value(value)! Don't hesitate to introduce new small helper structs!

move wait_on_signal overload for multiple semaphores to class root

3e9b739

remove wait_unitl_signal overload that requires manual construction of vk::SemaphoreWaitInfo

fix inconsistent indent issues in semaphore code

7491e52

in some places tabs were mixed with 4 spaces for indentation note: only fixed cases related to timeline semaphore code to avoid merge conflicts

WIP replace operator, and at_value function with operators = and >=

b713674

MoritzRoth force-pushed the timeline-semaphores branch from f9c647c to b713674 Compare February 27, 2024 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeline semaphore support #75

Timeline semaphore support #75

MoritzRoth commented Mar 30, 2023

johannesugb left a comment

johannesugb Apr 3, 2023

MoritzRoth Apr 26, 2023

MoritzRoth Jul 22, 2023

johannesugb Apr 3, 2023

johannesugb Apr 3, 2023

johannesugb Apr 3, 2023

MoritzRoth Jul 22, 2023

johannesugb left a comment

johannesugb Aug 1, 2023

MoritzRoth Oct 8, 2023

johannesugb Oct 9, 2023 •

edited

johannesugb Oct 10, 2023

MoritzRoth Dec 9, 2023 •

edited

johannesugb Aug 1, 2023

MoritzRoth Oct 8, 2023

johannesugb Aug 1, 2023

johannesugb Aug 1, 2023

johannesugb Aug 1, 2023

johannesugb Aug 1, 2023

johannesugb Aug 1, 2023

johannesugb left a comment

johannesugb Oct 10, 2023

johannesugb Oct 10, 2023

MoritzRoth Dec 9, 2023 •

edited

MoritzRoth Dec 9, 2023

MoritzRoth Dec 9, 2023 •

edited

Timeline semaphore support #75

Are you sure you want to change the base?

Timeline semaphore support #75

Conversation

MoritzRoth commented Mar 30, 2023

johannesugb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannesugb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannesugb Oct 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoritzRoth Dec 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannesugb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoritzRoth Dec 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoritzRoth Dec 9, 2023 • edited

Choose a reason for hiding this comment

johannesugb Oct 9, 2023 •

edited

MoritzRoth Dec 9, 2023 •

edited

MoritzRoth Dec 9, 2023 •

edited

MoritzRoth Dec 9, 2023 •

edited