-
Notifications
You must be signed in to change notification settings - Fork 59
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
Always return PrimitiveFloatList, even when cold #53
Merged
FelixGV
merged 1 commit into
linkedin:master
from
FelixGV:always_return_primitive_float_list_even_when_cold
May 29, 2020
Merged
Always return PrimitiveFloatList, even when cold #53
FelixGV
merged 1 commit into
linkedin:master
from
FelixGV:always_return_primitive_float_list_even_when_cold
May 29, 2020
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
avro-fastserde/src/main/java/com/linkedin/avro/api/PrimitiveFloatList.java
Outdated
Show resolved
Hide resolved
...-fastserde/src/main/java/com/linkedin/avro/fastserde/ByteBufferBackedPrimitiveFloatList.java
Show resolved
Hide resolved
avro-fastserde/src/main/java/com/linkedin/avro/fastserde/coldstart/ColdPrimitiveFloatList.java
Show resolved
Hide resolved
avro-fastserde/src/main/java/org/apache/avro/generic/ColdDatumReaderMixIn.java
Outdated
Show resolved
Hide resolved
radai-rosenblatt
approved these changes
May 29, 2020
LGTM. Curious to know what's the exact performance improvement to cold-mode. Worth a JMH benchmark :-) |
Actually, there is no performance improvement whatsoever (but there could be some improvements in the future with this method...). The goal of this PR is purely to ensure that the extended API is always provided, even in cold mode. |
FelixGV
force-pushed
the
always_return_primitive_float_list_even_when_cold
branch
from
May 29, 2020 06:00
70f7bca
to
ef90823
Compare
The PrimitiveFloatList is an API which users should expect to rely on, so it is wrong to degrade to the more constrained List<Float> API while Fast-Avro is still cold. This commit introduces several changes to make the extended API reliably present whenever using Fast-Avro, regardless of being cold or warm. - Changed PrimitiveFloatList to an interface, in a new package called: com.linkedin.avro.api; since the package name migration makes this an incompatible change, it would be desirable for the next release to not increment only the patch version. Having a proper package name for API extension should make things cleaner in the future as we add other optimized APIs (e.g. PR linkedin#45). - Renamed the old class to ByteBufferBackedPrimitiveFloatList, and made it implement the new interface. - Added new several new classes to ensure that the PrimitiveFloatList is always returned even when Fast-Avro falls back to vanilla Avro: - ColdPrimitiveFloatList which is a naive implementation that simply implements the new API by delegating to the regular Avro functions. This does not provide any GC benefits, but at least maintains the API. - ColdGenericDatumReader and ColdSpecificDatumReader which extend the GenericDatumReader and SpecificDatumReader classes, respectively, from vanilla Avro. - ColdDatumReaderMixIn which provides a utility function to minimize repeated code between the two DatumReader functions. - Significantly refactored the FastGenericDeserializerGeneratorTest so that it tests three permutations: vanilla, cold fast and warm fast. As part of doing this, several test short-comings were discovered and fixed. In particular, the decodeRecordSlow function had some flipped parameters which led to test failures on vanilla Avro, and those test failures were hidden by the fact that some tests ignored the provided permutation param and systematically tested only Fast-Avro.
FelixGV
force-pushed
the
always_return_primitive_float_list_even_when_cold
branch
from
May 29, 2020 06:13
ef90823
to
9871d08
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.
The PrimitiveFloatList is an API which users should expect to rely on,
so it is wrong to degrade to the more constrained List API while
Fast-Avro is still cold. This commit introduces several changes to make
the extended API reliably present whenever using Fast-Avro, regardless
of being cold or warm.
com.linkedin.avro.api; since the package name migration makes this an
incompatible change, it would be desirable for the next release to
not increment only the patch version. Having a proper package name
for API extension should make things cleaner in the future as we add
other optimized APIs (e.g. PR PrimitiveGenericRecord interface #45).
it implement the new interface.
is always returned even when Fast-Avro falls back to vanilla Avro:
implements the new API by delegating to the regular Avro functions.
This does not provide any GC benefits, but at least maintains the
API.
GenericDatumReader and SpecificDatumReader classes, respectively,
from vanilla Avro.
repeated code between the two DatumReader functions.
that it tests three permutations: vanilla, cold fast and warm fast.
As part of doing this, several test short-comings were discovered and
fixed. In particular, the decodeRecordSlow function had some flipped
parameters which led to test failures on vanilla Avro, and those test
failures were hidden by the fact that some tests ignored the provided
permutation param and systematically tested only Fast-Avro.