refactor: Replace RecursiveMutex with Mutex in timedata.cpp #19189

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:200605-timedata changing 1 files +14 −22

hebasto commented at 2:02 PM on June 6, 2020: member

Only GetTimeOffset() and AddTimeData() functions lock this mutex. They do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the g_timeoffset_mutex could be a non-recursive mutex.

Related to #19180.
refactor: Replace RecursiveMutex with Mutex in timedata.cpp c2410ceb84
DrahtBot added the label Refactoring on Jun 6, 2020
MarcoFalke commented at 10:53 AM on June 7, 2020: member

Concept ACK. (nit: Could add a new commit to clang-format the whole file while touching it anyway?)
refactor: Fix formatting of timedata.cpp cc5c0d2299
hebasto commented at 11:12 AM on June 7, 2020: member
Updated c2410ceb844a443caf6dd8c6df976b9e24724d06 -> cc5c0d2299b09c58cd9962ca5075ffa53f2633c0 (pr19189.01 -> pr19189.02, diff):
- addressed @MarcoFalke's comment:
nit: Could add a new commit to clang-format the whole file while touching it anyway?

MarcoFalke commented at 12:11 PM on June 7, 2020: member

ACK cc5c0d2299b09c58cd9962ca5075ffa53f2633c0 , checked the second commit with --word-diff-regex=. --ignore-all-space -U0 🦉

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash bfef319e8de4b4be6ce092abda9bc310590248e63368de1b85795fcaab7291cf -

</details>

in src/timedata.cpp:92 in cc5c0d2299
```
  99 |                  bool fMatch = false;
 100 | -                for (const int64_t nOffset : vSorted)
 101 | -                    if (nOffset != 0 && abs64(nOffset) < 5 * 60)
 102 | -                        fMatch = true;
 103 | +                for (const int64_t nOffset : vSorted) {
 104 | +                    if (nOffset != 0 && abs64(nOffset) < 5 * 60) fMatch = true;
```
vasild commented at 8:12 AM on June 8, 2020:

I know clang-format did it due to AllowShortIfStatementsOnASingleLine: true and so there is nothing to do in this PR, but I just want to mention that putting the if condition and body on one line like if (A) B;

makes life in gdb more difficult (if one wants to set a breakpoint on B)

makes backtraces less usefull (if it crashed on line 92, did it crash in A or in B?)

skews line based code coverage (if just A is executed and it was false then the code coverage may show the entire line 92 covered, giving the false impression that B was also executed).
hebasto commented at 8:25 AM on June 8, 2020:

Lean to agree with you. Just want to preserve ACKs :)

MarcoFalke commented at 11:08 AM on June 8, 2020:

We only allow trivial statements in single line ifs, so it should always be clear whether it crashed in A or B.

Also, coverage reports reports generally include branch coverage.

in src/timedata.cpp:19 in cc5c0d2299

  14 | @@ -15,9 +15,8 @@
  15 |  #include <util/translation.h>
  16 |  #include <warnings.h>
  17 |  
  18 | -
  19 | -static RecursiveMutex cs_nTimeOffset;
  20 | -static int64_t nTimeOffset GUARDED_BY(cs_nTimeOffset) = 0;
  21 | +static Mutex g_timeoffset_mutex;
  22 | +static int64_t nTimeOffset GUARDED_BY(g_timeoffset_mutex) = 0;

vasild commented at 8:16 AM on June 8, 2020:

nit: s/nTimeOffset/g_time_offset/, feel free to ignore. I just wonder - you changed the name of one global variable why not the other too?

hebasto commented at 8:26 AM on June 8, 2020:

I hope this commit is mutex related only :)

vasild approved
vasild commented at 8:21 AM on June 8, 2020: member

ACK cc5c0d22 verified that recursion is not happening
hebasto commented at 8:38 AM on June 8, 2020: member

@sipa Mind reviewing this PR?
MarcoFalke merged this on Jun 8, 2020
MarcoFalke closed this on Jun 8, 2020
hebasto deleted the branch on Jun 8, 2020
deadalnix referenced this in commit d579009862 on Mar 10, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19180 refactor: Replace RecursiveMutex with Mutex in Shutdown()#19303 Replace all of the RecursiveMutex instances with the Mutex ones