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

Ensure hpx_main is a proper thread_function #6321

Merged
merged 3 commits into from
Aug 19, 2023
Merged

Ensure hpx_main is a proper thread_function #6321

merged 3 commits into from
Aug 19, 2023

Conversation

Pansysk75
Copy link
Member

@Pansysk75 Pansysk75 commented Aug 11, 2023 •
edited
Loading

Ensure that hpx_main is launched as a thread_function, so that exit callbacks are respected, and the following code will actually be used:

hpx/libs/core/threading_base/include/hpx/threading_base/register_thread.hpp

Lines 28 to 52 in e7c31a4

struct thread_function
{
F f;
inline threads::thread_result_type operator()(
threads::thread_arg_type)
{
// execute the actual thread function
f(threads::thread_restart_state::signaled);
// Verify that there are no more registered locks for this
// OS-thread. This will throw if there are still any locks held.
util::force_error_on_lock();
// run and free all registered exit functions for this thread
auto* p = get_self_id_data();
p->run_thread_exit_callbacks();
p->free_thread_exit_callbacks();
return threads::thread_result_type(
threads::thread_schedule_state::terminated,
threads::invalid_thread_id);
}
};

I should add that previously the callbacks were never executed, so using add_thread_exit_callback would cause an assertion failure later on thread destruction (L163), due to this sequence of calls:

hpx/libs/core/threading_base/src/thread_data.cpp

Lines 107 to 111 in e7c31a4

thread_data::~thread_data()
{
LTM_(debug).format("thread_data::~thread_data({})", this);
free_thread_exit_callbacks();
}

hpx/libs/core/threading_base/src/thread_data.cpp

Lines 157 to 166 in e7c31a4

void thread_data::free_thread_exit_callbacks()
{
std::lock_guard<hpx::util::detail::spinlock> l(
spinlock_pool::spinlock_for(this));
// Exit functions should have been executed.
HPX_ASSERT(exit_funcs_.empty() || ran_exit_funcs_);
exit_funcs_.clear();
}

@hkaiser Let me know if this is a good idea, and whether you'd like me to add a regression test.

@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)??-

Info

PropertyBeforeAfter
HPX Commitdcb54153d750bb
HPX Datetime2023-05-10T12:07:53+00:002023-08-11T13:11:35+00:00
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Datetime2023-05-10T14:50:18.616050-05:002023-08-11T08:20:30.831731-05:00
Envfile
Clusternamerostamrostam
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch=

Info

PropertyBeforeAfter
HPX Commitdcb54153d750bb
HPX Datetime2023-05-10T12:07:53+00:002023-08-11T13:11:35+00:00
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Datetime2023-05-10T14:52:35.047119-05:002023-08-11T08:22:44.551663-05:00
Envfile
Clusternamerostamrostam
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Comparison

BENCHMARKFORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATORPARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATORSCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add(=)(=)(=)
Stream Benchmark - Scale(=)==
Stream Benchmark - Triad(=)=(=)
Stream Benchmark - Copy(=)=(=)

Info

PropertyBeforeAfter
HPX Commitdcb54153d750bb
HPX Datetime2023-05-10T12:07:53+00:002023-08-11T13:11:35+00:00
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1
Datetime2023-05-10T14:52:52.237641-05:002023-08-11T08:23:01.362401-05:00
Envfile
Clusternamerostamrostam
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (≤5%)
++/--Large performance improvement/degradation (≤10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

@hkaiser hkaiser added this to the 1.10.0 milestone Aug 11, 2023
hkaiser
hkaiser previously approved these changes Aug 11, 2023
View reviewed changes
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hkaiser
Copy link
Member

hkaiser commented Aug 11, 2023

Let me know if this is a good idea, and whether you'd like me to add a regression test.

I'd appreciate it if you added a regression test. Thanks!

@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)??-

Info

PropertyBeforeAfter
HPX Commitdcb54155bb3624
HPX Datetime2023-05-10T12:07:53+00:002023-08-11T19:23:41+00:00
Datetime2023-05-10T14:50:18.616050-05:002023-08-11T14:30:30.574652-05:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch=

Info

PropertyBeforeAfter
HPX Commitdcb54155bb3624
HPX Datetime2023-05-10T12:07:53+00:002023-08-11T19:23:41+00:00
Datetime2023-05-10T14:52:35.047119-05:002023-08-11T14:32:46.088755-05:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1

Comparison

BENCHMARKFORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATORPARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATORSCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add(=)(=)(=)
Stream Benchmark - Scale(=)==
Stream Benchmark - Triad(=)(=)(=)
Stream Benchmark - Copy(=)(=)(=)

Info

PropertyBeforeAfter
HPX Commitdcb54155bb3624
HPX Datetime2023-05-10T12:07:53+00:002023-08-11T19:23:41+00:00
Datetime2023-05-10T14:52:52.237641-05:002023-08-11T14:33:02.897976-05:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (≤5%)
++/--Large performance improvement/degradation (≤10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

@Pansysk75 Pansysk75 mentioned this pull request Aug 11, 2023
Merged
@Pansysk75
Copy link
Member Author

retest lsu

@hkaiser
Copy link
Member

hkaiser commented Aug 19, 2023

bors merge

@bors
Copy link

bors bot commented Aug 19, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit a9ce845 into STEllAR-GROUP:master Aug 19, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkaiser hkaiser hkaiser left review comments

@aurianer aurianer Awaiting requested review from aurianer

@biddisco biddisco Awaiting requested review from biddisco

@msimberg msimberg Awaiting requested review from msimberg

Assignees
No one assigned
Labels
category: core category: init type: compatibility issue type: enhancement
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Pansysk75 @StellarBot @hkaiser