perf: reduce hashmap allocations #1178

toddbaert · 2024-10-22T19:03:23Z

Continuation of #1156; further reduces allocations, particularly of ImmutableContext and HashMap.

HashMap instances are reduced by more than 66%, memory usage decreased by another 40% in the benchmark.

Open benchmark.txt for results.

toddbaert · 2024-10-22T19:11:56Z

src/main/java/dev/openfeature/sdk/OpenFeatureClient.java

-                : new ImmutableContext();
-
-        return apiContext.merge(transactionContext.merge(clientContext.merge(invocationContext)));
+        // avoid any unnecessary context instantiations and stream usage here; this is call with every evaluation.


This method is way uglier than before with a nice nested call to .merge() but this results in way less memory usage.

As I see it, we're creating a new ImmutableContext for each merge() invocation. It seems like we could optimize this a bit more by skipping the merging of empty contexts. How do you feel about that? Here's an example of what I have in mind:

final EvaluationContext apiContext = openfeatureApi.getEvaluationContext(); final EvaluationContext clientContext = this.getEvaluationContext(); final EvaluationContext transactionContext = openfeatureApi.getTransactionContext(); final List<EvaluationContext> contextsToMerge = new ArrayList<>(); contextsToMerge.add(apiContext); contextsToMerge.add(transactionContext); contextsToMerge.add(clientContext); contextsToMerge.add(invocationContext); EvaluationContext merged = new ImmutableContext(); for (EvaluationContext evaluationContext : contextsToMerge) { if (evaluationContext != null && !evaluationContext.isEmpty()) { merged = merged.merge(evaluationContext); } } return merged;

But even if we do so, in the worst case (all contexts are not empty), we create 5 ImmutableContext objects per invocation.
What about pulling the merge logic out into a factory method on the 'ImmutableContext' that accepts 4 named contexts to merge?

@ssharaev @guidobrei I've done both of these things, as well as:

use a new variadic method that looks nicer

refactor to move the static merge function from Structure to EvaluationContext

see: f3e0686

toddbaert · 2024-10-22T19:12:33Z

src/main/java/dev/openfeature/sdk/Structure.java

+     *
+     * @return all attributes on the structure into a Map
+     */
+    Map<String, Value> asUnmodifiableMap();


We can make good use of this internally when we want to just get the attributes as a map but don't care if we actually copy them or not - it just returns an immutable view, not a copy.

codecov · 2024-10-22T19:13:18Z

Codecov Report

Attention: Patch coverage is 88.67925% with 6 lines in your changes missing coverage. Please review.

