Improved Tags performance

Signed-off-by: etki <[email protected]>

Author: etki

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

@@ -71,11 +71,20 @@ private static boolean isSortedSet(Tag[] tags, int length) {
         if (length > tags.length) {
             return false;
         }
+
+        // This style is intentionally chosen to have only one array
+        // access per iteration (tags[i].compareTo(tags[i + 1]) would
+        // have two).
+        Tag current = tags[0];
+        Tag next;
+
         for (int i = 0; i < length - 1; i++) {
-            int cmp = tags[i].compareTo(tags[i + 1]);
+            next = tags[i + 1];
+            int cmp = current.compareTo(next);
             if (cmp >= 0) {
                 return false;
             }
+            current = next;
         }
         return true;
     }
@@ -96,7 +105,8 @@ private static Tags toTags(Tag[] tags) {
     }
 
     /**
-     * Removes duplicate tags from an ordered array of tags.
+     * Removes duplicate tags from an ordered array of tags. In the case of several
+     * consecutive tags with the same key, only the last one is preserved.
      * @param tags an ordered array of {@code Tag} objects.
      * @return the number of unique tags in the {@code tags} array after removing
      * duplicates.
@@ -111,9 +121,24 @@ private static int dedup(Tag[] tags) {
         // index of next unique element
         int j = 0;
 
-        for (int i = 0; i < n - 1; i++)
-            if (!tags[i].getKey().equals(tags[i + 1].getKey()))
+        // The following is intentionally written in this style to facilitate performance.
+        // Normally one would just do tags[i].getKey().equals(tags[i + 1].getKey()),
+        // but the tags[i + 1].getKey() value is exactly the same as tags[i].getKey()
+        // for the next iteration, thus caching it in a variable halves the number
+        // of lookups.
+        // The compiler is very unlikely to do this for us because of the absence of
+        // "this data is immutable" signs, even if MM doesn't enforce any HB
+        // relations with external modifications in this case. You can run the
+        // associated benchmarks to check the behavior for your specific JVM.
+        String current = tags[0].getKey();
+        String next;
+
+        for (int i = 0; i < n - 1; i++) {
+            next = tags[i + 1].getKey();
+            if (!current.equals(next))
                 tags[j++] = tags[i];
+            current = next;
+        }
 
         tags[j++] = tags[n - 1];
         return j;
@@ -126,12 +151,14 @@ private static int dedup(Tag[] tags) {
      * @return a {@code Tags} instance with the merged sets of tags.
      */
     private Tags merge(Tags other) {
-        if (other.length == 0) {
-            return this;
+        if (length == 0) {
+            return other;
         }
-        if (Objects.equals(this, other)) {
+
+        if (other.length == 0 || tagsEqual(other)) {
             return this;
         }
+
         Tag[] sortedSet = new Tag[this.length + other.length];
         int sortedIndex = 0;
         int thisIndex = 0;
@@ -215,13 +242,12 @@ public Tags and(@Nullable Tag... tags) {
      * @return a new {@code Tags} instance
      */
     public Tags and(@Nullable Iterable<? extends Tag> tags) {
-        if (tags == null || tags == EMPTY || !tags.iterator().hasNext()) {
-            return this;
-        }
-
         if (this.length == 0) {
             return Tags.of(tags);
         }
+
+        // Tags.of() will take care of nulls, empty iterables and so on
+        // merge() then will check if the argument is empty and reduce to no-op
         return merge(Tags.of(tags));
     }
 
@@ -276,7 +302,7 @@ public int hashCode() {
 
     @Override
     public boolean equals(@Nullable Object obj) {
-        return this == obj || obj != null && getClass() == obj.getClass() && tagsEqual((Tags) obj);
+        return this == obj || (obj != null && getClass() == obj.getClass() && tagsEqual((Tags) obj));
     }
 
     private boolean tagsEqual(Tags obj) {
@@ -323,16 +349,19 @@ public static Tags concat(@Nullable Iterable<? extends Tag> tags, @Nullable Stri
      * @return a new {@code Tags} instance
      */
     public static Tags of(@Nullable Iterable<? extends Tag> tags) {
-        if (tags == null || tags == EMPTY || !tags.iterator().hasNext()) {
-            return Tags.empty();
-        }
-        else if (tags instanceof Tags) {
+        if (tags instanceof Tags) {
             return (Tags) tags;
         }
         else if (tags instanceof Collection) {
             Collection<? extends Tag> tagsCollection = (Collection<? extends Tag>) tags;
+            if (tagsCollection.isEmpty()) {
+                return Tags.empty();
+            }
             return toTags(tagsCollection.toArray(EMPTY_TAG_ARRAY));
         }
+        else if (emptyIterable(tags)) {
+            return Tags.empty();
+        }
         else {
             return toTags(StreamSupport.stream(tags.spliterator(), false).toArray(Tag[]::new));
         }
@@ -346,7 +375,7 @@ else if (tags instanceof Collection) {
      * @return a new {@code Tags} instance
      */
     public static Tags of(String key, String value) {
-        return new Tags(new Tag[] { Tag.of(key, value) }, 1);
+        return of(Tag.of(key, value));
     }
 
     /**
@@ -373,6 +402,26 @@ private static boolean blankVarargs(@Nullable Object[] args) {
         return args == null || args.length == 0 || (args.length == 1 && args[0] == null);
     }
 
+    private static boolean emptyIterable(@Nullable Iterable<? extends Tag> iterable) {
+        // Doing the checks in the ascending cost order
+        if (iterable == null || iterable == EMPTY) {
+            return true;
+        }
+
+        if (iterable instanceof Tags) {
+            return ((Tags) iterable).length == 0;
+        }
+
+        if (iterable instanceof Collection) {
+            return ((Collection<?>) iterable).isEmpty();
+        }
+
+        // While the compiler can theoretically avoid Iterator allocation here
+        // (via scalarization), it is not guaranteed, thus leaving this check as the last
+        // one
+        return !iterable.iterator().hasNext();
+    }
+
     /**
      * Return a new {@code Tags} instance containing tags constructed from the specified
      * tags.

micrometer-core/src/test/java/io/micrometer/core/instrument/TagsTest.java

@@ -18,7 +18,10 @@
 import com.sun.management.ThreadMXBean;
 import io.micrometer.core.Issue;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.condition.*;
+import org.junit.jupiter.api.condition.DisabledIfSystemProperty;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 import java.lang.management.ManagementFactory;
 import java.util.*;
@@ -139,6 +142,27 @@ void concatOnTwoTagsWithSameKeyAreMergedIntoOneTag() {
         assertThat(tags).containsExactly(Tag.of("k", "v2"));
     }
 
+    static Stream<Arguments> concatenatedIterables() {
+        return Stream.of(Arguments.of(Tags.empty(), Tags.empty(), Tags.empty()),
+                Arguments.of(Tags.of("k1", "v1"), Tags.empty(), Tags.of("k1", "v1")),
+                Arguments.of(Tags.empty(), Tags.of("k1", "v1"), Tags.of("k1", "v1")),
+                Arguments.of(Tags.of("k1", "v1", "k2", "v2", "k4", "v4", "k3", "v3", "k5", "v5"),
+                        Tags.of("k0", "v0", "k2", "override", "k4", "override", "k6", "v6", "k7", "v7"),
+                        Tags.of("k0", "v0", "k1", "v1", "k2", "override", "k3", "v3", "k4", "override", "k5", "v5",
+                                "k6", "v6", "k7", "v7")));
+    }
+
+    @ParameterizedTest
+    @MethodSource("concatenatedIterables")
+    void concatOnTwoIterablesWithSameKeyAreMergedIntoOneTag(Tags left, Tags right, Tags expectation) {
+        // Converting to classes that are only iterables, not collections nor Tags
+        Iterable<Tag> first = left::iterator;
+        Iterable<Tag> second = right::iterator;
+
+        Iterable<Tag> tags = Tags.concat(first, second);
+        assertThat(tags).containsExactlyElementsOf(expectation);
+    }
+
     @Issue("#3851")
     @Test
     void concatWhenKeyValuesAreNullShouldReturnCurrentInstance() {