refactor: Cleanup thread ctor calls

hebasto commented at 3:45 pm on May 24, 2020: member

This PR does not change behavior. Its goal is to improve readability and maintainability of the code.

MarcoFalke commented at 4:29 pm on May 24, 2020: member

Concept ACK to enforce the void return type on threads

MarcoFalke added the label Refactoring on May 24, 2020

Empact commented at 11:15 pm on May 26, 2020: member

Concept ACK on the first two commits, ~0 on https://github.com/bitcoin/bitcoin/pull/19064/commits/2a816bd79093eac7d0bbae3106fdd7fddbc99cf2 as I don’t see the cited improvements justifying the change.

DrahtBot commented at 8:03 pm on May 27, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21800 ([WIP] mempool/validation: mempool ancestor/descendant limits for packages by glozow)
#21796 (index: Avoid async shutdown on init error by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/util/system.cpp:1135 in 2a816bd790 outdated

1131@@ -1132,6 +1132,25 @@ int GetNumCores()
1132     return std::thread::hardware_concurrency();
1133 }
1134 
1135+void TraceThread(const char* thread_name, std::function<void()> thread_func)

MarcoFalke commented at 0:13 am on May 28, 2020:

As you move this anyway, what about moving threadnames.h to thread.h and adding this to that file? Advantage would be that stuff like src/index/base.cpp that doesn’t need the whole system.h can just include the slim thread.h

hebasto commented at 9:40 am on April 25, 2021:

Thanks! Updated.

hebasto force-pushed on May 28, 2020

hebasto commented at 9:52 am on May 28, 2020: member

Updated 2a816bd79093eac7d0bbae3106fdd7fddbc99cf2 -> bcd04708a6fe32cf7fba2969ec378c34e6950278 (pr19064.01 -> pr19064.03, diff):

addressed @MarcoFalke’s comment:

As you move this anyway, what about moving threadnames.h to thread.h and adding this to that file? Advantage would be that stuff like src/index/base.cpp that doesn’t need the whole system.h can just include the slim thread.h

in src/util/thread.h:12 in bcd04708a6 outdated

