Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: reduce hashmap allocations #1178

Merged
merged 6 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,13 @@ mvn test -P e2e
There is a small JMH benchmark suite for testing allocations that can be run with:

```sh
mvn -P benchmark test-compile jmh:benchmark -Djmh.f=1 -Djmh.prof='dev.openfeature.sdk.benchmark.AllocationProfiler'
mvn -P benchmark clean compile test-compile jmh:benchmark -Djmh.f=1 -Djmh.prof='dev.openfeature.sdk.benchmark.AllocationProfiler'
```

If you are concerned about the repercussions of a change on memory usage, run this an compare the results to the committed. `benchmark.txt` file.
Note that the ONLY MEANINGFUL RESULTS of this benchmark are the `totalAllocatedBytes` and the `totalAllocatedInstances`.
The `run` score, and maven task time are not relevant since this benchmark is purely memory-related and has nothing to do with speed.
You can also view the heap breakdown to see which objects are taking up the most memory.

## Releasing

Expand Down
262 changes: 131 additions & 131 deletions benchmark.txt

Large diffs are not rendered by default.

11 changes: 10 additions & 1 deletion src/main/java/dev/openfeature/sdk/AbstractStructure.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.HashMap;
import java.util.Map;
import java.util.Collections;

@SuppressWarnings({ "PMD.BeanMembersShouldSerialize", "checkstyle:MissingJavadocType" })
abstract class AbstractStructure implements Structure {
Expand All @@ -18,7 +19,15 @@ public boolean isEmpty() {
}

AbstractStructure(Map<String, Value> attributes) {
this.attributes = new HashMap<>(attributes);
this.attributes = attributes;
}

/**
* Returns an unmodifiable representation of the internal attribute map.
* @return immutable map
*/
public Map<String, Value> asUnmodifiableMap() {
return Collections.unmodifiableMap(attributes);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

According to SO, Collections.unmodifiableMap already does this.

}

/**
Expand Down
39 changes: 39 additions & 0 deletions src/main/java/dev/openfeature/sdk/EvaluationContext.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
package dev.openfeature.sdk;

import java.util.Map;
import java.util.Map.Entry;
import java.util.function.Function;

/**
* The EvaluationContext is a container for arbitrary contextual data
* that can be used as a basis for dynamic evaluation.
Expand All @@ -19,4 +23,39 @@
* @return resulting merged context
*/
EvaluationContext merge(EvaluationContext overridingContext);

/**
* Recursively merges the overriding map into the base Value map.
* The base map is mutated, the overriding map is not.
* Null maps will cause no-op.
*
* @param newStructure function to create the right structure(s) for Values
* @param base base map to merge
* @param overriding overriding map to merge
*/
static void mergeMaps(Function<Map<String, Value>, Structure> newStructure,
Map<String, Value> base,
Map<String, Value> overriding) {

if (base == null) {
return;

Check warning on line 41 in src/main/java/dev/openfeature/sdk/EvaluationContext.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/dev/openfeature/sdk/EvaluationContext.java#L41

Added line #L41 was not covered by tests
}
if (overriding == null || overriding.isEmpty()) {
return;

Check warning on line 44 in src/main/java/dev/openfeature/sdk/EvaluationContext.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/dev/openfeature/sdk/EvaluationContext.java#L44

Added line #L44 was not covered by tests
}

for (Entry<String, Value> overridingEntry : overriding.entrySet()) {
String key = overridingEntry.getKey();
if (overridingEntry.getValue().isStructure() && base.containsKey(key) && base.get(key).isStructure()) {
Structure mergedValue = base.get(key).asStructure();
Structure overridingValue = overridingEntry.getValue().asStructure();
Map<String, Value> newMap = mergedValue.asMap();
mergeMaps(newStructure, newMap,
overridingValue.asUnmodifiableMap());
base.put(key, new Value(newStructure.apply(newMap)));
} else {
base.put(key, overridingEntry.getValue());
}
}
}
}
29 changes: 16 additions & 13 deletions src/main/java/dev/openfeature/sdk/ImmutableContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@
import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;

import dev.openfeature.sdk.internal.ExcludeFromGeneratedCoverageReport;
import lombok.ToString;
import lombok.experimental.Delegate;

/**
* The EvaluationContext is a container for arbitrary contextual data
* that can be used as a basis for dynamic evaluation.
* The ImmutableContext is an EvaluationContext implementation which is threadsafe, and whose attributes can
* The ImmutableContext is an EvaluationContext implementation which is
* threadsafe, and whose attributes can
* not be modified after instantiation.
*/
@ToString
Expand All @@ -21,7 +23,8 @@
private final ImmutableStructure structure;

