-
Notifications
You must be signed in to change notification settings - Fork 814
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
Use atomics instead of synchronized for Gauge #482
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:
|
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
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.
if (busy == 0 && casBusy()) { | ||
try { | ||
if (cells == as) { // recheck under lock | ||
// allocate new cells instead of updating existing cells |
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:
set
andinc / dec
)set
andinc / dec
)Relates to #480