From 2136a67d556dc7559dc201e74cfd7a8024136f8c Mon Sep 17 00:00:00 2001 From: Andrew Ash Date: Fri, 10 Jan 2025 22:53:57 -0800 Subject: [PATCH 1/3] Reduce Optional allocations in SafetyEvaluator --- .../conjure/java/types/SafetyEvaluator.java | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java index 3313e2600..f8004ed26 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java @@ -16,6 +16,8 @@ package com.palantir.conjure.java.types; +import com.google.common.collect.ImmutableTable; +import com.google.common.collect.Table; import com.palantir.conjure.java.util.TypeFunctions; import com.palantir.conjure.spec.AliasDefinition; import com.palantir.conjure.spec.ArgumentDefinition; @@ -37,17 +39,23 @@ import com.palantir.conjure.spec.UnionDefinition; import com.palantir.logsafe.Preconditions; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; +import java.util.stream.Stream; public final class SafetyEvaluator { + + // Make a constant to avoid allocating a new Optional every time we process a type definition for safety + private static final Optional OPTIONAL_OF_SAFE = Optional.of(LogSafety.SAFE); + /** * Enums contain an unknown variant, however we assume that the unknown variant is only used * for past and future values which are known at compile-time in that version. */ - public static final Optional ENUM_SAFETY = Optional.of(LogSafety.SAFE); + public static final Optional ENUM_SAFETY = OPTIONAL_OF_SAFE; /** * Unknown variant should be considered unsafe because we don't know what kind of data it may contain, * however this makes rollout much more challenging, so we will ratchet unknown safety once we have @@ -137,7 +145,7 @@ public Optional visitEnum(EnumDefinition _value) { @Override public Optional visitObject(ObjectDefinition value) { return with(value.getTypeName(), () -> { - Optional safety = Optional.of(LogSafety.SAFE); + Optional safety = OPTIONAL_OF_SAFE; for (FieldDefinition field : value.getFields()) { safety = combine(safety, getSafety(field.getType(), field.getSafety())); } @@ -164,7 +172,7 @@ public Optional visitUnknown(String unknownType) { private Optional with(TypeName typeName, Supplier> task) { if (!inProgress.add(typeName)) { // Given recursive evaluation, we return the least restrictive type: SAFE. - return Optional.of(LogSafety.SAFE); + return OPTIONAL_OF_SAFE; } Optional result = task.get(); if (!inProgress.remove(typeName)) { @@ -298,7 +306,29 @@ public Optional visitUnknown(String unknownValue) { } } + private static final Table, Optional, Optional> COMBINE_TABLE = + computeCombineTable(); + + private static Table, Optional, Optional> computeCombineTable() { + List> allValues = Stream.concat( + Stream.of(Optional.empty()), + LogSafety.values().stream().map(Optional::of)) + .toList(); + ImmutableTable.Builder, Optional, Optional> builder = + ImmutableTable.builder(); + for (Optional left : allValues) { + for (Optional right : allValues) { + builder.put(left, right, computeCombine(left, right)); + } + } + return builder.buildOrThrow(); + } + public static Optional combine(Optional one, Optional two) { + return Preconditions.checkNotNull(COMBINE_TABLE.get(one, two), "Missing an entry in the combine table"); + } + + private static Optional computeCombine(Optional one, Optional two) { if (one.isPresent() && two.isPresent()) { return Optional.of(combine(one.get(), two.get())); } @@ -307,7 +337,7 @@ public static Optional combine(Optional one, Optional !LogSafety.SAFE.equals(value)); } - public static LogSafety combine(LogSafety one, LogSafety two) { + private static LogSafety combine(LogSafety one, LogSafety two) { LogSafety.Value first = one.get(); LogSafety.Value second = two.get(); if (first == LogSafety.Value.UNKNOWN || second == LogSafety.Value.UNKNOWN) { From b1402bfffe6e03c62f67df2afa0278863a664d42 Mon Sep 17 00:00:00 2001 From: Andrew Ash Date: Fri, 10 Jan 2025 23:06:30 -0800 Subject: [PATCH 2/3] checkstyle --- .../com/palantir/conjure/java/types/SafetyEvaluator.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java index f8004ed26..1f3ad20a0 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/SafetyEvaluator.java @@ -324,10 +324,6 @@ private static Table, Optional, Optional combine(Optional one, Optional two) { - return Preconditions.checkNotNull(COMBINE_TABLE.get(one, two), "Missing an entry in the combine table"); - } - private static Optional computeCombine(Optional one, Optional two) { if (one.isPresent() && two.isPresent()) { return Optional.of(combine(one.get(), two.get())); @@ -352,6 +348,10 @@ private static LogSafety combine(LogSafety one, LogSafety two) { return one; } + public static Optional combine(Optional one, Optional two) { + return Preconditions.checkNotNull(COMBINE_TABLE.get(one, two), "Missing an entry in the combine table"); + } + public static boolean allows(Optional required, Optional given) { if (required.isEmpty() || given.isEmpty()) { // If there is no requirement, all inputs are allowed. From 571a5e8a8634269c031f40ee248c6d4d32936335 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Sat, 11 Jan 2025 07:07:18 +0000 Subject: [PATCH 3/3] Add generated changelog entries --- changelog/@unreleased/pr-2433.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2433.v2.yml diff --git a/changelog/@unreleased/pr-2433.v2.yml b/changelog/@unreleased/pr-2433.v2.yml new file mode 100644 index 000000000..0208d1090 --- /dev/null +++ b/changelog/@unreleased/pr-2433.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Reduce Optional allocations in SafetyEvaluator + links: + - https://github.com/palantir/conjure-java/pull/2433