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

Refactor tasks architecture #146

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kmichaelk
Copy link
Contributor

@kmichaelk kmichaelk commented Nov 9, 2024

Type safety. Using std::span in TaskData instead of raw pointers with separate supplement of their memory blocks sizes prevents heap overflow errors and simplifies the code overall. No more reinterpret_casts.

Less boilerplate. The current way of initializing TaskData results in a huge amount of boilerplate code that very few people actually understand due to their poor understanding of pointer arithmetic - the need to copy it from test to test increases the likelihood of introducing a hard-to-debug bug unrelated to the task.

No shared_ptr usage. If you look closely, it is impossible to imagine a situation where using shared_ptr is justified now - they are used even where they are definitely not needed - for example, in perf tests when creating PerfAnalyzer. If you can give arguments in favor of using shared_ptr, or an example of a situation that cannot be normally implemented without using it, let's discuss it.

@kmichaelk
Copy link
Contributor Author

While the migration is not yet complete, the tests that have already been migrated pass successfully.

The non-template part was separated from Task to speed up compilation.

Comment on lines +13 to 26
template <typename InType, typename OutType>
struct GenericTaskData {
const std::span<InType> input;
std::span<OutType> output;

GenericTaskData(const std::span<InType>& input_, std::span<OutType> output_)
: input(input_), output(std::move(output_)) {}
GenericTaskData(const InType& input_, OutType& output_)
: input(std::addressof(input_), 1), output(std::addressof(output_), 1) {}
GenericTaskData(const std::span<InType>& input_, OutType& output_)
: input(input_), output(std::addressof(output_), 1) {}
GenericTaskData(const InType& input_, std::span<OutType> output_)
: input(std::addressof(input_), 1), output(std::move(output_)) {}
};
Copy link
Member

Choose a reason for hiding this comment

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

please write code snippet, how yours GenericTaskData describe input - vector(uint8), vector(float), and output vector(uint8) and 1 element float?

Copy link
Contributor Author

@kmichaelk kmichaelk Nov 15, 2024

Choose a reason for hiding this comment

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

I'd say it depends on the task: it's possible to use std::pair<std::vector<uint8_t>, std::vector<float>> or std::tuple. If it's a more complex structure, I'd create a tiny struct descriptor for it (e.g. for matrix).

I.e. GenericTaskData<std::pair<std::vector<uint8_t>, std::vector<float>>, OutType> (or GenericTaskData<std::pair<Matrix, Matrix>, Matrix> for my 2nd task), but that's being specified in the class derived from Task actually.

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.

2 participants