FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

perf: skip unmodifiableMap wrapper when hookHints is empty by tobias-ibounig-dt · Pull Request #1953 · open-feature/java-sdk · GitHub

/ java-sdk Public

perf: skip unmodifiableMap wrapper when hookHints is empty - #1953

Merged
toddbaert merged 5 commits into
open-feature:mainfrom
tobias-ibounig-dt:perf/pr-2-hook-hints
Jun 10, 2026
ConversationCommits (5)ChecksFiles changed
Merged

perf: skip unmodifiableMap wrapper when hookHints is empty#1953
toddbaert merged 5 commits into
open-feature:mainfrom
tobias-ibounig-dt:perf/pr-2-hook-hints

Conversation

tobias-ibounig-dt commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

This PR

  • Skips the Collections.unmodifiableMap() wrapper when hookHints is empty (the common case)

Related Issues

None

Notes

hookHints is resolved once per flag evaluation. Previously it was always wrapped in an unmodifiable view, even when empty. This PR avoids that allocation by returning Collections.emptyMap() directly when there are
no hints.

Allocation impact is negligible, but reduces on totalAllocatedInstances can be seen.

Metric main (baseline) PR 2 Delta
run:+totalAllocatedBytes 124,265,984 124,268,232 ~0 (noise)
run:+totalAllocatedInstances 2,723,316 2,691,291 −32,025 (−1.2%)

Follow-up Tasks

  • More PRs with memory improvements
  • Update benchmark.txt after all are applied

tobias-ibounig-dt requested review from a team as code owners June 5, 2026 08:27
gemini-code-assist Bot reviewed Jun 5, 2026
View reviewed changes

gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code Review

This pull request optimizes the initialization of hookSupportData.hints in OpenFeatureClient.java by using Collections.emptyMap() when the hook hints are empty. A review comment points out that getHookHints() could return null, which would cause a NullPointerException when calling isEmpty(), and suggests adding a null check to handle this case safely.

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.

Comment thread src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
Comment on lines +173 to +174
var hookHints = flagOptions.getHookHints();
hookSupportData.hints = hookHints.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(hookHints);

gemini-code-assist Bot Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If flagOptions.getHookHints() returns null (which can happen if hookHints is explicitly set to null via the builder in FlagEvaluationOptions), calling hookHints.isEmpty() will throw a NullPointerException.

To ensure robust defensive programming, we should handle the null case gracefully by defaulting to Collections.emptyMap().

Suggested change
var hookHints = flagOptions.getHookHints();
hookSupportData.hints = hookHints.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(hookHints);
var hookHints = flagOptions.getHookHints();
hookSupportData.hints = hookHints == null || hookHints.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(hookHints);

chrfwow Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this is something that should be handled in the builder, but idk if we can modify this, since I assume it will be a lombok builder. Can we add a test to make sure we won't have a NPE?

tobias-ibounig-dt Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ensured in builder that it can't be null

perf: skip unmodifiableMap wrapper when hookHints is empty
d583cd2 
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>

toddbaert commented Jun 8, 2026

Copy link
Copy Markdown
Member

Same codecov-action issue as #1955 (comment) - rebasing this PR onto main should fix it. cc @chrfwow

ensure hookHints non null via builder, added test
796d5b7 
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>

tobias-ibounig-dt commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Realized another thing in FlagEvaluationOptions. Builder Default is currently new HashMap<>();, would it make sense to change this to Collections.emptyMap()?
Didn't do it yet, as it could theoretically break someone doing something like getHookHints().put(...).
Any preference from your side?

tobias-ibounig-dt requested a review from chrfwow June 8, 2026 15:39

codecov Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.28%. Comparing base (50da905) to head (a5b5795).

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #1953      +/-   ##
============================================
+ Coverage     92.26%   93.28%   +1.02%     
- Complexity      654      658       +4     
============================================
  Files            59       59              
  Lines          1590     1594       +4     
  Branches        179      181       +2     
============================================
+ Hits           1467     1487      +20     
+ Misses           76       62      -14     
+ Partials         47       45       -2
Flag Coverage Δ
unittests 93.28% <100.00%> (+1.02%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

test null does not throw exception
8a9fc31 
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>

chrfwow commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

The current build issue can be fixed by running spotless:apply

spotless
aad8866 
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>

tobias-ibounig-dt commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

The current build issue can be fixed by running spotless:apply

Oh sorry, I thought I made sure to run it every time.

toddbaert self-requested a review June 10, 2026 18:41

toddbaert commented Jun 10, 2026

Copy link
Copy Markdown
Member

Realized another thing in FlagEvaluationOptions. Builder Default is currently new HashMap<>();, would it make sense to change this to Collections.emptyMap()? Didn't do it yet, as it could theoretically break someone doing something like getHookHints().put(...). Any preference from your side?

That one seems like a breaking change probably not worth the improvement.

sonarqubecloud Bot commented Jun 10, 2026

Copy link
Copy Markdown

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

toddbaert merged commit b592514 into open-feature:main Jun 10, 2026
10 checks passed
openfeaturebot mentioned this pull request Jun 10, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

chrfwow chrfwow approved these changes
toddbaert toddbaert approved these changes
+1 more reviewer
gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL