diff --git a/changelog/@unreleased/pr-2431.v2.yml b/changelog/@unreleased/pr-2431.v2.yml new file mode 100644 index 000000000..0718424c6 --- /dev/null +++ b/changelog/@unreleased/pr-2431.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Optimize Jackson deserialization when deserializing optimized lists + of primitives. + links: + - https://github.com/palantir/conjure-java/pull/2431 diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java index 08358fe06..1cc484141 100644 --- a/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java +++ b/conjure-java-core/src/integrationInput/java/com/palantir/product/ListExample.java @@ -226,7 +226,6 @@ public Builder items(@Safe String items) { return this; } - @JsonSetter(value = "primitiveItems", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) public Builder primitiveItems(@Nonnull Iterable primitiveItems) { checkNotBuilt(); this.primitiveItems = ConjureCollections.newNonNullIntegerList( @@ -234,6 +233,7 @@ public Builder primitiveItems(@Nonnull Iterable primitiveItems) { return this; } + @JsonSetter(value = "primitiveItems", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) public Builder addAllPrimitiveItems(@Nonnull int... primitiveItems) { checkNotBuilt(); ConjureCollections.addAllToIntegerList( @@ -255,7 +255,6 @@ public Builder primitiveItems(int primitiveItems) { return this; } - @JsonSetter(value = "doubleItems", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) public Builder doubleItems(@Nonnull Iterable doubleItems) { checkNotBuilt(); this.doubleItems = ConjureCollections.newNonNullDoubleList( @@ -263,6 +262,7 @@ public Builder doubleItems(@Nonnull Iterable doubleItems) { return this; } + @JsonSetter(value = "doubleItems", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) public Builder addAllDoubleItems(@Nonnull double... doubleItems) { checkNotBuilt(); ConjureCollections.addAllToDoubleList( diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/PrimitiveExample.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/PrimitiveExample.java index 50ce772cc..7b0ca9ed2 100644 --- a/conjure-java-core/src/integrationInput/java/com/palantir/product/PrimitiveExample.java +++ b/conjure-java-core/src/integrationInput/java/com/palantir/product/PrimitiveExample.java @@ -123,7 +123,6 @@ public Builder from(PrimitiveExample other) { return this; } - @JsonSetter(value = "ints", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) public Builder ints(@Nonnull Iterable ints) { checkNotBuilt(); this.ints = @@ -131,6 +130,7 @@ public Builder ints(@Nonnull Iterable ints) { return this; } + @JsonSetter(value = "ints", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) public Builder addAllInts(@Nonnull int... ints) { checkNotBuilt(); ConjureCollections.addAllToIntegerList(this.ints, Preconditions.checkNotNull(ints, "ints cannot be null")); @@ -151,7 +151,6 @@ public Builder ints(int ints) { return this; } - @JsonSetter(value = "doubles", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) public Builder doubles(@Nonnull Iterable doubles) { checkNotBuilt(); this.doubles = ConjureCollections.newNonNullDoubleList( @@ -159,6 +158,7 @@ public Builder doubles(@Nonnull Iterable doubles) { return this; } + @JsonSetter(value = "doubles", nulls = Nulls.SKIP, contentNulls = Nulls.FAIL) public Builder addAllDoubles(@Nonnull double... doubles) { checkNotBuilt(); ConjureCollections.addAllToDoubleList( diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java index 9a535ec52..c95bf3e69 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderGenerator.java @@ -601,11 +601,12 @@ private Iterable createSetters( Map typesMap, boolean override, boolean strict) { - Collection setters = Lists.newArrayListWithExpectedSize(fields.size()); + // Each field tends to fan out into many setters + Collection setters = Lists.newArrayListWithExpectedSize(fields.size() * 5); for (EnrichedField field : fields) { setters.add(createSetter(field, typesMap, override)); if (!strict || field.conjureDef().getType().accept(TypeVisitor.IS_OPTIONAL)) { - setters.addAll(createAuxiliarySetters(field, override)); + setters.addAll(createAuxiliarySetters(field, typesMap, override)); } } return setters; @@ -617,23 +618,6 @@ private MethodSpec createSetter( boolean override) { FieldSpec field = enriched.poetSpec(); Type type = enriched.conjureDef().getType(); - AnnotationSpec.Builder annotationBuilder = AnnotationSpec.builder(JsonSetter.class) - .addMember("value", "$S", enriched.fieldName().get()); - if (type.accept(TypeVisitor.IS_OPTIONAL)) { - annotationBuilder.addMember("nulls", "$T.SKIP", Nulls.class); - } else if (isCollectionType(type)) { - annotationBuilder.addMember("nulls", "$T.SKIP", Nulls.class); - if (isOptionalInnerType(type)) { - annotationBuilder.addMember("contentNulls", "$T.AS_EMPTY", Nulls.class); - } else if (options.nonNullCollections()) { - annotationBuilder.addMember("contentNulls", "$T.FAIL", Nulls.class); - } - } else if (type.accept(TypeVisitor.IS_REFERENCE)) { - Type dealiased = TypeFunctions.toConjureTypeWithoutAliases(type, typesMap); - if (dealiased.accept(DefaultableTypeVisitor.INSTANCE)) { - annotationBuilder.addMember("nulls", "$T.AS_EMPTY", Nulls.class); - } - } boolean shouldClearFirst = true; MethodSpec.Builder setterBuilder = BeanBuilderAuxiliarySettersUtils.publicSetter(enriched, builderClass) @@ -651,11 +635,39 @@ private MethodSpec createSetter( setterBuilder.addCode("this.$L = true;", deriveFieldInitializedName(enriched)); } - return setterBuilder - .addStatement("return this") - .addAnnotations(ConjureAnnotations.override(override)) - .addAnnotation(annotationBuilder.build()) - .build(); + setterBuilder.addStatement("return this").addAnnotations(ConjureAnnotations.override(override)); + + // Certain type combinations and feature flags may produce highly optimized code paths. + // These optimized paths include deserialization, so if it isn't enabled we should add the + // simple deserializer. + if (!isPrimitiveOptimized(type)) { + setterBuilder.addAnnotation(createJacksonSetterAnnotation(enriched, typesMap)); + } + + return setterBuilder.build(); + } + + private AnnotationSpec createJacksonSetterAnnotation( + EnrichedField enriched, Map typesMap) { + Type type = enriched.conjureDef().getType(); + AnnotationSpec.Builder annotationBuilder = AnnotationSpec.builder(JsonSetter.class) + .addMember("value", "$S", enriched.fieldName().get()); + if (type.accept(TypeVisitor.IS_OPTIONAL)) { + annotationBuilder.addMember("nulls", "$T.SKIP", Nulls.class); + } else if (isCollectionType(type)) { + annotationBuilder.addMember("nulls", "$T.SKIP", Nulls.class); + if (isOptionalInnerType(type)) { + annotationBuilder.addMember("contentNulls", "$T.AS_EMPTY", Nulls.class); + } else if (options.nonNullCollections()) { + annotationBuilder.addMember("contentNulls", "$T.FAIL", Nulls.class); + } + } else if (type.accept(TypeVisitor.IS_REFERENCE)) { + Type dealiased = TypeFunctions.toConjureTypeWithoutAliases(type, typesMap); + if (dealiased.accept(DefaultableTypeVisitor.INSTANCE)) { + annotationBuilder.addMember("nulls", "$T.AS_EMPTY", Nulls.class); + } + } + return annotationBuilder.build(); } private MethodSpec createCollectionSetter(String prefix, EnrichedField enriched, boolean override) { @@ -671,7 +683,10 @@ private MethodSpec createCollectionSetter(String prefix, EnrichedField enriched, .build(); } - private MethodSpec createPrimitiveCollectionSetter(EnrichedField enriched, boolean override) { + private MethodSpec createPrimitiveCollectionSetter( + EnrichedField enriched, + Map typesMap, + boolean override) { FieldSpec field = enriched.poetSpec(); Type type = enriched.conjureDef().getType(); @@ -679,6 +694,8 @@ private MethodSpec createPrimitiveCollectionSetter(EnrichedField enriched, boole return BeanBuilderAuxiliarySettersUtils.createPrimitiveCollectionSetterBuilder( enriched, typeMapper, builderClass, safetyEvaluator) .addAnnotations(ConjureAnnotations.override(override)) + // Unlike the other setter, when this setter is present it is always responsible for deserialization + .addAnnotation(createJacksonSetterAnnotation(enriched, typesMap)) .addCode(verifyNotBuilt()) .addCode(CodeBlocks.statement( "$1T.addAllTo$2L(this.$3N, $4L)", @@ -783,7 +800,10 @@ private boolean isByteBuffer(Type type) { return type.accept(TypeVisitor.IS_BINARY) && !options.useImmutableBytes(); } - private List createAuxiliarySetters(EnrichedField enriched, boolean override) { + private List createAuxiliarySetters( + EnrichedField enriched, + Map typesMap, + boolean override) { Type type = enriched.conjureDef().getType(); Optional safety = safetyEvaluator.getUsageTimeSafety(enriched.conjureDef()); @@ -793,7 +813,7 @@ private List createAuxiliarySetters(EnrichedField enriched, boolean CollectionType collectionType = getCollectionType(type); if (collectionType.getConjureCollectionType().isPrimitiveCollection() && collectionType.useNonNullFactory()) { - builder.add(createPrimitiveCollectionSetter(enriched, override)); + builder.add(createPrimitiveCollectionSetter(enriched, typesMap, override)); } builder.add( @@ -1062,11 +1082,11 @@ private static final class CollectionType { this.nullHandlingMode = nullHandlingMode; } - public ConjureCollectionType getConjureCollectionType() { + ConjureCollectionType getConjureCollectionType() { return conjureCollectionType; } - public boolean useNonNullFactory() { + boolean useNonNullFactory() { return nullHandlingMode.shouldUseNonNullFactory(); } } @@ -1097,15 +1117,21 @@ private enum ConjureCollectionType { this.primitiveCollection = primitiveCollection; } - public String getCollectionName() { + String getCollectionName() { return collectionName; } - public Boolean isPrimitiveCollection() { + Boolean isPrimitiveCollection() { return primitiveCollection; } } + private boolean isPrimitiveOptimized(Type type) { + return type.accept(TypeVisitor.IS_LIST) + // Check above guards the getCollectionType call below + && getCollectionType(type).getConjureCollectionType().isPrimitiveCollection(); + } + private enum ConjureCollectionNullHandlingMode { NON_NULL_COLLECTION_FACTORY(true), NULLABLE_COLLECTION_FACTORY(false); @@ -1116,7 +1142,7 @@ private enum ConjureCollectionNullHandlingMode { this.useNonNullFactory = useNonNullFactory; } - public boolean shouldUseNonNullFactory() { + boolean shouldUseNonNullFactory() { return useNonNullFactory; } }