Skip to content

Conversation

@chrfwow
Copy link
Contributor

@chrfwow chrfwow commented Nov 20, 2025

Hooks are now stored in a map to allow efficient lookup of hooks supporting certain FlagValueTypes. This and other changes makes merging the hooks cheaper.

Related Issues

Part of #1718

Follow Up Tasks

Implement efficient Hook Storage in all providers (optional, but will speed things up)

Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
# Conflicts:
#	src/test/java/dev/openfeature/sdk/benchmark/AllocationBenchmark.java
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
@sonarqubecloud
Copy link

@chrfwow
Copy link
Contributor Author

chrfwow commented Nov 20, 2025

  • Make sure the additions of default methods to interfaces is not a byte code incompatibility

@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 94.73684% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.39%. Comparing base (bd70a3a) to head (b5a443f).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/main/java/dev/openfeature/sdk/HookSupport.java 86.95% 0 Missing and 3 partials ⚠️
...ava/dev/openfeature/sdk/FlagEvaluationOptions.java 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1734      +/-   ##
============================================
+ Coverage     92.47%   93.39%   +0.92%     
- Complexity      527      548      +21     
============================================
  Files            52       53       +1     
  Lines          1276     1333      +57     
  Branches        112      129      +17     
============================================
+ Hits           1180     1245      +65     
+ Misses           60       50      -10     
- Partials         36       38       +2     
Flag Coverage Δ
unittests 93.39% <94.73%> (+0.92%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chrfwow
Copy link
Contributor Author

chrfwow commented Nov 20, 2025

Benchmark results:
(I ran AllocationBenchmark#main with the IntelliJ Profiler. This is not a proper JMH benchmark, so the values are not representative per evaluation, but should be helpful to compare both versions)

Baseline:
Duration: 5267 ms
Allocations: 16.834 GB
Flamegraph of allocations:
image

This PR:
Duration: 5023 ms (this is an insignificant change, the deviations on those tests are quite large)
Allocations: 14.224 GB
Flamegraph of allocations:
image

@chrfwow chrfwow marked this pull request as ready for review November 20, 2025 16:47
@chrfwow chrfwow requested review from a team as code owners November 20, 2025 16:47
Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am torn a little bit and not 100% convinced, that this will make things easier. maybe we should think about v2 and rethink our hooksupport api overall.

But i also do not have big remarks to add

*/
public List<Hook> getHooks() {
return new ArrayList<>(this.apiHooks);
// Hooks can be duplicated if they support multiple FlagValueTypes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, the order of hooks is important, i know that you are now using other methods, to retrieve the needed hooks, but can this implementation now be a problem, because we are not keeping the order of the hooks. Now we merge multiple hook source after each other.

@Override
public OpenFeatureClient addHooks(Hook... hooks) {
this.clientHooks.addAll(Arrays.asList(hooks));
var types = FlagValueType.values();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] seems like this block of code repeats itself, should this maybe be a static helper function?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants