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.

Sign up for GitHub

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

Jump to bottom

Fix control variable initialization #1346

Merged
merged 1 commit into from
Apr 18, 2024
Merged

Fix control variable initialization #1346

merged 1 commit into from
Apr 18, 2024

Conversation

SoilRos
Copy link
Contributor

@SoilRos SoilRos commented Apr 15, 2024 •
edited by dnmokhov
Loading

Description

Fixes initialization order bug stated in #1341 (comment)

Fixes #1341

  • - git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details)

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

@pavelkumbrasev

Other information

@SoilRos
Copy link
Contributor Author

SoilRos commented Apr 15, 2024

I'll sign the commit once we agree that this is the correct solution...

@SoilRos SoilRos mentioned this pull request Apr 15, 2024
Closed
@omalyshe omalyshe requested a review from pavelkumbrasev April 16, 2024 10:33
@SoilRos SoilRos force-pushed the bugfix/1341 branch 2 times, most recently from 891f900 to d1fe291 Compare April 16, 2024 15:52
@SoilRos
Copy link
Contributor Author

SoilRos commented Apr 16, 2024

Previous commit fails due to C++14 compatibility on new alignment. Now this should hopefully be fixed.

phprus
phprus reviewed Apr 16, 2024
View reviewed changes
src/tbb/global_control.cpp Outdated Show resolved Hide resolved
pavelkumbrasev
pavelkumbrasev reviewed Apr 17, 2024
View reviewed changes
src/tbb/global_control.cpp Outdated Show resolved Hide resolved
src/tbb/global_control.cpp Outdated Show resolved Hide resolved
pavelkumbrasev
pavelkumbrasev reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor

@pavelkumbrasev pavelkumbrasev left a comment

Choose a reason for hiding this comment

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

Overall the patch looks good!

src/tbb/global_control.cpp Outdated Show resolved Hide resolved
src/tbb/global_control.cpp Outdated Show resolved Hide resolved
kboyarinov
kboyarinov reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor

@kboyarinov kboyarinov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Excluding the copyright comment - LGTM

src/tbb/global_control.cpp Show resolved Hide resolved
@SoilRos
Copy link
Contributor Author

SoilRos commented Apr 17, 2024

I addressed your reviews and squashed the commits, Thanks!

I was thinking that we may add a test for this with a global observer as presented in the original issue #1341. I just do not know where to put it. Do you have a hint where it can go? Or do you think it's not necessary?

@pavelkumbrasev
Copy link
Contributor

I was thinking that we may add a test for this with a global observer as presented in the original issue #1341. I just do not know where to put it. Do you have a hint where it can go? Or do you think it's not necessary?

As I understand we cannot reproduce this failure without compiling TBB statically. I don't know if we previously added separate tests for a specific platform.

@SoilRos
Delay control variable initialization
cfc9aa9 
Previous to this commit, global scheduler observers would be initialized before the global TBB controllers.

Signed off by: Santiago Ospina De Los Ríos <[email protected]>
@phprus
Copy link
Contributor

phprus commented Apr 17, 2024

As I understand we cannot reproduce this failure without compiling TBB statically. I don't know if we previously added separate tests for a specific platform.

Maybe add tests for static build?

@SoilRos
Copy link
Contributor Author

SoilRos commented Apr 17, 2024

Alright, then this is ready from my side. I just amended my commit with the copyright comment (I hope it's correct). There is no changes in the code since the last CI run.

@pavelkumbrasev
Copy link
Contributor

Maybe add tests for static build?

It might make sense since WASM compile TBB statically. Do you want to contribute it ? 😄

kboyarinov
kboyarinov approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@kboyarinov kboyarinov left a comment

Choose a reason for hiding this comment

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

LGTM

@pavelkumbrasev pavelkumbrasev merged commit 4a87ca1 into uxlfoundation:master Apr 18, 2024
19 of 20 checks passed
@phprus phprus mentioned this pull request Apr 18, 2024
Merged
14 tasks
@phprus
Copy link
Contributor

phprus commented Apr 18, 2024

Maybe add tests for static build?

It might make sense since WASM compile TBB statically. Do you want to contribute it ? 😄

PR: #1350

@andrjohns andrjohns mentioned this pull request Jun 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phprus phprus phprus left review comments

@kboyarinov kboyarinov kboyarinov approved these changes

@pavelkumbrasev pavelkumbrasev pavelkumbrasev approved these changes

Assignees
No one assigned
Labels
bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Global task_schedule_observer leads to error on WASM and static lib x86
5 participants
@SoilRos @pavelkumbrasev @phprus @kboyarinov @dnmokhov