-
Notifications
You must be signed in to change notification settings - Fork 43
perf: add heap benchmark and reduce allocations #1156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
17578fd
fae1582
e68f221
54ec304
c370a1f
02d57b3
c3a5434
38975fb
d4a85f0
c81c7d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,11 @@ | ||
package dev.openfeature.sdk; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
import java.util.function.Consumer; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.IntStream; | ||
import java.util.stream.Stream; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
import lombok.extern.slf4j.Slf4j; | ||
|
@@ -19,11 +17,7 @@ class HookSupport { | |
|
||
public EvaluationContext beforeHooks(FlagValueType flagValueType, HookContext hookCtx, List<Hook> hooks, | ||
Map<String, Object> hints) { | ||
Stream<EvaluationContext> result = callBeforeHooks(flagValueType, hookCtx, hooks, hints); | ||
return hookCtx.getCtx().merge( | ||
result.reduce(hookCtx.getCtx(), (EvaluationContext accumulated, EvaluationContext current) -> { | ||
return accumulated.merge(current); | ||
})); | ||
return callBeforeHooks(flagValueType, hookCtx, hooks, hints); | ||
} | ||
|
||
public void afterHooks(FlagValueType flagValueType, HookContext hookContext, FlagEvaluationDetails details, | ||
|
@@ -46,10 +40,11 @@ private <T> void executeHooks( | |
String hookMethod, | ||
Consumer<Hook<T>> hookCode) { | ||
if (hooks != null) { | ||
hooks | ||
.stream() | ||
.filter(hook -> hook.supportsFlagValueType(flagValueType)) | ||
.forEach(hook -> executeChecked(hook, hookCode, hookMethod)); | ||
for (Hook hook : hooks) { | ||
if (hook.supportsFlagValueType(flagValueType)) { | ||
executeChecked(hook, hookCode, hookMethod); | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
@@ -68,29 +63,29 @@ private <T> void executeHooksUnchecked( | |
FlagValueType flagValueType, List<Hook> hooks, | ||
Consumer<Hook<T>> hookCode) { | ||
if (hooks != null) { | ||
hooks | ||
.stream() | ||
.filter(hook -> hook.supportsFlagValueType(flagValueType)) | ||
.forEach(hookCode::accept); | ||
for (Hook hook : hooks) { | ||
if (hook.supportsFlagValueType(flagValueType)) { | ||
hookCode.accept(hook); | ||
} | ||
} | ||
Comment on lines
+66
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perf: streams 😢 |
||
} | ||
} | ||
|
||
private Stream<EvaluationContext> callBeforeHooks(FlagValueType flagValueType, HookContext hookCtx, | ||
private EvaluationContext callBeforeHooks(FlagValueType flagValueType, HookContext hookCtx, | ||
List<Hook> hooks, Map<String, Object> hints) { | ||
// These traverse backwards from normal. | ||
List<Hook> reversedHooks = IntStream | ||
.range(0, hooks.size()) | ||
.map(i -> hooks.size() - 1 - i) | ||
.mapToObj(hooks::get) | ||
.collect(Collectors.toList()); | ||
|
||
return reversedHooks | ||
.stream() | ||
.filter(hook -> hook.supportsFlagValueType(flagValueType)) | ||
.map(hook -> hook.before(hookCtx, hints)) | ||
.filter(Objects::nonNull) | ||
.filter(Optional::isPresent) | ||
.map(Optional::get) | ||
.map(EvaluationContext.class::cast); | ||
List<Hook> reversedHooks = new ArrayList<>(hooks); | ||
Collections.reverse(reversedHooks); | ||
EvaluationContext context = hookCtx.getCtx(); | ||
for (Hook hook : reversedHooks) { | ||
if (hook.supportsFlagValueType(flagValueType)) { | ||
Optional<EvaluationContext> optional = Optional.ofNullable(hook.before(hookCtx, hints)) | ||
.orElse(Optional.empty()); | ||
if (optional.isPresent()) { | ||
context = context.merge(optional.get()); | ||
} | ||
} | ||
} | ||
return context; | ||
Comment on lines
+77
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perf: streams 😢 |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import java.util.HashMap; | ||
import java.util.HashSet; | ||
import java.util.Map; | ||
import java.util.Map.Entry; | ||
import java.util.Optional; | ||
import java.util.Set; | ||
|
||
|
@@ -35,14 +36,7 @@ public ImmutableStructure() { | |
* @param attributes attributes. | ||
*/ | ||
public ImmutableStructure(Map<String, Value> attributes) { | ||
super(new HashMap<>(attributes.entrySet() | ||
.stream() | ||
.collect(HashMap::new, | ||
(accumulated, entry) -> accumulated.put(entry.getKey(), | ||
Optional.ofNullable(entry.getValue()) | ||
.map(Value::clone) | ||
.orElse(null)), | ||
HashMap::putAll))); | ||
super(copyAttributes(attributes)); | ||
} | ||
|
||
@Override | ||
|
@@ -53,7 +47,7 @@ public Set<String> keySet() { | |
// getters | ||
@Override | ||
public Value getValue(String key) { | ||
Value value = this.attributes.get(key); | ||
Value value = attributes.get(key); | ||
return value != null ? value.clone() : null; | ||
} | ||
|
||
|
@@ -64,14 +58,16 @@ public Value getValue(String key) { | |
*/ | ||
@Override | ||
public Map<String, Value> asMap() { | ||
return attributes | ||
.entrySet() | ||
.stream() | ||
.collect(HashMap::new, | ||
(accumulated, entry) -> accumulated.put(entry.getKey(), | ||
Optional.ofNullable(entry.getValue()) | ||
.map(Value::clone) | ||
.orElse(null)), | ||
HashMap::putAll); | ||
return copyAttributes(attributes); | ||
} | ||
|
||
private static Map<String, Value> copyAttributes(Map<String, Value> in) { | ||
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)); | ||
} | ||
return copy; | ||
} | ||
Comment on lines
+64
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perf: common, non-stream method to copy. |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,8 +114,11 @@ | |
*/ | ||
@Override | ||
public EvaluationContext merge(EvaluationContext overridingContext) { | ||
if (overridingContext == null) { | ||
return new MutableContext(this.asMap()); | ||
if (overridingContext == null || overridingContext.isEmpty()) { | ||
return this; | ||
} | ||
if (this.isEmpty()) { | ||
return overridingContext; | ||
Comment on lines
+117
to
+121
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perf: quit early with less |
||
} | ||
|
||
Map<String, Value> merged = this.merge( | ||
|
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.
perf: streams 😢