21@@ -21,6 +22,11 @@ void ThreadSetInternalName(std::string&&);
22 //! logging.
23 const std::string& ThreadGetInternalName();
24 
25+/**
26+ * A wrapper for do-something-once thread functions.

MarcoFalke commented at 4:45 pm on May 30, 2020:

Based on the circular dependency and what this function does (Run a thread with log statements), maybe this is better suited in logging.h? No strong opinion, so feel free to not move this at all.

hebasto commented at 4:49 pm on May 30, 2020:

Maybe keep threadnames.{h,cpp} and move TraceThread() to new thread.{h,cpp}?

MarcoFalke commented at 4:52 pm on May 30, 2020:

Sure if that works. Cutting down the util/system bloat can’t hurt.

hebasto commented at 5:35 pm on May 30, 2020:

Updated.

hebasto force-pushed on May 30, 2020

hebasto commented at 5:35 pm on May 30, 2020: member

Updated bcd04708a6fe32cf7fba2969ec378c34e6950278 -> 90aa1f7f503818a67d18ec20401707d5e4ca658d (pr19064.03 -> pr19064.04, diff):

fixed circular dependencies

DrahtBot added the label Needs rebase on Jun 8, 2020

hebasto force-pushed on Jun 8, 2020

hebasto commented at 3:14 pm on June 8, 2020: member

Rebased 90aa1f7f503818a67d18ec20401707d5e4ca658d -> dd00f7c2c3d3af64f8a09d6b36d80f80fa96ff48 (pr19064.04 -> pr19064.05) due to the conflict with #19180.

DrahtBot removed the label Needs rebase on Jun 8, 2020

hebasto force-pushed on Jun 9, 2020

hebasto commented at 6:13 am on June 9, 2020: member

Updated dd00f7c2c3d3af64f8a09d6b36d80f80fa96ff48 -> 73370d039ec42a71171569e9b3f012e936a4ebe3 (pr19064.05 -> pr19064.06, diff):

fixed Windows builds

DrahtBot added the label Needs rebase on Jul 1, 2020

hebasto force-pushed on Jul 3, 2020

hebasto commented at 8:06 am on July 3, 2020: member

Rebased 73370d039ec42a71171569e9b3f012e936a4ebe3 -> ea12b8f04dc869f740c41fdc28d9d9fb6486f20c (pr19064.06 -> pr19064.07) due to the conflicts with #19028 and #19197.

DrahtBot removed the label Needs rebase on Jul 3, 2020

DrahtBot added the label Needs rebase on Aug 5, 2020

hebasto force-pushed on Aug 5, 2020

hebasto commented at 5:52 pm on August 5, 2020: member

Rebased ea12b8f04dc869f740c41fdc28d9d9fb6486f20c -> 427e396b16564d9c735238a2b2d24472bfcd55c3 (pr19064.07 -> pr19064.08) due to the conflict with #15382.

DrahtBot removed the label Needs rebase on Aug 5, 2020

DrahtBot added the label Needs rebase on Aug 12, 2020

hebasto force-pushed on Aug 13, 2020

hebasto commented at 2:11 pm on August 13, 2020: member

Rebased 427e396b16564d9c735238a2b2d24472bfcd55c3 -> 338488f056169898905bf03fb3df34e6877833ac (pr19064.08 -> pr19064.09) due to the conflict with #19316.

DrahtBot removed the label Needs rebase on Aug 13, 2020

hebasto closed this on Aug 13, 2020

hebasto reopened this on Aug 13, 2020

in src/txmempool.cpp:22 in 338488f056 outdated

18@@ -19,6 +19,8 @@
19 #include <util/time.h>
20 #include <validationinterface.h>
21 
22+#include <math.h>

practicalswift commented at 7:00 am on August 16, 2020:

0#include <cmath>

hebasto commented at 7:22 am on August 16, 2020:

Thanks! Updated.

practicalswift commented at 7:00 am on August 16, 2020: contributor

Concept ACK

hebasto force-pushed on Aug 16, 2020

hebasto commented at 7:21 am on August 16, 2020: member

Updated 338488f056169898905bf03fb3df34e6877833ac -> 4335a15ac6aca65430471ead52d389813f4260e7 (pr19064.09 -> pr19064.10, diff):

addressed @practicalswift’s comment

DrahtBot added the label Needs rebase on Aug 26, 2020

hebasto force-pushed on Aug 26, 2020

hebasto commented at 9:45 am on August 26, 2020: member

Rebased 4335a15ac6aca65430471ead52d389813f4260e7 -> 806aedebc2b0627e681480be39c2d93af87e0863 (pr19064.10 -> pr19064.11) due to the conflict with #19779.

DrahtBot removed the label Needs rebase on Aug 26, 2020

DrahtBot added the label Needs rebase on Oct 2, 2020

hebasto force-pushed on Oct 2, 2020

hebasto commented at 9:05 pm on October 2, 2020: member

Rebased 806aedebc2b0627e681480be39c2d93af87e0863 -> 8cb5f901dfeb8de5427532e6646c3a5a131fed14 (pr19064.11 -> pr19064.12) due to the conflict with #19991.

DrahtBot removed the label Needs rebase on Oct 2, 2020

hebasto force-pushed on Oct 26, 2020

hebasto commented at 8:55 pm on October 26, 2020: member

Rebased 8cb5f901dfeb8de5427532e6646c3a5a131fed14 -> 7e7a999e9e4bdef9726037afa4ec0325a355528e (pr19064.12 -> pr19064.13).

DrahtBot added the label Needs rebase on Oct 29, 2020

hebasto force-pushed on Oct 29, 2020

hebasto commented at 3:11 pm on October 29, 2020: member

Rebased 7e7a999e9e4bdef9726037afa4ec0325a355528e -> 25c8e21b0f316ce7804e10de813c4bcd81d3c2f3 (pr19064.13 -> pr19064.14).

DrahtBot removed the label Needs rebase on Oct 29, 2020

DrahtBot added the label Needs rebase on Dec 9, 2020

hebasto force-pushed on Dec 10, 2020

hebasto commented at 7:33 pm on December 10, 2020: member

Rebased 25c8e21b0f316ce7804e10de813c4bcd81d3c2f3 -> 358a0447f59608af7bd51fe200e27baefb78159e (pr19064.14 -> pr19064.15) due to the conflict with #20323.

DrahtBot removed the label Needs rebase on Dec 10, 2020

DrahtBot added the label Needs rebase on Jan 7, 2021

hebasto force-pushed on Jan 7, 2021

hebasto commented at 11:42 pm on January 7, 2021: member

Updated 358a0447f59608af7bd51fe200e27baefb78159e -> 519838dbb91410544743b69c3d386fff9e4eb400 (pr19064.15 -> pr19064.16):

rebased due to the conflict with #18077
addressed @Empact’s comment and dropped the last controversial commit

DrahtBot removed the label Needs rebase on Jan 8, 2021

jonatack commented at 3:42 pm on January 27, 2021: member

Concept ACK. Rebased to current master 15a9df070 and this debug builds cleanly. Now that we have moved on from C++ 11, can this pull also use lambdas in the place of std::bind and std::function?

hebasto commented at 3:48 pm on January 27, 2021: member

Concept ACK. Rebased to current master 15a9df0 and this debug builds cleanly. Now that we have moved on from C++ 11, can this pull also use lambdas in the place of std::bind and std::function?

The dedicated commit was removed from this PR in the latest push, see #19064 (comment)

jonatack commented at 3:53 pm on January 27, 2021: member

It seems worthwhile to do. https://youtu.be/ZlHi8txU4aQ

jonatack commented at 5:27 pm on January 27, 2021: member

Also https://youtu.be/zt7ThwVfap0?t=1721 (I’m still going through it, though)

Screenshot from 2021-01-27 18-21-18

Screenshot from 2021-01-27 18-20-48

DrahtBot added the label Needs rebase on Feb 1, 2021

hebasto force-pushed on Apr 13, 2021

DrahtBot removed the label Needs rebase on Apr 13, 2021

hebasto commented at 7:26 pm on April 13, 2021: member

Updated 519838dbb91410544743b69c3d386fff9e4eb400 -> 33f9134f61d99903939c3048340927d051fd60f9 (pr19064.16 -> pr19064.17):

rebased due to the numerous conflicts
addressed @jonatack’s comment

in src/validation.cpp:49 in 33f9134f61 outdated

45@@ -46,6 +46,7 @@
46 #include <util/rbf.h>
47 #include <util/strencodings.h>
48 #include <util/system.h>
49+#include <util/threadnames.h>

kiminuo commented at 8:54 am on April 25, 2021:

Why is this added? I can’t find any util::* call.

hebasto commented at 9:36 am on April 25, 2021:

Thanks! Updated.

refactor: Make TraceThread a non-template free function

Also it is moved into its own module.

30e4448215

hebasto force-pushed on Apr 25, 2021

hebasto commented at 9:35 am on April 25, 2021: member

Updated 33f9134f61d99903939c3048340927d051fd60f9 -> 2a3ad116c8cf00ac44c9ad6c8585edf37054761d (pr19064.17 -> pr19064.18, diff):

addressed @kiminuo’s comment

jonatack commented at 7:01 pm on April 25, 2021: member

ACK 2a3ad116c8cf00ac44c9ad6c8585edf37054761d

Recommend reviewers view the diff squashed to one commit as the second and third commit rewrite parts of the first one.

in src/txmempool.cpp:21 in 2a3ad116c8 outdated

17@@ -18,6 +18,7 @@
18 #include <validation.h>
19 #include <validationinterface.h>
20 
21+#include <cmath>

jnewbery commented at 9:52 am on April 26, 2021:

Why are you adding this?

jonatack commented at 10:15 am on April 26, 2021:

I asked myself this as well–it’s a missing header for llround used in the file.

hebasto commented at 2:54 pm on April 29, 2021:

In a previous iteration of this PR this header was required to build successfully for the reason @jonatack pointed at.

Currently, this change looks unrelated. Should I drop it?

hebasto commented at 3:25 pm on April 29, 2021:

Updated.

in src/net.cpp:2550 in 2a3ad116c8 outdated

2546@@ -2546,15 +2547,14 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2547         return false;
2548     }
2549     if (connOptions.m_use_addrman_outgoing || !connOptions.m_specified_outgoing.empty())
2550-        threadOpenConnections = std::thread(&TraceThread<std::function<void()> >, "opencon", std::function<void()>(std::bind(&CConnman::ThreadOpenConnections, this, connOptions.m_specified_outgoing)));
2551+        threadOpenConnections = std::thread(&util::TraceThread, "opencon", [this, connOptions] { ThreadOpenConnections(connOptions.m_specified_outgoing); });

jnewbery commented at 9:54 am on April 26, 2021:

Consider just capturing what you need, rather than making a copy of the entire connOptions object:

0     if (connOptions.m_use_addrman_outgoing || !connOptions.m_specified_outgoing.empty())
1-        threadOpenConnections = std::thread(&util::TraceThread, "opencon", [this, connOptions] { ThreadOpenConnections(connOptions.m_specified_outgoing); });
2+        threadOpenConnections = std::thread(&util::TraceThread,
3+                                            "opencon",
4+                                            [this, connect = connOptions.m_specified_outgoing] { ThreadOpenConnections(connect); });
5 
6     // Process messages

hebasto commented at 3:46 pm on April 29, 2021:

Thanks! Updated.

in src/init.cpp:1270 in 2a3ad116c8 outdated

1266@@ -1266,7 +1267,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1267     node.scheduler = std::make_unique<CScheduler>();
1268 
1269     // Start the lightweight task scheduler thread
1270-    node.scheduler->m_service_thread = std::thread([&] { TraceThread("scheduler", [&] { node.scheduler->serviceQueue(); }); });
1271+    node.scheduler->m_service_thread = std::thread([&] { util::TraceThread("scheduler", [&] { node.scheduler->serviceQueue(); }); });

jnewbery commented at 10:01 am on April 26, 2021:

Perhaps:

0    node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { node.scheduler->serviceQueue(); });

To make this more readable and consistent with the other calls to std::thread ctor.

hebasto commented at 3:46 pm on April 29, 2021:

Thanks! Updated.

in src/test/util/setup_common.cpp:137 in 2a3ad116c8 outdated

133@@ -132,7 +134,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
134     // We have to run a scheduler thread to prevent ActivateBestChain
135     // from blocking due to queue overrun.
136     m_node.scheduler = std::make_unique<CScheduler>();
137-    m_node.scheduler->m_service_thread = std::thread([&] { TraceThread("scheduler", [&] { m_node.scheduler->serviceQueue(); }); });
138+    m_node.scheduler->m_service_thread = std::thread([&] { util::TraceThread("scheduler", [&] { m_node.scheduler->serviceQueue(); }); });

jnewbery commented at 10:01 am on April 26, 2021:

Perhaps:

0    m_node.scheduler->m_service_thread = std::thread(&util::TraceThread("scheduler", [&] { m_node.scheduler->serviceQueue(); });

To make this more readable and consistent with the other calls to std::thread ctor.

hebasto commented at 3:47 pm on April 29, 2021:

Thanks! Updated.

jnewbery commented at 10:03 am on April 26, 2021: member

Looks good. A few suggestions inline.

Perhaps thread.h and threadnames.h should be merged?

hebasto force-pushed on Apr 29, 2021

hebasto marked this as a draft on Apr 29, 2021

refactor: Use appropriate thread constructor a508f718f3

refactor: Replace std::bind with lambdas

Lambdas are shorter and more readable.
Changes are limited to std::thread ctor calls only.

792be53d3e

hebasto force-pushed on Apr 29, 2021

hebasto commented at 3:46 pm on April 29, 2021: member

Updated 2a3ad116c8cf00ac44c9ad6c8585edf37054761d -> 792be53d3e9e366b9f6aeee7a1eeb912fa28062e (pr19064.18 -> pr19064.19, diff):

addressed @jnewbery’s comments @jnewbery

Perhaps thread.h and threadnames.h should be merged?

It will introduce a new circular dependency “logging -> util/threadnames -> logging”.

hebasto marked this as ready for review on Apr 29, 2021

jnewbery commented at 3:50 pm on April 29, 2021: member

utACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e

jonatack commented at 6:56 pm on April 29, 2021: member

Changes are even nicer now.

tACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e

in src/init.cpp:1270 in 792be53d3e

1266@@ -1266,7 +1267,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1267     node.scheduler = std::make_unique<CScheduler>();
1268 
1269     // Start the lightweight task scheduler thread
1270-    node.scheduler->m_service_thread = std::thread([&] { TraceThread("scheduler", [&] { node.scheduler->serviceQueue(); }); });
1271+    node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { node.scheduler->serviceQueue(); });

MarcoFalke commented at 6:50 am on May 12, 2021:

Is there a reason or even a difference between sometimes using &util::TraceThread and other times util::TraceThread?

hebasto commented at 8:10 am on May 12, 2021:

No, there isn’t. See https://en.cppreference.com/w/cpp/language/pointer#Pointers_to_functions

It seems I overlooked that inconsistency.

MarcoFalke commented at 6:51 am on May 12, 2021: member

cr ACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e

MarcoFalke merged this on May 12, 2021

MarcoFalke closed this on May 12, 2021

hebasto deleted the branch on May 12, 2021

sidhujag referenced this in commit 51e4bee06f on May 12, 2021

gwillen referenced this in commit 99cc7a5245 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

refactor: Cleanup thread ctor calls #19064

Conflicts