feat: Layered context to reduce memory churn #1717
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,254 @@ | ||
| package dev.openfeature.sdk; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| /** | ||
| * LayeredEvaluationContext implements EvaluationContext by layering multiple contexts: | ||
| * API-level, Transaction-level, Client-level, Invocation-level, and Hook-level. | ||
| * The contexts are checked in that order for values, with Hook-level having the highest precedence. | ||
| */ | ||
| public class LayeredEvaluationContext implements EvaluationContext { | ||
|
Contributor
Author
There was a problem hiding this comment. Alternatively, I could also implement this as a list of
Member
There was a problem hiding this comment. so theoretically before hooks can also add information for the evaluation context - and this should also be somehow merged into this. would it make sense to simple add all the evaluationContext returned to the layer, and iterate over them as a list? Question if it is worth it - at least in theory it would be one less object |
||
| private final EvaluationContext apiContext; | ||
| private final EvaluationContext transactionContext; | ||
| private final EvaluationContext clientContext; | ||
| private final EvaluationContext invocationContext; | ||
|
|
||
| private ArrayList<EvaluationContext> hookContexts; | ||
| private String targetingKey; | ||
| private Set<String> keySet = null; | ||
|
|
||
| /** | ||
| * Constructor for LayeredEvaluationContext. | ||
| */ | ||
| public LayeredEvaluationContext( | ||
| EvaluationContext apiContext, | ||
| EvaluationContext transactionContext, | ||
| EvaluationContext clientContext, | ||
| EvaluationContext invocationContext) { | ||
| this.apiContext = apiContext; | ||
| this.transactionContext = transactionContext; | ||
| this.clientContext = clientContext; | ||
| this.invocationContext = invocationContext; | ||
|
|
||
| if (invocationContext != null && invocationContext.getTargetingKey() != null) { | ||
| this.targetingKey = invocationContext.getTargetingKey(); | ||
| } else if (clientContext != null && clientContext.getTargetingKey() != null) { | ||
| this.targetingKey = clientContext.getTargetingKey(); | ||
| } else if (transactionContext != null && transactionContext.getTargetingKey() != null) { | ||
| this.targetingKey = transactionContext.getTargetingKey(); | ||
| } else if (apiContext != null && apiContext.getTargetingKey() != null) { | ||
| this.targetingKey = apiContext.getTargetingKey(); | ||
| } else { | ||
| this.targetingKey = null; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String getTargetingKey() { | ||
| return targetingKey; | ||
| } | ||
|
|
||
| @Override | ||
| public EvaluationContext merge(EvaluationContext overridingContext) { | ||
| var merged = new LayeredEvaluationContext(apiContext, transactionContext, clientContext, invocationContext); | ||
|
|
||
| if (this.hookContexts == null) { | ||
| merged.hookContexts = new ArrayList<>(1); | ||
| } else { | ||
| merged.hookContexts = new ArrayList<>(this.hookContexts.size() + 1); | ||
| merged.hookContexts.addAll(this.hookContexts); | ||
| } | ||
| merged.hookContexts.add(overridingContext); | ||
|
|
||
| var otherTargetingKey = overridingContext.getTargetingKey(); | ||
| if (otherTargetingKey != null) { | ||
| merged.targetingKey = otherTargetingKey; | ||
| } | ||
| return merged; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isEmpty() { | ||
| return (hookContexts == null || hookContexts.isEmpty()) | ||
|
||
| && (invocationContext == null || invocationContext.isEmpty()) | ||
| && (clientContext == null || clientContext.isEmpty()) | ||
| && (transactionContext == null || transactionContext.isEmpty()) | ||
| && (apiContext == null || apiContext.isEmpty()); | ||
| } | ||
|
|
||
| @Override | ||
| public Set<String> keySet() { | ||
| return Collections.unmodifiableSet(ensureKeySet()); | ||
| } | ||
|
|
||
| private Set<String> ensureKeySet() { | ||
| if (this.keySet != null) { | ||
| return this.keySet; | ||
| } | ||
|
|
||
| var keys = new HashSet<String>(); | ||
|
|
||
| if (hookContexts != null) { | ||
| for (int i = 0; i < hookContexts.size(); i++) { | ||
| var current = hookContexts.get(i); | ||
| keys.addAll(current.keySet()); | ||
| } | ||
| } | ||
|
|
||
| if (invocationContext != null) { | ||
| keys.addAll(invocationContext.keySet()); | ||
| } | ||
| if (clientContext != null) { | ||
| keys.addAll(clientContext.keySet()); | ||
| } | ||
| if (transactionContext != null) { | ||
| keys.addAll(transactionContext.keySet()); | ||
| } | ||
| if (apiContext != null) { | ||
| keys.addAll(apiContext.keySet()); | ||
| } | ||
| this.keySet = keys; | ||
|
Member
There was a problem hiding this comment. Should we make use of the
Contributor
Author
There was a problem hiding this comment. I don't think I understand your comment, we only know that we have no keys at the end of this method
Member
There was a problem hiding this comment. I was saying perhaps we could more lazily create a key set if there actually is any keys, and otherwise just return the empty set and avoid allocating something, but it's a micro-optimization.
Contributor
Author
There was a problem hiding this comment. But how do we know if we have any keys without generating the keyset? And after we generated it, we might as well return it, as we have allocated it anyway
Member
There was a problem hiding this comment. It's the allocation of the
Contributor
There was a problem hiding this comment. could the |
||
| return keys; | ||
| } | ||
|
|
||
| private Value getFromContext(EvaluationContext context, String key) { | ||
| if (context != null) { | ||
| return context.getValue(key); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private Value getFromContext(ArrayList<EvaluationContext> context, String key) { | ||
| if (context == null) { | ||
| return null; | ||
| } | ||
|
|
||
| for (int i = context.size() - 1; i >= 0; i--) { | ||
| var current = context.get(i); | ||
| var value = getFromContext(current, key); | ||
| if (value != null) { | ||
| return value; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public Value getValue(String key) { | ||
| var hookValue = getFromContext(hookContexts, key); | ||
| if (hookValue != null) { | ||
| return hookValue; | ||
| } | ||
| var invocationValue = getFromContext(invocationContext, key); | ||
| if (invocationValue != null) { | ||
| return invocationValue; | ||
| } | ||
| var clientValue = getFromContext(clientContext, key); | ||
| if (clientValue != null) { | ||
| return clientValue; | ||
| } | ||
| var transactionValue = getFromContext(transactionContext, key); | ||
| if (transactionValue != null) { | ||
| return transactionValue; | ||
| } | ||
| return getFromContext(apiContext, key); | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, Value> asMap() { | ||
| if (keySet != null && keySet.isEmpty()) { | ||
| return new HashMap<>(0); | ||
toddbaert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| HashMap<String, Value> map; | ||
| if (keySet != null) { | ||
| map = new HashMap<>(keySet.size()); | ||
| } else { | ||
| map = new HashMap<>(); | ||
| } | ||
|
|
||
| if (apiContext != null) { | ||
| map.putAll(apiContext.asMap()); | ||
| } | ||
| if (transactionContext != null) { | ||
| map.putAll(transactionContext.asMap()); | ||
| } | ||
| if (clientContext != null) { | ||
| map.putAll(clientContext.asMap()); | ||
| } | ||
| if (invocationContext != null) { | ||
| map.putAll(invocationContext.asMap()); | ||
| } | ||
| if (hookContexts != null) { | ||
| for (int i = 0; i < hookContexts.size(); i++) { | ||
| EvaluationContext hookContext = hookContexts.get(i); | ||
| map.putAll(hookContext.asMap()); | ||
| } | ||
| } | ||
| return map; | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, Value> asUnmodifiableMap() { | ||
| if (keySet != null && keySet.isEmpty()) { | ||
| return Collections.emptyMap(); | ||
| } | ||
|
|
||
| return Collections.unmodifiableMap(asMap()); | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, Object> asObjectMap() { | ||
| if (keySet != null && keySet.isEmpty()) { | ||
| return new HashMap<>(0); | ||
toddbaert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| HashMap<String, Object> map; | ||
| if (keySet != null) { | ||
| map = new HashMap<>(keySet.size()); | ||
| } else { | ||
| map = new HashMap<>(); | ||
| } | ||
|
|
||
| if (apiContext != null) { | ||
| map.putAll(apiContext.asObjectMap()); | ||
| } | ||
| if (transactionContext != null) { | ||
| map.putAll(transactionContext.asObjectMap()); | ||
| } | ||
| if (clientContext != null) { | ||
| map.putAll(clientContext.asObjectMap()); | ||
| } | ||
| if (invocationContext != null) { | ||
| map.putAll(invocationContext.asObjectMap()); | ||
| } | ||
| if (hookContexts != null) { | ||
| for (int i = 0; i < hookContexts.size(); i++) { | ||
| EvaluationContext hookContext = hookContexts.get(i); | ||
| map.putAll(hookContext.asObjectMap()); | ||
| } | ||
| } | ||
| return map; | ||
| } | ||
|
|
||
| void putHookContext(EvaluationContext context) { | ||
| if (context == null || context.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| var targetingKey = context.getTargetingKey(); | ||
| if (targetingKey != null) { | ||
| this.targetingKey = targetingKey; | ||
| } | ||
| if (this.hookContexts == null) { | ||
| this.hookContexts = new ArrayList<>(); | ||
| } | ||
| this.hookContexts.add(context); | ||
| this.keySet = null; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are using Optionals already, we should use all the available syntax sugar, and switch to
.ifPresentand chain as much as possibleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be nice, but
Optionals in Java don't work well enough to get this with type safety. I would need some castings and redundantisPresentchecks to get this to workThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but do we need the Optional wrapping on line 61? we want to reduce object churn etc. but we are generating a new Optional all the time - so i am wondering if we really need the Optional or if a null check would be sufficient here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to change as little as possible of unrelated code so that we can scientifically see the benefit of the new context. Other optimizations will come in a later PR