Use atomics instead of synchronized for Gauge · Pull Request #482 · prometheus/client_java

ghost · 2019-06-05T10:52:59Z

Idea:

We could implement Gauge#set with initializing a new DoubleAdder to avoid exposing invalid state, but that might have significant allocation (GC) overhead.

Pros:

avoids thread contention for highly concurrent workloads (get, set)
reduces constant overhead (get, set)

Cons:

introduces allocations (set and inc / dec)
increases GC overhead for high throughput workloads (set and inc / dec)

Relates to #480

ghost · 2019-06-05T10:57:24Z

I ran multiple benchmarks with 24 threads and picked the result with the smallest error, but of course I can’t know how it behaves under different workloads.

Benchmark:

# JMH version: 1.21
# VM version: JDK 11.0.2, OpenJDK 64-Bit Server VM, 11.0.2+9
# VM invoker: /Library/Java/JavaVirtualMachines/openjdk-11.0.2.jdk/Contents/Home/bin/java
# VM options: -javaagent:/Applications/IntelliJ IDEA CE.app/Contents/lib/idea_rt.jar=49590:/Applications/IntelliJ IDEA CE.app/Contents/bin -Dfile.encoding=UTF-8
# Warmup: 5 iterations, 10 s each
# Measurement: 4 iterations, 10 s each
# Timeout: 10 min per iteration
# Threads: 24 threads, will synchronize iterations

Before:

Benchmark                                                 Mode  Cnt     Score     Error  Units
GaugeBenchmark.prometheusSimpleGaugeChildDecBenchmark     avgt    4    31.720 ±   1.850  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildIncBenchmark     avgt    4    33.041 ±   0.608  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildSetBenchmark     avgt    4  1441.987 ±  29.700  ns/op
GaugeBenchmark.prometheusSimpleGaugeDecBenchmark          avgt    4   154.518 ±  17.714  ns/op
GaugeBenchmark.prometheusSimpleGaugeIncBenchmark          avgt    4   155.453 ±   7.384  ns/op
GaugeBenchmark.prometheusSimpleGaugeSetBenchmark          avgt    4  3047.817 ± 168.989  ns/op

After:

Benchmark                                                 Mode  Cnt    Score    Error  Units
GaugeBenchmark.prometheusSimpleGaugeChildDecBenchmark     avgt    4   30.618 ±  0.867  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildIncBenchmark     avgt    4   31.543 ±  5.164  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildSetBenchmark     avgt    4  314.646 ±  4.892  ns/op
GaugeBenchmark.prometheusSimpleGaugeDecBenchmark          avgt    4  150.270 ±  9.382  ns/op
GaugeBenchmark.prometheusSimpleGaugeIncBenchmark          avgt    4  156.412 ±  9.367  ns/op
GaugeBenchmark.prometheusSimpleGaugeSetBenchmark          avgt    4  537.230 ± 67.680  ns/op

brian-brazil · 2019-06-05T12:51:07Z

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.

ghost · 2019-06-05T14:11:54Z

I’m just thinking out loudly:
We could introduce DoubleAdder#set as an atomic alternative to reset and add.
I'm not sure how to update base and cells atomically without locking using busy.

simpleclient/src/main/java/io/prometheus/client/Gauge.java

ghost · 2019-06-07T15:11:33Z

81283a20:

Benchmark                                                 Mode  Cnt    Score    Error  Units
GaugeBenchmark.codahaleCounterDecBenchmark                avgt    4   35.166 ± 16.052  ns/op
GaugeBenchmark.codahaleCounterIncBenchmark                avgt    4   31.423 ± 10.979  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildDecBenchmark     avgt    4   38.966 ±  3.249  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildIncBenchmark     avgt    4   37.798 ±  9.753  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildSetBenchmark     avgt    4  429.808 ± 92.764  ns/op
GaugeBenchmark.prometheusSimpleGaugeDecBenchmark          avgt    4  182.829 ± 38.544  ns/op
GaugeBenchmark.prometheusSimpleGaugeIncBenchmark          avgt    4  184.254 ± 43.754  ns/op
GaugeBenchmark.prometheusSimpleGaugeSetBenchmark          avgt    4  582.121 ± 86.330  ns/op
GaugeBenchmark.prometheusSimpleGaugeNoLabelsDecBenchmark  avgt    4   37.136 ±  6.599  ns/op
GaugeBenchmark.prometheusSimpleGaugeNoLabelsIncBenchmark  avgt    4   39.167 ±  3.439  ns/op
GaugeBenchmark.prometheusSimpleGaugeNoLabelsSetBenchmark  avgt    4  456.339 ± 61.951  ns/op

