Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

histogram.Write creates invalid result when racing with observe #275

Closed
lmb opened this issue Feb 9, 2017 · 3 comments
Closed

histogram.Write creates invalid result when racing with observe #275

lmb opened this issue Feb 9, 2017 · 3 comments
Assignees
@beorn7
Labels
enhancement
Milestone
v0.9

Comments

@lmb
Copy link

lmb commented Feb 9, 2017

Seems like the library will create invalid histograms when histogram.Observe races with histogram.Write.

$ go build && ./promrace
found race
sample_count: 9590
sample_sum: 18220.999999999192
bucket: <
  cumulative_count: 0
  upper_bound: 1
>
bucket: <
  cumulative_count: 9591
  upper_bound: 2
>

Note that sample_count is lower than cumulative_count of the highest bucket.

See https://gist.github.com/lmb/3caaeb79efc6d6e0d2bdde8a4865e636 to reproduce. It's possible to fix this by changing the order of the atomic adds, but that will then overstate the +Inf bucket by one in some cases.
@beorn7 beorn7 self-assigned this Feb 9, 2017
@beorn7
Copy link
Member

beorn7 commented Feb 9, 2017

Yeah, this is kind of by design. If we wanted to update all three, sample_count, sample_sum, and the respective bucket, we needed way more sophisticated locking. Which might be costly in terms of performance.

By incrementing the sample_count first, as you considered, we would at least make sure no bucket has a higher count than sample_count. Results look more consistent then, but are still incorrect.

I have some vague ideas with buffer swaps that could get proper atomicity of observation without excessive locking cost... will continue to think about it.

@beorn7 beorn7 added this to the v0.9 milestone Feb 9, 2017
@lmb
Copy link
Author

lmb commented Feb 22, 2017

Sorry for not replying earlier. To me it seems like changing the behaviour would be desirable, since then we at least generate histograms which are valid (but still not correct).

@beorn7
Copy link
Member

beorn7 commented Feb 22, 2017

Yes, this is on the 0.9 milestone.

@beorn7 beorn7 mentioned this issue Nov 9, 2017
Closed
beorn7 pushed a commit that referenced this issue Sep 7, 2018
Add test to expose #275
1e08f78 
Signed-off-by: beorn7 <beorn@soundcloud.com>
beorn7 pushed a commit that referenced this issue Sep 7, 2018
Make Histogram observations atomic
050e30a 
Fixes #275

Benchmarks haven't changed significantly, despite the additional step
to retrieve the hot counts during Observe, and the more involved
Write. (Write is a bit slower now, but note that Write is supposed to
be a relatively rare operation and thus not in the hot path compared
to Observe.) Allocts haven't changed at all.

OLD:

BenchmarkHistogramWithLabelValues-4     10000000               151 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramNoLabels-4            50000000                36.0 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve1-4            50000000                28.1 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve2-4            10000000               160 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve4-4             5000000               378 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve8-4             2000000               768 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramWrite1-4               1000000              1589 ns/op             896 B/op         37 allocs/op
BenchmarkHistogramWrite2-4                500000              2973 ns/op            1792 B/op         74 allocs/op
BenchmarkHistogramWrite4-4                300000              6979 ns/op            3584 B/op        148 allocs/op
BenchmarkHistogramWrite8-4                100000             10701 ns/op            7168 B/op        296 allocs/op

NEW:

BenchmarkHistogramWithLabelValues-4     10000000               152 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramNoLabels-4            30000000                39.2 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve1-4            50000000                29.4 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve2-4            10000000               163 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve4-4             5000000               347 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve8-4             2000000               684 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramWrite1-4               1000000              1807 ns/op             896 B/op         37 allocs/op
BenchmarkHistogramWrite2-4                500000              3321 ns/op            1792 B/op         74 allocs/op
BenchmarkHistogramWrite4-4                200000              7087 ns/op            3584 B/op        148 allocs/op
BenchmarkHistogramWrite8-4                100000             15366 ns/op            7168 B/op        296 allocs/op

Signed-off-by: beorn7 <beorn@soundcloud.com>
beorn7 pushed a commit that referenced this issue Sep 8, 2018
Make Histogram observations atomic while keeping them lock-free
eea820f 
Fixes #275

This is rather tricky and required some studying of the Go memory
model. I have added copious code comments to explain what's going on.

Benchmarks haven't changed significantly, despite the additional
atomic operations now required during Observe. Write performance is
noticable, but it is also much more involved now and has a mutex. (But
note that Write is supposed to be a relatively rare operation and thus
not in the hot path compared to Observe.) Allocs haven't changed at
all.

OLD:

BenchmarkHistogramWithLabelValues-4     10000000               151 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramNoLabels-4            50000000                36.0 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve1-4            50000000                28.1 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve2-4            10000000               160 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve4-4             5000000               378 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve8-4             2000000               768 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramWrite1-4               1000000              1589 ns/op             896 B/op         37 allocs/op
BenchmarkHistogramWrite2-4                500000              2973 ns/op            1792 B/op         74 allocs/op
BenchmarkHistogramWrite4-4                300000              6979 ns/op            3584 B/op        148 allocs/op
BenchmarkHistogramWrite8-4                100000             10701 ns/op            7168 B/op        296 allocs/op

