From a1f407a6c6d78ed331fbe1cc3ac840ae77296d38 Mon Sep 17 00:00:00 2001 From: Brandon Marc-Aurele Date: Tue, 22 Oct 2024 13:23:22 -0400 Subject: [PATCH] efficient primitive item addition --- changelog/@unreleased/pr-2389.v2.yml | 5 ++ .../com/palantir/product/ListExample.java | 14 +++++ .../BeanBuilderAuxiliarySettersUtils.java | 50 ++++++++++++--- .../java/types/BeanBuilderGenerator.java | 62 ++++++++++++++++--- .../palantir/conjure/java/lib/SafeLong.java | 3 - .../java/lib/internal/ConjureCollections.java | 16 +++-- .../java/lib/internal/ConjureDoubleList.java | 4 ++ .../java/lib/internal/ConjureIntegerList.java | 4 ++ 8 files changed, 134 insertions(+), 24 deletions(-) create mode 100644 changelog/@unreleased/pr-2389.v2.yml diff --git a/changelog/@unreleased/pr-2389.v2.yml b/changelog/@unreleased/pr-2389.v2.yml new file mode 100644 index 000000000..36e09bbaa --- /dev/null +++ b/changelog/@unreleased/pr-2389.v2.yml @@ -0,0 +1,5 @@ +type: feature +feature: + description: '[Feature] Non-boxing ''addAll'' Methods for Primitive Types' + links: + - https://github.com/palantir/conjure-java/pull/2389 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 8e739e799..08358fe06 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 @@ -234,6 +234,13 @@ public Builder primitiveItems(@Nonnull Iterable primitiveItems) { return this; } + public Builder addAllPrimitiveItems(@Nonnull int... primitiveItems) { + checkNotBuilt(); + ConjureCollections.addAllToIntegerList( + this.primitiveItems, Preconditions.checkNotNull(primitiveItems, "primitiveItems cannot be null")); + return this; + } + public Builder addAllPrimitiveItems(@Nonnull Iterable primitiveItems) { checkNotBuilt(); ConjureCollections.addAllAndCheckNonNull( @@ -256,6 +263,13 @@ public Builder doubleItems(@Nonnull Iterable doubleItems) { return this; } + public Builder addAllDoubleItems(@Nonnull double... doubleItems) { + checkNotBuilt(); + ConjureCollections.addAllToDoubleList( + this.doubleItems, Preconditions.checkNotNull(doubleItems, "doubleItems cannot be null")); + return this; + } + public Builder addAllDoubleItems(@Nonnull Iterable doubleItems) { checkNotBuilt(); ConjureCollections.addAllAndCheckNonNull( diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java index 3adef817e..fd9e29408 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/BeanBuilderAuxiliarySettersUtils.java @@ -17,6 +17,7 @@ package com.palantir.conjure.java.types; import com.palantir.conjure.java.ConjureAnnotations; +import com.palantir.conjure.java.lib.SafeLong; import com.palantir.conjure.java.types.BeanGenerator.EnrichedField; import com.palantir.conjure.java.util.Javadoc; import com.palantir.conjure.java.util.Primitives; @@ -28,6 +29,7 @@ import com.palantir.conjure.spec.PrimitiveType; import com.palantir.conjure.spec.Type; import com.palantir.conjure.visitor.TypeVisitor; +import com.palantir.javapoet.ArrayTypeName; import com.palantir.javapoet.ClassName; import com.palantir.javapoet.CodeBlock; import com.palantir.javapoet.FieldSpec; @@ -39,11 +41,43 @@ import javax.lang.model.element.Modifier; import org.apache.commons.lang3.StringUtils; -public final class BeanBuilderAuxiliarySettersUtils { +final class BeanBuilderAuxiliarySettersUtils { private BeanBuilderAuxiliarySettersUtils() {} - public static MethodSpec.Builder createCollectionSetterBuilder( + static MethodSpec.Builder createPrimitiveCollectionSetterBuilder( + EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass, SafetyEvaluator safetyEvaluator) { + FieldSpec field = enriched.poetSpec(); + FieldDefinition definition = enriched.conjureDef(); + TypeName innerTypeName = extractInnerTypeFromList(definition, typeMapper, safetyEvaluator); + + return MethodSpec.methodBuilder("addAll" + StringUtils.capitalize(field.name())) + .addJavadoc(Javadoc.render(definition.getDocs(), definition.getDeprecated()) + .map(rendered -> CodeBlock.of("$L", rendered)) + .orElseGet(() -> CodeBlock.builder().build())) + .addAnnotations(ConjureAnnotations.deprecation(definition.getDeprecated())) + .addModifiers(Modifier.PUBLIC) + // Forces the array argument to instead be variadic + .varargs() + .addParameter(Parameters.nonnullParameter(ArrayTypeName.of(innerTypeName), field.name())) + .returns(returnClass); + } + + private static TypeName extractInnerTypeFromList( + FieldDefinition conjureDef, TypeMapper typeMapper, SafetyEvaluator safetyEvaluator) { + Type innerType = conjureDef.getType().accept(TypeVisitor.LIST).getItemType(); + TypeName boxedTypeName = typeMapper.getClassName(innerType); + + // SafeLong is just a special case of long + if (boxedTypeName.equals(ClassName.get(SafeLong.class))) { + return ConjureAnnotations.withSafety(TypeName.LONG, safetyEvaluator.getUsageTimeSafety(conjureDef)); + } else { + return ConjureAnnotations.withSafety( + Primitives.unbox(boxedTypeName), safetyEvaluator.getUsageTimeSafety(conjureDef)); + } + } + + static MethodSpec.Builder createCollectionSetterBuilder( String prefix, EnrichedField enriched, TypeMapper typeMapper, @@ -66,7 +100,7 @@ public static MethodSpec.Builder createCollectionSetterBuilder( field.name())); } - public static MethodSpec.Builder createOptionalSetterBuilder( + static MethodSpec.Builder createOptionalSetterBuilder( EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass, SafetyEvaluator safetyEvaluator) { FieldSpec field = enriched.poetSpec(); OptionalType type = enriched.conjureDef().getType().accept(TypeVisitor.OPTIONAL); @@ -78,7 +112,7 @@ public static MethodSpec.Builder createOptionalSetterBuilder( field.name())); } - public static MethodSpec.Builder createItemSetterBuilder( + static MethodSpec.Builder createItemSetterBuilder( EnrichedField enriched, Type itemType, TypeMapper typeMapper, @@ -89,7 +123,7 @@ public static MethodSpec.Builder createItemSetterBuilder( .addParameter(ConjureAnnotations.withSafety(typeMapper.getClassName(itemType), safety), field.name()); } - public static MethodSpec.Builder createMapSetterBuilder( + static MethodSpec.Builder createMapSetterBuilder( EnrichedField enriched, TypeMapper typeMapper, ClassName returnClass) { MapType type = enriched.conjureDef().getType().accept(TypeVisitor.MAP); return publicSetter(enriched, returnClass) @@ -97,7 +131,7 @@ public static MethodSpec.Builder createMapSetterBuilder( .addParameter(typeMapper.getClassName(type.getValueType()), "value"); } - public static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName returnClass) { + static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName returnClass) { FieldDefinition definition = enriched.conjureDef(); return MethodSpec.methodBuilder(enriched.poetSpec().name()) .addJavadoc(Javadoc.render(definition.getDocs(), definition.getDeprecated()) @@ -108,7 +142,7 @@ public static MethodSpec.Builder publicSetter(EnrichedField enriched, ClassName .returns(returnClass); } - public static TypeName widenParameterIfPossible( + static TypeName widenParameterIfPossible( TypeName current, Type type, TypeMapper typeMapper, Optional safety) { if (type.accept(TypeVisitor.IS_LIST)) { Type innerType = type.accept(TypeVisitor.LIST).getItemType(); @@ -147,7 +181,7 @@ public static TypeName widenParameterIfPossible( // we want to widen containers of anything that's not a primitive, a conjure reference or an optional // since we know all of those are final. - public static boolean isWidenableContainedType(Type containedType) { + static boolean isWidenableContainedType(Type containedType) { return containedType.accept(new DefaultTypeVisitor() { @Override public Boolean visitPrimitive(PrimitiveType value) { 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 e791a2298..9a535ec52 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 @@ -671,6 +671,26 @@ private MethodSpec createCollectionSetter(String prefix, EnrichedField enriched, .build(); } + private MethodSpec createPrimitiveCollectionSetter(EnrichedField enriched, boolean override) { + FieldSpec field = enriched.poetSpec(); + Type type = enriched.conjureDef().getType(); + + CollectionType collectionType = getCollectionType(type); + return BeanBuilderAuxiliarySettersUtils.createPrimitiveCollectionSetterBuilder( + enriched, typeMapper, builderClass, safetyEvaluator) + .addAnnotations(ConjureAnnotations.override(override)) + .addCode(verifyNotBuilt()) + .addCode(CodeBlocks.statement( + "$1T.addAllTo$2L(this.$3N, $4L)", + ConjureCollections.class, + collectionType.getConjureCollectionType().getCollectionName(), + field.name(), + Expressions.requireNonNull( + field.name(), enriched.fieldName().get() + " cannot be null"))) + .addStatement("return this") + .build(); + } + @SuppressWarnings("checkstyle:CyclomaticComplexity") private CodeBlock typeAwareAssignment(EnrichedField enriched, Type type, boolean shouldClearFirst) { FieldSpec spec = enriched.poetSpec(); @@ -767,10 +787,19 @@ private List createAuxiliarySetters(EnrichedField enriched, boolean Type type = enriched.conjureDef().getType(); Optional safety = safetyEvaluator.getUsageTimeSafety(enriched.conjureDef()); + ImmutableList.Builder builder = ImmutableList.builder(); + if (type.accept(TypeVisitor.IS_LIST)) { - return ImmutableList.of( + CollectionType collectionType = getCollectionType(type); + if (collectionType.getConjureCollectionType().isPrimitiveCollection() + && collectionType.useNonNullFactory()) { + builder.add(createPrimitiveCollectionSetter(enriched, override)); + } + + builder.add( createCollectionSetter("addAll", enriched, override), createItemSetter(enriched, type.accept(TypeVisitor.LIST).getItemType(), override, safety)); + return builder.build(); } if (type.accept(TypeVisitor.IS_SET)) { @@ -822,7 +851,9 @@ private CodeBlock optionalAssignmentStatement(EnrichedField enriched, OptionalTy private static final EnumSet OPTIONAL_PRIMITIVES = EnumSet.of(PrimitiveType.Value.INTEGER, PrimitiveType.Value.DOUBLE, PrimitiveType.Value.BOOLEAN); - /** Check if the optionalType contains a primitive boolean, double or integer. */ + /** + * Check if the optionalType contains a primitive boolean, double or integer. + */ private boolean isPrimitiveOptional(OptionalType optionalType) { return optionalType.getItemType().accept(TypeVisitor.IS_PRIMITIVE) && OPTIONAL_PRIMITIVES.contains( @@ -1041,25 +1072,38 @@ public boolean useNonNullFactory() { } private enum ConjureCollectionType { - LIST("List"), - DOUBLE_LIST("DoubleList"), - INTEGER_LIST("IntegerList"), + LIST("List", false), + DOUBLE_LIST("DoubleList", true), + INTEGER_LIST("IntegerList", true), // Eclipse has a BooleanList type, but this use case implies // bit mask and it doesn't serialize efficiently as a collection // so let's just use the "naive" boxed collection - BOOLEAN_LIST("List"), - SAFE_LONG_LIST("SafeLongList"), - SET("Set"); + BOOLEAN_LIST("List", false), + // SafeLong is unique in this list. While it is technically backed with a long + // its logical limitations are captured in the boxed type SafeLong. Meaning, + // you must either expose this "implementation detail" on the public API or + // accept that you cannot optimize away the boxing. For now, given the focus + // on doubles, let's delay this optimization and have it as a separate discussion. + // Technically this type could be optimized at rest, but that would require a more + // complex enum to represent this trinary. So for now this is disabled. + SAFE_LONG_LIST("SafeLongList", false), + SET("Set", false); private final String collectionName; + private final Boolean primitiveCollection; - ConjureCollectionType(String collectionName) { + ConjureCollectionType(String collectionName, boolean primitiveCollection) { this.collectionName = collectionName; + this.primitiveCollection = primitiveCollection; } public String getCollectionName() { return collectionName; } + + public Boolean isPrimitiveCollection() { + return primitiveCollection; + } } private enum ConjureCollectionNullHandlingMode { diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/SafeLong.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/SafeLong.java index 4ff79b24e..65503bd82 100644 --- a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/SafeLong.java +++ b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/SafeLong.java @@ -26,9 +26,6 @@ public final class SafeLong implements Comparable { private static final long MIN_SAFE_VALUE = -(1L << 53) + 1; private static final long MAX_SAFE_VALUE = (1L << 53) - 1; - public static final SafeLong MAX_VALUE = SafeLong.of(MAX_SAFE_VALUE); - public static final SafeLong MIN_VALUE = SafeLong.of(MIN_SAFE_VALUE); - private final long longValue; private SafeLong(long longValue) { diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java index c8a5a4df8..009e472fa 100644 --- a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java +++ b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureCollections.java @@ -170,8 +170,12 @@ public static List newNonNullDoubleList(Iterable iterable) { // This method modifies a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static void addAllToDoubleList(Collection addTo, double[] elementsToAdd) { - for (double el : elementsToAdd) { - addTo.add(el); + if (addTo instanceof ConjureDoubleList) { + ((ConjureDoubleList) addTo).addAll(elementsToAdd); + } else { + for (double el : elementsToAdd) { + addTo.add(el); + } } } @@ -195,8 +199,12 @@ public static List newNonNullIntegerList(Iterable iterable) { // This method modifies a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set public static void addAllToIntegerList(Collection addTo, int[] elementsToAdd) { - for (int el : elementsToAdd) { - addTo.add(el); + if (addTo instanceof ConjureIntegerList) { + ((ConjureIntegerList) addTo).addAll(elementsToAdd); + } else { + for (int el : elementsToAdd) { + addTo.add(el); + } } } diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java index 58d63bbaa..6044da36b 100644 --- a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java +++ b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java @@ -55,6 +55,10 @@ public boolean addAll(int index, Collection collection) { return delegate.addAllAtIndex(index, target); } + void addAll(double[] source) { + this.delegate.addAll(source); + } + @Override public Double remove(int index) { return delegate.removeAtIndex(index); diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureIntegerList.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureIntegerList.java index 79ecd32b6..0a188d85e 100644 --- a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureIntegerList.java +++ b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureIntegerList.java @@ -55,6 +55,10 @@ public boolean addAll(int index, Collection collection) { return delegate.addAllAtIndex(index, target); } + void addAll(int[] source) { + this.delegate.addAll(source); + } + @Override public Integer remove(int index) { return delegate.removeAtIndex(index);