From 80e344539913441c700533563caefe6d30aee661 Mon Sep 17 00:00:00 2001 From: nickhill Date: Fri, 22 Feb 2019 00:02:22 -0800 Subject: [PATCH 1/3] Yet another labels-to-Child lookup optimization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is an optimization of the SimpleCollector.labels(...) lookups with a similar goal to #445 and #459. It has some things in common with those PRs (including overridden fixed-args versions) but aims to provide best of all worlds - zero garbage and higher throughput for all label counts, without any reliance on thread reuse. To achieve this, ConcurrentHashMap is abandoned in favour of a custom copy-on-write linear-probe hashtable. Benchmark results Before: Benchmark Mode Cnt Score Error Units baseline thrpt 20 84731357.558 ± 535745.023 ops/s oneLabel thrpt 20 36415789.294 ± 441116.974 ops/s twoLabels thrpt 20 33301282.259 ± 313669.132 ops/s threeLabels thrpt 20 24560630.904 ± 2247040.286 ops/s fourLabels thrpt 20 24424456.896 ± 288989.596 ops/s fiveLabels thrpt 20 18356036.944 ± 949244.712 ops/s After: Benchmark Mode Cnt Score Error Units baseline thrpt 20 84866162.495 ± 823753.503 ops/s oneLabel thrpt 20 84554174.645 ± 804735.949 ops/s twoLabels thrpt 20 85004332.529 ± 689559.035 ops/s threeLabels thrpt 20 73395533.440 ± 3022384.940 ops/s fourLabels thrpt 20 68736143.734 ± 1872048.923 ops/s fiveLabels thrpt 20 53482207.003 ± 488751.990 ops/s This benchmark, like the prior ones, only tests with a single sequence of labels for each count. It would be good to extend it to cover cases where the map is populated with a larger number of children. Signed-off-by: nickhill --- benchmark/pom.xml | 4 +- .../LabelsToChildLookupBenchmark.java | 70 +++++ .../java/io/prometheus/client/Counter.java | 15 +- .../main/java/io/prometheus/client/Gauge.java | 17 +- .../java/io/prometheus/client/Histogram.java | 10 +- .../io/prometheus/client/SimpleCollector.java | 284 ++++++++++++++++-- .../java/io/prometheus/client/Summary.java | 10 +- 7 files changed, 365 insertions(+), 45 deletions(-) create mode 100644 benchmark/src/main/java/io/prometheus/benchmark/LabelsToChildLookupBenchmark.java diff --git a/benchmark/pom.xml b/benchmark/pom.xml index a1bcf0485..35781048b 100644 --- a/benchmark/pom.xml +++ b/benchmark/pom.xml @@ -28,12 +28,12 @@ org.openjdk.jmh jmh-core - 1.3.2 + 1.21 org.openjdk.jmh jmh-generator-annprocess - 1.3.2 + 1.21 diff --git a/benchmark/src/main/java/io/prometheus/benchmark/LabelsToChildLookupBenchmark.java b/benchmark/src/main/java/io/prometheus/benchmark/LabelsToChildLookupBenchmark.java new file mode 100644 index 000000000..78e547ad6 --- /dev/null +++ b/benchmark/src/main/java/io/prometheus/benchmark/LabelsToChildLookupBenchmark.java @@ -0,0 +1,70 @@ +package io.prometheus.benchmark; + +import io.prometheus.client.Counter; + +import org.openjdk.jmh.annotations.*; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +import java.util.concurrent.TimeUnit; + +@State(Scope.Benchmark) +@Warmup(iterations = 5, time = 1500, timeUnit = TimeUnit.MILLISECONDS) +@Measurement(iterations = 10, time = 1500, timeUnit = TimeUnit.MILLISECONDS) +@Fork(2) +public class LabelsToChildLookupBenchmark { + + private static final String LABEL1 = "label1", LABEL2 = "label2", LABEL3 = "label3"; + private static final String LABEL4 = "label4", LABEL5 = "label5"; + + private Counter noLabelsCollector, oneLabelCollector, twoLabelsCollector, threeLabelsCollector; + private Counter fourLabelsCollector, fiveLabelsCollector; + + @Setup + public void setup() { + Counter.Builder builder = new Counter.Builder().name("testCollector").help("testHelp"); + noLabelsCollector = builder.create(); + oneLabelCollector = builder.labelNames("name1").create(); + twoLabelsCollector = builder.labelNames("name1", "name2").create(); + threeLabelsCollector = builder.labelNames("name1", "name2", "name3").create(); + fourLabelsCollector = builder.labelNames("name1", "name2", "name3", "name4").create(); + fiveLabelsCollector = builder.labelNames("name1", "name2", "name3", "name4", "name5").create(); + } + + @Benchmark + public void baseline(LabelsToChildLookupBenchmark state) { + noLabelsCollector.inc(); + } + + @Benchmark + public void oneLabel(LabelsToChildLookupBenchmark state) { + oneLabelCollector.labels(LABEL1).inc(); + } + + @Benchmark + public void twoLabels(LabelsToChildLookupBenchmark state) { + twoLabelsCollector.labels(LABEL1, LABEL2).inc(); + } + + @Benchmark + public void threeLabels(LabelsToChildLookupBenchmark state) { + threeLabelsCollector.labels(LABEL1, LABEL2, LABEL3).inc(); + } + + @Benchmark + public void fourLabels(LabelsToChildLookupBenchmark state) { + fourLabelsCollector.labels(LABEL1, LABEL2, LABEL3, LABEL4).inc(); + } + + @Benchmark + public void fiveLabels(LabelsToChildLookupBenchmark state) { + fiveLabelsCollector.labels(LABEL1, LABEL2, LABEL3, LABEL4, LABEL5).inc(); + } + + public static void main(String[] args) throws RunnerException { + new Runner(new OptionsBuilder() + .include(LabelsToChildLookupBenchmark.class.getSimpleName()) + .build()).run(); + } +} \ No newline at end of file diff --git a/simpleclient/src/main/java/io/prometheus/client/Counter.java b/simpleclient/src/main/java/io/prometheus/client/Counter.java index 180a8d5c9..9ca7e1bb7 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Counter.java +++ b/simpleclient/src/main/java/io/prometheus/client/Counter.java @@ -112,14 +112,14 @@ public static class Child { * Increment the counter by 1. */ public void inc() { - inc(1); + inc(1.0); } /** * Increment the counter by the given amount. * @throws IllegalArgumentException If amt is negative. */ public void inc(double amt) { - if (amt < 0) { + if (amt < 0.0) { throw new IllegalArgumentException("Amount to increment must be non-negative."); } value.add(amt); @@ -137,7 +137,7 @@ public double get() { * Increment the counter with no labels by 1. */ public void inc() { - inc(1); + inc(1.0); } /** * Increment the counter with no labels by the given amount. @@ -156,9 +156,12 @@ public double get() { @Override public List collect() { - List samples = new ArrayList(children.size()); - for(Map.Entry, Child> c: children.entrySet()) { - samples.add(new MetricFamilySamples.Sample(fullname, labelNames, c.getKey(), c.getValue().get())); + final Map.Entry, Child>[] children = children(); + List samples = new ArrayList(children.length); + for(Map.Entry, Child> c : children) { + if (c != null) { + samples.add(new MetricFamilySamples.Sample(fullname, labelNames, c.getKey(), c.getValue().get())); + } } return familySamplesList(Type.COUNTER, samples); } diff --git a/simpleclient/src/main/java/io/prometheus/client/Gauge.java b/simpleclient/src/main/java/io/prometheus/client/Gauge.java index d646dc370..5f07cc51e 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Gauge.java +++ b/simpleclient/src/main/java/io/prometheus/client/Gauge.java @@ -143,7 +143,7 @@ public static class Child { * Increment the gauge by 1. */ public void inc() { - inc(1); + inc(1.0); } /** * Increment the gauge by the given amount. @@ -155,7 +155,7 @@ public void inc(double amt) { * Decrement the gauge by 1. */ public void dec() { - dec(1); + dec(1.0); } /** * Decrement the gauge by the given amount. @@ -238,7 +238,7 @@ public double get() { * Increment the gauge with no labels by 1. */ public void inc() { - inc(1); + inc(1.0); } /** * Increment the gauge with no labels by the given amount. @@ -250,7 +250,7 @@ public void inc(double amt) { * Decrement the gauge with no labels by 1. */ public void dec() { - dec(1); + dec(1.0); } /** * Decrement the gauge with no labels by the given amount. @@ -312,9 +312,12 @@ public double get() { @Override public List collect() { - List samples = new ArrayList(children.size()); - for(Map.Entry, Child> c: children.entrySet()) { - samples.add(new MetricFamilySamples.Sample(fullname, labelNames, c.getKey(), c.getValue().get())); + final Map.Entry, Child>[] children = children(); + List samples = new ArrayList(children.length); + for(Map.Entry, Child> c : children) { + if (c != null) { + samples.add(new MetricFamilySamples.Sample(fullname, labelNames, c.getKey(), c.getValue().get())); + } } return familySamplesList(Type.GAUGE, samples); } diff --git a/simpleclient/src/main/java/io/prometheus/client/Histogram.java b/simpleclient/src/main/java/io/prometheus/client/Histogram.java index 2b289806a..8f3fed341 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Histogram.java +++ b/simpleclient/src/main/java/io/prometheus/client/Histogram.java @@ -255,7 +255,7 @@ public void observe(double amt) { for (int i = 0; i < upperBounds.length; ++i) { // The last bucket is +Inf, so we always increment. if (amt <= upperBounds[i]) { - cumulativeCounts[i].add(1); + cumulativeCounts[i].add(1.0); break; } } @@ -323,8 +323,12 @@ public E time(Callable timeable){ @Override public List collect() { - List samples = new ArrayList(); - for(Map.Entry, Child> c: children.entrySet()) { + final Map.Entry, Child>[] children = children(); + List samples = new ArrayList(children.length); + for(Map.Entry,Child> c : children) { + if (c == null) { + continue; + } Child.Value v = c.getValue().get(); List labelNamesWithLe = new ArrayList(labelNames); labelNamesWithLe.add("le"); diff --git a/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java b/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java index ed8c5c8a0..7d88c649f 100644 --- a/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java +++ b/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java @@ -1,10 +1,12 @@ package io.prometheus.client; +import java.util.AbstractList; import java.util.ArrayList; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import java.util.Arrays; import java.util.List; +import java.util.Map; +import java.util.Map.Entry; /** * Common functionality for {@link Gauge}, {@link Counter}, {@link Summary} and {@link Histogram}. @@ -50,32 +52,265 @@ public abstract class SimpleCollector extends Collector { protected final String fullname; protected final String help; protected final List labelNames; + protected final int labelCount; + + @SuppressWarnings("rawtypes") + private static final ChildEntry[] EMPTY = new ChildEntry[1]; // one null slot + + @SuppressWarnings("rawtypes") + private static final AtomicReferenceFieldUpdater UPDATER + = AtomicReferenceFieldUpdater.newUpdater( + SimpleCollector.class, ChildEntry[].class, "children"); + + // linear-probe table, updated via copy-on-write + @SuppressWarnings("unchecked") + private volatile ChildEntry[] children = EMPTY; - protected final ConcurrentMap, Child> children = new ConcurrentHashMap, Child>(); protected Child noLabelsChild; + static final class ChildEntry extends AbstractList + implements Entry, Child> { + final String[] labels; // should be considered immutable + final int hashCode; + final Child child; + + public ChildEntry(String[] labels, int hash, Child child) { + this.labels = labels; + this.hashCode = hash; + this.child = child; + } + @Override + public String get(int index) { + return labels[index]; + } + @Override + public int size() { + return labels.length; + } + @Override + public List getKey() { + return this; + } + @Override + public Child getValue() { + return child; + } + @Override + public Child setValue(Child value) { + throw new UnsupportedOperationException(); + } + } + + Map.Entry,Child>[] children() { + return children; + } + + /** + * @throws IllegalArgumentException for null label + */ + private static int labelsHashCode(String... labelValues) { + int hashCode = 1; + for (String label : labelValues) { + hashCode = buildHashCode(hashCode, label); + } + return hashCode; + } + + private static int hash(int hashCode, int length) { + // Multiply by -127, and left-shift to use least bit as part of hash + // Based on the equivalent function in java.util.IdentityHashMap + return ((hashCode << 1) - (hashCode << 8)) & (length - 1); + } + + /** + * @throws IllegalArgumentException for null label + */ + private static int buildHashCode(int hashCode, String label) { + if (label == null) { + throw new IllegalArgumentException("Label cannot be null."); + } + return 31 * hashCode + label.hashCode(); + } + + private static int nextIdx(int i, int len) { + return (i == len - 1) ? 0 : (i + 1); + } + + private void validateCount(int count) { + if (count != labelCount) { + throw new IllegalArgumentException("Incorrect number of labels."); + } + } + /** * Return the Child with the given labels, creating it if needed. *

- * Must be passed the same number of labels are were passed to {@link #labelNames}. + * Must be passed the same number of labels as were passed to {@link #labelNames}. */ public Child labels(String... labelValues) { - if (labelValues.length != labelNames.size()) { - throw new IllegalArgumentException("Incorrect number of labels."); + validateCount(labelValues.length); + + int hashCode = labelsHashCode(labelValues); // also checks for null values + final ChildEntry[] arr = children; + int arrLen = arr.length, i = hash(hashCode, arrLen); + for (ChildEntry ce; (ce = arr[i]) != null; i = nextIdx(i, arrLen)) { + if (ce.hashCode == hashCode && Arrays.equals(ce.labels, labelValues)) { + return ce.child; + } } - for (String label: labelValues) { - if (label == null) { - throw new IllegalArgumentException("Label cannot be null."); + return labelsMiss(hashCode, arr, i, labelValues); + } + + public Child labels() { + validateCount(0); + return noLabelsChild; + } + + public Child labels(String v1) { + validateCount(1); + + int hashCode = buildHashCode(1, v1); + final ChildEntry[] arr = children; + int arrLen = arr.length, i = hash(hashCode, arrLen); + for (ChildEntry ce; (ce = arr[i]) != null; i = nextIdx(i, arrLen)) { + if (v1.equals(ce.labels[0])) { + return ce.child; } } - List key = Arrays.asList(labelValues); - Child c = children.get(key); - if (c != null) { - return c; + return labelsMiss(hashCode, arr, i, v1); + } + + public Child labels(String v1, String v2) { + validateCount(2); + + int hashCode = buildHashCode(buildHashCode(1, v1), v2); + final ChildEntry[] arr = children; + int arrLen = arr.length, i = hash(hashCode, arrLen); + for (ChildEntry ce; (ce = arr[i]) != null; i = nextIdx(i, arrLen)) { + if (ce.hashCode == hashCode) { + String[] ls = ce.labels; + if (ls[0].equals(v1) && ls[1].equals(v2)) { + return ce.child; + } + } + } + return labelsMiss(hashCode, arr, i, v1, v2); + } + + public Child labels(String v1, String v2, String v3) { + validateCount(3); + + int hashCode = buildHashCode(buildHashCode(buildHashCode(1, v1), v2), v3); + final ChildEntry[] arr = children; + int arrLen = arr.length, i = hash(hashCode, arrLen); + for (ChildEntry ce; (ce = arr[i]) != null; i = nextIdx(i, arrLen)) { + if (ce.hashCode == hashCode) { + String[] ls = ce.labels; + if (ls[0].equals(v1) && ls[1].equals(v2) && ls[2].equals(v3)) { + return ce.child; + } + } + } + return labelsMiss(hashCode, arr, i, v1, v2, v3); + } + + public Child labels(String v1, String v2, String v3, String v4) { + validateCount(4); + + int hashCode = buildHashCode(buildHashCode(buildHashCode(buildHashCode(1, v1), v2), v3), v4); + final ChildEntry[] arr = children; + int arrLen = arr.length, i = hash(hashCode, arrLen); + for (ChildEntry ce; (ce = arr[i]) != null; i = nextIdx(i, arrLen)) { + if (ce.hashCode == hashCode) { + String[] ls = ce.labels; + if (ls[0].equals(v1) && ls[1].equals(v2) && ls[2].equals(v3) && ls[3].equals(v4)) { + return ce.child; + } + } + } + return labelsMiss(hashCode, arr, i, v1, v2, v3, v4); + } + + private Child labelsMiss(int hashCode, ChildEntry[] arr, int pos, String... values) { + return updateChild(values, hashCode, arr, pos, + new ChildEntry(values, hashCode, newChild())); + } + + /** + * Multi-purpose used for set, remove and get after not found (cache miss) + * + * @param pos -1 for set and remove, otherwise insert (null) position in arr + * @param newEntry null for remove, otherwise new entry to add + * @return new/existing Child in case of get, prior Child in case of set/remove + */ + @SuppressWarnings("unchecked") + private Child updateChild(String[] values, int hashCode, + ChildEntry[] arr, int pos, ChildEntry newEntry) { + final boolean set = pos == -1; + int arrLen = arr.length; + while (true) { + final ChildEntry[] newArr; + ChildEntry replacing = null; + if (pos == -1) { + pos = hash(hashCode, arrLen); + while (true) { + ChildEntry ce = arr[pos]; + if (ce == null) { + if (newEntry == null) { + return null; + } + break; + } + if (ce.hashCode == hashCode && Arrays.equals(ce.labels, values)) { + if (set) { + replacing = ce; + break; + } + return ce.child; + } + pos = nextIdx(pos, arrLen); + } + } + boolean resizeNeeded; + if (newEntry == null | replacing != null) { + resizeNeeded = false; + } else { + int count = 1; + for (ChildEntry ce : arr) { + if (ce != null) { + count++; + } + } + resizeNeeded = count > arrLen / 2; + } + if (resizeNeeded) { + newArr = new ChildEntry[arrLen * 2]; + int newArrLen = newArr.length; + newArr[hash(hashCode, newArrLen)] = newEntry; + for (ChildEntry ce : arr) { + if (ce != null & ce != replacing) { + for (int j = hash(ce.hashCode, newArrLen);; j = nextIdx(j, newArrLen)) { + if (newArr[j] == null) { + newArr[j] = ce; + break; + } + } + } + } + } else { + newArr = Arrays.copyOf(arr, arrLen, ChildEntry[].class); + newArr[pos] = newEntry; + } + if (UPDATER.compareAndSet(this, arr, newArr)) { + if (set) { + return replacing != null ? replacing.child : null; + } + return newEntry.child; + } + arr = children; + arrLen = arr.length; + pos = -1; } - Child c2 = newChild(); - Child tmp = children.putIfAbsent(key, c2); - return tmp == null ? c2 : tmp; } /** @@ -84,7 +319,7 @@ public Child labels(String... labelValues) { * Any references to the Child are invalidated. */ public void remove(String... labelValues) { - children.remove(Arrays.asList(labelValues)); + updateChild(labelValues, labelsHashCode(labelValues), children, -1, null); initializeNoLabelsChild(); } @@ -94,7 +329,7 @@ public void remove(String... labelValues) { * Any references to any children are invalidated. */ public void clear() { - children.clear(); + UPDATER.set(this, EMPTY); initializeNoLabelsChild(); } @@ -103,8 +338,8 @@ public void clear() { */ protected void initializeNoLabelsChild() { // Initialize metric if it has no labels. - if (labelNames.size() == 0) { - noLabelsChild = labels(); + if (labelCount == 0) { + noLabelsChild = labels(new String[0]); } } @@ -132,10 +367,10 @@ protected void initializeNoLabelsChild() { * A metric should be either all callbacks, or none. */ public T setChild(Child child, String... labelValues) { - if (labelValues.length != labelNames.size()) { - throw new IllegalArgumentException("Incorrect number of labels."); - } - children.put(Arrays.asList(labelValues), child); + validateCount(labelValues.length); + int hashCode = labelsHashCode(labelValues); + updateChild(labelValues, hashCode, children, -1, + new ChildEntry(labelValues, hashCode, child)); return (T)this; } @@ -165,6 +400,7 @@ protected SimpleCollector(Builder b) { if (b.help.isEmpty()) throw new IllegalStateException("Help hasn't been set."); help = b.help; labelNames = Arrays.asList(b.labelNames); + labelCount = b.labelNames.length; for (String n: labelNames) { checkMetricLabelName(n); diff --git a/simpleclient/src/main/java/io/prometheus/client/Summary.java b/simpleclient/src/main/java/io/prometheus/client/Summary.java index a341f2515..1ac562e5d 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Summary.java +++ b/simpleclient/src/main/java/io/prometheus/client/Summary.java @@ -275,7 +275,7 @@ private Child(List quantiles, long maxAgeSeconds, int ageBuckets) { * Observe the given amount. */ public void observe(double amt) { - count.add(1); + count.add(1.0); sum.add(amt); if (quantileValues != null) { quantileValues.insert(amt); @@ -346,8 +346,12 @@ public Child.Value get() { @Override public List collect() { - List samples = new ArrayList(); - for(Map.Entry, Child> c: children.entrySet()) { + final Map.Entry, Child>[] children = children(); + List samples = new ArrayList(children.length); + for(Map.Entry,Child> c : children) { + if (c == null) { + continue; + } Child.Value v = c.getValue().get(); List labelNamesWithQuantile = new ArrayList(labelNames); labelNamesWithQuantile.add("quantile"); From 90d74651ef549b439eb75983082e0949e9a25506 Mon Sep 17 00:00:00 2001 From: nickhill Date: Sun, 24 Feb 2019 00:22:50 -0800 Subject: [PATCH 2/3] Fix hash function, refine logic and add lots of comments Signed-off-by: nickhill --- .../io/prometheus/client/SimpleCollector.java | 67 +++++++++++++------ 1 file changed, 45 insertions(+), 22 deletions(-) diff --git a/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java b/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java index 7d88c649f..78ce51e32 100644 --- a/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java +++ b/simpleclient/src/main/java/io/prometheus/client/SimpleCollector.java @@ -117,9 +117,7 @@ private static int labelsHashCode(String... labelValues) { } private static int hash(int hashCode, int length) { - // Multiply by -127, and left-shift to use least bit as part of hash - // Based on the equivalent function in java.util.IdentityHashMap - return ((hashCode << 1) - (hashCode << 8)) & (length - 1); + return hashCode & (length - 1); // length is a power of 2 } /** @@ -165,6 +163,9 @@ public Child labels() { validateCount(0); return noLabelsChild; } + + // Logic for the fixed-arg overloads is identical apart from number of strings + // used for hashcode generation and key comparison public Child labels(String v1) { validateCount(1); @@ -232,49 +233,64 @@ public Child labels(String v1, String v2, String v3, String v4) { } private Child labelsMiss(int hashCode, ChildEntry[] arr, int pos, String... values) { - return updateChild(values, hashCode, arr, pos, + return updateChild(values, hashCode, true, arr, pos, new ChildEntry(values, hashCode, newChild())); } /** * Multi-purpose used for set, remove and get after not found (cache miss) * - * @param pos -1 for set and remove, otherwise insert (null) position in arr + * @param getIfPresent if true, entry won't be updated if it already exists + * @param pos insertion position for new entry if known (get case), otherwise -1 * @param newEntry null for remove, otherwise new entry to add * @return new/existing Child in case of get, prior Child in case of set/remove */ @SuppressWarnings("unchecked") - private Child updateChild(String[] values, int hashCode, + private Child updateChild(String[] values, int hashCode, boolean getIfPresent, ChildEntry[] arr, int pos, ChildEntry newEntry) { - final boolean set = pos == -1; - int arrLen = arr.length; + // This loop is just for retries after CAS failures of the children field while (true) { + final int arrLen = arr.length; final ChildEntry[] newArr; + // This gets set to the entry we're replacing, if/when applicable ChildEntry replacing = null; + // If pos >= 0, we already know where the new entry should go if (pos == -1) { + // Scan the table looking for the a matching entry starting at + // the labels' hash position pos = hash(hashCode, arrLen); while (true) { ChildEntry ce = arr[pos]; if (ce == null) { + // If we reach a null entry then the labels aren't present, and + // our current pos is the target location for inserting the new entry if (newEntry == null) { + // Nothing to do in removal case return null; } break; } if (ce.hashCode == hashCode && Arrays.equals(ce.labels, values)) { - if (set) { - replacing = ce; - break; + // We found an entry matching the target labels + if (getIfPresent) { + return ce.child; } - return ce.child; + replacing = ce; + break; } - pos = nextIdx(pos, arrLen); + pos = nextIdx(pos, arrLen); // wrap around } } boolean resizeNeeded; + // If we reached here we'll be updating the table by either inserting + // replacing, or removing an entry if (newEntry == null | replacing != null) { + // No need to increase the table capacity if removing or replacing resizeNeeded = false; } else { + // We are inserting a new entry so count the existing entries to determine + // how full the table is - we resize (double) it if greater than half full. + // This avoids having to separately track the count int count = 1; for (ChildEntry ce : arr) { if (ce != null) { @@ -284,10 +300,14 @@ private Child updateChild(String[] values, int hashCode, resizeNeeded = count > arrLen / 2; } if (resizeNeeded) { - newArr = new ChildEntry[arrLen * 2]; - int newArrLen = newArr.length; + // Double the size of the table + final int newArrLen = arrLen * 2; + newArr = new ChildEntry[newArrLen]; + // Insert our new entry first newArr[hash(hashCode, newArrLen)] = newEntry; + // Then re-hash/insert the existing entries for (ChildEntry ce : arr) { + // Exclude the entry we're replacing (if applicable - replacing will be non-null) if (ce != null & ce != replacing) { for (int j = hash(ce.hashCode, newArrLen);; j = nextIdx(j, newArrLen)) { if (newArr[j] == null) { @@ -298,17 +318,20 @@ private Child updateChild(String[] values, int hashCode, } } } else { + // If we're not resizing the table, just make a straight copy + // and insert our new entry at the target position newArr = Arrays.copyOf(arr, arrLen, ChildEntry[].class); newArr[pos] = newEntry; } + // Attempt to update the shared field atomically if (UPDATER.compareAndSet(this, arr, newArr)) { - if (set) { - return replacing != null ? replacing.child : null; - } - return newEntry.child; + // Successful table modification + // Return the new entry in the get case, old entry in the remove/set cases + return getIfPresent ? newEntry.child + : (replacing != null ? replacing.child : null); } + // Someone else changed the table, read it and start again arr = children; - arrLen = arr.length; pos = -1; } } @@ -319,7 +342,7 @@ private Child updateChild(String[] values, int hashCode, * Any references to the Child are invalidated. */ public void remove(String... labelValues) { - updateChild(labelValues, labelsHashCode(labelValues), children, -1, null); + updateChild(labelValues, labelsHashCode(labelValues), false, children, -1, null); initializeNoLabelsChild(); } @@ -369,7 +392,7 @@ protected void initializeNoLabelsChild() { public T setChild(Child child, String... labelValues) { validateCount(labelValues.length); int hashCode = labelsHashCode(labelValues); - updateChild(labelValues, hashCode, children, -1, + updateChild(labelValues, hashCode, false, children, -1, new ChildEntry(labelValues, hashCode, child)); return (T)this; } From 6e4462041cfb6d22878ce9927c9d7ea44049ebb6 Mon Sep 17 00:00:00 2001 From: nickhill Date: Tue, 5 Nov 2019 17:23:36 -0800 Subject: [PATCH 3/3] Bump JMH version to latest 1.22 --- benchmark/pom.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/benchmark/pom.xml b/benchmark/pom.xml index 35781048b..e4e54a4a8 100644 --- a/benchmark/pom.xml +++ b/benchmark/pom.xml @@ -28,12 +28,12 @@ org.openjdk.jmh jmh-core - 1.21 + 1.22 org.openjdk.jmh jmh-generator-annprocess - 1.21 + 1.22