-
Notifications
You must be signed in to change notification settings - Fork 773
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 TimeWindowQuantiles #483
Conversation
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
I ran multiple benchmarks with It looks like this doesn't make a significant difference. 🙁 Benchmark:
Before:
After:
Ps.: I'm still running benchmarks with more frequent rotations. |
simpleclient/src/main/java/io/prometheus/client/TimeWindowQuantiles.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
simpleclient/src/main/java/io/prometheus/client/TimeWindowQuantiles.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
|
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.
There's so much noise in the benchmark that we can't tell much.
What if you hack it to rotate on every insert, and benchmark then?
simpleclient/src/main/java/io/prometheus/client/TimeWindowQuantiles.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
simpleclient/src/main/java/io/prometheus/client/TimeWindowQuantiles.java
Outdated
Show resolved
Hide resolved
|
Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
Hmm, test failure but I don't think it's due to you. |
Benchmarks with unrealistically frequent rotations by hardcoding Before - 4e0e752 -
After - ca4d1c6 -
Before - 4e0e752 -
After - ca4d1c6 -
|
Hmm, it's showing as slower. |
I'll experiment with |
…tLinkedQueue<CKMSQuantiles> Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>
ed59f81 -
ed59f81 -
|
I'll close this Pull Request in a couple of days, as I won't be able to test and profile the changes, and #481 is significantly faster under high thread contention according to benchmarks. |
Idea:
Pros:
get
,insert
)get
,insert
)Cons:
rotate
)reduces locality (CKMSQuantiles[]
vsConcurrentLinkedQueue<CKMSQuantiles>
)Neutral:
synchronized
vsCAS & retry
)Please check the code comments for considerations about correctness.
Relates to #480 , #481