perf: eliminate merge allocation in setHooks by accepting hook sources directly#1956
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors hook handling in HookSupport and OpenFeatureClient by passing separate hook collections (provider, option, client, and API) directly to setHooks instead of merging them beforehand. This eliminates the need for ObjectUtils.merge. The review feedback suggests adding null checks to prevent potential NullPointerExceptions and pre-sizing the ArrayList to optimize performance when collecting the filtered hooks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| public void setHooks( | ||
| HookSupportData hookSupportData, | ||
| Collection<Hook> providerHooks, | ||
| Collection<Hook> optionHooks, | ||
| Collection<Hook> clientHooks, | ||
| Collection<Hook> apiHooks, | ||
| FlagValueType type) { | ||
| List<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>(); | ||
| for (Hook hook : hooks) { | ||
| addFilteredHooks(hookContextPairs, providerHooks, type); | ||
| addFilteredHooks(hookContextPairs, optionHooks, type); | ||
| addFilteredHooks(hookContextPairs, clientHooks, type); | ||
| addFilteredHooks(hookContextPairs, apiHooks, type); | ||
| hookSupportData.hooks = hookContextPairs; | ||
| } | ||
|
|
||
| private static void addFilteredHooks( | ||
| List<Pair<Hook, HookContext>> dest, Collection<Hook> source, FlagValueType type) { | ||
| for (Hook hook : source) { | ||
| if (hook.supportsFlagValueType(type)) { | ||
| hookContextPairs.add(Pair.of(hook, null)); | ||
| dest.add(Pair.of(hook, null)); | ||
| } | ||
| } | ||
| hookSupportData.hooks = hookContextPairs; | ||
| } |
There was a problem hiding this comment.
To prevent potential NullPointerExceptions and further optimize performance, we should:
- Add Null Checks: Ensure that we safely handle cases where any of the hook collections (such as
providerHooksoroptionHooks) or individual hooks within them arenull. - Pre-size the ArrayList: Since we are aiming to optimize performance and eliminate unnecessary allocations, pre-sizing the
ArrayListwith the sum of the sizes of the non-null collections avoids internal array resizing overhead during population.
public void setHooks(
HookSupportData hookSupportData,
Collection<Hook> providerHooks,
Collection<Hook> optionHooks,
Collection<Hook> clientHooks,
Collection<Hook> apiHooks,
FlagValueType type) {
int expectedSize = 0;
if (providerHooks != null) {
expectedSize += providerHooks.size();
}
if (optionHooks != null) {
expectedSize += optionHooks.size();
}
if (clientHooks != null) {
expectedSize += clientHooks.size();
}
if (apiHooks != null) {
expectedSize += apiHooks.size();
}
List<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>(expectedSize);
addFilteredHooks(hookContextPairs, providerHooks, type);
addFilteredHooks(hookContextPairs, optionHooks, type);
addFilteredHooks(hookContextPairs, clientHooks, type);
addFilteredHooks(hookContextPairs, apiHooks, type);
hookSupportData.hooks = hookContextPairs;
}
private static void addFilteredHooks(
List<Pair<Hook, HookContext>> dest, Collection<Hook> source, FlagValueType type) {
if (source == null) {
return;
}
for (Hook hook : source) {
if (hook != null && hook.supportsFlagValueType(type)) {
dest.add(Pair.of(hook, null));
}
}
}There was a problem hiding this comment.
we don't know which hooks will be added to the array list, so we would be overestimating the size of the list. I'm not sure if this is worth it, and if we can come up with a benchmark that represensts real life usage
There was a problem hiding this comment.
Under 10 hooks, it looks like the runtime will allocate at least that many anyway: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/ArrayList.java#L119 - if I'm reading this correctly.
I don't think we'll often have more than 10, so I think doing more than what's here now is a micro-optimization.
There was a problem hiding this comment.
Are null checks that Gemini mentions are concern?
As far as I can tell there were none in place previously, which is why I didn't consider adding them.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1956 +/- ##
============================================
+ Coverage 92.28% 93.06% +0.77%
- Complexity 655 657 +2
============================================
Files 59 59
Lines 1594 1600 +6
Branches 181 180 -1
============================================
+ Hits 1471 1489 +18
+ Misses 76 66 -10
+ Partials 47 45 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
chrfwow
left a comment
chrfwow
left a comment
There was a problem hiding this comment.
lgtm, just left a comment for the tests
| public void setHooks( | ||
| HookSupportData hookSupportData, | ||
| Collection<Hook> providerHooks, | ||
| Collection<Hook> optionHooks, | ||
| Collection<Hook> clientHooks, | ||
| Collection<Hook> apiHooks, | ||
| FlagValueType type) { | ||
| List<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>(); | ||
| for (Hook hook : hooks) { | ||
| addFilteredHooks(hookContextPairs, providerHooks, type); | ||
| addFilteredHooks(hookContextPairs, optionHooks, type); | ||
| addFilteredHooks(hookContextPairs, clientHooks, type); | ||
| addFilteredHooks(hookContextPairs, apiHooks, type); | ||
| hookSupportData.hooks = hookContextPairs; | ||
| } | ||
|
|
||
| private static void addFilteredHooks( | ||
| List<Pair<Hook, HookContext>> dest, Collection<Hook> source, FlagValueType type) { | ||
| for (Hook hook : source) { | ||
| if (hook.supportsFlagValueType(type)) { | ||
| hookContextPairs.add(Pair.of(hook, null)); | ||
| dest.add(Pair.of(hook, null)); | ||
| } | ||
| } | ||
| hookSupportData.hooks = hookContextPairs; | ||
| } |
There was a problem hiding this comment.
we don't know which hooks will be added to the array list, so we would be overestimating the size of the list. I'm not sure if this is worth it, and if we can come up with a benchmark that represensts real life usage
| new LayeredEvaluationContext(evaluationContextWithValue("key", "value"), null, null, null); | ||
| hookSupportData.evaluationContext = layeredEvaluationContext; | ||
| hookSupport.setHooks(hookSupportData, List.of(recursiveHook, emptyHook), FlagValueType.STRING); | ||
| hookSupport.setHooks( |
There was a problem hiding this comment.
I'd pass the non empty list in different order for each of these tests, and I think we should also add a test to verify that the hooks are placed in the correct order in the resulting list
There was a problem hiding this comment.
Adapted the tests as suggested and added an extra test to ensure order.
|
I realized |
Yes, I think we can remove it |
Just a call out - this was a public method, so this makes this change breaking - however - it was in the WDYT @chrfwow ? Would this surprise you if you were a user? |
True, I did not see that.
I hope nobody uses an internal helper method from some library, but I guess we can never know. Not sure if we should remove it then. On the other hand, it's obviously not part of the functionality of OpenFeature, and can be easily reproduced by our users, so I think the impact on them should be minimal. |
…s directly Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>
tobias-ibounig-dt
force-pushed
the
perf/pr-5-set-hooks-refactor
branch
from
c4c79b0 to
940651a
Compare
|
Rebased, reintroduced |
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
|
I think it's my least favorite part of Java (a language I mostly like) that it doesn't really have the concept of "jar private" (like .NET has with I guess we could do some jar-packaging stuff as well, but TBH I think that's overkill. |
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
Pushed one small comment and approved.
3 participants
This PR
setHooksby accepting the four hook sources directlyRelated Issues
None
Notes
Previously
OpenFeatureClientcalledObjectUtils.merge(providerHooks, optionHooks, clientHooks, apiHooks)to concatenate all hook sources into a singleArrayListbefore passing it toHookSupport.setHooks. This allocated one list per flag evaluation solely to iterate it once. The change accepts the four sources as separateCollection<Hook>parameters and iterates them directly via a privateaddFilteredHookshelper, removing the intermediate allocation entirely.main(baseline)run:+totalAllocatedBytesrun:+totalAllocatedInstancesFollow-up Tasks