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

[SofaSimulationCore] FIX Task scheduler memory leak #1927

Merged
merged 5 commits into from Apr 14, 2021

Conversation

alxbilger
Copy link
Contributor

Fix memory leak in task scheduler:

  • Add comments
  • Add unit tests
  • Use shared pointer instead of raw pointer:

The static raw pointer had two bad effects:

  • Memory leak
  • Task scheduler not being stopped

Note: The current API for the task scheduler is very confusing. I suggest to clarify the expected behavior.


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).

@fredroy
Copy link
Contributor

fredroy commented Mar 16, 2021

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

Copy link
Contributor

@fspadoni fspadoni left a comment

Choose a reason for hiding this comment

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

Good to use a smart pointer and fix the memory leak.

What about using a unique_ptr ?
The task scheduler should manage itself and no other class should control its lifetime storing a refernce to it.

@alxbilger
Copy link
Contributor Author

@fspadoni you're right, I'll change it

@hugtalbot hugtalbot added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request labels Mar 17, 2021
@alxbilger
Copy link
Contributor Author

@fspadoni Is the task scheduler designed to have local schedulers? For example, can I have a task scheduler specific to a component? Is it restricted to have only one instance per derived type?

@fspadoni
Copy link
Contributor

@fspadoni Is the task scheduler designed to have local schedulers? For example, can I have a task scheduler specific to a component? Is it restricted to have only one instance per derived type?

There is only one global instance of the task scheduler class and this is shared between all components.
Any components can create and submit tasks to task scheduler.
The tasks are queued and process by the worker threads.
The task scheduler manages the worker threads (only one per cpu core by default)

@hugtalbot hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Mar 24, 2021
@alxbilger
Copy link
Contributor Author

@hugtalbot @guparan I am not sure I just need to switch from EXPECT_EQ to EXPECT_NE. When I'll refactor the interface of the TaskScheduler, a large majority of the test will not make sense. I suggest to merge this PR now, and I'll change the tests in my next PR.

@alxbilger alxbilger removed the pr: status wip Development in the pull-request is still in progress label Apr 9, 2021
@alxbilger
Copy link
Contributor Author

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

@alxbilger
Copy link
Contributor Author

I removed completely the unit tests. I'll add them later in another pull request.

@hugtalbot hugtalbot added pr: status to review To notify reviewers to review this pull-request 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 pr: status ready Approved a pull-request, ready to be squashed labels Apr 9, 2021
@hugtalbot
Copy link
Contributor

Looks fine to me but I am really no MT / TaskScheduler expert

@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 Apr 9, 2021
@fredroy fredroy merged commit e6b15fa into sofa-framework:master Apr 14, 2021
alxbilger added a commit to alxbilger/sofa that referenced this pull request Apr 16, 2021
sofa-framework#1927 is suspected to causes crashes in multi-threaded scenes on Jenkins
alxbilger added a commit to alxbilger/sofa that referenced this pull request Apr 16, 2021
alxbilger added a commit to alxbilger/sofa that referenced this pull request Apr 16, 2021
fredroy pushed a commit that referenced this pull request Apr 22, 2021
* [SofaSimulationCore] Revert changes in #1927

#1927 is suspected to causes crashes in multi-threaded scenes on Jenkins

* [SofaSimulationCore] Introduce change from #1927 to check CI

* Revert "[SofaSimulationCore] Introduce change from #1927 to check CI"

This reverts commit cadedce
@guparan guparan added this to the v21.06 milestone Jun 28, 2021
@alxbilger alxbilger deleted the taskScheduler branch June 28, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants