Have AttributesMap extend HashMap instead of contain LinkedHashMap (#2906)

* Reduce HashMp allocations for AttributeMap.

* Finish

* Add comment on why wrapper is actually needed.

api/all/src/main/java/io/opentelemetry/api/baggage/Baggage.java

@@ -79,7 +79,7 @@ public interface Baggage extends ImplicitContextKeyed {
   }
 
   /** Iterates over all the entries in this {@link Baggage}. */
-  void forEach(BiConsumer<String, BaggageEntry> consumer);
+  void forEach(BiConsumer<? super String, ? super BaggageEntry> consumer);
 
   /** Returns a read-only view of this {@link Baggage} as a {@link Map}. */
   Map<String, BaggageEntry> asMap();

api/all/src/main/java/io/opentelemetry/api/common/Attributes.java

@@ -36,7 +36,7 @@ public interface Attributes {
   <T> T get(AttributeKey<T> key);
 
   /** Iterates over all the key-value pairs of attributes contained by this instance. */
-  void forEach(BiConsumer<AttributeKey<?>, Object> consumer);
+  void forEach(BiConsumer<? super AttributeKey<?>, ? super Object> consumer);
 
   /** The number of attributes contained in this. */
   int size();

api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java

@@ -81,7 +81,7 @@ public abstract class ImmutableKeyValuePairs<K, V> {
 
   /** Iterates over all the key-value pairs of labels contained by this instance. */
   @SuppressWarnings("unchecked")
-  public final void forEach(BiConsumer<K, V> consumer) {
+  public final void forEach(BiConsumer<? super K, ? super V> consumer) {
     if (consumer == null) {
       return;
     }

api/metrics/src/main/java/io/opentelemetry/api/metrics/common/Labels.java

@@ -102,7 +102,7 @@ public interface Labels {
   }
 
   /** Iterates over all the key-value pairs of labels contained by this instance. */
-  void forEach(BiConsumer<String, String> consumer);
+  void forEach(BiConsumer<? super String, ? super String> consumer);
 
   /** The number of key-value pairs of labels in this instance. */
   int size();

build.gradle.kts

@@ -127,6 +127,7 @@ subprojects {
             withType(JavaCompile::class) {
                 options.release.set(8)
 
+                if (name != "jmhCompileGeneratedClasses") {
                     options.compilerArgs.addAll(listOf(
                             "-Xlint:all",
                             // We suppress the "try" warning because it disallows managing an auto-closeable with
@@ -141,6 +142,7 @@ subprojects {
                             // Fail build on any warning
                             "-Werror"
                     ))
+                }
 
                 options.encoding = "UTF-8"
 
@@ -383,6 +385,7 @@ subprojects {
             // Always include the jmhreport plugin and run it after jmh task.
             plugins.apply("io.morethan.jmhreport")
             dependencies {
+                add("jmh", platform(project(":dependencyManagement")))
                 add("jmh", "org.openjdk.jmh:jmh-core")
                 add("jmh", "org.openjdk.jmh:jmh-generator-bytecode")
             }

dependencyManagement/build.gradle.kts

@@ -55,7 +55,7 @@ val DEPENDENCY_SETS = listOf(
         ),
         DependencySet(
                 "org.openjdk.jmh",
-                "1.26",
+                "1.27",
                 listOf("jmh-core", "jmh-generator-bytecode")
         ),
         DependencySet(

sdk/trace/src/jmh/java/io/opentelemetry/sdk/trace/FillSpanBenchmark.java

@@ -0,0 +1,42 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.sdk.trace;
+
+import io.opentelemetry.api.common.AttributeKey;
+import io.opentelemetry.api.trace.SpanBuilder;
+import java.util.concurrent.TimeUnit;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Warmup;
+
+@Threads(value = 1)
+@Fork(3)
+@Warmup(iterations = 10, time = 1)
+@Measurement(iterations = 20, time = 1)
+@OutputTimeUnit(TimeUnit.MILLISECONDS)
+public class FillSpanBenchmark {
+
+  private static final SpanBuilder spanBuilder =
+      SdkTracerProvider.builder().build().get("benchmark").spanBuilder("benchmark");
+
+  private static final AttributeKey<String> KEY1 = AttributeKey.stringKey("key1");
+  private static final AttributeKey<String> KEY2 = AttributeKey.stringKey("key2");
+  private static final AttributeKey<String> KEY3 = AttributeKey.stringKey("key3");
+  private static final AttributeKey<String> KEY4 = AttributeKey.stringKey("key4");
+
+  @Benchmark
+  public void setFourAttributes() {
+    spanBuilder
+        .startSpan()
+        .setAttribute(KEY1, "value1")
+        .setAttribute(KEY2, "value2")
+        .setAttribute(KEY3, "value3")
+        .setAttribute(KEY4, "value4");
+  }
+}

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/AttributesMap.java

@@ -9,40 +9,32 @@ import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
 import java.util.Collections;
-import java.util.LinkedHashMap;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.function.BiConsumer;
 
-/**
- * A map with a fixed capacity that drops attributes when the map gets full.
- *
- * <p>Note: this doesn't implement the Map interface, but behaves very similarly to one.
- */
+/** A map with a fixed capacity that drops attributes when the map gets full. */
 @SuppressWarnings({"rawtypes", "unchecked"})
-final class AttributesMap implements Attributes {
-  private final Map<AttributeKey<?>, Object> data;
+final class AttributesMap extends HashMap<AttributeKey<?>, Object> implements Attributes {
+
+  private static final long serialVersionUID = -5072696312123632376L;
 
   private final long capacity;
   private int totalAddedValues = 0;
 
-  private AttributesMap(long capacity, Map<AttributeKey<?>, Object> data) {
-    this.capacity = capacity;
-    this.data = data;
-  }
-
   AttributesMap(long capacity) {
-    this(capacity, new LinkedHashMap<>());
+    this.capacity = capacity;
   }
 
-  public <T> void put(AttributeKey<T> key, T value) {
+  <T> void put(AttributeKey<T> key, T value) {
     if (key == null || key.getKey() == null || value == null) {
       return;
     }
     totalAddedValues++;
-    if (data.size() >= capacity && !data.containsKey(key)) {
+    if (size() >= capacity && !containsKey(key)) {
       return;
     }
-    data.put(key, value);
+    super.put(key, value);
   }
 
   int getTotalAddedValues() {
@@ -52,32 +44,27 @@ final class AttributesMap implements Attributes {
   @SuppressWarnings("unchecked")
   @Override
   public <T> T get(AttributeKey<T> key) {
-    return (T) data.get(key);
-  }
-
-  @Override
-  public int size() {
-    return data.size();
-  }
-
-  @Override
-  public boolean isEmpty() {
-    return data.isEmpty();
+    return (T) super.get(key);
   }
 
   @SuppressWarnings({"rawtypes", "unchecked"})
   @Override
-  public void forEach(BiConsumer<AttributeKey<?>, Object> consumer) {
-    for (Map.Entry<AttributeKey<?>, Object> entry : data.entrySet()) {
+  public void forEach(BiConsumer<? super AttributeKey<?>, ? super Object> consumer) {
+    for (Map.Entry<AttributeKey<?>, Object> entry : entrySet()) {
       AttributeKey key = entry.getKey();
       Object value = entry.getValue();
-      consumer.accept(key, value);
+      consumer.accept((AttributeKey<?>) key, value);
     }
   }
 
   @Override
   public Map<AttributeKey<?>, Object> asMap() {
-    return Collections.unmodifiableMap(data);
+    // Because Attributes is marked Immutable, IDEs may recognize this as redundant usage. However,
+    // this class is private and is actually mutable, so we need to wrap with unmodifiableMap
+    // anyways. We implement the immutable Attributes for this class to support the
+    // Attributes.builder().putAll usage - it is tricky but an implementation detail of this private
+    // class.
+    return Collections.unmodifiableMap(this);
   }
 
   @Override
@@ -89,7 +76,7 @@ final class AttributesMap implements Attributes {
   public String toString() {
     return "AttributesMap{"
         + "data="
-        + data
+        + super.toString()
         + ", capacity="
         + capacity
         + ", totalAddedValues="
@@ -98,7 +85,6 @@ final class AttributesMap implements Attributes {
   }
 
   Attributes immutableCopy() {
-    Map<AttributeKey<?>, Object> dataCopy = new LinkedHashMap<>(data);
-    return new AttributesMap(capacity, Collections.unmodifiableMap(dataCopy));
+    return Attributes.builder().putAll(this).build();
   }
 }

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/AttributesMapTest.java

@@ -9,29 +9,9 @@ import static io.opentelemetry.api.common.AttributeKey.longKey;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.entry;
 
-import io.opentelemetry.api.common.AttributeKey;
-import io.opentelemetry.api.common.Attributes;
-import java.util.Arrays;
-import java.util.List;
-import java.util.function.BiConsumer;
 import org.junit.jupiter.api.Test;
 
 class AttributesMapTest {
-  @Test
-  void attributesAreInOrder() {
-    AttributesMap attributesMap = new AttributesMap(5);
-    attributesMap.put(longKey("one"), 1L);
-    attributesMap.put(longKey("three"), 3L);
-    attributesMap.put(longKey("two"), 2L);
-    attributesMap.put(longKey("four"), 4L);
-    attributesMap.put(longKey("five"), 5L);
-
-    List<String> expectedKeyOrder = Arrays.asList("one", "three", "two", "four", "five");
-    List<Long> expectedValueOrder = Arrays.asList(1L, 3L, 2L, 4L, 5L);
-
-    assertOrdering(attributesMap, expectedKeyOrder, expectedValueOrder);
-    assertOrdering(attributesMap.immutableCopy(), expectedKeyOrder, expectedValueOrder);
-  }
 
   @Test
   void asMap() {
@@ -40,22 +20,6 @@ class AttributesMapTest {
     attributesMap.put(longKey("two"), 2L);
 
     assertThat(attributesMap.asMap())
-        .containsExactly(entry(longKey("one"), 1L), entry(longKey("two"), 2L));
-  }
-
-  private void assertOrdering(
-      Attributes attributes, List<String> expectedKeyOrder, List<Long> expectedValueOrder) {
-    attributes.forEach(
-        new BiConsumer<AttributeKey<?>, Object>() {
-          private int counter = 0;
-
-          @Override
-          public void accept(AttributeKey<?> key, Object value) {
-            String k = key.getKey();
-            Long val = (Long) value;
-            assertThat(val).isEqualTo(expectedValueOrder.get(counter));
-            assertThat(k).isEqualTo(expectedKeyOrder.get(counter++));
-          }
-        });
+        .containsOnly(entry(longKey("one"), 1L), entry(longKey("two"), 2L));
   }
 }

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java

@@ -875,8 +875,7 @@ class SdkSpanBuilderTest {
                 + "kind=INTERNAL, "
                 + "startEpochNanos=[0-9]+, "
                 + "endEpochNanos=[0-9]+, "
-                + "attributes=AttributesMap\\{data=\\{http.status_code=500, "
-                + "http.url=https://opentelemetry.io}, capacity=128, totalAddedValues=2}, "
+                + "attributes=AttributesMap\\{data=\\{[^}]*}, capacity=128, totalAddedValues=2}, "
                 + "totalAttributeCount=2, "
                 + "events=\\[], "
                 + "totalRecordedEvents=0, "

settings.gradle.kts

@@ -8,7 +8,7 @@ pluginManagement {
         id("de.undercouch.download") version "4.1.1"
         id("io.codearte.nexus-staging") version "0.22.0"
         id("io.morethan.jmhreport") version "0.9.0"
-        id("me.champeau.gradle.jmh") version "0.5.2"
+        id("me.champeau.gradle.jmh") version "0.5.3"
         id("nebula.release") version "15.3.0"
         id("net.ltgt.errorprone") version "1.3.0"
         id("org.jetbrains.kotlin.jvm") version "1.4.21"