sync: simplify and remove unused code from sync.h #25676

vasild commented at 1:06 PM on July 22, 2022: contributor

Summary:

Reduce 4 of the MaybeCheckNotHeld() definitions to 2 by using a template.
Remove unused template parameter from ::UniqueLock.
Use MutexType instead of Mutex for a template parameter name to avoid overlap/confusion with the Mutex class.
Rename AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock to avoid overlap/confusion with the global UniqueLock and for consistency with UniqueLock::reverse_lock.

The first commit sync: simplify MaybeCheckNotHeld() definitions by using a template is also part of https://github.com/bitcoin/bitcoin/pull/25390

DrahtBot commented at 1:08 AM on July 23, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25390 (sync: introduce a thread-safe smart container and use it to remove a bunch of "GlobalMutex"es by vasild)

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.

DrahtBot cross-referenced this on Jul 23, 2022 from issue sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild

DrahtBot cross-referenced this on Jul 23, 2022 from issue tracing: lock contention analysis by martinus

DrahtBot cross-referenced this on Jul 23, 2022 from issue Rework logging timer by maflcko

fanquake requested review from ajtowns on Jul 25, 2022

in src/sync.h:155 in 332cfac809 outdated

 152 | -class SCOPED_LOCKABLE UniqueLock : public Base
 153 | +template <typename MutexType>
 154 | +class SCOPED_LOCKABLE DebugLock : public MutexType::UniqueLock
 155 |  {
 156 |  private:
 157 | +    using PARENT = typename MutexType::UniqueLock;

maflcko commented at 9:13 AM on August 1, 2022:

I don't think we need to unify the names. Base seems fine to keep as is

vasild commented at 1:36 PM on August 10, 2022:

Changed to Base.

vasild commented at 1:36 PM on August 10, 2022: contributor

332cfac809...19fce0321b: address suggestion

vasild force-pushed on Aug 10, 2022

DrahtBot cross-referenced this on Aug 13, 2022 from issue refactor: Remove trailing semicolon from LOCK2 macro by aureleoules

DrahtBot added the label Needs rebase on Sep 14, 2022

sync: simplify MaybeCheckNotHeld() definitions by using a template

Reduce 4 of the `MaybeCheckNotHeld()` definitions to 2 by using a
template. This also makes the function usable for other
[BasicLockable](https://en.cppreference.com/w/cpp/named_req/BasicLockable)
types.

11c190e3f1

sync: remove unused template parameter from ::UniqueLock

The template parameter `typename Base = typename Mutex::UniqueLock` is
not used, so remove it. Use internally defined type `Base` to avoid
repetitions of `Mutex::UniqueLock`.

9d7ae4b66c

vasild force-pushed on Sep 14, 2022

vasild commented at 12:23 PM on September 14, 2022: contributor

19fce0321b...f5b529369c: rebase due to conflict

DrahtBot removed the label Needs rebase on Sep 14, 2022

in src/sync.h:150 in 757d02ad88 outdated

 147 | @@ -148,11 +148,11 @@ inline void AssertLockNotHeldInline(const char* name, const char* file, int line
 148 |  #define AssertLockNotHeld(cs) AssertLockNotHeldInline(#cs, __FILE__, __LINE__, &cs)
 149 |  
 150 |  /** Wrapper around std::unique_lock style lock for Mutex. */

ryanofsky commented at 6:38 PM on October 6, 2022:

In commit "sync: avoid confusing name overlap (Mutex)" (757d02ad88716924d7ca6935b1351b8d4d804420)

Should s/Mutex/MutexType/ this line as well

vasild commented at 7:22 AM on October 10, 2022:

Done.

ryanofsky approved

ryanofsky commented at 7:07 PM on October 6, 2022: contributor

Code review ACK f5b529369c8e5dee655ed559fd9583947188b18c. This makes nice simplifications and gets rid of some confusing shadowing, but I would suggest an alternative to the last commit.

I don't think the last commit "sync: rename the global UniqueLock to DebugLock" (f5b529369c8e5dee655ed559fd9583947188b18c) is good because it gets rid of the naming consistency between standard c++ thread primitives and our thread primitives. The standard c++ mutex class is called std::mutex while ours is called Mutex. The standard c++ recursive mutex class is called std::recursive_mutex while ours is called RecursiveMutex. Than standard c++ lock class is called std::unique_lock, so I think ours should be called UniqueLock, not DebugLock. It is true that the main features our lock class adds currently are related to debugging (though it also adds support for reverse locking), but we are not naming our custom classes after what features they add, otherwise we would be calling Mutex AnnotatedMutex etc. And I don't think it is a good idea to name our locking primitives after the particular features they add, because we should have flexibility to add and remove features from our locking primitives in the future.

Another benefit of dropping the last commit is that it would confine changes in this PR to sync.h and not change outside code in src/node/interfaces.cpp.

I do think one benefit of the last commit is that disambiguates global UniqueLock from AnnotatedMixin::UniqueLock. But I would fix this by renaming AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock. I think AnnotatedMixin::unique_lock name makes sense because it is a typedef for std::unique_lock, and would also be consistent with current UniqueLock::reverse_lock.

vasild force-pushed on Oct 10, 2022

sync: avoid confusing name overlap (Mutex)

Use `MutexType` instead of `Mutex` for the template parameter of
`UniqueLock` because there is already a class named `Mutex` and the
naming overlap is confusing. `MutexType` is used elsewhere in `sync.h`.

4b2e16763f

sync: remove DebugLock alias template

Use `UniqueLock` directly. Type deduction works just fine from the first
argument to the constructor of `UniqueLock`, so there is no need to
repeat

```cpp
UniqueLock<typename std::remove_reference<typename std::remove_pointer<decltype(cs)>::type>::type>
```

five times in the `LOCK` macros. Just `UniqueLock` suffices.

8d9ee8efe8

sync: rename AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock

This avoids confusion with the global `UniqueLock` and the snake case
is consistent with `UniqueLock::reverse_lock.

75c3f9f880

vasild force-pushed on Oct 10, 2022

vasild commented at 7:23 AM on October 10, 2022: contributor

f5b529369c...75c3f9f880: @ryanofsky, done!

fanquake commented at 9:12 AM on October 10, 2022: member

done! @vasild looks like you now also need to update the PR description?

vasild commented at 9:31 AM on October 10, 2022: contributor

@fanquake, updated, thanks!

aureleoules approved

aureleoules commented at 9:43 AM on October 10, 2022: member

ACK 75c3f9f8806259ac7ac02e725d2f2f48e5a1d954 - LGTM I verified all commits compile and unit tests pass.

ryanofsky approved

ryanofsky commented at 7:49 PM on October 10, 2022: contributor

Code review ACK 75c3f9f8806259ac7ac02e725d2f2f48e5a1d954. Nice cleanups! Just suggested changes since last review: keeping UniqueLock name and fixing a missed rename in a code comment

fanquake merged this on Oct 11, 2022

fanquake closed this on Oct 11, 2022

vasild deleted the branch on Oct 11, 2022

ajtowns commented at 9:05 AM on October 11, 2022: contributor

Post-merge ACK 75c3f9f8806259ac7ac02e725d2f2f48e5a1d954

Nice cleanup, especially with the final simplifications

jonatack commented at 4:03 PM on October 11, 2022: contributor

Post-merge ACK 75c3f9f8806259ac7ac02e725d2f2f48e5a1d954

hebasto commented at 11:31 AM on October 12, 2022: member

Post-merge ACK 75c3f9f8806259ac7ac02e725d2f2f48e5a1d954, I have reviewed the code and it looks OK.

sidhujag referenced this in commit 5984166da6 on Oct 23, 2022

bitcoin locked this on Oct 12, 2023