diff --git a/core/build.gradle b/core/build.gradle index e7630c9..8ea862c 100644 --- a/core/build.gradle +++ b/core/build.gradle @@ -19,3 +19,39 @@ // THE SOFTWARE. description = 'Interfaces and utilities to report metrics to M3' + +sourceSets { + jmh { + java.srcDirs = ['src/jmh/java'] + resources.srcDirs = ['src/jmh/resources'] + compileClasspath += sourceSets.main.runtimeClasspath + compileClasspath += sourceSets.test.runtimeClasspath + } +} + +dependencies { + jmhImplementation 'org.openjdk.jmh:jmh-core:1.23' + jmhImplementation 'org.openjdk.jmh:jmh-generator-annprocess:1.23' +} + +task jmh(type: JavaExec, dependsOn: jmhClasses) { + main = 'org.openjdk.jmh.Main' + classpath = sourceSets.jmh.compileClasspath + sourceSets.jmh.runtimeClasspath +} + +classes.finalizedBy(jmhClasses) + +//jmh { +// benchmarkMode = ['Throughput'] +// +// warmupIterations = 5 +// +// iterations = 5 +// batchSize = 1 +// +// fork = 1 +// forceGC = true +// includeTests = true +// +// duplicateClassesStrategy = 'warn' +//} diff --git a/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java new file mode 100644 index 0000000..0e60cab --- /dev/null +++ b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java @@ -0,0 +1,119 @@ +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.m3.tally; + +import com.uber.m3.util.Duration; +import com.uber.m3.util.ImmutableMap; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; + +import java.util.Random; +import java.util.concurrent.TimeUnit; + +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.MILLISECONDS) +@Fork(value = 2, jvmArgsAppend = { "-server", "-XX:+UseG1GC" }) +public class ScopeImplBenchmark { + + private static final DurationBuckets EXPONENTIAL_BUCKETS = DurationBuckets.linear(Duration.ofMillis(1), Duration.ofMillis(10), 128); + + private static final String[] COUNTER_NAMES = { + "first-counter", + "second-counter", + "third-counter", + "fourth-counter", + "fifth-counter", + }; + + private static final String[] GAUGE_NAMES = { + "first-gauge", + "second-gauge", + "third-gauge", + "fourth-gauge", + "fifth-gauge", + }; + + private static final String[] HISTOGRAM_NAMES = { + "first-histogram", + "second-histogram", + "third-histogram", + "fourth-histogram", + "fifth-histogram", + }; + + @Benchmark + public void scopeReportingBenchmark(BenchmarkState state) { + state.scope.reportLoopIteration(); + } + + @State(org.openjdk.jmh.annotations.Scope.Benchmark) + public static class BenchmarkState { + + private ScopeImpl scope; + + @Setup + public void setup() { + this.scope = + (ScopeImpl) new RootScopeBuilder() + .reporter(new TestStatsReporter()) + .tags( + ImmutableMap.of( + "service", "some-service", + "application", "some-application", + "instance", "some-instance" + ) + ) + .reportEvery(Duration.MAX_VALUE); + + for (String counterName : COUNTER_NAMES) { + scope.counter(counterName).inc(1); + } + + for (String gaugeName : GAUGE_NAMES) { + scope.gauge(gaugeName).update(0.); + } + + for (String histogramName : HISTOGRAM_NAMES) { + Histogram h = scope.histogram(histogramName, EXPONENTIAL_BUCKETS); + + Random r = new Random(); + + // Populate at least 20% of the buckets + int bucketsCount = EXPONENTIAL_BUCKETS.buckets.size(); + for (int i = 0; i < bucketsCount / 5; ++i) { + h.recordDuration(EXPONENTIAL_BUCKETS.buckets.get(r.nextInt(bucketsCount))); + } + } + } + + @TearDown + public void teardown() { + scope.close(); + } + + } +} diff --git a/core/src/main/java/com/uber/m3/tally/CounterImpl.java b/core/src/main/java/com/uber/m3/tally/CounterImpl.java index bb56e96..d40fcb5 100644 --- a/core/src/main/java/com/uber/m3/tally/CounterImpl.java +++ b/core/src/main/java/com/uber/m3/tally/CounterImpl.java @@ -27,15 +27,24 @@ /** * Default implementation of a {@link Counter}. */ -class CounterImpl implements Counter { +class CounterImpl extends MetricBase implements Counter { private AtomicLong prev = new AtomicLong(0); private AtomicLong curr = new AtomicLong(0); + protected CounterImpl(String fqn) { + super(fqn); + } + @Override public void inc(long delta) { curr.getAndAdd(delta); } + @Override + public String getQualifiedName() { + return super.getQualifiedName(); + } + long value() { long current = curr.get(); long previous = prev.get(); diff --git a/core/src/main/java/com/uber/m3/tally/GaugeImpl.java b/core/src/main/java/com/uber/m3/tally/GaugeImpl.java index e4b75de..cc8025f 100644 --- a/core/src/main/java/com/uber/m3/tally/GaugeImpl.java +++ b/core/src/main/java/com/uber/m3/tally/GaugeImpl.java @@ -28,16 +28,25 @@ /** * Default implementation of a {@link Gauge}. */ -class GaugeImpl implements Gauge { +class GaugeImpl extends MetricBase implements Gauge { private AtomicBoolean updated = new AtomicBoolean(false); private AtomicLong curr = new AtomicLong(0); + protected GaugeImpl(String fqn) { + super(fqn); + } + @Override public void update(double value) { curr.set(Double.doubleToLongBits(value)); updated.set(true); } + @Override + public String getQualifiedName() { + return super.getQualifiedName(); + } + double value() { return Double.longBitsToDouble(curr.get()); } diff --git a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java index e559733..027a7a5 100644 --- a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java +++ b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java @@ -32,9 +32,8 @@ /** * Default implementation of a {@link Histogram}. */ -class HistogramImpl implements Histogram, StopwatchRecorder { +class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder { private Type type; - private String name; private ImmutableMap tags; private Buckets specification; private List buckets; @@ -42,11 +41,12 @@ class HistogramImpl implements Histogram, StopwatchRecorder { private List lookupByDuration; HistogramImpl( - String name, + String fqn, ImmutableMap tags, - StatsReporter reporter, Buckets buckets ) { + super(fqn); + if (buckets instanceof DurationBuckets) { type = Type.DURATION; } else { @@ -56,12 +56,12 @@ class HistogramImpl implements Histogram, StopwatchRecorder { BucketPair[] pairs = BucketPairImpl.bucketPairs(buckets); int pairsLen = pairs.length; - this.name = name; this.tags = tags; - specification = buckets; + this.specification = buckets; + this.buckets = new ArrayList<>(pairsLen); - lookupByValue = new ArrayList<>(pairsLen); - lookupByDuration = new ArrayList<>(pairsLen); + this.lookupByValue = new ArrayList<>(pairsLen); + this.lookupByDuration = new ArrayList<>(pairsLen); for (BucketPair pair : pairs) { addBucket(new HistogramBucket( @@ -81,40 +81,30 @@ private void addBucket(HistogramBucket bucket) { @Override public void recordValue(double value) { - int index = Collections.binarySearch(lookupByValue, value); - - if (index < 0) { - // binarySearch returns the index of the search key if it is contained in the list; - // otherwise, (-(insertion point) - 1). - index = -(index + 1); - } - - // binarySearch can return collections.size(), guarding against that. - // pointing to last bucket is fine in that case because it's [_,infinity). - if (index >= buckets.size()) { - index = buckets.size() - 1; - } - + int index = toBucketIndex(Collections.binarySearch(lookupByValue, value)); buckets.get(index).samples.inc(1); } @Override public void recordDuration(Duration duration) { - int index = Collections.binarySearch(lookupByDuration, duration); + int index = toBucketIndex(Collections.binarySearch(lookupByDuration, duration)); + buckets.get(index).samples.inc(1); + } - if (index < 0) { + private int toBucketIndex(int binarySearchResult) { + if (binarySearchResult < 0) { // binarySearch returns the index of the search key if it is contained in the list; // otherwise, (-(insertion point) - 1). - index = -(index + 1); + binarySearchResult = -(binarySearchResult + 1); } // binarySearch can return collections.size(), guarding against that. // pointing to last bucket is fine in that case because it's [_,infinity). - if (index >= buckets.size()) { - index = buckets.size() - 1; + if (binarySearchResult >= buckets.size()) { + binarySearchResult = buckets.size() - 1; } - buckets.get(index).samples.inc(1); + return binarySearchResult; } @Override @@ -122,14 +112,11 @@ public Stopwatch start() { return new Stopwatch(System.nanoTime(), this); } - String getName() { - return name; - } - ImmutableMap getTags() { return tags; } + @Override void report(String name, ImmutableMap tags, StatsReporter reporter) { for (HistogramBucket bucket : buckets) { long samples = bucket.samples.value(); @@ -199,7 +186,7 @@ class HistogramBucket { Duration durationLowerBound, Duration durationUpperBound ) { - samples = new CounterImpl(); + this.samples = new CounterImpl(getQualifiedName()); this.valueLowerBound = valueLowerBound; this.valueUpperBound = valueUpperBound; this.durationLowerBound = durationLowerBound; diff --git a/core/src/main/java/com/uber/m3/tally/MetricBase.java b/core/src/main/java/com/uber/m3/tally/MetricBase.java new file mode 100644 index 0000000..36956ee --- /dev/null +++ b/core/src/main/java/com/uber/m3/tally/MetricBase.java @@ -0,0 +1,38 @@ +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.m3.tally; + +import com.uber.m3.util.ImmutableMap; + +abstract class MetricBase { + + private final String fullyQualifiedName; + + protected MetricBase(String fqn) { + this.fullyQualifiedName = fqn; + } + + String getQualifiedName() { + return fullyQualifiedName; + } + + abstract void report(String name, ImmutableMap tags, StatsReporter reporter); +} diff --git a/core/src/main/java/com/uber/m3/tally/ScopeImpl.java b/core/src/main/java/com/uber/m3/tally/ScopeImpl.java index fe42e82..1ef56ac 100644 --- a/core/src/main/java/com/uber/m3/tally/ScopeImpl.java +++ b/core/src/main/java/com/uber/m3/tally/ScopeImpl.java @@ -27,6 +27,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.ScheduledExecutorService; /** @@ -42,14 +43,14 @@ class ScopeImpl implements Scope { private ScheduledExecutorService scheduler; private Registry registry; - // ConcurrentHashMap nearly always allowing read operations seems like a good - // performance upside to the consequence of reporting a newly-made metric in - // the middle of looping and reporting through all metrics. Therefore, we only - // synchronize on these maps when having to allocate new metrics. - private final Map counters = new ConcurrentHashMap<>(); - private final Map gauges = new ConcurrentHashMap<>(); - private final Map timers = new ConcurrentHashMap<>(); - private final Map histograms = new ConcurrentHashMap<>(); + private final ConcurrentHashMap counters = new ConcurrentHashMap<>(); + private final ConcurrentHashMap gauges = new ConcurrentHashMap<>(); + private final ConcurrentHashMap histograms = new ConcurrentHashMap<>(); + + // TODO validate if needs to be concurrent at all + private final ConcurrentLinkedQueue reportingQueue = new ConcurrentLinkedQueue<>(); + + private final ConcurrentHashMap timers = new ConcurrentHashMap<>(); // Private ScopeImpl constructor. Root scopes should be built using the RootScopeBuilder class ScopeImpl(ScheduledExecutorService scheduler, Registry registry, ScopeBuilder builder) { @@ -65,82 +66,31 @@ class ScopeImpl implements Scope { @Override public Counter counter(String name) { - CounterImpl counter = counters.get(name); - - if (counter != null) { - return counter; - } - - synchronized (counters) { - if (!counters.containsKey(name)) { - counters.put(name, new CounterImpl()); - } - - counter = counters.get(name); - } - - return counter; + return counters.computeIfAbsent(name, ignored -> + // NOTE: This will called at most once + addToReportingQueue(new CounterImpl(fullyQualifiedName(name))) + ); } @Override public Gauge gauge(String name) { - GaugeImpl gauge = gauges.get(name); - - if (gauge != null) { - return gauge; - } - - synchronized (gauges) { - if (!gauges.containsKey(name)) { - gauges.put(name, new GaugeImpl()); - } - - gauge = gauges.get(name); - } - - return gauge; + return gauges.computeIfAbsent(name, ignored -> + // NOTE: This will called at most once + addToReportingQueue(new GaugeImpl(fullyQualifiedName(name)))); } @Override public Timer timer(String name) { - TimerImpl timer = timers.get(name); - - if (timer != null) { - return timer; - } - - synchronized (timers) { - if (!timers.containsKey(name)) { - timers.put(name, new TimerImpl(fullyQualifiedName(name), tags, reporter)); - } - - timer = timers.get(name); - } - - return timer; + // Timers report directly to the {@code StatsReporter}, and therefore not added to reporting queue + // i.e. they are not buffered + return timers.computeIfAbsent(name, ignored -> new TimerImpl(fullyQualifiedName(name), tags, reporter)); } @Override public Histogram histogram(String name, Buckets buckets) { - if (buckets == null) { - buckets = defaultBuckets; - } - - HistogramImpl histogram = histograms.get(name); - - if (histogram != null) { - return histogram; - } - - synchronized (histograms) { - if (!histograms.containsKey(name)) { - histograms.put(name, new HistogramImpl(fullyQualifiedName(name), tags, reporter, buckets)); - } - - histogram = histograms.get(name); - } - - return histogram; + return histograms.computeIfAbsent(name, ignored -> + // NOTE: This will called at most once + addToReportingQueue(new HistogramImpl(fullyQualifiedName(name), tags, buckets))); } @Override @@ -178,24 +128,18 @@ public void close() { } } + private T addToReportingQueue(T metric) { + reportingQueue.add(metric); + return metric; + } + /** * Reports using the specified reporter. * @param reporter the reporter to report */ void report(StatsReporter reporter) { - for (Map.Entry counter : counters.entrySet()) { - counter.getValue().report(fullyQualifiedName(counter.getKey()), tags, reporter); - } - - for (Map.Entry gauge : gauges.entrySet()) { - gauge.getValue().report(fullyQualifiedName(gauge.getKey()), tags, reporter); - } - - // No operations on timers required here; they report directly to the StatsReporter - // i.e. they are not buffered - - for (Map.Entry histogram : histograms.entrySet()) { - histogram.getValue().report(fullyQualifiedName(histogram.getKey()), tags, reporter); + for (MetricBase metric : reportingQueue) { + metric.report(metric.getQualifiedName(), tags, reporter); } } @@ -342,7 +286,7 @@ private Scope subScopeHelper(String prefix, Map tags) { } // One iteration of reporting this scope and all its subscopes - private void reportLoopIteration() { + void reportLoopIteration() { Collection subscopes = registry.subscopes.values(); if (reporter != null) { diff --git a/core/src/test/java/com/uber/m3/tally/CounterImplTest.java b/core/src/test/java/com/uber/m3/tally/CounterImplTest.java index d5600c5..cd4c26f 100644 --- a/core/src/test/java/com/uber/m3/tally/CounterImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/CounterImplTest.java @@ -32,7 +32,7 @@ public class CounterImplTest { @Before public void setUp() { reporter = new TestStatsReporter(); - counter = new CounterImpl(); + counter = new CounterImpl("counter"); } @Test diff --git a/core/src/test/java/com/uber/m3/tally/GaugeImplTest.java b/core/src/test/java/com/uber/m3/tally/GaugeImplTest.java index cf578fe..4614069 100644 --- a/core/src/test/java/com/uber/m3/tally/GaugeImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/GaugeImplTest.java @@ -34,7 +34,7 @@ public class GaugeImplTest { @Before public void setUp() { reporter = new TestStatsReporter(); - gauge = new GaugeImpl(); + gauge = new GaugeImpl("gauge"); } @Test diff --git a/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java index 2f633e9..cacd62c 100644 --- a/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java @@ -45,7 +45,6 @@ public void recordValue() { histogram = new HistogramImpl( "", null, - reporter, buckets ); @@ -57,7 +56,7 @@ public void recordValue() { histogram.recordValue(50 + Math.random() * 10); } - histogram.report(histogram.getName(), histogram.getTags(), reporter); + histogram.report(histogram.getQualifiedName(), histogram.getTags(), reporter); assertEquals(new Long(3L), reporter.getValueSamples().get(10d)); assertEquals(new Long(5L), reporter.getValueSamples().get(60d)); @@ -71,7 +70,6 @@ public void recordDuration() { histogram = new HistogramImpl( "", null, - reporter, buckets ); @@ -83,7 +81,7 @@ public void recordDuration() { histogram.recordDuration(Duration.ofMillis(50).add(Duration.ofMillis(Math.random() * 10))); } - histogram.report(histogram.getName(), histogram.getTags(), reporter); + histogram.report(histogram.getQualifiedName(), histogram.getTags(), reporter); assertEquals(new Long(3L), reporter.getDurationSamples().get(Duration.ofMillis(10))); assertEquals(new Long(5L), reporter.getDurationSamples().get(Duration.ofMillis(60))); @@ -97,7 +95,6 @@ public void recordStopwatch() { histogram = new HistogramImpl( "", null, - reporter, buckets ); @@ -105,7 +102,7 @@ public void recordStopwatch() { assertNotNull(stopwatch); stopwatch.stop(); - histogram.report(histogram.getName(), histogram.getTags(), reporter); + histogram.report(histogram.getQualifiedName(), histogram.getTags(), reporter); assertEquals(new Long(1L), reporter.getDurationSamples().get(Duration.ofMillis(10))); } @@ -117,7 +114,6 @@ public void snapshotValues() { histogram = new HistogramImpl( "", null, - reporter, buckets ); @@ -152,7 +148,6 @@ public void snapshotDurations() { histogram = new HistogramImpl( "", null, - reporter, buckets ); diff --git a/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java b/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java index 7252987..86353e9 100644 --- a/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java +++ b/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java @@ -25,12 +25,12 @@ import java.util.HashMap; import java.util.Map; import java.util.Queue; -import java.util.concurrent.LinkedBlockingDeque; +import java.util.concurrent.ConcurrentLinkedQueue; public class TestStatsReporter implements StatsReporter { - private Queue> counters = new LinkedBlockingDeque<>(); - private Queue> gauges = new LinkedBlockingDeque<>(); - private Queue> timers = new LinkedBlockingDeque<>(); + private Queue> counters = new ConcurrentLinkedQueue<>(); + private Queue> gauges = new ConcurrentLinkedQueue<>(); + private Queue> timers = new ConcurrentLinkedQueue<>(); private Buckets buckets; private Map valueSamples = new HashMap<>(); private Map durationSamples = new HashMap<>(); diff --git a/m3/build.gradle b/m3/build.gradle index 5e17944..8c0e542 100644 --- a/m3/build.gradle +++ b/m3/build.gradle @@ -20,7 +20,6 @@ plugins { id "org.jruyi.thrift" version "0.4.0" - id "me.champeau.gradle.jmh" version "0.4.3" } description = 'tally M3 reporter' @@ -42,16 +41,3 @@ compileThrift { project.hasProperty('genThrift') } } - -jmh { - benchmarkMode = ['Throughput'] - - warmupIterations = 0 - - iterations = 5 - batchSize = 1 - - fork = 1 - forceGC = true - includeTests = true -}