brian-brazil

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/Gauge.java

simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java

brian-brazil

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

ghost · 2019-06-13T11:16:42Z

simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java

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.

ghost · 2019-06-13T11:20:03Z

2874c5ee:

Benchmark                                                 Mode  Cnt    Score    Error  Units
GaugeBenchmark.codahaleCounterDecBenchmark                avgt    4   35.740 ± 20.478  ns/op
GaugeBenchmark.codahaleCounterIncBenchmark                avgt    4   32.936 ±  1.891  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildDecBenchmark     avgt    4   36.128 ±  3.627  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildIncBenchmark     avgt    4   38.462 ±  1.829  ns/op
GaugeBenchmark.prometheusSimpleGaugeChildSetBenchmark     avgt    4  444.700 ± 97.494  ns/op
GaugeBenchmark.prometheusSimpleGaugeDecBenchmark          avgt    4  163.684 ±  4.272  ns/op
GaugeBenchmark.prometheusSimpleGaugeIncBenchmark          avgt    4  161.102 ± 12.008  ns/op
GaugeBenchmark.prometheusSimpleGaugeSetBenchmark          avgt    4  492.110 ± 50.022  ns/op
GaugeBenchmark.prometheusSimpleGaugeNoLabelsDecBenchmark  avgt    4   37.190 ± 12.319  ns/op
GaugeBenchmark.prometheusSimpleGaugeNoLabelsIncBenchmark  avgt    4   35.756 ±  5.226  ns/op
GaugeBenchmark.prometheusSimpleGaugeNoLabelsSetBenchmark  avgt    4  471.402 ± 92.968  ns/op

Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>

ghost · 2019-07-31T16:26:35Z

I rebased the changes on master.
I'll share load testing and profiling results soon™️.

ghost · 2019-08-15T15:45:09Z

I couldn't find Gauge in YourKit snapshots and screenshots from July. 🙁
I'll need to profile the changes again, so I'll try to prove that it eliminated blocking.

ghost · 2019-09-05T09:44:37Z

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?

brian-brazil · 2019-09-05T13:42:17Z

Thanks!

ghost · 2019-09-13T13:31:13Z

Thank you. 🙂

ghost mentioned this pull request Jun 6, 2019

HdrSummary based on Summary and HdrHistogram #484

Closed

ghost commented Jun 7, 2019

View reviewed changes

simpleclient/src/main/java/io/prometheus/client/Gauge.java Outdated Show resolved Hide resolved

brian-brazil reviewed Jun 10, 2019

View reviewed changes

simpleclient/src/main/java/io/prometheus/client/Gauge.java Outdated Show resolved Hide resolved

simpleclient/src/main/java/io/prometheus/client/Gauge.java Outdated Show resolved Hide resolved

simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java Outdated Show resolved Hide resolved

brian-brazil reviewed Jun 12, 2019

View reviewed changes

simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java Outdated Show resolved Hide resolved

ghost commented Jun 13, 2019

View reviewed changes

ghost mentioned this pull request Jun 18, 2019

Thread contention in TimeWindowQuantiles because of synchronized #480

Closed

Rudolf Rakos added 5 commits July 31, 2019 17:41

Use atomic update instead of synchronized for Gauge

6b9b4db

Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>

Introduce DoubleAdder#set to avoid allocations in Gauge#set

71ea00a

Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>

Avoid unnecessary allocation in DoubleAdder#set [code review]

8c99e9c

Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>

Avoid invalid result in DoubleAdder#sum [code review]

a54c15e

Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>

Simplify code by deallocating cells on DoubleAdder#set [code review]

368d5e5

Signed-off-by: Rudolf Rakos <rrakos@evolutiongaming.com>

brian-brazil merged commit d7825cf into prometheus:master Sep 5, 2019

ghost deleted the gauge-atomic branch October 16, 2019 13:59

Conversation

ghost commented Jun 5, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 5, 2019

Uh oh!

brian-brazil commented Jun 5, 2019

Uh oh!

ghost commented Jun 5, 2019

Uh oh!

Uh oh!

ghost commented Jun 7, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-brazil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brian-brazil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 13, 2019

Uh oh!

ghost commented Jul 31, 2019

Uh oh!

ghost commented Aug 15, 2019

Uh oh!

ghost commented Sep 5, 2019

Uh oh!

brian-brazil commented Sep 5, 2019

Uh oh!

ghost commented Sep 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ghost commented Jun 5, 2019 •

edited by ghost

Loading

ghost commented Jun 7, 2019 •

edited by ghost

Loading