Fix write path lock contention #6168
Conversation
|
Good profiling work here. We'll be fine for now without those stats. Choosing those speedups over the stats is a no-brainer. |
| @@ -1311,6 +1291,7 @@ func (a Measurements) union(other Measurements) Measurements { | |||
|
|
|||
| // Series belong to a Measurement and represent unique time series in a database | |||
| type Series struct { | |||
| mu sync.RWMutex | |||
There was a problem hiding this comment.
Can you move shardIDs to under mu since it's now being protected by mu?
There was a problem hiding this comment.
mu is protecting everything right now.
There was a problem hiding this comment.
Ah the space between Tags and id threw me. Can you close that up if mu is protecting everything?
|
Aside from moving location of |
| @@ -1360,11 +1349,16 @@ func (s *Series) UnmarshalBinary(buf []byte) error { | |||
|
|
|||
| // InitializeShards initializes the list of shards. | |||
| func (s *Series) InitializeShards() { | |||
| s.mu.Lock() | |||
| defer s.mu.Unlock() | |||
There was a problem hiding this comment.
This doesn't need to be deferred, and can just be called directly after s.shardIDs is assigned.
| @@ -1360,11 +1349,16 @@ func (s *Series) UnmarshalBinary(buf []byte) error { | |||
|
|
|||
| // InitializeShards initializes the list of shards. | |||
| func (s *Series) InitializeShards() { | |||
| s.mu.Lock() | |||
| defer s.mu.Unlock() | |||
| s.shardIDs = make(map[uint64]bool) | |||
| } | |||
|
|
|||
| // match returns true if all tags match the series' tags. | |||
| func (s *Series) match(tags map[string]string) bool { | |||
There was a problem hiding this comment.
I can't find where this function is called, and since it isn't exported, we should just remove it.
|
Needs a rebase (changelog, I'm guessing), but otherwise looks good to me 👍 |
| @@ -1329,11 +1309,16 @@ func NewSeries(key string, tags map[string]string) *Series { | |||
| } | |||
There was a problem hiding this comment.
Is there a lot of read contention on Series.shardIDs? If there's not then it would be more efficient to use a sync.Mutex.
There was a problem hiding this comment.
I'm going to leave this as a sync.RWMutex because I've had to change almost all of our uses of sync.Mutex to sync.RWMutex due to lock contention issues that appear under different workloads.
The stats setup ends up creating a lot of lock contention which signifcantly impacts write throughput when a large number of measurements are used. Fixes #6131
Required for all non-trivial PRs
This fixes two performance regressions in the write path.
cc @mark-rushakoff
Fixes #6131