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

[Simulation] Separate factory code from TaskScheduler #3480

Merged
merged 10 commits into from Dec 1, 2022

Conversation

alxbilger
Copy link
Contributor

TaskScheduler was doing several things:

  • A base class for task schedulers
  • A factory to register and create task schedulers

The factory code is extracted from the TaskScheduler class and moved into its dedicated class and file.

I refactored a bit how the factory works:
Before, it was like there could be only one TaskScheduler at a time. Everytime a new TaskScheduler was created, the previous one is destroyed. It prevents the use of multiple TaskScheduler with different type (derived class). Instead, the task schedulers are now stored in a map.
The only issue is related to the task allocator. The task allocator is used every time a task is created. The task allocator is supposed to depend on the TaskScheduler where the task will be added. But in practice, a Task is created independently from the TaskScheduler. So I kept the old system that calls a static function setting the allocator when the task scheduler is created.

Some unit tests are introduced


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@alxbilger alxbilger added pr: breaking Change possibly inducing a compilation error pr: status to review To notify reviewers to review this pull-request refactoring Refactor code pr: clean Cleaning the code labels Nov 14, 2022
@alxbilger
Copy link
Contributor Author

[ci-build][with-all-tests]

@alxbilger
Copy link
Contributor Author

For some reasons, the static task scheduler could not be destroyed by the end of the program because it results into a deadlock: the worker threads are not woken up by the destructor of the task scheduler. I did not find how to fix this issue. Therefore, I clear the task schedulers before the end of the program, in the module cleanup function.

It did not happened before because the task scheduler was never destroyed resulting in a memory leak and not-joined idle threads.

@damienmarchal
Copy link
Contributor

damienmarchal commented Nov 23, 2022

The rational of the PR is sounded and the existing design in sofa that mix the base class with a kind of hardcoded singleton/registry is bad and should be refactored (thank for the PR so).

From my experience the refactoring step in such situation depends if the static part is actually implementing kind of singleton or a kind registry of instances. But in both case the refactoring consists in moving all the static methods and static attributes in a new class with the sole purpose of handle the registry/singleton.

In your PR you are moving the static method and the singleton into TaskSchedulerFactory.
If found using "factory" is confusing as to me a factory's roles (and design pattern) is to map to different construction methods and not holding the created instances. In order to keep the registry separated from the factory part I recommend keeping the two concerns separated in different classes.

To me a clean layout separating implementions and concerns is the following:

class TaskScheduler {};                          // The base class for a scheduler
class TaskSchedulerRegistry:: {};           // The class implementing a registry of taskschedulers
class TaskSchedulerFactory:: {};            // The class implementing a factory for taskscheduler
class MainTaskSchedulerRegistry::{};    // The "static" class which contains a singleton of a TaskSchedulerRegistry 
                                                               // and public static methods to manipulate it (with mutex in each of them) 
class MainTaskSchedulerFactory::{}      // The "static" class which contains a singleton of a TaskSchedulerFactory
                                                               // and pubic static methods to manipulate it.  

@alxbilger
Copy link
Contributor Author

@damienmarchal It makes sense. Thanks for the feedback

@hugtalbot
Copy link
Contributor

Looks good to me, but I am no expert of the TaskScheduler

@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Nov 30, 2022
@fredroy fredroy merged commit c74e913 into sofa-framework:master Dec 1, 2022
Multithreading automation moved this from Doing to Done Dec 1, 2022
@hugtalbot hugtalbot added this to the v22.12 milestone Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: breaking Change possibly inducing a compilation error pr: clean Cleaning the code pr: status ready Approved a pull-request, ready to be squashed refactoring Refactor code
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants