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

Add primitive array setter optimization [Primitive Arrays Pt.4] #2312

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.errorprone.annotations.CheckReturnValue;
import com.palantir.conjure.java.ConjureAnnotations;
import com.palantir.conjure.java.Options;
import com.palantir.conjure.java.lib.SafeLong;
import com.palantir.conjure.java.lib.internal.ConjureCollections;
import com.palantir.conjure.java.types.BeanGenerator.EnrichedField;
import com.palantir.conjure.java.util.JavaNameSanitizer;
Expand Down Expand Up @@ -57,6 +58,7 @@
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import com.palantir.logsafe.exceptions.SafeIllegalStateException;
import com.squareup.javapoet.AnnotationSpec;
import com.squareup.javapoet.ArrayTypeName;
import com.squareup.javapoet.ClassName;
import com.squareup.javapoet.CodeBlock;
import com.squareup.javapoet.FieldSpec;
Expand Down Expand Up @@ -671,6 +673,36 @@ private MethodSpec createCollectionSetter(String prefix, EnrichedField enriched,
.build();
}

private MethodSpec createPrimitiveCollectionSetter(EnrichedField enriched) {
FieldSpec field = enriched.poetSpec();
Type type = enriched.conjureDef().getType();

Type innerType = type.accept(TypeVisitor.LIST).getItemType();
TypeName boxedTypeName = typeMapper.getClassName(innerType);
TypeName innerTypeName;
// SafeLong is just a special case of long
if (boxedTypeName.equals(ClassName.get(SafeLong.class))) {
innerTypeName = ConjureAnnotations.withSafety(
TypeName.LONG, safetyEvaluator.getUsageTimeSafety(enriched.conjureDef()));
} else {
innerTypeName = ConjureAnnotations.withSafety(
Primitives.unbox(boxedTypeName), safetyEvaluator.getUsageTimeSafety(enriched.conjureDef()));
}
CollectionType collectionType = getCollectionType(type);
MethodSpec.Builder setterBuilder = BeanBuilderAuxiliarySettersUtils.publicSetter(enriched, builderClass)
.addParameter(Parameters.nonnullParameter(ArrayTypeName.of(innerTypeName), field.name))
.addCode(verifyNotBuilt())
.addCode(CodeBlocks.statement(
"this.$1N = $2T.newNonNull$3L($4L)",
enriched.poetSpec().name,
ConjureCollections.class,
collectionType.getConjureCollectionType().getCollectionName(),
Expressions.requireNonNull(
enriched.poetSpec().name, enriched.fieldName().get() + " cannot be null")));

return setterBuilder.addStatement("return this").build();
}