Project coverage is 93.26%. Comparing base (7a1eb9b) to head (0bddca9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...in/java/dev/openfeature/sdk/EvaluationContext.java	70.58%	2 Missing and 3 partials ⚠️
...ain/java/dev/openfeature/sdk/ImmutableContext.java	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1178      +/-   ##
============================================
+ Coverage     93.01%   93.26%   +0.25%     
- Complexity      436      442       +6     
============================================
  Files            40       41       +1     
  Lines          1016     1025       +9     
  Branches         84       85       +1     
============================================
+ Hits            945      956      +11     
  Misses           43       43              
+ Partials         28       26       -2

Flag	Coverage Δ
unittests	`93.26% <88.67%> (+0.25%)`	⬆️

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.

ssharaev · 2024-10-23T04:35:30Z

benchmark.txt

-                 +totalAllocatedBytes:     138762960.000 bytes
-                 +totalAllocatedInstances: 4474389.000 instances
+0.132 s/op
+                 +totalAllocatedBytes:     105727792.000 bytes


Wow, great result! 👍

Even better now with #1178 (comment)

guidobrei · 2024-10-23T07:14:50Z

src/main/java/dev/openfeature/sdk/ImmutableContext.java

@@ -79,14 +78,14 @@ public String getTargetingKey() {
    @Override
    public EvaluationContext merge(EvaluationContext overridingContext) {
        if (overridingContext == null || overridingContext.isEmpty()) {
-            return new ImmutableContext(this.asMap());
+            return new ImmutableContext(this.asUnmodifiableMap());


If we're already in an ImmutableContext and there's nothing to add, why not just return this?

This goes in the direction @ssharaev suggested in https://github.com/open-feature/java-sdk/pull/1178/files#r1811813866

Because then it's not a new instance, and I think always returning a new object from this method is less surprising than sometimes returning the same one.

src/main/java/dev/openfeature/sdk/MutableContext.java

guidobrei · 2024-10-23T07:44:38Z

src/main/java/dev/openfeature/sdk/OpenFeatureClient.java

-                : new ImmutableContext();
-
-        return apiContext.merge(transactionContext.merge(clientContext.merge(invocationContext)));
+        // avoid any unnecessary context instantiations and stream usage here; this is call with every evaluation.


But even if we do so, in the worst case (all contexts are not empty), we create 5 ImmutableContext objects per invocation.
What about pulling the merge logic out into a factory method on the 'ImmutableContext' that accepts 4 named contexts to merge?

Signed-off-by: Todd Baert <[email protected]>

toddbaert · 2024-10-23T16:43:51Z

@guidobrei @ssharaev Please re-review; according to your advice (including @guidobrei 's comment here) I've made additional improvements. Please note, that during this process I merged an improvement to the benchmark to include more non-empty context data at various levels, so we could better gauge improvements in merging allocations.

With your additional feedback, we now even have a more dramatic improvement:

before:

                 +totalAllocatedBytes:       244216640.000 bytes
                 +totalAllocatedInstances: 7264791.000 instances

after:

                 +totalAllocatedBytes:       139359040.000 bytes
                 +totalAllocatedInstances: 4452140.000 instances

Signed-off-by: Todd Baert <[email protected]>

aepfli

I am sorry, I tried reviewing this on my mobile, but I failed half way through ;) I will give it a shot later. There is one comment, which might also reduce the amount of collections again. (Not sure, if this is maybe handled by the function call anyways)

aepfli · 2024-10-23T18:33:36Z

src/main/java/dev/openfeature/sdk/AbstractStructure.java

+     * @return immutable map
+     */
+    public Map<String, Value> asUnmodifiableMap() {
+        return Collections.unmodifiableMap(attributes);


Is it worth to check if this is an unmodifiable nap, before wrapping it?

According to SO, Collections.unmodifiableMap already does this.

toddbaert · 2024-10-23T18:53:23Z

src/main/java/dev/openfeature/sdk/Structure.java

@@ -107,41 +113,6 @@ default Object convertValue(Value value) {
        throw new ValueNotConvertableError();
    }

-    /**


This removal is very technically a breaking change, but it was never intended to be public and it's very awkward to use. It has more to do with EvaluationContexts not Structures anyway and I doubt it's widely used.

sonarcloud · 2024-10-24T12:30:03Z

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 SonarCloud

toddbaert requested a review from a team as a code owner October 22, 2024 19:03

toddbaert force-pushed the chore/more-mem-improvements branch from 87e6c5d to 5c86f3a Compare October 22, 2024 19:03

toddbaert requested review from beeme1mr, ssharaev, aepfli, liran2000 and guidobrei October 22, 2024 19:05

toddbaert commented Oct 22, 2024

View reviewed changes

toddbaert requested a review from justinabrahms October 22, 2024 19:16

toddbaert changed the title ~~chore: reduce hashmap allocations~~ perf: reduce hashmap allocations Oct 22, 2024

toddbaert requested a review from thisthat October 22, 2024 20:37

ssharaev reviewed Oct 23, 2024

View reviewed changes

guidobrei reviewed Oct 23, 2024

View reviewed changes

toddbaert added 4 commits October 23, 2024 12:35

chore: reduce hashmap allocations

896df53

Signed-off-by: Todd Baert <[email protected]>

fixup: use Collections.unmodifiableMap

6a879b1

Signed-off-by: Todd Baert <[email protected]>

fixup: pr feedback

5cd181f

Signed-off-by: Todd Baert <[email protected]>

fixup: make mergemap mutate

bddf7f2

Signed-off-by: Todd Baert <[email protected]>

toddbaert force-pushed the chore/more-mem-improvements branch from f3e0686 to bddf7f2 Compare October 23, 2024 16:37

toddbaert requested review from guidobrei and ssharaev October 23, 2024 16:48

fixup: pmd

e9c251c

Signed-off-by: Todd Baert <[email protected]>

ssharaev approved these changes Oct 23, 2024

View reviewed changes

aepfli reviewed Oct 23, 2024

View reviewed changes

toddbaert commented Oct 23, 2024

View reviewed changes

aepfli approved these changes Oct 24, 2024

View reviewed changes

Merge branch 'main' into chore/more-mem-improvements

0bddca9

toddbaert merged commit fd7659a into main Oct 24, 2024
9 of 10 checks passed

toddbaert deleted the chore/more-mem-improvements branch October 24, 2024 12:41

github-actions bot mentioned this pull request Oct 24, 2024

chore(main): release 1.12.2 #1162

Merged

aepfli mentioned this pull request Oct 24, 2024

Add JMH Benchmarks for the critical code paths #1146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: reduce hashmap allocations #1178

perf: reduce hashmap allocations #1178

toddbaert commented Oct 22, 2024 •

edited

Loading

toddbaert Oct 22, 2024

ssharaev Oct 23, 2024

guidobrei Oct 23, 2024

toddbaert Oct 23, 2024 •

edited

Loading

toddbaert Oct 22, 2024

codecov bot commented Oct 22, 2024 •

edited

Loading

ssharaev Oct 23, 2024

toddbaert Oct 23, 2024

guidobrei Oct 23, 2024

toddbaert Oct 23, 2024

guidobrei Oct 23, 2024

toddbaert commented Oct 23, 2024 •

edited

Loading

aepfli left a comment

aepfli Oct 23, 2024

toddbaert Oct 23, 2024

toddbaert Oct 23, 2024

sonarcloud bot commented Oct 24, 2024

perf: reduce hashmap allocations #1178

perf: reduce hashmap allocations #1178

Conversation

toddbaert commented Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert commented Oct 23, 2024 • edited Loading

aepfli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 24, 2024

Quality Gate passed

toddbaert commented Oct 22, 2024 •

edited

Loading

toddbaert Oct 23, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

toddbaert commented Oct 23, 2024 •

edited

Loading