chore: reduce hashmap allocations

Signed-off-by: Todd Baert <[email protected]>
open-feature · Oct 22, 2024 · 5c86f3a · 5c86f3a
1 parent cedad9c
commit 5c86f3a
Show file tree

Hide file tree

Showing 9 changed files with 254 additions and 158 deletions.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -35,10 +35,13 @@ mvn test -P e2e
 There is a small JMH benchmark suite for testing allocations that can be run with:
 
 ```sh
-mvn -P benchmark test-compile jmh:benchmark -Djmh.f=1  -Djmh.prof='dev.openfeature.sdk.benchmark.AllocationProfiler'
+mvn -P benchmark clean compile test-compile jmh:benchmark -Djmh.f=1  -Djmh.prof='dev.openfeature.sdk.benchmark.AllocationProfiler'
 ```
 
 If you are concerned about the repercussions of a change on memory usage, run this an compare the results to the committed. `benchmark.txt` file.
+Note that the ONLY MEANINGFUL RESULTS of this benchmark are the `totalAllocatedBytes` and the `totalAllocatedInstances`.
+The `run` score, and maven task time are not relevant since this benchmark is purely memory-related and has nothing to do with speed.
+You can also view the heap breakdown to see which objects are taking up the most memory.
 
 ## Releasing
 

