-
Notifications
You must be signed in to change notification settings - Fork 5
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
Task Template refactor #58
Conversation
bc0926d
to
43e6370
Compare
There was a problem hiding this 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:
- 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.
- 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 inidle
function of the main-thread) - If
MPI_Init_thread()
is used insted ofMPI_Init()
, it should be allowed by the MPI-Spec to use MPI-calls from any thread. - Nevertheless, having a dedicated
MPIWorker
class much better!
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.
I haven't profiled the code yet, so I appreciate this insight.
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
Thanks for the report. I was able to reproduce it with clang. Will fix it. |
Implemented changes and bumped minor version of RedGrapes |
2471b77
to
f385b51
Compare
f385b51
to
f158501
Compare
Is @psychocoderHPC fine with the updated changes? |
Broadly yes, he will review the updates next week before merging. |
0cffc90
to
169cd22
Compare
7e0dcd6
into
ComputationalRadiationPhysics:dev
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