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

Catch exception in lightweight node #623

Merged
merged 17 commits into from
Dec 8, 2021
Merged

Catch exception in lightweight node #623

merged 17 commits into from
Dec 8, 2021

Conversation

Iliamish
Copy link
Contributor

@Iliamish Iliamish commented Oct 19, 2021 •
edited
Loading

Description

If user code throws exception inside body of lightweight node, that deadlock graph. In this patch we require from user mark body noexcept for using lightweight policy. If body can throw exception, policy implicitly implements queuing behavior.

Fixes # - #420

Type of change

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

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

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

Tests

  • added - required for new features and for 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

List users with @ to send notifications

Other information

@Iliamish Iliamish self-assigned this Oct 19, 2021
@Iliamish Iliamish changed the title Catch exception in lightweight node WIP: Catch exception in lightweight node Oct 19, 2021
aleksei-fedotov
aleksei-fedotov reviewed Oct 27, 2021
View reviewed changes
test/tbb/test_flow_graph_priorities.cpp Show resolved Hide resolved
include/oneapi/tbb/flow_graph.h Show resolved Hide resolved
include/oneapi/tbb/detail/_flow_graph_node_impl.h Show resolved Hide resolved
include/oneapi/tbb/detail/_flow_graph_node_impl.h Outdated Show resolved Hide resolved
test/tbb/test_multifunction_node.cpp Outdated Show resolved Hide resolved
test/tbb/test_multifunction_node.cpp Outdated Show resolved Hide resolved
aleksei-fedotov
aleksei-fedotov reviewed Nov 10, 2021
View reviewed changes
test/tbb/test_flow_graph_priorities.cpp Show resolved Hide resolved
test/common/graph_utils.h Outdated Show resolved Hide resolved
include/oneapi/tbb/detail/_flow_graph_node_impl.h Outdated Show resolved Hide resolved
test/tbb/test_multifunction_node.cpp Outdated Show resolved Hide resolved
@Iliamish
Add raii to aggregator to prevent stuck
865a9bb 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
Add test
ea5daa8 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
Reuse tbb raii guard
2383550 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
Add header
b98c30c 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
execute_and_wait lightweight task
bf87053 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
remove unused headers
8e43970 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
remove space
0913a43 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
remove std::function
5b46ba8 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
noexcept lightweight body
910b3dc 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
fix cmake
e06f25c 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
fix space
b97d875 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
gcc error
058f5c0 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
declval gateway_type
7cc2e29 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
new tests
dad5e1b 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish
template tests
80d0385 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
@Iliamish Iliamish force-pushed the dev/iliamish/fix_exception_in_lightweight_policy branch from c0445be to 80d0385 Compare November 10, 2021 18:37
@Iliamish
macro TBB_USE_EXCEPTIONS
4c67760 
Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>
aleksei-fedotov
aleksei-fedotov reviewed Nov 12, 2021
View reviewed changes
test/common/graph_utils.h Outdated Show resolved Hide resolved
test/common/graph_utils.h Outdated Show resolved Hide resolved
test/tbb/test_flow_graph_priorities.cpp Show resolved Hide resolved
@Iliamish @aleksei-fedotov
Apply suggestions from code review
89077b2 
Co-authored-by: Aleksei Fedotov <aleksei.fedotov@intel.com>
@Iliamish Iliamish changed the title WIP: Catch exception in lightweight node Catch exception in lightweight node Nov 12, 2021
@vlserov vlserov merged commit 324afd9 into master Dec 8, 2021
@vlserov vlserov deleted the dev/iliamish/fix_exception_in_lightweight_policy branch December 8, 2021 08:46
kboyarinov pushed a commit that referenced this pull request Dec 27, 2021
@Iliamish @aleksei-fedotov @kboyarinov
Catch exception in lightweight node (#623)
70455c7 
* Add raii to aggregator to prevent stuck

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* Add test

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* Reuse tbb raii guard

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* Add header

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* execute_and_wait lightweight task

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* remove unused headers

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* remove space

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* remove std::function

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* noexcept lightweight body

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* fix cmake

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* fix space

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* gcc error

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* declval gateway_type

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* new tests

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* template tests

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* macro TBB_USE_EXCEPTIONS

Signed-off-by: Mishin, Ilya <ilya.mishin@intel.com>

* Apply suggestions from code review

Co-authored-by: Aleksei Fedotov <aleksei.fedotov@intel.com>

Co-authored-by: Aleksei Fedotov <aleksei.fedotov@intel.com>
@akukanov
Copy link
Contributor

we require from user mark body noexcept for using lightweight policy. If body can throw exception, policy implicitly implements queuing behavior.

Is it requirement reflected in the oneTBB specification? I think it should.
@alexey-katranov @aleksei-fedotov

@aleksei-fedotov
Copy link
Contributor

we require from user mark body noexcept for using lightweight policy. If body can throw exception, policy implicitly implements queuing behavior.

Is it requirement reflected in the oneTBB specification? I think it should. @alexey-katranov @aleksei-fedotov

PR regarding the reflection: uxlfoundation/oneAPI-spec#407. @akukanov @alexey-katranov

@akukanov
Copy link
Contributor

akukanov commented Feb 8, 2022

Another concern is that, though backward compatibility is not formally broken, the behavior of codes that use lightweight policies may implicitly change, resulting in a possible performance loss. @alexey-katranov, @aleksei-fedotov, have you considered some form of a warning to be issued when lightweight policies are applied without noexcept?

@isaevil isaevil mentioned this pull request Sep 30, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivankochin ivankochin ivankochin left review comments

@aleksei-fedotov aleksei-fedotov aleksei-fedotov approved these changes

Assignees

@Iliamish Iliamish

Labels
bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Iliamish @akukanov @aleksei-fedotov @ivankochin @vlserov