Name all util/mutexes #16975
Name all util/mutexes #16975
Conversation
|
This sort of came up in the spike investigation, where it was useful to have named mutexes in order to report generically when a certain mutex had a wait that exceeded a given threshold. This change doesn't actually include that kind of logging or anything but as I side effect I decide to name all mutexes and cross that TODO off the list. The entirety of the first change is just naming mutexes. The second change removes the default constructor which allows the creation of unnamed (well, named "mutex") mutexes. |
|
See also #6330 |
|
@BenPope - ah, I hadn't seen that one. Do you think I should pull in some or all of that? Aaron had chosen different names in some places and definitely went a bit further with dynamic naming, e.g., naming based on the ntp in places where the mutex is ntp-specific. |
I never got round to reviewing it, so I can't be sure. I'm happy to have you make the call, you have most context here, especially if you've taken a look at the PR I linked!. |
| @@ -315,7 +315,7 @@ class disk_log_impl final : public log { | |||
| model::offset _start_offset; | |||
| // Used to serialize updates to _start_offset. See the update_start_offset | |||
| // method. | |||
| mutex _start_offset_lock; | |||
| mutex _start_offset_lock{"disk_log_impl::start_offset_lock"}; | |||
There was a problem hiding this comment.
Is there any reasoning why some names have the class name in them while others don't?
There was a problem hiding this comment.
Well I sort of evolved my strategy over time. My overall thought was "it doesn't matter too much as these are going to be searched", but what I settled on was mostly: use class name::member name except in places where a class just had some single general mutex in which case I sometimes just used the class name. Also strip the leading _. Also remove "mutex" or "lock" for things named like foo_mutex or foo_lock.
I think most of the cases that "don't have class name" are actually cases where I only used the class name, and not the member name?
I'm fine to do anything really to get this in: if you suggest a strategy I will redo the change following that strategy.
There was a problem hiding this comment.
I also toyed with using std::source_location to keep the default constructor around and name the mutex based on say dir/file.cc:LINE but that seems mostly worse I think (if you're willing to do the work to rename them all).
There was a problem hiding this comment.
Yeah, I also immediately thought source_location, but then, there's a lot more useful info that could be added and it doesn't gain much over a name. I wouldn't block an improvement in the pursuit of the perfect bike-shed, though.
|
@travisdowns wondering if we just remove mutex by putting the only mutex in the ss:: ns forcing a ctor named param |
|
@emaxerrno wrote:
Currently there is no Does that address your feedback or did I misunderstand? |
Name all mutexes that are not already named as precursor to removing the unnamed constructor from utils/mutex (see next change for additional details and reasoning).
Certain structures like semaphore and mutex can be "named" which is useful to identify a particular structure in log messages, exceptions or just about anything you can imagine. When we added support for named mutexes, we did not give names to all existing mutexes, perhaps because there are a lot of them: the unnamed version of the constructor was left and remove it was left as a TODO. This change crosses that TODO off the list and removes the unnamed constructor (the prior change in this series named all existing mutexes that weren't already named). Issue redpanda-data#5489.
travisdowns
dismissed stale reviews from BenPope and StephanDollberg
via
7142caa
travisdowns
force-pushed
the
td-name-all-mutexes
branch
from
5e688fd
to
7142caa
Compare
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46182#018e3d2a-343b-41f5-887a-900da6acad74 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46182#018e3d3b-dcde-4acc-be38-343f6530de1a |
Certain structures like semaphore and mutex can be "named" which is
useful to identify a particular structure in log messages, exceptions
or just about anything you can imagine.
When we added support for named mutexes, we did not give names to all
existing mutexes, perhaps because there are a lot of them: the
unnamed version of the constructor was left and removing it was
left as a TODO.
This series crosses that TODO off the list and names all existing
mutexes that weren't already named and removes the unnamed
constructor.
See also #5489.
Backports Required
Release Notes