/**
* Create an immutable context with an empty targeting_key and attributes provided.
* Create an immutable context with an empty targeting_key and attributes
* provided.
*/
public ImmutableContext() {
this(new HashMap<>());
Expand All @@ -42,7 +45,7 @@
* @param attributes evaluation context attributes
*/
public ImmutableContext(Map<String, Value> attributes) {
this("", attributes);
this(null, attributes);
}

/**
Expand All @@ -53,9 +56,7 @@
*/
public ImmutableContext(String targetingKey, Map<String, Value> attributes) {
if (targetingKey != null && !targetingKey.trim().isEmpty()) {
final Map<String, Value> actualAttribs = new HashMap<>(attributes);
actualAttribs.put(TARGETING_KEY, new Value(targetingKey));
this.structure = new ImmutableStructure(actualAttribs);
this.structure = new ImmutableStructure(targetingKey, attributes);
} else {
this.structure = new ImmutableStructure(attributes);
}
Expand All @@ -71,31 +72,33 @@
}

/**
* Merges this EvaluationContext object with the passed EvaluationContext, overriding in case of conflict.
* Merges this EvaluationContext object with the passed EvaluationContext,
* overriding in case of conflict.
*
* @param overridingContext overriding context
* @return new, resulting merged context
*/
@Override
public EvaluationContext merge(EvaluationContext overridingContext) {
if (overridingContext == null || overridingContext.isEmpty()) {
return new ImmutableContext(this.asMap());
return new ImmutableContext(this.asUnmodifiableMap());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
if (this.isEmpty()) {
return new ImmutableContext(overridingContext.asMap());
return new ImmutableContext(overridingContext.asUnmodifiableMap());

Check warning on line 87 in src/main/java/dev/openfeature/sdk/ImmutableContext.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L87 was not covered by tests
}

return new ImmutableContext(
this.merge(ImmutableStructure::new, this.asMap(), overridingContext.asMap()));
Map<String, Value> attributes = this.asMap();
EvaluationContext.mergeMaps(ImmutableStructure::new, attributes,
overridingContext.asUnmodifiableMap());
return new ImmutableContext(attributes);
}

@SuppressWarnings("all")
private static class DelegateExclusions {
@ExcludeFromGeneratedCoverageReport
public <T extends Structure> Map<String, Value> merge(Function<Map<String, Value>, Structure> newStructure,
public <T extends Structure> Map<String, Value> merge(Function<Map<String, Value>, Structure> newStructure,
Map<String, Value> base,
Map<String, Value> overriding) {

return null;
}
}
Expand Down
13 changes: 12 additions & 1 deletion src/main/java/dev/openfeature/sdk/ImmutableStructure.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ public ImmutableStructure() {
* @param attributes attributes.
*/
public ImmutableStructure(Map<String, Value> attributes) {
super(copyAttributes(attributes));
super(copyAttributes(attributes, null));
}

protected ImmutableStructure(String targetingKey, Map<String, Value> attributes) {
super(copyAttributes(attributes, targetingKey));
}

@Override
Expand All @@ -62,11 +66,18 @@ public Map<String, Value> asMap() {
}

private static Map<String, Value> copyAttributes(Map<String, Value> in) {
return copyAttributes(in, null);
}

private static Map<String, Value> copyAttributes(Map<String, Value> in, String targetingKey) {
Map<String, Value> copy = new HashMap<>();
for (Entry<String, Value> entry : in.entrySet()) {
copy.put(entry.getKey(),
Optional.ofNullable(entry.getValue()).map((Value val) -> val.clone()).orElse(null));
}
if (targetingKey != null) {
copy.put(EvaluationContext.TARGETING_KEY, new Value(targetingKey));
}
return copy;
}

Expand Down
11 changes: 6 additions & 5 deletions src/main/java/dev/openfeature/sdk/MutableContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public MutableContext(String targetingKey) {
}

public MutableContext(Map<String, Value> attributes) {
this("", attributes);
this(null, new HashMap<>(attributes));
}

/**
Expand All @@ -44,7 +44,7 @@ public MutableContext(Map<String, Value> attributes) {
* @param attributes evaluation context attributes
*/
public MutableContext(String targetingKey, Map<String, Value> attributes) {
this.structure = new MutableStructure(attributes);
this.structure = new MutableStructure(new HashMap<>(attributes));
if (targetingKey != null && !targetingKey.trim().isEmpty()) {
this.structure.attributes.put(TARGETING_KEY, new Value(targetingKey));
}
Expand Down Expand Up @@ -121,9 +121,10 @@ public EvaluationContext merge(EvaluationContext overridingContext) {
return overridingContext;
}

Map<String, Value> merged = this.merge(
MutableStructure::new, this.asMap(), overridingContext.asMap());
return new MutableContext(merged);
Map<String, Value> attributes = this.asMap();
EvaluationContext.mergeMaps(
MutableStructure::new, attributes, overridingContext.asUnmodifiableMap());
return new MutableContext(attributes);
}

/**
Expand Down
Loading
Loading