Introduce replacement for getting and setting thread name: #4312

HowardHinnant · 2022-09-22T19:29:25Z

High Level Overview of Change

In namespace ripple, introduces get_name function that takes a
std::thread::native_handle_type and returns a std::string.
In namespace ripple, introduces get_name function that takes a
std::thread or std::jthread and returns a std::string.
In namespace ripple::this_thread, introduces get_name function
that takes no parameters and returns the name of the current
thread as a std::string.
In namespace ripple::this_thread, introduces set_name function
that takes a std::string_view and sets the name of the current
thread.
Intended to replace the beast utilities setCurrentThreadName
and getCurrentThreadName.

The new API for getting and setting the thread name:

rippled/src/ripple/basics/ThreadUtilities.h

Lines 38 to 46 in a955057

    
           namespace this_thread { 
        
           std::string 
        
           get_name(); 
        
           void 
        
           set_name(std::string s); 
        
           }  // namespace this_thread

Example:

auto name = ripple::this_thread::set_name("A");

The maximum length on the name is 15. There is an assert on that, and if asserts are turned off, the name is truncated to a max of 15.

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

src/ripple/basics/impl/ThreadUtilities.cpp

greg7mdp · 2022-09-22T20:20:21Z

Also the linux and mac sections are almost identical. Maybe combine them?

greg7mdp · 2022-09-23T17:18:24Z

Great, thanks for the updates @HowardHinnant! I learned some things as well :-).

src/ripple/app/main/GRPCServer.cpp

intelliot · 2023-03-22T23:00:04Z

@HowardHinnant does this change still make sense to apply? If so, could you suggest 2 potential reviewers?

HowardHinnant · 2023-03-23T02:12:34Z

Yes. I recommend @greg7mdp and @seelabs as reviewers.

greg7mdp · 2023-03-23T13:26:57Z

src/ripple/basics/ThreadUtilities.h

