Improve tracethread usability and trace loadblk thread #17312

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2019_10_tracedthread changing 6 files +44 −41

laanwj commented at 9:18 AM on October 30, 2019: member

This consists of three commits, the first has the most overall code change but is a refactor. The second adds tracing around the loadblk thread. The third is a simple refactor to get rid again of two c_str().

(yes, I considered making TracedThread return a std::thread, but this isn't possible because it's passed to threadGroup.create_thread in two places)
laanwj added the label Utils/log/libs on Oct 30, 2019
laanwj force-pushed on Oct 30, 2019

in src/util/system.cpp:1248 in e678598659 outdated

1241 | @@ -1242,6 +1242,32 @@ int ScheduleBatchPriority()
1242 |  #endif
1243 |  }
1244 |  
1245 | +std::function<void()> TracedThread(const std::string name, std::function<void()> func)
1246 | +{
1247 | +    return [name, func]() {
1248 | +        util::ThreadRename(name.c_str());

laanwj commented at 9:38 AM on October 30, 2019:

unfortunately, even though ThreadRename takes std::string, I can't get rid of this c_str()

…/bitcoin/src/util/system.cpp:1248:28: error: binding reference of type 'basic_string<...>' to value of type 'const basic_string<...>' drops 'const' qualifier
        util::ThreadRename(name);
                           ^~~~
…/bitcoin/src/util/threadnames.h:15:32: note: passing argument to parameter here
void ThreadRename(std::string&&);
                               ^

Anyone have ideas what would be a better way here?

laanwj commented at 9:48 AM on October 30, 2019:

Thanks to @elichai I found a better solution here.

DrahtBot commented at 1:04 PM on October 30, 2019: 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:
- #15502 (p2p: Speed up initial connection to p2p network by ajtowns)
- #14501 (Fix possible data race when committing block files by luke-jr)
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.
promag commented at 11:14 PM on November 3, 2019: member

Concept ACK, you can squash those fixups.
sipa commented at 11:44 PM on November 3, 2019: member

Concept ACK

refactor: Improve TraceThread usability

Simplify and modernize this function and its usage.

- Improve comment.
- Use `std::function` instead of a template.
- Make the function return a closure.
- Replace `std::bind` usage with C++11 lambdas.
- Rename to `TracedThread` to prevent accidentally using it in the old way.
- Move `TracedThread` to implementation file.
- Make `TracedThread` take std::string instead of legacy C string: This
  is safer in the case of dynamic names (such as in the case of Index),
  because the closure owns the string. No potentially dangling `char*`.

8499331abf

init: Trace "loadblk" thread

It's useful to know what the lifespan of this thread is.

007a608de2

refactor: Make `PrintExceptionContinue` take std::string

The argument is used by our own logging subsystem. There's no reason to
be passing a legacy C string here.

e119b15081

laanwj force-pushed on Nov 4, 2019
laanwj commented at 10:10 AM on December 5, 2019: member

I don't really feel comfortable making this change.
laanwj closed this on Dec 5, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Improve tracethread usability and trace loadblk thread #17312

Conflicts