perf: eliminate merge allocation in setHooks by accepting hook sources directly#1956
perf: eliminate merge allocation in setHooks by accepting hook sources directly#1956tobias-ibounig-dt wants to merge 3 commits into
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));
}
}
}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.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:
|
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.
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>
|
I realized |
Yes, I think we can remove it |
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>
|
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