diff --git a/changelog/@unreleased/pr-2976.v2.yml b/changelog/@unreleased/pr-2976.v2.yml new file mode 100644 index 000000000..87c0593d3 --- /dev/null +++ b/changelog/@unreleased/pr-2976.v2.yml @@ -0,0 +1,8 @@ +type: improvement +improvement: + description: SafetyEvaluator memoizes per-type safety results, making objects + codegen cost proportional to type-graph size rather than the number of paths + through it. Large, dense conjure definitions generate up to 10-25x faster + with byte-identical output. + links: + - https://github.com/palantir/conjure-java/pull/2976 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 1f3ad20a0..138ae39eb 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 @@ -38,6 +38,7 @@ import com.palantir.conjure.spec.TypeName; import com.palantir.conjure.spec.UnionDefinition; import com.palantir.logsafe.Preconditions; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -64,6 +65,7 @@ public final class SafetyEvaluator { public static final Optional UNKNOWN_UNION_VARINT_SAFETY = Optional.empty(); private final Map definitionMap; + private final Map> safetyCache = new HashMap<>(); public SafetyEvaluator(ConjureDefinition definition) { this(TypeFunctions.toTypesMap(definition)); @@ -75,12 +77,12 @@ public SafetyEvaluator(Map definitionMap) { public Optional evaluate(TypeDefinition def) { return Preconditions.checkNotNull(def, "TypeDefinition is required") - .accept(new TypeDefinitionSafetyVisitor(definitionMap, new HashSet<>())); + .accept(new TypeDefinitionSafetyVisitor(definitionMap, new HashSet<>(), safetyCache)); } public Optional evaluate(Type type) { return Preconditions.checkNotNull(type, "TypeDefinition is required") - .accept(new TypeDefinitionSafetyVisitor(definitionMap, new HashSet<>()).fieldVisitor); + .accept(new TypeDefinitionSafetyVisitor(definitionMap, new HashSet<>(), safetyCache).fieldVisitor); } public Optional evaluate(Type type, Optional declaredSafety) { @@ -126,9 +128,15 @@ public Optional getUsageTimeSafety(FieldDefinition field) { private static final class TypeDefinitionSafetyVisitor implements TypeDefinition.Visitor> { private final Set inProgress; private final Type.Visitor> fieldVisitor; + private final Map> cache; + private int cycleHits = 0; - private TypeDefinitionSafetyVisitor(Map definitionMap, Set inProgress) { + private TypeDefinitionSafetyVisitor( + Map definitionMap, + Set inProgress, + Map> cache) { this.inProgress = inProgress; + this.cache = cache; this.fieldVisitor = new FieldSafetyVisitor(definitionMap, this); } @@ -170,15 +178,30 @@ public Optional visitUnknown(String unknownType) { } private Optional with(TypeName typeName, Supplier> task) { + Optional cached = cache.get(typeName); + if (cached != null) { + return cached; + } + boolean outermost = inProgress.isEmpty(); if (!inProgress.add(typeName)) { + cycleHits++; // Given recursive evaluation, we return the least restrictive type: SAFE. return OPTIONAL_OF_SAFE; } + int cycleHitsBefore = cycleHits; Optional result = task.get(); if (!inProgress.remove(typeName)) { throw new IllegalStateException( "Failed to remove " + typeName + " from in-progress, something is very wrong!"); } + // Without memoization, evaluation cost is proportional to the number of paths through + // the type graph, which grows combinatorially on dense type graphs. Results computed + // without hitting the in-progress cycle guard are context-free and safe to memoize; + // outermost results are the canonical evaluation for a type even when a cycle guard + // fired within the subtree. + if (outermost || cycleHits == cycleHitsBefore) { + cache.put(typeName, result); + } return result; } diff --git a/conjure-java-core/src/test/java/com/palantir/conjure/java/types/SafetyEvaluatorTest.java b/conjure-java-core/src/test/java/com/palantir/conjure/java/types/SafetyEvaluatorTest.java index 922ec9459..ab2af6248 100644 --- a/conjure-java-core/src/test/java/com/palantir/conjure/java/types/SafetyEvaluatorTest.java +++ b/conjure-java-core/src/test/java/com/palantir/conjure/java/types/SafetyEvaluatorTest.java @@ -408,4 +408,72 @@ private static Stream getTypes(Type externalReference) { Arguments.of(Named.of("Alias", aliasType), conjureAliasDef), Arguments.of(Named.of("Union", unionType), conjureUnionDef)); } + + @Test + void testSharedEvaluatorMatchesFreshEvaluations() { + TypeDefinition firstObject = TypeDefinition.object(ObjectDefinition.builder() + .typeName(FOO) + .fields(FieldDefinition.builder() + .fieldName(FieldName.of("bar")) + .type(Type.reference(BAR)) + .build()) + .build()); + TypeDefinition secondObject = TypeDefinition.object(ObjectDefinition.builder() + .typeName(BAR) + .fields(FieldDefinition.builder() + .fieldName(FieldName.of("aliasRef")) + .type(Type.reference(UNSAFE_ALIAS_NAME)) + .build()) + .build()); + ConjureDefinition conjureDef = ConjureDefinition.builder() + .version(1) + .types(firstObject) + .types(secondObject) + .types(UNSAFE_ALIAS) + .build(); + ConjureDefinitionValidator.validateAll(conjureDef, SafetyDeclarationRequirements.ALLOWED); + SafetyEvaluator shared = new SafetyEvaluator(conjureDef); + assertThat(shared.evaluate(firstObject)).hasValue(LogSafety.UNSAFE); + // Repeated evaluations on the same instance take the memoized path and must agree + // with fresh evaluators. + assertThat(shared.evaluate(firstObject)).hasValue(LogSafety.UNSAFE); + assertThat(shared.evaluate(secondObject)).isEqualTo(new SafetyEvaluator(conjureDef).evaluate(secondObject)); + } + + @Test + void testRecursiveTypeSubtreeIsNotMemoizedWithCycleSubstitution() { + // Foo <-> Bar cycle where only Foo carries unsafe data. While evaluating Foo, the + // cycle guard substitutes SAFE for the in-progress Foo reference, making Bar appear + // SAFE within that traversal. A later standalone evaluation of Bar on the same + // evaluator must still see through the cycle to Foo's unsafe field rather than + // reusing the substituted value. + TypeDefinition firstObject = TypeDefinition.object(ObjectDefinition.builder() + .typeName(FOO) + .fields(FieldDefinition.builder() + .fieldName(FieldName.of("bar")) + .type(Type.reference(BAR)) + .build()) + .fields(FieldDefinition.builder() + .fieldName(FieldName.of("aliasRef")) + .type(Type.reference(UNSAFE_ALIAS_NAME)) + .build()) + .build()); + TypeDefinition secondObject = TypeDefinition.object(ObjectDefinition.builder() + .typeName(BAR) + .fields(FieldDefinition.builder() + .fieldName(FieldName.of("foo")) + .type(Type.reference(FOO)) + .build()) + .build()); + ConjureDefinition conjureDef = ConjureDefinition.builder() + .version(1) + .types(firstObject) + .types(secondObject) + .types(UNSAFE_ALIAS) + .build(); + SafetyEvaluator shared = new SafetyEvaluator(conjureDef); + assertThat(shared.evaluate(firstObject)).hasValue(LogSafety.UNSAFE); + assertThat(shared.evaluate(secondObject)).hasValue(LogSafety.UNSAFE); + assertThat(shared.evaluate(firstObject)).hasValue(LogSafety.UNSAFE); + } }