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

perf: return ImmutableContext.EMPTY when merging two empty contexts by tobias-ibounig-dt · Pull Request #1973 · open-feature/java-sdk · GitHub

/ java-sdk Public

perf: return ImmutableContext.EMPTY when merging two empty contexts - #1973

Merged
toddbaert merged 1 commit into
open-feature:mainfrom
tobias-ibounig-dt:perf/pr-10-empty-context-merge
Jun 22, 2026
ConversationCommits (1)ChecksFiles changed
Merged

perf: return ImmutableContext.EMPTY when merging two empty contexts#1973
toddbaert merged 1 commit into
open-feature:mainfrom
tobias-ibounig-dt:perf/pr-10-empty-context-merge

Conversation

tobias-ibounig-dt commented Jun 19, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

This PR

  • ImmutableContext.merge() returns ImmutableContext.EMPTY when both this and the overriding context are empty

Related Issues

None

Notes

When merging two empty contexts the previous code still allocated a new ImmutableContext (via this.asUnmodifiableMap()). The result is always semantically equivalent to EMPTY, so we can return the existing singleton directly.

This path is not exercised by the current benchmark workload, so no allocation numbers are provided.

Follow-up Tasks

  • More PRs with memory improvements

tobias-ibounig-dt requested review from a team as code owners June 19, 2026 11:41

coderabbitai Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info ⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 02802ded-1d75-4873-a416-c0d8da448921

📥 Commits

Reviewing files that changed from the base of the PR and between c7ea4db and 1d4609c.

📒 Files selected for processing (2)
  • src/main/java/dev/openfeature/sdk/ImmutableContext.java
  • src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/java/dev/openfeature/sdk/ImmutableContext.java
  • src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
📝 Walkthrough

Walkthrough

ImmutableContext.merge() is updated to return the EMPTY singleton instead of constructing a new instance when both the current context and the overriding context are empty. New tests cover constructor branch behavior for null/empty inputs and verify the EMPTY singleton short-circuit semantics.

Changes

ImmutableContext EMPTY singleton short-circuit

Layer / File(s) Summary
merge() short-circuit and comprehensive test coverage
src/main/java/dev/openfeature/sdk/ImmutableContext.java, src/test/java/dev/openfeature/sdk/ImmutableContextTest.java		 merge() now returns ImmutableContext.EMPTY when this.isEmpty() and the overriding context is null or empty, avoiding unnecessary allocation. New nested test classes assert EMPTY singleton identity for all empty-context merge combinations, verify non-empty contexts do not return EMPTY, and cover constructor branches for null/empty targeting key and attributes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • open-feature/java-sdk#1972: Both PRs modify ImmutableContext empty-context handling around the shared ImmutableContext.EMPTY singleton—main PR changes ImmutableContext.merge(...) to short-circuit to EMPTY for empty/null overrides, while retrieved PR alters ImmutableContext constructors/empty structure to reduce allocations and support consistent empty-singleton behavior.

Suggested reviewers

  • toddbaert
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: returning ImmutableContext.EMPTY when merging two empty contexts, which is the core optimization in this PR.
Description check ✅ Passed The description clearly explains the optimization, the rationale, and acknowledges that the code path is not exercised by current benchmarks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Comment @coderabbitai help to get the list of available commands and usage tips.

codecov Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.20%. Comparing base (62fda56) to head (1d4609c).

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #1973      +/-   ##
============================================
+ Coverage     92.21%   93.20%   +0.98%     
- Complexity      666      670       +4     
============================================
  Files            59       59              
  Lines          1631     1633       +2     
  Branches        185      186       +1     
============================================
+ Hits           1504     1522      +18     
+ Misses           80       66      -14     
+ Partials         47       45       -2
Flag Coverage Δ
unittests 93.20% <100.00%> (+0.98%) ⬆️

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.

perf: return ImmutableContext.EMPTY when merging two empty contexts
1d4609c 
Signed-off-by: Tobias Ibounig <tobias.ibounig@dynatrace.com>
toddbaert force-pushed the perf/pr-10-empty-context-merge branch from c7ea4db to 1d4609c Compare June 22, 2026 15:13
toddbaert merged commit d492685 into open-feature:main Jun 22, 2026
12 checks passed

sonarqubecloud Bot commented Jun 22, 2026

Copy link
Copy Markdown

Quality Gate passed

Issues
0 New issues
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

openfeaturebot mentioned this pull request Jun 22, 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

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