NEW:

BenchmarkHistogramWithLabelValues-4     10000000               191 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramNoLabels-4            30000000                50.1 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve1-4            30000000                40.0 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve2-4            20000000                91.5 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve4-4             5000000               317 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve8-4             2000000               636 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramWrite1-4               1000000              2072 ns/op             896 B/op         37 allocs/op
BenchmarkHistogramWrite2-4                300000              3729 ns/op            1792 B/op         74 allocs/op
BenchmarkHistogramWrite4-4                200000              7847 ns/op            3584 B/op        148 allocs/op
BenchmarkHistogramWrite8-4                100000             16975 ns/op            7168 B/op        296 allocs/op

Signed-off-by: beorn7 <beorn@soundcloud.com>
beorn7 pushed a commit that referenced this issue Sep 8, 2018
Make Histogram observations atomic while keeping them lock-free
f445a20 
Fixes #275

This is rather tricky and required some studying of the Go memory
model. I have added copious code comments to explain what's going on.

Benchmarks haven't changed significantly, despite the additional
atomic operations now required during Observe. Write performance is
noticable, but it is also much more involved now and has a mutex. (But
note that Write is supposed to be a relatively rare operation and thus
not in the hot path compared to Observe.) Allocs haven't changed at
all.

OLD:

BenchmarkHistogramWithLabelValues-4     10000000               151 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramNoLabels-4            50000000                36.0 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve1-4            50000000                28.1 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve2-4            10000000               160 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve4-4             5000000               378 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve8-4             2000000               768 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramWrite1-4               1000000              1589 ns/op             896 B/op         37 allocs/op
BenchmarkHistogramWrite2-4                500000              2973 ns/op            1792 B/op         74 allocs/op
BenchmarkHistogramWrite4-4                300000              6979 ns/op            3584 B/op        148 allocs/op
BenchmarkHistogramWrite8-4                100000             10701 ns/op            7168 B/op        296 allocs/op

NEW:

BenchmarkHistogramWithLabelValues-4     10000000               191 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramNoLabels-4            30000000                50.1 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve1-4            30000000                40.0 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve2-4            20000000                91.5 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve4-4             5000000               317 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve8-4             2000000               636 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramWrite1-4               1000000              2072 ns/op             896 B/op         37 allocs/op
BenchmarkHistogramWrite2-4                300000              3729 ns/op            1792 B/op         74 allocs/op
BenchmarkHistogramWrite4-4                200000              7847 ns/op            3584 B/op        148 allocs/op
BenchmarkHistogramWrite8-4                100000             16975 ns/op            7168 B/op        296 allocs/op

Signed-off-by: beorn7 <beorn@soundcloud.com>
@beorn7 beorn7 mentioned this issue Sep 8, 2018
Merged
beorn7 pushed a commit that referenced this issue Sep 12, 2018
Make Histogram observations atomic while keeping them lock-free
1b2bd1d 
Fixes #275

This is rather tricky and required some studying of the Go memory
model. I have added copious code comments to explain what's going on.

Benchmarks haven't changed significantly, despite the additional
atomic operations now required during Observe. Write performance is
noticable, but it is also much more involved now and has a mutex. (But
note that Write is supposed to be a relatively rare operation and thus
not in the hot path compared to Observe.) Allocs haven't changed at
all.

OLD:

BenchmarkHistogramWithLabelValues-4     10000000               151 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramNoLabels-4            50000000                36.0 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve1-4            50000000                28.1 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve2-4            10000000               160 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve4-4             5000000               378 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve8-4             2000000               768 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramWrite1-4               1000000              1589 ns/op             896 B/op         37 allocs/op
BenchmarkHistogramWrite2-4                500000              2973 ns/op            1792 B/op         74 allocs/op
BenchmarkHistogramWrite4-4                300000              6979 ns/op            3584 B/op        148 allocs/op
BenchmarkHistogramWrite8-4                100000             10701 ns/op            7168 B/op        296 allocs/op

NEW:

BenchmarkHistogramWithLabelValues-4     10000000               191 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramNoLabels-4            30000000                50.1 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve1-4            30000000                40.0 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve2-4            20000000                91.5 ns/op             0 B/op          0 allocs/op
BenchmarkHistogramObserve4-4             5000000               317 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramObserve8-4             2000000               636 ns/op               0 B/op          0 allocs/op
BenchmarkHistogramWrite1-4               1000000              2072 ns/op             896 B/op         37 allocs/op
BenchmarkHistogramWrite2-4                300000              3729 ns/op            1792 B/op         74 allocs/op
BenchmarkHistogramWrite4-4                200000              7847 ns/op            3584 B/op        148 allocs/op
BenchmarkHistogramWrite8-4                100000             16975 ns/op            7168 B/op        296 allocs/op

Signed-off-by: beorn7 <beorn@soundcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@beorn7 beorn7

Labels
enhancement
Projects
None yet
Milestone
v0.9
Development

No branches or pull requests
2 participants
@lmb @beorn7