refactor: Cleanup thread ctor calls by hebasto · Pull Request #19064 · bitcoin/bitcoin

hebasto · 2020-05-24T15:45:04Z

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

maflcko · 2020-05-24T16:29:38Z

Concept ACK to enforce the void return type on threads

Empact · 2020-05-26T23:15:24Z

Concept ACK on the first two commits, ~0 on 2a816bd as I don't see the cited improvements justifying the change.

DrahtBot · 2020-05-27T20:03:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

mempool/validation: mempool ancestor/descendant limits for packages #21800 ([WIP] mempool/validation: mempool ancestor/descendant limits for packages by glozow)
index: Avoid async shutdown on init error #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.

src/util/system.cpp

hebasto · 2020-05-28T09:52:10Z

Updated 2a816bd -> bcd0470 (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

src/util/thread.h

hebasto · 2020-05-30T17:35:21Z

Updated bcd0470 -> 90aa1f7 (pr19064.03 -> pr19064.04, diff):

fixed circular dependencies

hebasto · 2020-06-08T15:14:17Z

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

hebasto · 2020-06-09T06:13:53Z

Updated dd00f7c -> 73370d0 (pr19064.05 -> pr19064.06, diff):

fixed Windows builds

hebasto · 2021-04-13T19:26:37Z

Updated 519838d -> 33f9134 (pr19064.16 -> pr19064.17):

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

src/validation.cpp

Also it is moved into its own module.

hebasto · 2021-04-25T09:35:37Z

Updated 33f9134 -> 2a3ad11 (pr19064.17 -> pr19064.18, diff):

addressed @kiminuo's comment

jonatack

ACK 2a3ad11

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

jnewbery

Looks good. A few suggestions inline.

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

src/txmempool.cpp

src/net.cpp

src/init.cpp

src/test/util/setup_common.cpp

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

hebasto · 2021-04-29T15:46:00Z

Updated 2a3ad11 -> 792be53 (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".

jnewbery · 2021-04-29T15:50:53Z

utACK 792be53

jonatack · 2021-04-29T18:56:59Z

Changes are even nicer now.

tACK 792be53

maflcko · 2021-05-12T06:50:25Z

src/init.cpp


    // Start the lightweight task scheduler thread
-    node.scheduler->m_service_thread = std::thread([&] { TraceThread("scheduler", [&] { node.scheduler->serviceQueue(); }); });
+    node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { node.scheduler->serviceQueue(); });


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

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

It seems I overlooked that inconsistency.

maflcko · 2021-05-12T06:51:04Z

cr ACK 792be53

maflcko added the Refactoring label May 24, 2020

maflcko reviewed May 28, 2020

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request May 28, 2020

p2p: Speed up initial connection to p2p network #15502

Closed

hebasto force-pushed the 200524-bind branch from 2a816bd to f96b2c8 Compare May 28, 2020 09:36

maflcko reviewed May 30, 2020

View reviewed changes

src/util/thread.h Show resolved Hide resolved

hebasto force-pushed the 200524-bind branch from bcd0470 to 90aa1f7 Compare May 30, 2020 17:32

DrahtBot mentioned this pull request Jun 6, 2020

refactor: Replace RecursiveMutex with Mutex in Shutdown() #19180

Merged

DrahtBot added the Needs rebase label Jun 8, 2020

hebasto force-pushed the 200524-bind branch from 90aa1f7 to dd00f7c Compare June 8, 2020 15:13

DrahtBot removed the Needs rebase label Jun 8, 2020

hebasto force-pushed the 200524-bind branch from dd00f7c to 73370d0 Compare June 9, 2020 05:54

This was referenced Jun 9, 2020

[WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) #19183

Closed

UI external signer support (e.g. hardware wallet) #16549

Closed

External signer support - Wallet Box edition #16546

Merged

DrahtBot added the Needs rebase label Feb 1, 2021

hebasto force-pushed the 200524-bind branch from 519838d to 33f9134 Compare April 13, 2021 18:23

DrahtBot removed the Needs rebase label Apr 13, 2021

DrahtBot mentioned this pull request Apr 14, 2021

rpc/validation: enable packages through testmempoolaccept #20833

Merged

kiminuo reviewed Apr 25, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

refactor: Make TraceThread a non-template free function

30e4448

Also it is moved into its own module.

hebasto force-pushed the 200524-bind branch from 33f9134 to 2a3ad11 Compare April 25, 2021 09:32

jonatack reviewed Apr 25, 2021

View reviewed changes

jnewbery reviewed Apr 26, 2021

View reviewed changes

src/txmempool.cpp Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

This was referenced Apr 28, 2021

fuzz: Add fuzzing syscall sandbox: detect use of unexpected syscalls when fuzzing ("syscall sanitizer") #21538

Closed

index: Avoid async shutdown on init error #21796

Merged

mempool/validation: mempool ancestor/descendant limits for packages #21800

Merged

hebasto force-pushed the 200524-bind branch from 2a3ad11 to 1edb237 Compare April 29, 2021 15:21

hebasto marked this pull request as draft April 29, 2021 15:25

hebasto added 2 commits April 29, 2021 18:39

refactor: Use appropriate thread constructor

a508f71

refactor: Replace std::bind with lambdas

792be53

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

hebasto force-pushed the 200524-bind branch from 1edb237 to 792be53 Compare April 29, 2021 15:42

hebasto marked this pull request as ready for review April 29, 2021 15:46

maflcko reviewed May 12, 2021

View reviewed changes

maflcko merged commit 2e30e32 into bitcoin:master May 12, 2021

hebasto deleted the 200524-bind branch May 12, 2021 08:13

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 12, 2021

Merge bitcoin#19064: refactor: Cleanup thread ctor calls

51e4bee

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 2e30e32 into merged_master (Bitcoin PR bitcoin/bitcoin#19064)

99cc7a5

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

hebasto commented May 24, 2020

Uh oh!

maflcko commented May 24, 2020

Uh oh!

Empact commented May 26, 2020

Uh oh!

DrahtBot commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

hebasto commented May 28, 2020

Uh oh!

Uh oh!

hebasto commented May 30, 2020

Uh oh!

hebasto commented Jun 8, 2020

Uh oh!

hebasto commented Jun 9, 2020

Uh oh!

hebasto commented Apr 13, 2021

Uh oh!

Uh oh!

hebasto commented Apr 25, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Apr 29, 2021

Uh oh!

jnewbery commented Apr 29, 2021

Uh oh!

jonatack commented Apr 29, 2021

Uh oh!

maflcko May 12, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto May 12, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented May 27, 2020 •

edited

Loading