refactor: Use std::string for thread and index names #25971

stickies-v · 2022-09-01T11:26:58Z

As a follow-up to #25967 (comment), this PR changes the return type of BaseIndex::GetName() to const std::string& instead of const char*. The first commit is not essential for this change, but since the code is touched and index names are commonly used to specify thread names, I've made the same update there.

No behaviour change, just refactoring to further phase out C-style strings.

Note: util::ThreadRename() used to take an rvalue ref, but since it then passes this to SetInternalName() by value, I don't think there's any benefit to having both an rvalue and lvalue ref function so I just changed it into lvalue ref. Not 100% sure I'm missing something?

src/index/base.h

src/index/coinstatsindex.h

theStack · 2022-09-01T12:11:19Z

Concept ACK

brunoerg · 2022-09-01T13:00:55Z

Concept ACK

sipa · 2022-09-01T13:43:44Z

What about:

Adding an const std::string m_name; field to the base class.
Require a name to be passed in to the base class constructor (invoked from the child class constructors).
Adding a non-virtual accessor const std::string& GetName() const { return m_name; } to return that name.
Remove the corresponding m_name / GetName from the BlockFilterIndex class.
Make all child classes pass a name to the parent class constructor.

stickies-v · 2022-09-01T17:00:00Z

Force pushed to address review comments about simplifying the GetName() logic and using string_view instead of const std::string&

src/util/thread.cpp

DrahtBot · 2022-09-01T21:35:06Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25193 (indexes: Read the locator's top block during init, allow interaction with reindex-chainstate by mzumsande)

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/index/blockfilterindex.h

hebasto · 2022-09-02T09:13:03Z

Concept ACK.

stickies-v

Force pushed to address review feedback regarding:

util::TraceThread() to take a std::string copy and avoid potential issues with lifetime introduced by the threading, also updated util::ThreadRename() to take a const lvalue ref to avoid copying twice
remove unused includes leftover from earlier versions

src/index/blockfilterindex.h

src/util/thread.cpp

stickies-v · 2022-09-02T16:22:21Z

Force pushed to partly reverse the previous force push based on sipa's suggestion, again using a string_view for util::TraceThread() and undoing the changes to util::ThreadRename() (now no longer in this PR's diff).

My apologies to reviewers for the back and forth.

stickies-v · 2022-09-12T22:19:03Z

Force pushed to rebase on latest master to fix failing CI (wallet_groups.py) - no other changes.

maflcko · 2022-09-13T08:12:56Z

