diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/Tags.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/Tags.java index 1c696066e8..2c0ec32e4a 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/Tags.java +++ b/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 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 tags, @Nullable Stri * @return a new {@code Tags} instance */ public static Tags of(@Nullable Iterable 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 tagsCollection = (Collection) 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 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. diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/TagsTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/TagsTest.java index b382b9647a..9dc82045c5 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/instrument/TagsTest.java +++ b/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 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 first = left::iterator; + Iterable second = right::iterator; + + Iterable tags = Tags.concat(first, second); + assertThat(tags).containsExactlyElementsOf(expectation); + } + @Issue("#3851") @Test void concatWhenKeyValuesAreNullShouldReturnCurrentInstance() {