[Feature] Add Serialization Optimization for Primitive Collection Types #2386
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bmarcaur
changed the title
carterkozak
reviewed
Oct 23, 2024
conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java
Outdated
Show resolved
Hide resolved
raiju
reviewed
Oct 29, 2024
conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureSafeLongList.java
Outdated
Show resolved
Hide resolved
bmarcaur
force-pushed
the
bmarcaurele/conjure-double-list-serialization
branch
from
1fec024 to
b6196ea
Compare
carterkozak
reviewed
Nov 10, 2024
conjure-lib/src/test/java/com/palantir/conjure/java/lib/internal/ConjureCollectionsTest.java
Outdated
Show resolved
Hide resolved
carterkozak
reviewed
Nov 10, 2024
conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureDoubleList.java
Outdated
Show resolved
Hide resolved
bmarcaur
force-pushed
the
bmarcaurele/conjure-double-list-serialization
branch
4 times, most recently
from
a76bb0d to
505cc4c
Compare
bmarcaur
commented
Nov 14, 2024
| import org.eclipse.collections.impl.utility.Iterate; | ||
|
|
||
| /** | ||
| * ConjureDoubleList is a boxed list wrapper for the eclipse-collections DoubleArrayList. In eclipse-collections 12, | ||
| * a BoxedMutableDoubleList will be released. Once available, ConjureDoubleList should be replaced with that. | ||
| */ | ||
| final class ConjureDoubleList extends AbstractList<Double> implements RandomAccess { | ||
| private final DoubleArrayList delegate; | ||
| private final MutableDoubleList delegate; |
There was a problem hiding this comment.
Changing this type so that I can wrap both DoubleArrayList and UnmodifiableDoubleList which share this interface as the lowest common denominator.
bmarcaur
commented
Nov 14, 2024
| import org.eclipse.collections.impl.utility.Iterate; | ||
|
|
||
| /** | ||
| * ConjureIntegerList is a boxed list wrapper for the eclipse-collections IntArrayList. In eclipse-collections 12, | ||
| * a BoxedMutableIntList will be released. Once available, ConjureIntegerList should be replaced with that. | ||
| */ | ||
| final class ConjureIntegerList extends AbstractList<Integer> implements RandomAccess { | ||
| private final IntArrayList delegate; | ||
| private final MutableIntList delegate; |
There was a problem hiding this comment.
bmarcaur
commented
Nov 14, 2024
| import org.eclipse.collections.impl.utility.Iterate; | ||
|
|
||
| /** | ||
| * ConjureSafeLongList is a boxed list wrapper for the eclipse-collections LongArrayList. This handles boxing/unboxing | ||
| * with SafeLongs. | ||
| */ | ||
| final class ConjureSafeLongList extends AbstractList<SafeLong> implements RandomAccess { | ||
| private final LongArrayList delegate; | ||
| private final MutableLongList delegate; |
There was a problem hiding this comment.
bmarcaur
commented
Nov 14, 2024
| // This is a serialization optimization that avoids boxing, but does copy | ||
| @JsonValue | ||
| double[] jacksonSerialize() { | ||
| return delegate.toArray(); |
There was a problem hiding this comment.
This is how we avoid boxing out the door (I checked the Jackson code paths manually, it does infact operate on the array), while also respecting the various Jackson flags we use like excludeEmptyCollections. It does make a copy, but implementing a version that avoided this copy was very complex as it required leaking details of various Jackson settings.
bmarcaur
commented
Nov 14, 2024
| // This is a serialization optimization that avoids boxing, but does copy | ||
| @JsonValue | ||
| int[] jacksonSerialize() { | ||
| return delegate.toArray(); |
There was a problem hiding this comment.
bmarcaur
commented
Nov 14, 2024
| // This is a serialization optimization that avoids boxing, but does copy | ||
| @JsonValue | ||
| long[] jacksonSerialize() { | ||
| return delegate.toArray(); |
There was a problem hiding this comment.
bmarcaur
force-pushed
the
bmarcaurele/conjure-double-list-serialization
branch
from
505cc4c to
8afd7d8
Compare
bmarcaur
commented
Nov 14, 2024
| @@ -259,7 +260,7 @@ private static MethodSpec createConstructor(Collection<EnrichedField> fields, Co | |||
| // is private and necessarily called from the builder, which does its own defensive copying. | |||
| if (field.conjureDef().getType().accept(TypeVisitor.IS_LIST)) { | |||
| // TODO(melliot): contribute a fix to JavaPoet that parses $T correctly for a JavaPoet FieldSpec | |||
| body.addStatement("this.$1N = $2T.unmodifiableList($1N)", spec, Collections.class); | |||
| body.addStatement("this.$1N = $2T.unmodifiableList($1N)", spec, ConjureCollections.class); | |||
There was a problem hiding this comment.
Pulled this out in to a separate PR so this one can continue to focus on the optimizations: #2409
bmarcaur
force-pushed
the
bmarcaurele/conjure-double-list-serialization
branch
from
8afd7d8 to
8376624
Compare
bmarcaur
changed the base branch from
develop
to
bmarcaurele/small-conjure-collections-refactor
bmarcaur
force-pushed
the
bmarcaurele/small-conjure-collections-refactor
branch
from
1eecccf to
74269fc
Compare
bmarcaur
force-pushed
the
bmarcaurele/conjure-double-list-serialization
branch
from
8376624 to
42e78f4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR
When Jackson attempted to serialize a primitive collection type, e.g.
ConjureDoubleList, it utilized Jackon's default serializers, specificallyIndexedListSerializer. This makes sense when you remember thatConjureDoubleListdoes not have an implemented serializer, so at best its going to be treated like List at time of serialization. This causes unnecessary boxing no matter how optimally you this collection stores the primitive types. You can see evidence of these points in the debugging screenshot below:After this PR
Adds serialization paths that avoid boxing and directly leverage Jackson primitive serialization routines.
I purposely didn't implement this feature forThis has been removed #2390ConjureBooleanListbecause it does not have a simple serialized format that benefits from primitive optimization. i.e. it expands a single bit out totrueorfalseover the wire. Frankly, this type feels like it was added for completeness and not for usefulness. A denseboolean[]is abstractly a bit mask and if that is the intent should have its own type. I think the existence of this class is just maintenance overhead at this point.Possible downsides?
Creates an alternative path for serialization that needs to match the default serialization paths.