-
Notifications
You must be signed in to change notification settings - Fork 826
Use atomics instead of synchronized for Gauge #482
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.
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
Conversation
|
I ran multiple benchmarks with Benchmark: Before: After: |
|
What I was thinking about back when adding this is that we could adjust DoubleAdder to handle this more efficiently if it ever became an issue. |
|
I’m just thinking out loudly: |
|
81283a20: |
brian-brazil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this, I don't think it's correct when there's the (rare) mix of inc and set.
simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java
Outdated
Show resolved
Hide resolved
brian-brazil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, it'd be simpler to just set the values on the Cells - but we'd still want to keep the cas on busy around for the right semantics in sum.
simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, it'd be simpler to just set the values on the Cells - but we'd still want to keep the cas on busy around for the right semantics in sum.
I don't think we could ensure correctness in sum with updating existing cells in set, as it's not possible to take a snapshot of cells atomically, so there would be no way to detect the intermediate state of cells caused by concurrent updates of individual cells.
|
2874c5ee: |
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
|
I rebased the changes on |
|
I couldn't find |
|
I can confirm that there's no more blocking. I've been trying to get profiling results to have something to show that proves that, but because it's so fast now it doesn't show up during sampling using a reasonable sampling rate, that does not affect performance by itself. Do you have any further concerns about merging this? |
|
Thanks! |
|
Thank you. 🙂 |
Idea:
Pros:
get,set)get,set)Cons:
setandinc / dec)setandinc / dec)Relates to #480