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

Task Template refactor #58

Conversation

ikbuibui
Copy link
Collaborator

Refactor RedGrapes away from macro based configuration, to using a templated task type
Change the init method
Move away from a lot of global variables
Fix the MPI example. MPI calls are now executed from the same thread
We can now have multiple thread pools/single threads with different kinds of workers
Bump to C++ 20 and start using concepts
A lot more

CMakeLists.txt Outdated Show resolved Hide resolved
examples/1_resources.cpp Outdated Show resolved Hide resolved
examples/4_refinements.cpp Outdated Show resolved Hide resolved
examples/CMakeLists.txt Outdated Show resolved Hide resolved
examples/cholesky.cpp Outdated Show resolved Hide resolved
redGrapes/sync/cv.hpp Outdated Show resolved Hide resolved
redGrapes/task/future.hpp Outdated Show resolved Hide resolved
redGrapes/task/property/inherit.hpp Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/random_graph.cpp Outdated Show resolved Hide resolved
Copy link
Member

@michaelsippel michaelsippel left a comment

Choose a reason for hiding this comment

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

Just some thoughts pointing out some potential issues but despite them i am not against these changes.
In general I get the reasoning that having a template-based configuration is much better than preprocessor and additionally using a scoped redGrapes object to get rid of finalize() is a good thing.
However: this object needs to be passed around, requiring an additional reference which needs to be captured in every task-functor, which imho creates two potential problems:

  1. Using [&] in all lambdas should be considered bad practice when writing redGrapes applications since this makes is very easy to introduce race conditions or lifetime issues. It is much safer to write out all capture-by-references explicitly. Requiring to capture the redGrapes-context in each task therefore degrades usability.
  2. It increases the size of each task by another 8-bytes to store the reference. Having a singleton is more efficient from this perspective, as we have seen that the memory-footprint of tasks is crucial for cache-utilization and thus overall performance.

Further, I saw that we now add another reference to the scheduler-object in each task , bloating the task-struct even more. But thats fine for now, since we could probably optimize this later by using an 8-bit Index instead and storing the references to the scheduler objects in a global map.

At least on my system, the examples 1_resources , 4_refinements, game_of_life and mpi all fail to compile with the following error:

redGrapes/redGrapes/resource/access/area.hpp:75:25: error: call of overloaded 'format_to(fmt::v10::basic_format_context<fmt::v10::appender, char>::iterator, const char [46], const std::array<long unsigned int, 2>::value_type&, const std::array<long unsigned int, 2>::value_type&)' is ambiguous
   75 |         return format_to(ctx.out(), "{{ \"area\" : {{ \"begin\" : {}, \"end\" : {} }} }}", acc[0], acc[1]);

To clarify:

Fix the MPI example. MPI calls are now executed from the same thread

  • The old version did already execute MPI tasks on the main thread. (execute_task called in idle function of the main-thread)
  • If MPI_Init_thread() is used insted of MPI_Init() , it should be allowed by the MPI-Spec to use MPI-calls from any thread.
  • Nevertheless, having a dedicated MPIWorker class much better!

redGrapes/task/task_builder.hpp Show resolved Hide resolved
examples/mpi.cpp Outdated Show resolved Hide resolved
redGrapes/scheduler/thread_scheduler.hpp Show resolved Hide resolved
redGrapes/dispatch/thread/cpuset.hpp Outdated Show resolved Hide resolved
@ikbuibui
Copy link
Collaborator Author

1. Using [&] in all lambdas should be considered bad practice when writing redGrapes applications since this makes is _very easy_ to introduce race conditions or lifetime issues. It is much safer to write out all capture-by-references explicitly.  Requiring to capture the redGrapes-context in each task therefore degrades usability.

Yes, the capture by reference of the "redGrapes-context" should be done explicitly. However it currently isn't strictly required to capture it in each task, only in task which need to access methods in the redGrapes class inside the task from the user side, like emplace (for child tasks), scope_depth, create_event etc.

2. It increases the size of each task by another 8-bytes to store the reference. Having a singleton is more efficient from this perspective, as we have seen that the memory-footprint of tasks is crucial for cache-utilization and thus overall performance.

I haven't profiled the code yet, so I appreciate this insight.

Further, I saw that we now add another reference to the scheduler-object in each task , bloating the task-struct even more. But thats fine for now, since we could probably optimize this later by using an 8-bit Index instead and storing the references to the scheduler objects in a global map.

That is a good point and I also dislike storing the reference to the scheduler in the task. I was thinking of storing the scheduler information only as a type with the task. But this leads to some other complications so I havent decided on a way forward yet. Thanks for the suggesion

At least on my system, the examples 1_resources , 4_refinements, game_of_life and mpi all fail to compile with the following error:

redGrapes/redGrapes/resource/access/area.hpp:75:25: error: call of overloaded 'format_to(fmt::v10::basic_format_context<fmt::v10::appender, char>::iterator, const char [46], const std::array<long unsigned int, 2>::value_type&, const std::array<long unsigned int, 2>::value_type&)' is ambiguous
   75 |         return format_to(ctx.out(), "{{ \"area\" : {{ \"begin\" : {}, \"end\" : {} }} }}", acc[0], acc[1]);

Thanks for the report. I was able to reproduce it with clang. Will fix it.

@ikbuibui
Copy link
Collaborator Author

Implemented changes and bumped minor version of RedGrapes

@michaelsippel
Copy link
Member

Is @psychocoderHPC fine with the updated changes?

@ikbuibui
Copy link
Collaborator Author

ikbuibui commented Apr 5, 2024

Is @psychocoderHPC fine with the updated changes?

Broadly yes, he will review the updates next week before merging.

@psychocoderHPC psychocoderHPC merged commit 7e0dcd6 into ComputationalRadiationPhysics:dev Apr 15, 2024
1 check passed
@ikbuibui ikbuibui deleted the gerenic_task_refactor branch April 16, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants