Skip to content

Commit a5ba3c6

Browse files
tobias-ibounig-dttoddbaert
tobias-ibounig-dt
and
toddbaert
authored
perf: eliminate merge allocation in setHooks by accepting hook sources directly (#1956)
* perf: eliminate merge allocation in setHooks by accepting hook sources directly Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com> * improve test, to ensure hook order. Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com> * Remove unused ObjectUtils#merge Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com> * reintroduce and deprecate ObjectUtils.merge Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com> * fixup: added comment Signed-off-by: Todd Baert <todd.baert@dynatrace.com> --------- Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
1 parent 75391aa commit a5ba3c6

4 files changed

Lines changed: 94 additions & 14 deletions

File tree

‎src/main/java/dev/openfeature/sdk/HookSupport.java‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package dev.openfeature.sdk;
22

33
import java.util.ArrayList;
4+
import java.util.Collection;
45
import java.util.List;
56
import java.util.Optional;
67
import lombok.extern.slf4j.Slf4j;
@@ -15,19 +16,40 @@ class HookSupport {
1516
/**
1617
* Sets the {@link Hook}-{@link HookContext}-{@link Pair} list in the given data object with {@link HookContext}
1718
* set to null. Filters hooks by supported {@link FlagValueType}.
19+
* Sources are iterated in order: provider, options, client, API (reversed for the {@code before} stage by
20+
* {@link #executeBeforeHooks}).
21+
*
22+
* <p>The four hook sources are accepted as separate collections to avoid allocation on the evaluation hot path.
1823
*
1924
* @param hookSupportData the data object to modify
20-
* @param hooks the hooks to set
25+
* @param providerHooks provider-level hooks
26+
* @param optionHooks per-evaluation option hooks
27+
* @param clientHooks client-level hooks
28+
* @param apiHooks API-level hooks
2129
* @param type the flag value type to filter unsupported hooks
2230
*/
23-
public void setHooks(HookSupportData hookSupportData, List<Hook> hooks, FlagValueType type) {
31+
public void setHooks(
32+
HookSupportData hookSupportData,
33+
Collection<Hook> providerHooks,
34+
Collection<Hook> optionHooks,
35+
Collection<Hook> clientHooks,
36+
Collection<Hook> apiHooks,
37+
FlagValueType type) {
2438
List<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>();
25-
for (Hook hook : hooks) {
39+
addFilteredHooks(hookContextPairs, providerHooks, type);
40+
addFilteredHooks(hookContextPairs, optionHooks, type);
41+
addFilteredHooks(hookContextPairs, clientHooks, type);
42+
addFilteredHooks(hookContextPairs, apiHooks, type);
43+
hookSupportData.hooks = hookContextPairs;
44+
}
45+
46+
private static void addFilteredHooks(
47+
List<Pair<Hook, HookContext>> dest, Collection<Hook> source, FlagValueType type) {
48+
for (Hook hook : source) {
2649
if (hook.supportsFlagValueType(type)) {
27-
hookContextPairs.add(Pair.of(hook, null));
50+
dest.add(Pair.of(hook, null));
2851
}
2952
}
30-
hookSupportData.hooks = hookContextPairs;
3153
}
3254

3355
/**

‎src/main/java/dev/openfeature/sdk/OpenFeatureClient.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
import dev.openfeature.sdk.exceptions.GeneralError;
66
import dev.openfeature.sdk.exceptions.OpenFeatureError;
77
import dev.openfeature.sdk.exceptions.ProviderNotReadyError;
8-
import dev.openfeature.sdk.internal.ObjectUtils;
98
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
109
import java.util.ArrayList;
1110
import java.util.Arrays;
@@ -188,9 +187,13 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
188187
final var state = stateManager.getState();
189188

190189
// Hooks are initialized as early as possible to enable the execution of error stages
191-
var mergedHooks = ObjectUtils.merge(
192-
provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks());
193-
hookSupport.setHooks(hookSupportData, mergedHooks, type);
190+
hookSupport.setHooks(
191+
hookSupportData,
192+
provider.getProviderHooks(),
193+
flagOptions.getHooks(),
194+
clientHooks,
195+
openfeatureApi.getMutableHooks(),
196+
type);
194197

195198
var sharedHookContext =
196199
new SharedHookContext(key, type, this.getMetadata(), provider.getMetadata(), defaultValue);

‎src/main/java/dev/openfeature/sdk/internal/ObjectUtils.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,9 @@ public static <T> T defaultIfNull(T source, Supplier<T> defaultValue) {
6363
* @param sources bunch of lists.
6464
* @param <T> list type
6565
* @return resulting object
66+
* @deprecated Not used in the project anymore. This method will be removed in a future release.
6667
*/
68+
@Deprecated(forRemoval = true)
6769
@SafeVarargs
6870
public static <T> List<T> merge(Collection<T>... sources) {
6971
List<T> merged = new ArrayList<>();

‎src/test/java/dev/openfeature/sdk/HookSupportTest.java‎

Lines changed: 58 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import dev.openfeature.sdk.fixtures.HookFixtures;
1111
import java.util.Arrays;
12+
import java.util.Collections;
1213
import java.util.HashMap;
1314
import java.util.List;
1415
import java.util.Map;
@@ -38,7 +39,13 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
3839
var sharedContext = getBaseHookContextForType(FlagValueType.STRING);
3940
var hookSupportData = new HookSupportData();
4041
hookSupportData.evaluationContext = layered;
41-
hookSupport.setHooks(hookSupportData, Arrays.asList(hook1, hook2), FlagValueType.STRING);
42+
hookSupport.setHooks(
43+
hookSupportData,
44+
Collections.emptyList(),
45+
Collections.emptyList(),
46+
Arrays.asList(hook1, hook2),
47+
Collections.emptyList(),
48+
FlagValueType.STRING);
4249
hookSupport.setHookContexts(hookSupportData, sharedContext, layered);
4350

4451
hookSupport.executeBeforeHooks(hookSupportData);
@@ -57,7 +64,13 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
5764
Hook<?> genericHook = mockGenericHook();
5865

5966
var hookSupportData = new HookSupportData();
60-
hookSupport.setHooks(hookSupportData, List.of(genericHook), flagValueType);
67+
hookSupport.setHooks(
68+
hookSupportData,
69+
List.of(genericHook),
70+
Collections.emptyList(),
71+
Collections.emptyList(),
72+
Collections.emptyList(),
73+
flagValueType);
6174

6275
callAllHooks(hookSupportData);
6376

@@ -73,7 +86,13 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
7386
void shouldPassDataAcrossStages(FlagValueType flagValueType) {
7487
var testHook = new TestHookWithData();
7588
var hookSupportData = new HookSupportData();
76-
hookSupport.setHooks(hookSupportData, List.of(testHook), flagValueType);
89+
hookSupport.setHooks(
90+
hookSupportData,
91+
Collections.emptyList(),
92+
List.of(testHook),
93+
Collections.emptyList(),
94+
Collections.emptyList(),
95+
flagValueType);
7796
hookSupport.setHookContexts(
7897
hookSupportData,
7998
getBaseHookContextForType(flagValueType),
@@ -102,7 +121,13 @@ void shouldIsolateDataBetweenHooks(FlagValueType flagValueType) {
102121
var testHook2 = new TestHookWithData(2);
103122

104123
var hookSupportData = new HookSupportData();
105-
hookSupport.setHooks(hookSupportData, List.of(testHook1, testHook2), flagValueType);
124+
hookSupport.setHooks(
125+
hookSupportData,
126+
Collections.emptyList(),
127+
Collections.emptyList(),
128+
Collections.emptyList(),
129+
List.of(testHook1, testHook2),
130+
flagValueType);
106131
hookSupport.setHookContexts(
107132
hookSupportData,
108133
getBaseHookContextForType(flagValueType),
@@ -114,6 +139,28 @@ void shouldIsolateDataBetweenHooks(FlagValueType flagValueType) {
114139
assertHookData(testHook2, 2, "before", "after", "finallyAfter", "error");
115140
}
116141

142+
@Test
143+
@DisplayName("should place hooks in provider → options → client → API order")
144+
void shouldOrderHooksBySource() {
145+
Hook<?> providerHook = mockGenericHook();
146+
Hook<?> optionHook = mockGenericHook();
147+
Hook<?> clientHook = mockGenericHook();
148+
Hook<?> apiHook = mockGenericHook();
149+
150+
var hookSupportData = new HookSupportData();
151+
hookSupport.setHooks(
152+
hookSupportData,
153+
List.of(providerHook),
154+
List.of(optionHook),
155+
List.of(clientHook),
156+
List.of(apiHook),
157+
FlagValueType.STRING);
158+
159+
assertThat(hookSupportData.getHooks())
160+
.extracting(Pair::getKey)
161+
.containsExactly(providerHook, optionHook, clientHook, apiHook);
162+
}
163+
117164
@Test
118165
void hookThatReturnsTheGivenContext_doesNotResultInAStackOverflow() {
119166
var hookSupportData = new HookSupportData();
@@ -132,7 +179,13 @@ public Optional<EvaluationContext> before(HookContext ctx, Map hints) {
132179
var layeredEvaluationContext =
133180
new LayeredEvaluationContext(evaluationContextWithValue("key", "value"), null, null, null);
134181
hookSupportData.evaluationContext = layeredEvaluationContext;
135-
hookSupport.setHooks(hookSupportData, List.of(recursiveHook, emptyHook), FlagValueType.STRING);
182+
hookSupport.setHooks(
183+
hookSupportData,
184+
Collections.emptyList(),
185+
Collections.emptyList(),
186+
List.of(recursiveHook, emptyHook),
187+
Collections.emptyList(),
188+
FlagValueType.STRING);
136189
hookSupport.setHookContexts(
137190
hookSupportData, getBaseHookContextForType(FlagValueType.STRING), layeredEvaluationContext);
138191

0 commit comments

Comments
 (0)