diff --git a/benchmark.txt b/benchmark.txt
diff --git a/src/main/java/dev/openfeature/sdk/AbstractStructure.java b/src/main/java/dev/openfeature/sdk/AbstractStructure.java
@@ -18,7 +18,15 @@ public boolean isEmpty() {
     }
 
     AbstractStructure(Map<String, Value> attributes) {
-        this.attributes = new HashMap<>(attributes);
+        this.attributes = attributes;
+    }
+
+    /**
+     * Returns an unmodifiable representation of the internal attribute map.
+     * @return immutable map
+     */
+    public Map<String, Value> asUnmodifiableMap() {
+        return Map.copyOf(attributes);
     }
 
     /**

diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java
@@ -3,6 +3,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.function.Function;
+
 import dev.openfeature.sdk.internal.ExcludeFromGeneratedCoverageReport;
 import lombok.ToString;
 import lombok.experimental.Delegate;
@@ -42,7 +43,7 @@ public ImmutableContext(String targetingKey) {
      * @param attributes evaluation context attributes
      */
     public ImmutableContext(Map<String, Value> attributes) {
-        this("", attributes);
+        this(null, attributes);
     }
 
     /**
@@ -53,9 +54,7 @@ public ImmutableContext(Map<String, Value> attributes) {
      */
     public ImmutableContext(String targetingKey, Map<String, Value> attributes) {
         if (targetingKey != null && !targetingKey.trim().isEmpty()) {
-            final Map<String, Value> actualAttribs = new HashMap<>(attributes);
-            actualAttribs.put(TARGETING_KEY, new Value(targetingKey));
-            this.structure = new ImmutableStructure(actualAttribs);
+            this.structure = new ImmutableStructure(targetingKey, attributes);
         } else {
             this.structure = new ImmutableStructure(attributes);
         }
@@ -79,14 +78,14 @@ public String getTargetingKey() {
     @Override
     public EvaluationContext merge(EvaluationContext overridingContext) {
         if (overridingContext == null || overridingContext.isEmpty()) {
-            return new ImmutableContext(this.asMap());
+            return new ImmutableContext(this.asUnmodifiableMap());
         }
         if (this.isEmpty()) {
-            return new ImmutableContext(overridingContext.asMap());
+            return new ImmutableContext(overridingContext.asUnmodifiableMap());
         }
 
         return new ImmutableContext(
-                this.merge(ImmutableStructure::new, this.asMap(), overridingContext.asMap()));
+                this.merge(ImmutableStructure::new, this.asUnmodifiableMap(), overridingContext.asUnmodifiableMap()));
     }
 
     @SuppressWarnings("all")

diff --git a/src/main/java/dev/openfeature/sdk/ImmutableStructure.java b/src/main/java/dev/openfeature/sdk/ImmutableStructure.java
@@ -36,7 +36,11 @@ public ImmutableStructure() {
      * @param attributes attributes.
      */
     public ImmutableStructure(Map<String, Value> attributes) {
-        super(copyAttributes(attributes));
+        super(copyAttributes(attributes, null));
+    }
+
+    protected ImmutableStructure(String targetingKey, Map<String, Value> attributes) {
+        super(copyAttributes(attributes, targetingKey));
     }
 
     @Override
@@ -62,11 +66,18 @@ public Map<String, Value> asMap() {
     }
 
     private static Map<String, Value> copyAttributes(Map<String, Value> in) {
+        return copyAttributes(in, null);
+    }
+
+    private static Map<String, Value> copyAttributes(Map<String, Value> in, String targetingKey) {
         Map<String, Value> copy = new HashMap<>();
         for (Entry<String, Value> entry : in.entrySet()) {
             copy.put(entry.getKey(),
                     Optional.ofNullable(entry.getValue()).map((Value val) -> val.clone()).orElse(null));
         }
+        if (targetingKey != null) {
+            copy.put(EvaluationContext.TARGETING_KEY, new Value(targetingKey));
+        }
         return copy;
     }
 

diff --git a/src/main/java/dev/openfeature/sdk/MutableContext.java b/src/main/java/dev/openfeature/sdk/MutableContext.java
@@ -33,7 +33,7 @@ public MutableContext(String targetingKey) {
     }
 
     public MutableContext(Map<String, Value> attributes) {
-        this("", attributes);
+        this("", new HashMap<>(attributes));
     }
 
     /**
@@ -44,7 +44,7 @@ public MutableContext(Map<String, Value> attributes) {
      * @param attributes   evaluation context attributes
      */
     public MutableContext(String targetingKey, Map<String, Value> attributes) {
-        this.structure = new MutableStructure(attributes);
+        this.structure = new MutableStructure(new HashMap<>(attributes));
         if (targetingKey != null && !targetingKey.trim().isEmpty()) {
             this.structure.attributes.put(TARGETING_KEY, new Value(targetingKey));
         }
@@ -122,7 +122,7 @@ public EvaluationContext merge(EvaluationContext overridingContext) {
         }
 
         Map<String, Value> merged = this.merge(
-                MutableStructure::new, this.asMap(), overridingContext.asMap());
+                MutableStructure::new, this.asUnmodifiableMap(), overridingContext.asUnmodifiableMap());
         return new MutableContext(merged);
     }
 

diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
@@ -110,7 +110,6 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
         FlagEvaluationOptions flagOptions = ObjectUtils.defaultIfNull(options,
                 () -> FlagEvaluationOptions.builder().build());
         Map<String, Object> hints = Collections.unmodifiableMap(flagOptions.getHookHints());
-        ctx = ObjectUtils.defaultIfNull(ctx, () -> new ImmutableContext());
 
         FlagEvaluationDetails<T> details = null;
         List<Hook> mergedHooks = null;
@@ -183,17 +182,29 @@ private static <T> void enrichDetailsWithErrorDefaults(T defaultValue, FlagEvalu
      * @return merged evaluation context
      */
     private EvaluationContext mergeEvaluationContext(EvaluationContext invocationContext) {
-        final EvaluationContext apiContext = openfeatureApi.getEvaluationContext() != null
-                ? openfeatureApi.getEvaluationContext()
-                : new ImmutableContext();
-        final EvaluationContext clientContext = this.getEvaluationContext() != null
-                ? this.getEvaluationContext()
-                : new ImmutableContext();
-        final EvaluationContext transactionContext = openfeatureApi.getTransactionContext() != null
-                ? openfeatureApi.getTransactionContext()
-                : new ImmutableContext();
-
-        return apiContext.merge(transactionContext.merge(clientContext.merge(invocationContext)));
+        // avoid any unnecessary context instantiations and stream usage here; this is call with every evaluation.
+        final EvaluationContext apiContext = openfeatureApi.getEvaluationContext();
+        final EvaluationContext clientContext = this.getEvaluationContext();
+        final EvaluationContext transactionContext = openfeatureApi.getTransactionContext();
+        final List<EvaluationContext> contextsToMerge = new ArrayList<>();
+        if (apiContext != null) {
+            contextsToMerge.add(apiContext);
+        }
+        if (transactionContext != null) {
+            contextsToMerge.add(transactionContext);
+        }
+        if (clientContext != null) {
+            contextsToMerge.add(clientContext);
+        }
+        if (invocationContext != null) {
+            contextsToMerge.add(invocationContext);
+        }
+
+        EvaluationContext merged = new ImmutableContext();
+        for (EvaluationContext evaluationContext : contextsToMerge) {
+            merged = merged.merge(evaluationContext);
+        }
+        return merged;
     }
 
     private <T> ProviderEvaluation<?> createProviderEvaluation(

diff --git a/src/main/java/dev/openfeature/sdk/Structure.java b/src/main/java/dev/openfeature/sdk/Structure.java
@@ -46,6 +46,14 @@ public interface Structure {
      */
     Map<String, Value> asMap();
 
+    /**
+     * Get all values, as a map of Values.
+     *
+     * @return all attributes on the structure into a Map
+     */
+    Map<String, Value> asUnmodifiableMap();
+
+
     /**
      * Get all values, with as a map of Object.
      *
@@ -95,7 +103,7 @@ default Object convertValue(Value value) {
 
         if (value.isStructure()) {
             Structure s = value.asStructure();
-            return s.asMap()
+            return s.asUnmodifiableMap()
                     .entrySet()
                     .stream()
                     .collect(HashMap::new,
@@ -126,14 +134,15 @@ default <T extends Structure> Map<String, Value> merge(Function<Map<String, Valu
         if (overriding.isEmpty()) {
             return base;
         }
-                                                            
+
         final Map<String, Value> merged = new HashMap<>(base);
         for (Entry<String, Value> overridingEntry : overriding.entrySet()) {
             String key = overridingEntry.getKey();
             if (overridingEntry.getValue().isStructure() && merged.containsKey(key) && merged.get(key).isStructure()) {
                 Structure mergedValue = merged.get(key).asStructure();
                 Structure overridingValue = overridingEntry.getValue().asStructure();
-                Map<String, Value> newMap = this.merge(newStructure, mergedValue.asMap(), overridingValue.asMap());
+                Map<String, Value> newMap = this.merge(newStructure, mergedValue.asUnmodifiableMap(),
+                        overridingValue.asUnmodifiableMap());
                 merged.put(key, new Value(newStructure.apply(newMap)));
             } else {
                 merged.put(key, overridingEntry.getValue());

diff --git a/src/main/java/dev/openfeature/sdk/Value.java b/src/main/java/dev/openfeature/sdk/Value.java
@@ -274,7 +274,7 @@ protected Value clone() {
             return new Value(copy);
         }
         if (this.isStructure()) {
-            return new Value(new ImmutableStructure(this.asStructure().asMap()));
+            return new Value(new ImmutableStructure(this.asStructure().asUnmodifiableMap()));
         }
         if (this.isInstant()) {
             Instant copy = Instant.ofEpochMilli(this.asInstant().toEpochMilli());