feat: equivalent EvaluationContext impls are .equal #1771
Conversation
Summary of ChangesHello @chrfwow, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the core Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 the ImmutableContext and LayeredEvaluationContext classes to improve performance and correctness, particularly concerning object equality and hashing. For ImmutableContext, the Lombok-generated equals and hashCode methods were replaced with custom implementations that lazily compute and cache the hash code based on the context's unmodifiable map. For LayeredEvaluationContext, a cachedMap was introduced to store the resolved attribute map, which is lazily computed and invalidated when underlying contexts change, optimizing repeated map access. Custom equals and hashCode methods were also added for LayeredEvaluationContext, relying on this cached map. The review comment suggests an optimization for LayeredEvaluationContext's map creation, recommending to call ensureKeySet() before initializing the HashMap for the cachedMap to pre-calculate the required capacity and prevent potential HashMap resizing, thus improving performance. New tests were added to validate these changes, including equality and hash code stability for both context types, and behavior of asObjectMap() for LayeredEvaluationContext.
| if (keySet != null && keySet.isEmpty()) { | ||
| return new HashMap<>(0); | ||
| cachedMap = Collections.emptyMap(); | ||
| return cachedMap; | ||
| } | ||
|
|
||
| HashMap<String, Value> map; | ||
| if (keySet != null) { | ||
| map = new HashMap<>(keySet.size()); | ||
| // use helper to size the map based on expected entries | ||
| map = HashMapUtils.forEntries(keySet.size()); | ||
| } else { | ||
| map = new HashMap<>(); | ||
| } |
There was a problem hiding this comment.
To improve performance and avoid potential HashMap resizing, it's better to compute the full set of keys before initializing the map. By calling ensureKeySet() upfront, you can guarantee that the HashMap is created with the correct initial capacity to hold all entries, which is more efficient than potentially creating a default-sized map that needs to be rehashed.
ensureKeySet();
if (this.keySet.isEmpty()) {
cachedMap = Collections.emptyMap();
return cachedMap;
}
// use helper to size the map based on expected entries
HashMap<String, Value> map = HashMapUtils.forEntries(this.keySet.size());
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1771 +/- ##
============================================
+ Coverage 92.53% 93.22% +0.69%
- Complexity 599 617 +18
============================================
Files 55 55
Lines 1406 1446 +40
Branches 154 161 +7
============================================
+ Hits 1301 1348 +47
+ Misses 64 54 -10
- Partials 41 44 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
toddbaert
force-pushed
the
fix-as-object-map
branch
3 times, most recently
from
c432b35 to
b31f3d2
Compare
toddbaert
force-pushed
the
fix-as-object-map
branch
from
82a8df5 to
08cd40d
Compare
This reverts commit 4cb39a4. Co-Authored-By: Vishalup29 <[email protected]> Signed-off-by: Todd Baert <[email protected]>
toddbaert
force-pushed
the
fix-as-object-map
branch
from
9777d71 to
f75201a
Compare
|
toddbaert
changed the title
3 participants
This PR
Re-revert of #1768 with associated fixes: The object map of the LayeredEvaluationContext needs to be computed by calling the
asObjectMapmethod of the layered contexts. Otherwise, nested EvaluationContexts will not get unwrapped (i.e. converted to HashMaps)Related Issues
Fixes #1754
See also open-feature/java-sdk-contrib#1665