-/** Returns the name of the caller thread.
+template <class Thread>
+inline auto
+get_name(Thread& t) -> decltype(t.native_handle(), t.join(), std::string{})


That is so cool! Am I understanding correctly that the intent is to make the template substitution fail if Thread doesn't have native_handle() or join() members?
Reviewing your code is always fun as I learn modern best practices!

seelabs

There are two issues we'll need to resolve:

On linux, threads do not start with an empty name (see: https://man7.org/linux/man-pages/man3/pthread_setname_np.3.html). This isn't a killer, but it makes a test fail.
On linux (or at least my system) thread names are limited to 16 characters. We could just truncate the thread names, but it is unlikely anyone will notice their nice thread names were truncated until they look in a debugger - and fixing the thread names won't be a high priority if people are actively debugging other code.

I don't have a solution to offer. Thread names are usually known at compile time, so we could get "clever" and detect this at compile time, but honestly I'm not sure that kind of complexity is worth it for something like this.

At any rate, I'll let you think about solutions and I'll do so as well.

intelliot · 2023-07-18T00:14:28Z

@HowardHinnant any thoughts on the issues mentioned above?

HowardHinnant · 2023-08-17T20:09:22Z

Addressed reviewer comments and rebased to develop.

seelabs · 2023-08-25T16:56:13Z

src/ripple/basics/impl/ThreadUtilities.cpp

+void
+set_name(std::string s)
+{
+    s.resize(15);


Instead of silently truncating, what do you think about asserting the size instead? If someone renames or add a thread with too long of a name, right now it won't be caught until someone looks at it in a debugger (which can be a LONG time). Instead we could assert and it would be caught the first time rippled was run in debug mode.

Incidentally, it also save a string copy because we could pass by const& again, but honestly I don't think we care; I'm much more worried about catching the long thread name.

* In namespace ripple, introduces get_name function that takes a std::thread::native_handle_type and returns a std::string. * In namespace ripple, introduces get_name function that takes a std::thread or std::jthread and returns a std::string. * In namespace ripple::this_thread, introduces get_name function that takes no parameters and returns the name of the current thread as a std::string. * In namespace ripple::this_thread, introduces set_name function that takes a std::string_view and sets the name of the current thread. * Intended to replace the beast utilities setCurrentThreadName and getCurrentThreadName.

…Name

1. Remove test which assumes null default thread name 2. Limit names to 15 characters (16 including trailing null)

HowardHinnant · 2023-08-25T17:33:36Z

Addressed reviewer comments and rebased to develop.

seelabs

👍

* In namespace ripple, introduces get_name function that takes a std::thread::native_handle_type and returns a std::string. * In namespace ripple, introduces get_name function that takes a std::thread or std::jthread and returns a std::string. * In namespace ripple::this_thread, introduces get_name function that takes no parameters and returns the name of the current thread as a std::string. * In namespace ripple::this_thread, introduces set_name function that takes a std::string_view and sets the name of the current thread. * Intended to replace the beast utilities setCurrentThreadName and getCurrentThreadName.

intelliot · 2023-09-11T17:04:32Z

note: libxrpl should be considered part of the "API" since this change potentially breaks dependents such as Clio

intelliot · 2023-09-11T17:11:53Z

nb: this was merged after 1.12 and may be included in the next release (expected to be 1.13 or 2.0)

intelliot · 2023-09-11T22:56:01Z

From now on, we'll need to be more careful about making changes that break libxrpl users.

Test Plan

Can write C++ code to call libxrpl code. If those calls break, then libxrpl has been changed in an incompatible way. Such changes need to be documented and considered by Clio.
The code can run quickly since it just calls every interface in libxrpl.
An expansion to the test could verify that the return type from each call is the same.
A further improvement would be to consider the return values and fail the test on any changes.
@scottschurr and @HowardHinnant can assist with creating these tests.
Could be a lot of work without too much benefit. Ensure this work has a small scope (1-2 weeks of effort).
- Just do it for 10-20 function calls, then see how many function calls the library has. Maybe it's a very large API surface area (10s of 1000s).
- Many objects, and many of them have 10s of methods, all of which would need to be exercised to ensure no breaking changes.
- Could be automatable. Again, that might be a lot of work without much benefit. Also wouldn't be complete since default arguments could change.

Notes

Changes can be identified in code reviews.
Relevant breaking changes will be quickly identified by Clio.
Making this an actual library that we support is a big deal.
It's unrealistic to commit to a stable API since we'll make changes as necessary to make rippled better.

…RPLF#4312)" This reverts commit 36cb5f9.

* Revert "Remove CurrentThreadName.h from RippledCore.cmake (#4697)" This reverts commit 3b5fcd5. * Revert "Introduce replacement for getting and setting thread name: (#4312)" This reverts commit 36cb5f9.

* In namespace ripple, introduces get_name function that takes a std::thread::native_handle_type and returns a std::string. * In namespace ripple, introduces get_name function that takes a std::thread or std::jthread and returns a std::string. * In namespace ripple::this_thread, introduces get_name function that takes no parameters and returns the name of the current thread as a std::string. * In namespace ripple::this_thread, introduces set_name function that takes a std::string_view and sets the name of the current thread. * Intended to replace the beast utilities setCurrentThreadName and getCurrentThreadName.

* Revert "Remove CurrentThreadName.h from RippledCore.cmake (XRPLF#4697)" This reverts commit 3b5fcd5. * Revert "Introduce replacement for getting and setting thread name: (XRPLF#4312)" This reverts commit 36cb5f9.

* In namespace ripple, introduces get_name function that takes a std::thread::native_handle_type and returns a std::string. * In namespace ripple, introduces get_name function that takes a std::thread or std::jthread and returns a std::string. * In namespace ripple::this_thread, introduces get_name function that takes no parameters and returns the name of the current thread as a std::string. * In namespace ripple::this_thread, introduces set_name function that takes a std::string_view and sets the name of the current thread. * Intended to replace the beast utilities setCurrentThreadName and getCurrentThreadName.

* Revert "Remove CurrentThreadName.h from RippledCore.cmake (XRPLF#4697)" This reverts commit 3b5fcd5. * Revert "Introduce replacement for getting and setting thread name: (XRPLF#4312)" This reverts commit 36cb5f9.

* In namespace ripple, introduces get_name function that takes a std::thread::native_handle_type and returns a std::string. * In namespace ripple, introduces get_name function that takes a std::thread or std::jthread and returns a std::string. * In namespace ripple::this_thread, introduces get_name function that takes no parameters and returns the name of the current thread as a std::string. * In namespace ripple::this_thread, introduces set_name function that takes a std::string_view and sets the name of the current thread. * Intended to replace the beast utilities setCurrentThreadName and getCurrentThreadName.

* Revert "Remove CurrentThreadName.h from RippledCore.cmake (XRPLF#4697)" This reverts commit 3b5fcd5. * Revert "Introduce replacement for getting and setting thread name: (XRPLF#4312)" This reverts commit 36cb5f9.

greg7mdp reviewed Sep 22, 2022

View reviewed changes

greg7mdp reviewed Sep 23, 2022

View reviewed changes

src/ripple/app/main/GRPCServer.cpp Show resolved Hide resolved

greg7mdp approved these changes Sep 23, 2022

View reviewed changes

intelliot requested review from seelabs and nbougalis March 22, 2023 22:59

intelliot assigned HowardHinnant Mar 22, 2023

greg7mdp approved these changes Mar 23, 2023

View reviewed changes

seelabs requested changes Mar 23, 2023

View reviewed changes

intelliot added the Tech Debt Non-urgent improvements label Apr 27, 2023

HowardHinnant force-pushed the thread_name branch from f66ff10 to d842eea Compare August 17, 2023 20:08

HowardHinnant requested a review from seelabs August 17, 2023 20:09

intelliot assigned seelabs and unassigned HowardHinnant Aug 21, 2023

seelabs reviewed Aug 25, 2023

View reviewed changes

HowardHinnant added 7 commits August 25, 2023 13:32

Use ripple::this_thread::set_name in favor of beast::setCurrentThread…

2605fd8

…Name

Remove CurrentThreadName

e59a5c2

[FOLD] Remove use of string_view

3b3c68d

[FOLD] Remove extra call to set_name

2a9a5c8

[FOLD] Address reviewer comments:

3fc43db

1. Remove test which assumes null default thread name 2. Limit names to 15 characters (16 including trailing null)

[FOLD] Address reviewer comments

a83a2a3

HowardHinnant force-pushed the thread_name branch from d842eea to a83a2a3 Compare August 25, 2023 17:32

seelabs approved these changes Aug 25, 2023

View reviewed changes

HowardHinnant added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 25, 2023

intelliot unassigned seelabs Aug 27, 2023

intelliot added this to the 1.13 milestone Aug 27, 2023

Merge branch 'develop' into thread_name

9adf64c

intelliot merged commit 36cb5f9 into XRPLF:develop Sep 7, 2023
15 checks passed

intelliot added the API Change label Sep 11, 2023

intelliot modified the milestones: 1.13 or 2.0, 1.12 Sep 11, 2023

HowardHinnant added a commit to HowardHinnant/rippled that referenced this pull request Sep 12, 2023

Revert "Introduce replacement for getting and setting thread name: (X…

a9231cc

…RPLF#4312)" This reverts commit 36cb5f9.

ximinez added a commit to ximinez/rippled that referenced this pull request Sep 13, 2023

Revert "Introduce replacement for getting and setting thread name: (X…

b0d6efc

…RPLF#4312)" This reverts commit 36cb5f9.

ximinez added a commit to ximinez/rippled that referenced this pull request Sep 13, 2023

Revert "Introduce replacement for getting and setting thread name: (X…

5f5efea

…RPLF#4312)" This reverts commit 36cb5f9.

ximinez added a commit to ximinez/rippled that referenced this pull request Sep 14, 2023

Revert "Introduce replacement for getting and setting thread name: (X…

f7670b1

…RPLF#4312)" This reverts commit 36cb5f9.

intelliot mentioned this pull request Sep 14, 2023

Proposed 2.0.0-b1 #4705

Merged

1 task

intelliot added Reverted Changes which should still be considered for re-merging. See "Closed" PRs with this label and removed API Change labels Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce replacement for getting and setting thread name: #4312

Introduce replacement for getting and setting thread name: #4312

HowardHinnant commented Sep 22, 2022 •

edited by intelliot

Loading

greg7mdp commented Sep 22, 2022

greg7mdp commented Sep 23, 2022

intelliot commented Mar 22, 2023

HowardHinnant commented Mar 23, 2023

greg7mdp Mar 23, 2023 •

edited

Loading

seelabs left a comment

intelliot commented Jul 18, 2023

HowardHinnant commented Aug 17, 2023

seelabs Aug 25, 2023

HowardHinnant commented Aug 25, 2023

seelabs left a comment

intelliot commented Sep 11, 2023

intelliot commented Sep 11, 2023 •

edited

Loading

intelliot commented Sep 11, 2023 •

edited

Loading

	namespace this_thread {

	std::string
	get_name();

	void
	set_name(std::string s);

	} // namespace this_thread

Introduce replacement for getting and setting thread name: #4312

Introduce replacement for getting and setting thread name: #4312

Conversation

HowardHinnant commented Sep 22, 2022 • edited by intelliot Loading

High Level Overview of Change

Context of Change

Type of Change

greg7mdp commented Sep 22, 2022

greg7mdp commented Sep 23, 2022

intelliot commented Mar 22, 2023

HowardHinnant commented Mar 23, 2023

greg7mdp Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

intelliot commented Jul 18, 2023

HowardHinnant commented Aug 17, 2023

seelabs Aug 25, 2023

Choose a reason for hiding this comment

HowardHinnant commented Aug 25, 2023

seelabs left a comment

Choose a reason for hiding this comment

intelliot commented Sep 11, 2023

intelliot commented Sep 11, 2023 • edited Loading

intelliot commented Sep 11, 2023 • edited Loading

Test Plan

HowardHinnant commented Sep 22, 2022 •

edited by intelliot

Loading

greg7mdp Mar 23, 2023 •

edited

Loading

intelliot commented Sep 11, 2023 •

edited

Loading

intelliot commented Sep 11, 2023 •

edited

Loading