dkg: strengthen logic for steps update #2797
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2797 +/- ##
==========================================
- Coverage 53.27% 53.22% -0.06%
==========================================
Files 199 199
Lines 27616 27629 +13
==========================================
- Hits 14713 14706 -7
- Misses 11081 11097 +16
- Partials 1822 1826 +4 ☔ View full report in Codecov by Sentry. |
| // Sleep for 100ms to let clients connect with each other. | ||
| time.Sleep(time.Millisecond * 100) | ||
| // Sleep for 250ms to let clients connect with each other. | ||
| // Must be at least two times greater than the sync messages period specified in client.go NewClient(). |
There was a problem hiding this comment.
can you please point me to the code where this is specified?
There was a problem hiding this comment.
Line 41 in d2748d5
| @@ -53,7 +53,7 @@ func NewClient(tcpNode host.Host, peer peer.ID, hashSig []byte, version version. | |||
| // supports reestablishing on relay circuit recycling, and supports soft shutdown. | |||
| type Client struct { | |||
| // Mutable state | |||
| mu sync.Mutex | |||
| mu sync.RWMutex | |||
There was a problem hiding this comment.
why do we need a read-write mutex here?
There was a problem hiding this comment.
Because in many places we need just read. Using RWMutex optimizes performance (less locks and waits) and is widely adopted in golang applications. However, I found that Charon often uses normal Mutex, and it is unclear why. I would wish we eventually replace everything to RWMutex where appropriate.
There was a problem hiding this comment.
@pinebit Absolutely agree that RWMutexes are more optimized. But wondering if we necessarily require this. In charon, we have tried to refrain from doing "premature optimizations" wherever possible, hence you would see more mutexes used than rw-mutexes.
By what magnitude do you believe using RWMutex can optimize the performance here?
Else, we can discuss this async and create a ticket to refactor mutexes in the codebase to use RWMutex in a separate PR
There was a problem hiding this comment.
This is more about training muscle memory and develop good habits to use the appropriate API. Not necessary to optimize the performance in this particular case. Also, this improves readability - you show an intention. For example if I see RLock(), I can quickly realize this function should not modify the state and then I will think twice when introducing a change.
I do not suggest to go and rewrite the entire codebase to use RWLock right away, we can do this whenever we make a change in an affect file, for example, like adding/changing a getter or setter.
There was a problem hiding this comment.
For example if I see RLock(), I can quickly realize this function should not modify the state and then I will think twice when introducing a change.
Yes precisely, this removes congnitive load!
Co-authored-by: Abhishek Kumar <43061995+xenowits@users.noreply.github.com>
Co-authored-by: Abhishek Kumar <43061995+xenowits@users.noreply.github.com>
Co-authored-by: Abhishek Kumar <43061995+xenowits@users.noreply.github.com>
|
pinebit
added
the
merge when ready
Improved logic for steps update in DKG process. category: misc ticket: none
Improved logic for steps update in DKG process.
category: misc
ticket: none