Enforce last-value-wins semantics in AttributesMap without performance regression

sdk/common/src/jmh/java/io/opentelemetry/sdk/common/internal/AttributesMapBenchmark.java

@@ -0,0 +1,136 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.sdk.common.internal;
+
+import static io.opentelemetry.api.common.AttributeKey.booleanKey;
+import static io.opentelemetry.api.common.AttributeKey.stringKey;
+
+import io.opentelemetry.api.common.AttributeKey;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+/**
+ * Microbenchmark for {@link AttributesMap}. Parametrized by number of attributes.
+ *
+ * <p>Write scenarios:
+ *
+ * <ul>
+ *   <li>{@code uniqueKeys} — normal production case: each key name is distinct
+ *   <li>{@code putThenForEach} — combined write + export cycle: N unique puts followed by one
+ *       {@code forEach}, modeling the dominant span lifecycle
+ * </ul>
+ *
+ * <p>Read scenarios (run on a pre-filled map of {@code numAttributes} unique string entries):
+ *
+ * <ul>
+ *   <li>{@code getHit} — lookup with the exact stored key type (hit)
+ *   <li>{@code getTypeMiss} — lookup with a different type for the same key name (type miss)
+ *   <li>{@code forEachAll} — full iteration over all entries
+ * </ul>
+ */
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+@Warmup(iterations = 5, time = 200, timeUnit = TimeUnit.MILLISECONDS)
+@Measurement(iterations = 10, time = 200, timeUnit = TimeUnit.MILLISECONDS)
+@Fork(2)
+@State(Scope.Thread)
+public class AttributesMapBenchmark {
+
+  @Param({"4", "16", "128"})
+  int numAttributes;
+
+  private List<AttributeKey<String>> stringKeys;
+  private List<AttributeKey<Boolean>> boolKeys;
+  private List<String> values;
+
+  // Pre-filled map used by read benchmarks — populated once in @Setup.
+  private AttributesMap filledMap;
+
+  @Setup
+  public void setup() {
+    stringKeys = new ArrayList<>(numAttributes);
+    boolKeys = new ArrayList<>(numAttributes);
+    values = new ArrayList<>(numAttributes);
+    for (int i = 0; i < numAttributes; i++) {
+      stringKeys.add(stringKey("key" + i));
+      boolKeys.add(booleanKey("key" + i));
+      values.add("value" + i);
+    }
+    filledMap = AttributesMap.create(numAttributes, Integer.MAX_VALUE);
+    for (int i = 0; i < numAttributes; i++) {
+      filledMap.put(stringKeys.get(i), values.get(i));
+    }
+  }
+
+  /** Each key name is unique — the common production case. */
+  @Benchmark
+  public AttributesMap uniqueKeys() {
+    AttributesMap map = AttributesMap.create(numAttributes, Integer.MAX_VALUE);
+    for (int i = 0; i < numAttributes; i++) {
+      map.put(stringKeys.get(i), values.get(i));
+    }
+    return map;
+  }
+
+  // ---- Read benchmarks (operate on pre-filled map) ----
+
+  /**
+   * Lookup with the exact stored key type — always a hit. Measures the cost of a successful {@code
+   * get()} for each entry in the map.
+   */
+  @Benchmark
+  public void getHit(Blackhole bh) {
+    for (int i = 0; i < numAttributes; i++) {
+      bh.consume(filledMap.get(stringKeys.get(i)));
+    }
+  }
+
+  /**
+   * Lookup with a different type for the same key name — always returns null.
+   *
+   * <p>The map holds N string-typed entries; boolean keys for the same names locate each entry by
+   * name but fail the type check. Isolates the cost of a name-hit / type-miss lookup.
+   */
+  @Benchmark
+  public void getTypeMiss(Blackhole bh) {
+    for (int i = 0; i < numAttributes; i++) {
+      bh.consume(filledMap.get(boolKeys.get(i)));
+    }
+  }
+
+  /** Full iteration over all entries via {@code forEach}. */
+  @Benchmark
+  public void forEachAll(Blackhole bh) {
+    filledMap.forEach((k, v) -> bh.consume(v));
+  }
+
+  /**
+   * Combined write + read cycle: fill a fresh map with N unique string keys, then iterate all
+   * entries once. Models the dominant production path: N puts during span building, followed by one
+   * forEach at export time.
+   */
+  @Benchmark
+  public void putThenForEach(Blackhole bh) {
+    AttributesMap map = AttributesMap.create(numAttributes, Integer.MAX_VALUE);
+    for (int i = 0; i < numAttributes; i++) {
+      map.put(stringKeys.get(i), values.get(i));
+    }
+    map.forEach((k, v) -> bh.consume(v));
+  }
+}

