Switch MinMaxSumCount to a mutex lock instead of StateLocker #667
Conversation
With multiple values being modified for each Update(), a single mutex lock and non-atomic operations ends up being faster than making each value update into an atomic operation.
evantorrie
requested review from
jmacd,
krnowak,
lizthegrey,
MrAlias,
paivagustavo and
tedsuo
as code owners
|
I've left the old |
| @@ -66,8 +68,8 @@ var ( | |||
| func TestMain(m *testing.M) { | |||
| fields := []ottest.FieldOffset{ | |||
| { | |||
| Name: "Aggregator.states", | |||
| Offset: unsafe.Offsetof(Aggregator{}.states), | |||
| Name: "Aggregator.current", | |||
There was a problem hiding this comment.
Since this is no longer using atomics, we don't strictly need to check any offsets for alignment. Shall I just remove?
There was a problem hiding this comment.
Instead of this change, you could just change Aggregator to AggregatorSL.
There was a problem hiding this comment.
Ah, wait. AggregatorSL will be dropped anyway. Then this whole TestMain function could be dropped.
| @@ -66,8 +68,8 @@ var ( | |||
| func TestMain(m *testing.M) { | |||
| fields := []ottest.FieldOffset{ | |||
| { | |||
| Name: "Aggregator.states", | |||
| Offset: unsafe.Offsetof(Aggregator{}.states), | |||
| Name: "Aggregator.current", | |||
There was a problem hiding this comment.
Instead of this change, you could just change Aggregator to AggregatorSL.
| @@ -66,8 +68,8 @@ var ( | |||
| func TestMain(m *testing.M) { | |||
| fields := []ottest.FieldOffset{ | |||
| { | |||
| Name: "Aggregator.states", | |||
| Offset: unsafe.Offsetof(Aggregator{}.states), | |||
| Name: "Aggregator.current", | |||
There was a problem hiding this comment.
Ah, wait. AggregatorSL will be dropped anyway. Then this whole TestMain function could be dropped.
|
On my somewhat underpowered machine, here are what I get for comparison between the mutex and stateLocker approach using the benchmarks in mmsc_test.go |
|
This is ready to merge. Do you want to remove the SL version before merging? I don't mind doing that in a future PR (which could also remove the StateLocker). |
Removed StateLocker and the referencing Benchmarks |
|
I believe only this remaining question needs to be answered before merging. |
Fixes open-telemetry#657 With the changes in open-telemetry#667 and open-telemetry#669 to use a plain-old-mutex for concurrent access of Histogram and MinMaxSumCount aggregators, the StateLocker implementation is no longer used in the project.
Fixes #625
Unblocks #657
As noted in #600, with multiple values being modified for each Update(), a single mutex lock and non-atomic operations ends up being faster than making each value update into an atomic operation.