From 3625a9eae0a0275eded5237468be078e300c7936 Mon Sep 17 00:00:00 2001 From: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com> Date: Fri, 20 Dec 2024 15:45:31 +0900 Subject: [PATCH] Fix performance regression in MeterRegistry#remove (#5750) Adds a reverse look-up for the pre-filter meter ID for use when removing a Meter. This avoids the need to iterate over the meters in the cache (preFilterIdMeterMap), which scales linearly with the number of meters, and is problematic because it does this while holding the meterMap lock needed to add new meters. Also adds benchmarks for measuring the performance of the remove method with different numbers of meters registered. Resolves gh-5466 --- .../benchmark/core/MeterRemovalBenchmark.java | 70 +++++++++++++++++++ .../core/instrument/MeterRegistry.java | 23 +++--- .../core/instrument/MeterRegistryTest.java | 5 ++ 3 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 benchmarks/benchmarks-core/src/jmh/java/io/micrometer/benchmark/core/MeterRemovalBenchmark.java diff --git a/benchmarks/benchmarks-core/src/jmh/java/io/micrometer/benchmark/core/MeterRemovalBenchmark.java b/benchmarks/benchmarks-core/src/jmh/java/io/micrometer/benchmark/core/MeterRemovalBenchmark.java new file mode 100644 index 0000000000..1f046bd926 --- /dev/null +++ b/benchmarks/benchmarks-core/src/jmh/java/io/micrometer/benchmark/core/MeterRemovalBenchmark.java @@ -0,0 +1,70 @@ +/* + * Copyright 2024 VMware, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.micrometer.benchmark.core; + +import io.micrometer.core.instrument.Meter; +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import org.openjdk.jmh.annotations.*; +import org.openjdk.jmh.profile.GCProfiler; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +import java.util.concurrent.TimeUnit; + +@State(Scope.Benchmark) +@Fork(1) +@Warmup(iterations = 2) +@Measurement(iterations = 5) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +public class MeterRemovalBenchmark { + + public static void main(String[] args) throws RunnerException { + Options opt = new OptionsBuilder().include(MeterRemovalBenchmark.class.getSimpleName()) + .addProfiler(GCProfiler.class) + .build(); + + new Runner(opt).run(); + } + + @Param({ "10000", "100000" }) + int meterCount; + + MeterRegistry registry = new SimpleMeterRegistry(); + + @Setup + public void setup() { + for (int i = 0; i < meterCount; i++) { + registry.counter("counter", "key", String.valueOf(i)); + } + } + + /** + * Benchmark the time to remove one meter from a registry with many meters. This uses + * the single shot mode because otherwise it would measure the time to remove a meter + * not in the registry after the first call, and that is not what we want to measure. + */ + @Benchmark + @Warmup(iterations = 100) + @Measurement(iterations = 500) + @BenchmarkMode(Mode.SingleShotTime) + public Meter remove() { + return registry.remove(registry.counter("counter", "key", String.valueOf(meterCount / 2))); + } + +} diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java index 4e0dc533db..6173a4e3cb 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java @@ -112,6 +112,12 @@ public abstract class MeterRegistry { */ private final Map preFilterIdToMeterMap = new HashMap<>(); + /** + * For reverse looking up pre-filter ID in {@link #preFilterIdToMeterMap} from the + * Meter being removed in {@link #remove(Id)}. Guarded by the {@link #meterMapLock}. + */ + private final Map meterToPreFilterIdMap = new HashMap<>(); + /** * Only needed when MeterFilter configured after Meters registered. Write/remove * guarded by meterMapLock, remove in {@link #unmarkStaleId(Id)} and other operations @@ -684,6 +690,7 @@ private Meter getOrCreateMeter(@Nullable DistributionStatisticConfig config, } meterMap.put(mappedId, m); preFilterIdToMeterMap.put(originalId, m); + meterToPreFilterIdMap.put(m, originalId); unmarkStaleId(originalId); } } @@ -774,14 +781,9 @@ public Meter remove(Meter.Id mappedId) { synchronized (meterMapLock) { final Meter removedMeter = meterMap.remove(mappedId); if (removedMeter != null) { - Iterator> iterator = preFilterIdToMeterMap.entrySet().iterator(); - while (iterator.hasNext()) { - Map.Entry nextEntry = iterator.next(); - if (nextEntry.getValue().equals(removedMeter)) { - stalePreFilterIds.remove(nextEntry.getKey()); - iterator.remove(); - } - } + Id preFilterIdToRemove = meterToPreFilterIdMap.remove(removedMeter); + preFilterIdToMeterMap.remove(preFilterIdToRemove); + stalePreFilterIds.remove(preFilterIdToRemove); Set synthetics = syntheticAssociations.remove(mappedId); if (synthetics != null) { @@ -1213,6 +1215,11 @@ Map _getPreFilterIdToMeterMap() { return Collections.unmodifiableMap(preFilterIdToMeterMap); } + // VisibleForTesting + Map _getMeterToPreFilterIdMap() { + return Collections.unmodifiableMap(meterToPreFilterIdMap); + } + // VisibleForTesting Set _getStalePreFilterIds() { return Collections.unmodifiableSet(stalePreFilterIds); diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/MeterRegistryTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/MeterRegistryTest.java index 25246d662b..678583b234 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/instrument/MeterRegistryTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/MeterRegistryTest.java @@ -287,10 +287,12 @@ void differentPreFilterIdsMapToSameId_thenCacheIsBounded() { // to the map because it would result in a memory leak with a high cardinality tag // that's otherwise limited in cardinality by a MeterFilter assertThat(registry._getPreFilterIdToMeterMap()).hasSize(1); + assertThat(registry._getMeterToPreFilterIdMap()).hasSize(1); assertThat(registry.remove(c1)).isSameAs(c2); assertThat(registry.getMeters()).isEmpty(); assertThat(registry._getPreFilterIdToMeterMap()).isEmpty(); + assertThat(registry._getMeterToPreFilterIdMap()).isEmpty(); } @Test @@ -318,6 +320,7 @@ void removingStaleMeterRemovesItFromAllInternalState() { registry.remove(c1.getId()); assertThat(registry.getMeters()).isEmpty(); assertThat(registry._getPreFilterIdToMeterMap()).isEmpty(); + assertThat(registry._getMeterToPreFilterIdMap()).isEmpty(); assertThat(registry._getStalePreFilterIds()).isEmpty(); } @@ -331,6 +334,8 @@ void multiplePreFilterIdsMapToSameId_removeByPreFilterId() { Meter.Id preFilterId = new Meter.Id("counter", Tags.of("secret", "value2"), null, null, Meter.Type.COUNTER); assertThat(registry.removeByPreFilterId(preFilterId)).isSameAs(c1).isSameAs(c2); assertThat(registry.getMeters()).isEmpty(); + assertThat(registry._getPreFilterIdToMeterMap()).isEmpty(); + assertThat(registry._getMeterToPreFilterIdMap()).isEmpty(); } @Test