Skip to content

Conversation

@chrfwow
Copy link
Contributor

@chrfwow chrfwow commented Nov 25, 2025

This Pr introduces some performance enhacements, that together yield the following results:

Baseline (AllocationBenchmark.main run five times with the IntelliJ Profiler and averaged):
runtime: 5329 ms
memory allocations: 16.47 GB

This PR:
runtime: 5404ms (no significant change, the deviation of runs is quite large)
memory allocations: 14.62 GB

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
# Conflicts:
#	src/test/java/dev/openfeature/sdk/benchmark/AllocationBenchmark.java
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
@chrfwow chrfwow marked this pull request as ready for review November 25, 2025 12:55
@chrfwow chrfwow requested review from a team as code owners November 25, 2025 12:55
# Conflicts:
#	src/main/java/dev/openfeature/sdk/ImmutableMetadata.java
@codecov
Copy link

codecov bot commented Nov 25, 2025

Codecov Report

❌ Patch coverage is 96.87500% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.66%. Comparing base (ab04409) to head (293c062).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...n/java/dev/openfeature/sdk/ImmutableStructure.java 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1741      +/-   ##
============================================
+ Coverage     92.51%   93.66%   +1.15%     
- Complexity      593      601       +8     
============================================
  Files            53       55       +2     
  Lines          1403     1406       +3     
  Branches        150      154       +4     
============================================
+ Hits           1298     1317      +19     
+ Misses           64       50      -14     
+ Partials         41       39       -2     
Flag Coverage Δ
unittests 93.66% <96.87%> (+1.15%) ⬆️

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.

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
@toddbaert
Copy link
Member

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several performance enhancements aimed at reducing memory allocations. The changes include using singleton EMPTY instances for various classes, replacing object builders with direct constructor calls, removing Optional usage in favor of direct null checks, and pre-sizing HashMaps. These are all excellent optimizations that contribute to the stated goal of reducing memory footprint. I've added a couple of suggestions to further refine the HashMap pre-sizing logic for even better performance. Overall, this is a great set of improvements.

Map<String, Value> copy = new HashMap<>();
Map<String, Value> copy;
if (in != null) {
var numMappings = in.size() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To make the HashMap pre-sizing more accurate, you can check if targetingKey is non-null here. This avoids overallocating when targetingKey is null, which is a good practice for performance-sensitive code.

Suggested change
var numMappings = in.size() + 1;
var numMappings = in.size() + (targetingKey != null ? 1 : 0);

copy.put(key, cloned);
}
} else {
copy = new HashMap<>(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To avoid allocating a HashMap with capacity 1 when targetingKey is null, you can conditionally set the capacity. This makes the allocation more efficient by not allocating memory if it's not needed.

Suggested change
copy = new HashMap<>(1);
copy = new HashMap<>(targetingKey != null ? 1 : 0);

Comment on lines 106 to 124
for (int j = 0; j < 2; j++) {
blackhole.consume(state.client.getBooleanValue(BOOLEAN_FLAG_KEY, false));
blackhole.consume(state.client.getStringValue(STRING_FLAG_KEY, "default"));
blackhole.consume(state.client.getIntegerValue(INT_FLAG_KEY, 0, state.invocationContext));
blackhole.consume(state.client.getDoubleValue(FLOAT_FLAG_KEY, 0.0));
blackhole.consume(state.client.getObjectDetails(
OBJECT_FLAG_KEY, new Value(new ImmutableStructure()), state.invocationContext));
}

OpenFeatureAPI.getInstance().setTransactionContext(new ImmutableContext(state.transactionAttr2));

for (int j = 0; j < 2; j++) {
blackhole.consume(state.client.getBooleanValue(BOOLEAN_FLAG_KEY, false));
blackhole.consume(state.client.getStringValue(STRING_FLAG_KEY, "default"));
blackhole.consume(state.client.getIntegerValue(INT_FLAG_KEY, 0, state.invocationContext));
blackhole.consume(state.client.getDoubleValue(FLOAT_FLAG_KEY, 0.0));
blackhole.consume(state.client.getObjectDetails(
OBJECT_FLAG_KEY, new Value(new ImmutableStructure()), state.invocationContext));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

There is significant code duplication in the context benchmark method, and also between context and main. To improve readability and maintainability, you could extract the repeated block of flag evaluation calls into a private helper method. For example:

private static void runEvals(Blackhole blackhole, AllocationBenchmarkState state, int iterations) {
    for (int j = 0; j < iterations; j++) {
        blackhole.consume(state.client.getBooleanValue(BOOLEAN_FLAG_KEY, false));
        blackhole.consume(state.client.getStringValue(STRING_FLAG_KEY, "default"));
        blackhole.consume(state.client.getIntegerValue(INT_FLAG_KEY, 0, state.invocationContext));
        blackhole.consume(state.client.getDoubleValue(FLOAT_FLAG_KEY, 0.0));
        blackhole.consume(state.client.getObjectDetails(
                OBJECT_FLAG_KEY, new Value(new ImmutableStructure()), state.invocationContext));
    }
}

You can then call this helper from both context() and main().

@toddbaert
Copy link
Member

I'll merge this one tomorrow @chrfwow . Up to you if you want to address the minor Gemini issues.

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
@chrfwow chrfwow enabled auto-merge (squash) November 27, 2025 14:56
@sonarqubecloud
Copy link

@chrfwow chrfwow merged commit 9052e91 into open-feature:main Nov 27, 2025
13 checks passed
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.

3 participants