My preference would still be to use std::string in TraceThread (#25971 (comment)) as a belt-and-suspenders, but the current code is not worse than it was previously.

review ACK e5d9c6f 🎯

Show signature

Signature:

stickies-v · 2022-09-13T18:14:54Z

Force pushed to fix merge conflict from #24513 that renamed CChainstate -> Chainstate - no other changes.

My preference would still be to use std::string in TraceThread (#25971 (comment)) as a belt-and-suspenders, but the current code is not worse than it was previously.

I'm not opposed to it, but as I don't have a strong view on it, we've already had some back-and-forth and the current implementation doesn't introduce new assumptions there I'd prefer to keep that for a follow-up (which I won't do).

maflcko · 2022-09-13T18:23:20Z

re-ACK 200d84d only change is rebase 🚼

Show signature

Signature:

w0xlt

ACK 200d84d

src/util/system.cpp

Since it is now a string_view instead of a const char*, update the name to reflect that the variable is no longer a "Pointer to String, Zero-terminated" (psz). -BEGIN VERIFY SCRIPT- sed -i s/pszThread/thread_name/ $(git grep -l pszThread src) -END VERIFY SCRIPT-

maflcko · 2022-09-14T10:20:37Z

review ACK 26cf9ea only change is new scripted-diff 😀

Show signature

Signature:

hebasto

ACK 26cf9ea, I have reviewed the code and it looks OK.

src/util/thread.cpp

hebasto · 2022-09-15T19:47:52Z

src/util/thread.cpp

@@ -10,10 +10,12 @@

 #include <exception>
 #include <functional>
+#include <string>
+#include <utility>


nit: Why #include <utility>?

std::move (Edit: Oh yeah. That one isn't used.)

$ git grep move -- src//util/thread.cpp | wc -l 0

iwyu:

util/thread.h should add these lines: #include <string_view> // for string_view util/thread.h should remove these lines: - #include <string> // lines 9-9 The full include-list for util/thread.h: #include <functional> // for function #include <string_view> // for string_view --- util/thread.cpp should add these lines: util/thread.cpp should remove these lines: - #include <utility> // lines 14-14 The full include-list for util/thread.cpp: #include <util/thread.h> #include <logging.h> // for LogPrintf_, LogPrintf #include <util/system.h> // for PrintExceptionContinue #include <util/threadnames.h> // for ThreadRename #include <exception> // for exception #include <functional> // for function #include <string> // for allocator, string ---

w0xlt

reACK 26cf9ea

…ames

stickies-v mentioned this pull request Sep 1, 2022

refactor: add LIFETIMEBOUND to blockfilter where needed #25967

Merged

fanquake added the Refactoring label Sep 1, 2022

jonatack reviewed Sep 1, 2022

View reviewed changes

src/index/base.h Outdated Show resolved Hide resolved

jonatack reviewed Sep 1, 2022

View reviewed changes

src/index/coinstatsindex.h Outdated Show resolved Hide resolved

stickies-v force-pushed the baseindex-getname-string branch from 22eaf5b to 4f7a2b6 Compare September 1, 2022 12:05

stickies-v force-pushed the baseindex-getname-string branch from 4f7a2b6 to a6927e8 Compare September 1, 2022 16:48

maflcko reviewed Sep 1, 2022

View reviewed changes

src/util/thread.cpp Show resolved Hide resolved

This was referenced Sep 2, 2022

indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

Merged

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

davidgumberg reviewed Sep 2, 2022

View reviewed changes

src/index/blockfilterindex.h Outdated Show resolved Hide resolved

stickies-v force-pushed the baseindex-getname-string branch from a6927e8 to 4b1a0bb Compare September 2, 2022 13:27

stickies-v commented Sep 2, 2022

View reviewed changes

src/index/blockfilterindex.h Outdated Show resolved Hide resolved

src/util/thread.cpp Show resolved Hide resolved

bitcoin deleted a comment from Ellajoke Sep 2, 2022

bitcoin deleted a comment Sep 2, 2022

stickies-v force-pushed the baseindex-getname-string branch from 4b1a0bb to 296b831 Compare September 2, 2022 16:17

DrahtBot mentioned this pull request Sep 8, 2022

CChainState -> Chainstate #24513

Merged

stickies-v force-pushed the baseindex-getname-string branch from 296b831 to e5d9c6f Compare September 12, 2022 22:17

maflcko approved these changes Sep 13, 2022

View reviewed changes

DrahtBot added the Needs rebase label Sep 13, 2022

stickies-v added 2 commits September 13, 2022 19:07

refactor: use std::string for thread names

97f5b20

refactor: use std::string for index names

200d84d

stickies-v force-pushed the baseindex-getname-string branch from e5d9c6f to 200d84d Compare September 13, 2022 18:10

DrahtBot removed the Needs rebase label Sep 13, 2022

w0xlt approved these changes Sep 13, 2022

View reviewed changes

sipa reviewed Sep 14, 2022

View reviewed changes

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

hebasto approved these changes Sep 15, 2022

View reviewed changes

w0xlt approved these changes Sep 15, 2022

View reviewed changes

maflcko merged commit 5eb9781 into bitcoin:master Sep 16, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 20, 2022

Merge bitcoin#25971: refactor: Use std::string for thread and index n…

e32da13

…ames

stickies-v deleted the baseindex-getname-string branch September 22, 2022 13:08

bitcoin locked and limited conversation to collaborators Sep 22, 2023

refactor: Use std::string for thread and index names #25971

refactor: Use std::string for thread and index names #25971

Uh oh!

Conversation

stickies-v commented Sep 1, 2022

Uh oh!

Uh oh!

Uh oh!

theStack commented Sep 1, 2022

Uh oh!

brunoerg commented Sep 1, 2022

Uh oh!

sipa commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v commented Sep 1, 2022

Uh oh!

Uh oh!

DrahtBot commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

hebasto commented Sep 2, 2022

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stickies-v commented Sep 2, 2022

Uh oh!

stickies-v commented Sep 12, 2022

Uh oh!

maflcko commented Sep 13, 2022

Uh oh!

stickies-v commented Sep 13, 2022

Uh oh!

maflcko commented Sep 13, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Sep 14, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sipa commented Sep 1, 2022 •

edited

Loading

DrahtBot commented Sep 1, 2022 •

edited

Loading

hebasto Sep 15, 2022 •

edited

Loading

maflcko Sep 15, 2022 •

edited

Loading