feat: Layered context to reduce memory churn #1717
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| package dev.openfeature.sdk; | ||
|
|
||
| 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 final HashMap<String, Value> hookContext = new HashMap<>(); | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| /** | ||
| * Using this method should be avoided as it comes with a performance cost. | ||
| * Consider constructing a new LayeredEvaluationContext instead. | ||
| * | ||
| * <p> | ||
| * Does not modify this object. | ||
| * | ||
| * @param overridingContext overriding context | ||
| * @return A new LayeredEvaluationContext containing the context from this object, with the overridingContext | ||
| * merged on top. | ||
| * @deprecated Use of this method is discouraged due to performance considerations. | ||
| */ | ||
| @Deprecated | ||
| @Override | ||
| public EvaluationContext merge(EvaluationContext overridingContext) { | ||
| var merged = new LayeredEvaluationContext(apiContext, transactionContext, clientContext, invocationContext); | ||
| merged.hookContext.putAll(this.hookContext); | ||
| merged.hookContext.putAll(overridingContext.asMap()); | ||
| var otherTargetingKey = overridingContext.getTargetingKey(); | ||
| if (otherTargetingKey != null) { | ||
| merged.targetingKey = otherTargetingKey; | ||
| } | ||
| return merged; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isEmpty() { | ||
| return hookContext.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<>(hookContext.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; | ||
| } | ||
|
|
||
| @Override | ||
| public Value getValue(String key) { | ||
| var hookValue = hookContext.get(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() { | ||
| var keySet = ensureKeySet(); | ||
| var keys = keySet.size(); | ||
| if (keys == 0) { | ||
| return new HashMap<>(1); | ||
| } | ||
| var map = new HashMap<String, Value>(keys); | ||
|
|
||
| for (String key : keySet) { | ||
| map.put(key, getValue(key)); | ||
| } | ||
| return map; | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, Value> asUnmodifiableMap() { | ||
| var keySet = ensureKeySet(); | ||
| var keys = keySet.size(); | ||
| if (keys == 0) { | ||
| return Collections.emptyMap(); | ||
| } | ||
| var map = new HashMap<String, Value>(keys); | ||
|
|
||
| for (String key : keySet) { | ||
| map.put(key, getValue(key)); | ||
| } | ||
| return Collections.unmodifiableMap(map); | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, Object> asObjectMap() { | ||
| var keySet = ensureKeySet(); | ||
| var keys = keySet.size(); | ||
| if (keys == 0) { | ||
| return new HashMap<>(1); | ||
| } | ||
| var map = new HashMap<String, Object>(keys); | ||
|
|
||
| for (String key : keySet) { | ||
| map.put(key, convertValue(getValue(key))); | ||
| } | ||
| return map; | ||
| } | ||
|
|
||
| void putHookContext(Map<String, Value> context) { | ||
| if (context == null) { | ||
| return; | ||
| } | ||
|
|
||
| var targetingKey = context.get("targetingKey"); | ||
| if (targetingKey != null) { | ||
| var targetingKeyStr = targetingKey.asString(); | ||
| if (targetingKeyStr != null) { | ||
| this.targetingKey = targetingKeyStr; | ||
| this.hookContext.put("targetingKey", targetingKey); | ||
| } | ||
| } | ||
| this.hookContext.putAll(context); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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