@SuppressWarnings("checkstyle:CyclomaticComplexity")
private CodeBlock typeAwareAssignment(EnrichedField enriched, Type type, boolean shouldClearFirst) {
FieldSpec spec = enriched.poetSpec();
Expand Down Expand Up @@ -766,9 +798,18 @@ private List<MethodSpec> createAuxiliarySetters(EnrichedField enriched, boolean
Optional<LogSafety> safety = safetyEvaluator.getUsageTimeSafety(enriched.conjureDef());

if (type.accept(TypeVisitor.IS_LIST)) {
return ImmutableList.of(
createCollectionSetter("addAll", enriched, override),
createItemSetter(enriched, type.accept(TypeVisitor.LIST).getItemType(), override, safety));
CollectionType collectionType = getCollectionType(type);
if (collectionType.getConjureCollectionType().isPrimitiveCollection()
&& collectionType.useNonNullFactory()) {
return ImmutableList.of(
createPrimitiveCollectionSetter(enriched),
createCollectionSetter("addAll", enriched, override),
createItemSetter(enriched, type.accept(TypeVisitor.LIST).getItemType(), override, safety));
} else {
return ImmutableList.of(
createCollectionSetter("addAll", enriched, override),
createItemSetter(enriched, type.accept(TypeVisitor.LIST).getItemType(), override, safety));
}
}

if (type.accept(TypeVisitor.IS_SET)) {
Expand Down Expand Up @@ -820,7 +861,9 @@ private CodeBlock optionalAssignmentStatement(EnrichedField enriched, OptionalTy
private static final EnumSet<PrimitiveType.Value> 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(
Expand Down Expand Up @@ -1044,22 +1087,28 @@ public boolean useNonNullFactory() {
}

private enum ConjureCollectionType {
LIST("List"),
DOUBLE_LIST("DoubleList"),
INTEGER_LIST("IntegerList"),
BOOLEAN_LIST("BooleanList"),
SAFE_LONG_LIST("SafeLongList"),
SET("Set");
LIST("List", false),
DOUBLE_LIST("DoubleList", true),
INTEGER_LIST("IntegerList", true),
BOOLEAN_LIST("BooleanList", true),
SAFE_LONG_LIST("SafeLongList", true),
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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.palantir.conjure.java.lib.SafeLong;
import com.palantir.logsafe.Preconditions;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedHashSet;
Expand Down Expand Up @@ -140,11 +141,22 @@ public static <T> Set<T> newNonNullSet(Iterable<? extends T> iterable) {
return set;
}

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops don't know how I accidentally left the comment block down there :(

* The following Conjure boxed list wrappers for the eclipse-collections [type]ArrayList are temporary (except
* ConjureSafeLongList). In eclipse-collections 12, a BoxedMutable[type]List will be released. Once available,
* Conjure[type]List should be replaced with that.
*/

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<Double> newNonNullDoubleList() {
return new ConjureDoubleList(new DoubleArrayList());
}

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<Double> newNonNullDoubleList(double[] doubles) {
return new ConjureDoubleList(DoubleArrayList.newListWith(doubles));
}

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<Double> newNonNullDoubleList(Iterable<Double> iterable) {
List<Double> doubleList;
Expand All @@ -158,17 +170,16 @@ public static List<Double> newNonNullDoubleList(Iterable<Double> iterable) {
return doubleList;
}

/**
* The following Conjure boxed list wrappers for the eclipse-collections [type]ArrayList are temporary (except
* ConjureSafeLongList). In eclipse-collections 12, a BoxedMutable[type]List will be released. Once available,
* Conjure[type]List should be replaced with that.
*/

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<Integer> newNonNullIntegerList() {
return new ConjureIntegerList(new IntArrayList());
}

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<Integer> newNonNullIntegerList(int[] integers) {
return new ConjureIntegerList(IntArrayList.newListWith(integers));
}

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<Integer> newNonNullIntegerList(Iterable<Integer> iterable) {
List<Integer> integerList;
Expand All @@ -187,6 +198,11 @@ public static List<Boolean> newNonNullBooleanList() {
return new ConjureBooleanList(new BooleanArrayList());
}

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<Boolean> newNonNullBooleanList(boolean[] booleans) {
return new ConjureBooleanList(BooleanArrayList.newListWith(booleans));
}

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<Boolean> newNonNullBooleanList(Iterable<Boolean> iterable) {
List<Boolean> booleanList;
Expand All @@ -205,6 +221,18 @@ public static List<SafeLong> newNonNullSafeLongList() {
return new ConjureSafeLongList(new LongArrayList());
}

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<SafeLong> newNonNullSafeLongList(long[] longs) {
for (long value : longs) {
if (!safeLongCheck(value)) {
throw new SafeIllegalArgumentException(
"number must be safely representable in javascript i.e. lie between -9007199254740991 and "
+ "9007199254740991");
}
}
return new ConjureSafeLongList(LongArrayList.newListWith(longs));
}

// This method returns a list that can't handle nulls. Do not use this unless the nonNullCollections flag is set
public static List<SafeLong> newNonNullSafeLongList(Iterable<SafeLong> iterable) {
List<SafeLong> safeLongList;
Expand All @@ -217,4 +245,8 @@ public static List<SafeLong> newNonNullSafeLongList(Iterable<SafeLong> iterable)

return safeLongList;
}

private static boolean safeLongCheck(long value) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicated check from SafeLong. Not sure if its worth moving this to be public

return SafeLong.MIN_VALUE.longValue() <= value && value <= SafeLong.MAX_VALUE.longValue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@
package com.palantir.conjure.java.lib.internal;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

import com.palantir.conjure.java.lib.SafeLong;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import java.util.List;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -51,6 +53,12 @@ public void newNonNullDoubleList() {

doubleList.clear();
assertThat(doubleList).hasSize(0);

doubleList = ConjureCollections.newNonNullDoubleList(new double[] {0.1, 0.2, 0.3});
assertThat(doubleList).hasSize(3);
assertThat(doubleList.get(0)).isEqualTo(0.1);
assertThat(doubleList.get(1)).isEqualTo(0.2);
assertThat(doubleList.get(2)).isEqualTo(0.3);
}

@Test
Expand Down Expand Up @@ -81,5 +89,15 @@ public void newNonNullSafeLongList() {

safeLongList.clear();
assertThat(safeLongList).hasSize(0);

safeLongList = ConjureCollections.newNonNullSafeLongList(new long[] {1L, 2L, 3L});
assertThat(safeLongList).hasSize(3);
assertThat(safeLongList.get(0)).isEqualTo(SafeLong.of(1L));
assertThat(safeLongList.get(1)).isEqualTo(SafeLong.of(2L));
assertThat(safeLongList.get(2)).isEqualTo(SafeLong.of(3L));

assertThatExceptionOfType(SafeIllegalArgumentException.class)
.isThrownBy(() -> ConjureCollections.newNonNullSafeLongList(
new long[] {1L, 2L, SafeLong.MAX_VALUE.longValue() + 1}));
}
}