perf: eliminate merge allocation in setHooks by accepting hook sources directly#1956
Conversation
…s directly Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>
|
![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));
}
}
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1956 +/- ##
============================================
+ Coverage 92.27% 93.05% +0.77%
- Complexity 653 655 +2
============================================
Files 59 59
Lines 1592 1598 +6
Branches 179 178 -1
============================================
+ Hits 1469 1487 +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:
|
1 participant
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