sdk/common/src/main/java/io/opentelemetry/sdk/common/internal/AttributesMap.java

@@ -8,6 +8,7 @@
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -18,28 +19,50 @@
  * A map with a fixed capacity that drops attributes when the map gets full, and which truncates
  * string and array string attribute values to the {@link #lengthLimit}.
  *
- * <p>WARNING: In order to reduce memory allocation, this class extends {@link HashMap} when it
- * would be more appropriate to delegate. The problem with extending is that we don't enforce that
- * all {@link HashMap} methods for reading / writing data conform to the configured attribute
- * limits. Therefore, it's easy to accidentally call something like {@link Map#putAll(Map)} and
- * bypass the restrictions (see <a
- * href="https://github.com/open-telemetry/opentelemetry-java/issues/7135">#7135</a>). Callers MUST
- * take care to only call methods from {@link AttributesMap}, and not {@link HashMap}.
+ * <p>Keyed internally by attribute name, so that attributes with the same name but different types
+ * are treated as the same key (last-value-wins), consistent with the OpenTelemetry specification.
+ *
+ * <p>Backed by parallel arrays and an open-addressing {@code int[]} hash table (linear probing,
+ * load factor ≤ 0.5). Avoids per-entry object allocation; {@code forEach} is a tight sequential
+ * array loop with no pointer chasing.
  *
  * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
  * at any time.
  */
-public final class AttributesMap extends HashMap<AttributeKey<?>, Object> implements Attributes {
+public final class AttributesMap implements Attributes {
 
-  private static final long serialVersionUID = -5072696312123632376L;
+  /**
+   * Sentinel value meaning "slot is empty" in the hash table. Using 0 lets {@code new int[n]} (JVM
+   * zero-initialization) serve as the initial fill, eliminating explicit {@code Arrays.fill} calls.
+   * Occupied slots store {@code entryIndex + 1} so that index 0 is distinguishable from EMPTY.
+   */
+  private static final int EMPTY = 0;
 
   private final long capacity;
   private final int lengthLimit;
   private int totalAddedValues = 0;
+  private int size = 0;
+
+  /**
+   * Open-addressing hash table: {@code hashTable[slot]} = index into entry arrays, or {@link
+   * #EMPTY}. Length is always a power of 2 and ≥ 2× the entry array length (load factor ≤ 0.5).
+   */
+  private int[] hashTable;
+
+  /** Parallel entry arrays. Index {@code i} holds the i-th inserted entry. */
+  private String[] entryNames;
+
+  private AttributeKey<?>[] entryKeys;
+  private Object[] entryValues;
 
   private AttributesMap(long capacity, int lengthLimit) {
     this.capacity = capacity;
     this.lengthLimit = lengthLimit;
+    int init = (int) Math.min(capacity, 16L);
+    entryNames = new String[init];
+    entryKeys = new AttributeKey<?>[init];
+    entryValues = new Object[init];
+    hashTable = new int[tableSizeFor(init)]; // JVM zero-init == EMPTY
   }
 
   /**
@@ -55,18 +78,40 @@ public static AttributesMap create(long capacity, int lengthLimit) {
   /**
    * Add the attribute key value pair, applying capacity and length limits. Callers MUST ensure the
    * {@code value} type matches the type required by {@code key}.
+   *
+   * <p>If an attribute with the same string key name already exists (regardless of type), it is
+   * overwritten — last-value-wins, consistent with the OTel spec.
    */
-  @Override
   @Nullable
   public Object put(AttributeKey<?> key, @Nullable Object value) {
     if (value == null) {
       return null;
     }
     totalAddedValues++;
-    if (size() >= capacity && !containsKey(key)) {
+    String name = key.getKey();
+    int slot = findSlot(name);
+    int stored = hashTable[slot]; // EMPTY or 1-based index
+    if (stored == EMPTY) {
+      if (size >= capacity) {
+        return null;
+      }
+      Object limited = AttributeUtil.applyAttributeLengthLimit(value, lengthLimit);
+      if (size == entryNames.length) {
+        grow();
+        slot = findSlot(name); // hashTable was rebuilt by grow()
+      }
+      entryNames[size] = name;
+      entryKeys[size] = key;
+      entryValues[size] = limited;
+      hashTable[slot] = size + 1; // 1-based
+      size++;
       return null;
     }
-    return super.put(key, AttributeUtil.applyAttributeLengthLimit(value, lengthLimit));
+    int idx = stored - 1;
+    Object old = entryValues[idx];
+    entryKeys[idx] = key;
+    entryValues[idx] = AttributeUtil.applyAttributeLengthLimit(value, lengthLimit);
+    return old;
   }
 
   /** Generic overload of {@link #put(AttributeKey, Object)}. */
@@ -83,17 +128,34 @@ public int getTotalAddedValues() {
   @Override
   @Nullable
   public <T> T get(AttributeKey<T> key) {
-    return (T) super.get(key);
+    int stored = hashTable[findSlot(key.getKey())];
+    if (stored == EMPTY) {
+      return null;
+    }
+    int idx = stored - 1;
+    if (!entryKeys[idx].getType().equals(key.getType())) {
+      return null;
+    }
+    return (T) entryValues[idx];
+  }
+
+  @Override
+  public int size() {
+    return size;
+  }
+
+  @Override
+  public boolean isEmpty() {
+    return size == 0;
   }
 
   @Override
   public Map<AttributeKey<?>, Object> asMap() {
-    // 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);
+    Map<AttributeKey<?>, Object> snapshot = new HashMap<>(size);
+    for (int i = 0; i < size; i++) {
+      snapshot.put(entryKeys[i], entryValues[i]);
+    }
+    return Collections.unmodifiableMap(snapshot);
   }
 
   @Override
@@ -103,17 +165,32 @@ public AttributesBuilder toBuilder() {
 
   @Override
   public void forEach(BiConsumer<? super AttributeKey<?>, ? super Object> action) {
-    // https://github.com/open-telemetry/opentelemetry-java/issues/4161
-    // Help out android desugaring by having an explicit call to HashMap.forEach, when forEach is
-    // just called through Attributes.forEach desugaring is unable to correctly handle it.
-    super.forEach(action);
+    for (int i = 0; i < size; i++) {
+      action.accept(entryKeys[i], entryValues[i]);
+    }
+  }
+
+  @Override
+  public boolean equals(@Nullable Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (!(o instanceof Attributes)) {
+      return false;
+    }
+    return asMap().equals(((Attributes) o).asMap());
+  }
+
+  @Override
+  public int hashCode() {
+    return asMap().hashCode();
   }
 
   @Override
   public String toString() {
     return "AttributesMap{"
         + "data="
-        + super.toString()
+        + asMap()
         + ", capacity="
         + capacity
         + ", totalAddedValues="
@@ -125,4 +202,47 @@ public String toString() {
   public Attributes immutableCopy() {
     return Attributes.builder().putAll(this).build();
   }
+
+  /**
+   * Returns the hash table slot that either contains the entry for {@code name} or is the first
+   * empty slot available for insertion. Single shared probe loop used by {@code put}, {@code get},
+   * and {@code grow}. Slots store {@code entryIndex + 1}; 0 ({@link #EMPTY}) means unoccupied.
+   */
+  private int findSlot(String name) {
+    int mask = hashTable.length - 1;
+    int slot = name.hashCode() & mask;
+    int stored;
+    while ((stored = hashTable[slot]) != EMPTY && !entryNames[stored - 1].equals(name)) {
+      slot = (slot + 1) & mask;
+    }
+    return slot;
+  }
+
+  private void grow() {
+    long maxLen = Math.min(capacity, (long) Integer.MAX_VALUE - 8);
+    int newLen = (int) Math.min((long) entryNames.length * 2, maxLen);
+    entryNames = Arrays.copyOf(entryNames, newLen);
+    entryKeys = Arrays.copyOf(entryKeys, newLen);
+    entryValues = Arrays.copyOf(entryValues, newLen);
+    hashTable = new int[tableSizeFor(newLen)]; // JVM zero-init == EMPTY
+    for (int i = 0; i < size; i++) {
+      int slot = findSlot(entryNames[i]);
+      hashTable[slot] = i + 1; // 1-based
+    }
+  }
+
+  /**
+   * Returns the smallest power of 2 that is ≥ 2n, guaranteeing load factor ≤ 0.5. Using {@code
+   * (2n-1)} instead of {@code 2n} prevents doubling the result when {@code n} is itself a power of
+   * 2.
+   */
+  private static int tableSizeFor(int n) {
+    if (n <= 2) {
+      return 4;
+    }
+    if (n >= (1 << 29)) {
+      return 1 << 30;
+    }
+    return Integer.highestOneBit(2 * n - 1) << 1;
+  }
 }