From 6b9b4db6b8e8e48f65319c52384d66fa2400cb7c Mon Sep 17 00:00:00 2001 From: Rudolf Rakos Date: Tue, 4 Jun 2019 21:34:16 +0200 Subject: [PATCH 1/5] Use atomic update instead of synchronized for Gauge Signed-off-by: Rudolf Rakos --- .../main/java/io/prometheus/client/Gauge.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/simpleclient/src/main/java/io/prometheus/client/Gauge.java b/simpleclient/src/main/java/io/prometheus/client/Gauge.java index dc1b0d568..39b7a74e9 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Gauge.java +++ b/simpleclient/src/main/java/io/prometheus/client/Gauge.java @@ -134,7 +134,8 @@ public void close() { * {@link SimpleCollector#remove} or {@link SimpleCollector#clear}, */ public static class Child { - private final DoubleAdder value = new DoubleAdder(); + + private volatile DoubleAdder value = new DoubleAdder(); static TimeProvider timeProvider = new TimeProvider(); @@ -166,13 +167,13 @@ public void dec(double amt) { * Set the gauge to the given value. */ public void set(double val) { - synchronized(this) { - value.reset(); - // If get() were called here it'd see an invalid value, so use a lock. - // inc()/dec() don't need locks, as all the possible outcomes - // are still possible if set() were atomic so no new races are introduced. - value.add(val); - } + DoubleAdder updated = new DoubleAdder(); + updated.add(val); + + // On concurrent `get` and `set`, it is acceptable to `get` an outdated `value`. + // On concurrent `set` and `set`, it should be acceptable to lose one `set` measurement. + // On concurrent `set` and `inc / dec`, it might be acceptable to lose the `inc / dec` measurement. + value = updated; } /** * Set the gauge to the current unixtime. @@ -234,9 +235,7 @@ public E setToTime(Callable timeable){ * Get the value of the gauge. */ public double get() { - synchronized(this) { - return value.sum(); - } + return value.sum(); } } From 71ea00a539001db2fdb1840e28bdfbc50fd9e01c Mon Sep 17 00:00:00 2001 From: Rudolf Rakos Date: Fri, 7 Jun 2019 16:59:19 +0200 Subject: [PATCH 2/5] Introduce DoubleAdder#set to avoid allocations in Gauge#set Signed-off-by: Rudolf Rakos --- .../io/prometheus/client/DoubleAdder.java | 31 +++++++++++++++++++ .../main/java/io/prometheus/client/Gauge.java | 11 +++---- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java b/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java index cf24031ce..43925a3aa 100644 --- a/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java +++ b/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java @@ -118,6 +118,37 @@ public void reset() { internalReset(0L); } + public void set(double x) { + for (;;) { + Cell[] as; + if ((as = cells) != null) { // have cells + if (busy == 0 && casBusy()) { + try { + if (cells == as) { // recheck under lock + // allocate new cells instead of updating existing cells + int n = as.length; + Cell[] rs = new Cell[n]; + for (int i = 0; i < n; ++i) { + Cell a = as[i]; + if (a != null) // avoid unused cells + rs[i] = new Cell(0L); + } + // update cells and base (not atomic) + cells = rs; + base = Double.doubleToLongBits(x); + break; + } + } finally { + busy = 0; + } + } + } else { // no cells + base = Double.doubleToLongBits(x); // update base (atomic) + break; + } + } + } + /** * Equivalent in effect to {@link #sum} followed by {@link * #reset}. This method may apply for example during quiescent diff --git a/simpleclient/src/main/java/io/prometheus/client/Gauge.java b/simpleclient/src/main/java/io/prometheus/client/Gauge.java index 39b7a74e9..12f58462e 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Gauge.java +++ b/simpleclient/src/main/java/io/prometheus/client/Gauge.java @@ -135,7 +135,7 @@ public void close() { */ public static class Child { - private volatile DoubleAdder value = new DoubleAdder(); + private final DoubleAdder value = new DoubleAdder(); static TimeProvider timeProvider = new TimeProvider(); @@ -167,13 +167,9 @@ public void dec(double amt) { * Set the gauge to the given value. */ public void set(double val) { - DoubleAdder updated = new DoubleAdder(); - updated.add(val); - - // On concurrent `get` and `set`, it is acceptable to `get` an outdated `value`. // On concurrent `set` and `set`, it should be acceptable to lose one `set` measurement. // On concurrent `set` and `inc / dec`, it might be acceptable to lose the `inc / dec` measurement. - value = updated; + value.set(val); } /** * Set the gauge to the current unixtime. @@ -235,6 +231,9 @@ public E setToTime(Callable timeable){ * Get the value of the gauge. */ public double get() { + // On concurrent `get` and `set`, it is acceptable to `get` an outdated `value`. + // On concurrent `get` and `inc / dec`, it is acceptable to `get` an outdated `value`. + // On concurrent `get` and `set` and `inc / dec`, it is possible to `get` an invalid `value`. return value.sum(); } } From 8c99e9c5163862cd5c2432002fb5cc8de4d757b5 Mon Sep 17 00:00:00 2001 From: Rudolf Rakos Date: Tue, 11 Jun 2019 15:13:36 +0200 Subject: [PATCH 3/5] Avoid unnecessary allocation in DoubleAdder#set [code review] Signed-off-by: Rudolf Rakos --- .../src/main/java/io/prometheus/client/DoubleAdder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java b/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java index 43925a3aa..6b28620c0 100644 --- a/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java +++ b/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java @@ -131,7 +131,7 @@ public void set(double x) { for (int i = 0; i < n; ++i) { Cell a = as[i]; if (a != null) // avoid unused cells - rs[i] = new Cell(0L); + rs[i] = a.value == 0L ? a : new Cell(0L); // reuse or initialize cell } // update cells and base (not atomic) cells = rs; From a54c15e54eea9e1311d618e5f85cab3e3bceda47 Mon Sep 17 00:00:00 2001 From: Rudolf Rakos Date: Tue, 11 Jun 2019 17:57:08 +0200 Subject: [PATCH 4/5] Avoid invalid result in DoubleAdder#sum [code review] Signed-off-by: Rudolf Rakos --- .../io/prometheus/client/DoubleAdder.java | 31 +++++++++++++++++-- .../main/java/io/prometheus/client/Gauge.java | 2 +- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java b/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java index 6b28620c0..8f90cf6cd 100644 --- a/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java +++ b/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java @@ -94,8 +94,32 @@ public void add(double x) { * @return the sum */ public double sum() { - Cell[] as = cells; - double sum = Double.longBitsToDouble(base); + // Correctness is guaranteed by `volatile` memory access ordering and visibility semantics. + // Program order: + // - writes in `set` - `busy` (CAS), `cells` (Wc), `base` (Wb), `busy` + // - reads in `sum` - `cells` (Rc), `base` (Rb), `busy`, `cells` (Cc), `base` (Cb) + // Note that: + // - `busy` is written after `cells` and `base` + // - `busy` is read after `cells` and `base`, `cells` and `base` is re-read after `busy` + // In other words: + // - if we see the write to `busy`, then we must see the write to `cells` and `busy` on re-read + // - if we don't see the write to `busy`, then we must retry as we have no guarantees + // Execution order (in the former case): + // - serial + // - old result - Rc, Rb, Cc, Cb, Wc, Wb + // - new result - Wc, Wb, Rc, Rb, Cc, Cb + // - concurrent + // - old result - Rc, Wc, Rb, Wb, Cc, Cb - retry (superfluous) + // - new result - Wc, Rc, Wb, Rb, Cc, Cb + // - invalid result - Rc, Wc, Wb, Rb, Cc, Cb - retry + // - invalid result - Wc, Rc, Rb, Wb, Cc, Cb - retry + Cell[] as = cells; long b = base; + while (as != null && !(busy == 0 && cells == as && base == b)) { + Thread.yield(); // busy waiting, retry loop + as = cells; b = base; + } + + double sum = Double.longBitsToDouble(b); if (as != null) { int n = as.length; for (int i = 0; i < n; ++i) { @@ -143,7 +167,8 @@ public void set(double x) { } } } else { // no cells - base = Double.doubleToLongBits(x); // update base (atomic) + // update base (atomic) + base = Double.doubleToLongBits(x); break; } } diff --git a/simpleclient/src/main/java/io/prometheus/client/Gauge.java b/simpleclient/src/main/java/io/prometheus/client/Gauge.java index 12f58462e..d36719492 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Gauge.java +++ b/simpleclient/src/main/java/io/prometheus/client/Gauge.java @@ -233,7 +233,7 @@ public E setToTime(Callable timeable){ public double get() { // On concurrent `get` and `set`, it is acceptable to `get` an outdated `value`. // On concurrent `get` and `inc / dec`, it is acceptable to `get` an outdated `value`. - // On concurrent `get` and `set` and `inc / dec`, it is possible to `get` an invalid `value`. + // On concurrent `get` and `set` and `inc / dec`, it is possible to `get` an outdated `value`. return value.sum(); } } From 368d5e5ab74807169dfc68045031c7ade6b9c6f4 Mon Sep 17 00:00:00 2001 From: Rudolf Rakos Date: Thu, 13 Jun 2019 17:49:57 +0200 Subject: [PATCH 5/5] Simplify code by deallocating cells on DoubleAdder#set [code review] Signed-off-by: Rudolf Rakos --- .../io/prometheus/client/DoubleAdder.java | 30 ++++++++++++------- .../main/java/io/prometheus/client/Gauge.java | 5 ---- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java b/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java index 8f90cf6cd..8be7a6e26 100644 --- a/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java +++ b/simpleclient/src/main/java/io/prometheus/client/DoubleAdder.java @@ -94,13 +94,17 @@ public void add(double x) { * @return the sum */ public double sum() { + // On concurrent `sum` and `set`, it is acceptable to `get` an outdated `value`. + // On concurrent `sum` and `add`, it is acceptable to `get` an outdated `value`. + // On concurrent `sum` and `set` and `add`, it is possible to `get` an outdated `value`. + // Correctness is guaranteed by `volatile` memory access ordering and visibility semantics. // Program order: // - writes in `set` - `busy` (CAS), `cells` (Wc), `base` (Wb), `busy` // - reads in `sum` - `cells` (Rc), `base` (Rb), `busy`, `cells` (Cc), `base` (Cb) // Note that: // - `busy` is written after `cells` and `base` - // - `busy` is read after `cells` and `base`, `cells` and `base` is re-read after `busy` + // - `busy` is read after `cells` and `base`, then `cells` and `base` is re-read after `busy` // In other words: // - if we see the write to `busy`, then we must see the write to `cells` and `busy` on re-read // - if we don't see the write to `busy`, then we must retry as we have no guarantees @@ -115,7 +119,8 @@ public double sum() { // - invalid result - Wc, Rc, Rb, Wb, Cc, Cb - retry Cell[] as = cells; long b = base; while (as != null && !(busy == 0 && cells == as && base == b)) { - Thread.yield(); // busy waiting, retry loop + // busy waiting, retry loop + Thread.yield(); as = cells; b = base; } @@ -143,22 +148,25 @@ public void reset() { } public void set(double x) { + // On concurrent `set` and `set`, it should be acceptable to lose one `set` measurement. + // On concurrent `set` and `add`, it should be acceptable to lose the `add` measurement. + + // Correctness is ensured by different techniques: + // - `set` waits on contention (blocking) + // - `add` avoids contention (non-blocking) + // - `sum` retries on conflicts (non-blocking) + // Performance characteristics by use cases: + // - only `set` - `cells` is always `null` - no allocations + // - only `add` - `cells` allocated on contention + // - mixed `set` and `add` - `cells` allocated on contention, `cells` deallocated on `set` for (;;) { Cell[] as; if ((as = cells) != null) { // have cells if (busy == 0 && casBusy()) { try { if (cells == as) { // recheck under lock - // allocate new cells instead of updating existing cells - int n = as.length; - Cell[] rs = new Cell[n]; - for (int i = 0; i < n; ++i) { - Cell a = as[i]; - if (a != null) // avoid unused cells - rs[i] = a.value == 0L ? a : new Cell(0L); // reuse or initialize cell - } // update cells and base (not atomic) - cells = rs; + cells = null; base = Double.doubleToLongBits(x); break; } diff --git a/simpleclient/src/main/java/io/prometheus/client/Gauge.java b/simpleclient/src/main/java/io/prometheus/client/Gauge.java index d36719492..d646dc370 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Gauge.java +++ b/simpleclient/src/main/java/io/prometheus/client/Gauge.java @@ -167,8 +167,6 @@ public void dec(double amt) { * Set the gauge to the given value. */ public void set(double val) { - // On concurrent `set` and `set`, it should be acceptable to lose one `set` measurement. - // On concurrent `set` and `inc / dec`, it might be acceptable to lose the `inc / dec` measurement. value.set(val); } /** @@ -231,9 +229,6 @@ public E setToTime(Callable timeable){ * Get the value of the gauge. */ public double get() { - // On concurrent `get` and `set`, it is acceptable to `get` an outdated `value`. - // On concurrent `get` and `inc / dec`, it is acceptable to `get` an outdated `value`. - // On concurrent `get` and `set` and `inc / dec`, it is possible to `get` an outdated `value`. return value.sum(); } }