Fix performance regression in MeterRegistry#remove (micrometer-metric…

…s#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 micrometer-metricsgh-5466

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)));
+    }
+
+}

micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java

@@ -112,6 +112,12 @@ public abstract class MeterRegistry {
      */
     private final Map<Id, Meter> 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<Meter, Id> 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<Map.Entry<Id, Meter>> iterator = preFilterIdToMeterMap.entrySet().iterator();
-                    while (iterator.hasNext()) {
-                        Map.Entry<Id, Meter> 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<Id> synthetics = syntheticAssociations.remove(mappedId);
                     if (synthetics != null) {
@@ -1213,6 +1215,11 @@ Map<Id, Meter> _getPreFilterIdToMeterMap() {
         return Collections.unmodifiableMap(preFilterIdToMeterMap);
     }
 
+    // VisibleForTesting
+    Map<Meter, Id> _getMeterToPreFilterIdMap() {
+        return Collections.unmodifiableMap(meterToPreFilterIdMap);
+    }
+
     // VisibleForTesting
     Set<Id> _getStalePreFilterIds() {
         return Collections.unmodifiableSet(stalePreFilterIds);

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