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

Always return PrimitiveFloatList, even when cold #53

Conversation

FelixGV
Copy link
Collaborator

@FelixGV FelixGV commented May 28, 2020

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.

  • 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 PrimitiveGenericRecord interface #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.

@volauvent
Copy link
Collaborator

LGTM. Curious to know what's the exact performance improvement to cold-mode. Worth a JMH benchmark :-)

@FelixGV
Copy link
Collaborator Author

FelixGV commented May 29, 2020

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 FelixGV force-pushed the always_return_primitive_float_list_even_when_cold branch from 70f7bca to ef90823 Compare May 29, 2020 06:00
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 FelixGV force-pushed the always_return_primitive_float_list_even_when_cold branch from ef90823 to 9871d08 Compare May 29, 2020 06:13
@FelixGV FelixGV merged commit 6c3fa71 into linkedin:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants