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

[fast-avro] Added a few serde optimization #517

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

gaojieliu
Copy link
Collaborator

Serializer:
This code change will try to reuse the backed bytes if the float list is 'BufferBackedPrimitiveFloatList' when writing float list. If an instance of BufferBackedPrimitiveFloatList is changed after deserialization: readPrimitiveFloatArray, Fast Serializer won't use the backed bytes because of the divergence.

Deserializer:
Use reset instead of clear when reusing GenericArray since reset is cheaper than clear and the behavior difference is that reset won't nullify the elements in current array, but just resize the array length to be 0.

Copy link
Collaborator

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks Gaojie! Left just a few minor comments...

@alex-dubrouski
Copy link
Collaborator

Should we wait until we finish benchmarking deserialization path?

@FelixGV
Copy link
Collaborator

FelixGV commented Sep 20, 2023

Should we wait until we finish benchmarking deserialization path?

Why wait? We can always submit another PR if we discover other opportunities, right?

Serializer:
This code change will try to reuse the backed bytes if the float list
is 'BufferBackedPrimitiveFloatList' when writing float list.
If an instance of `BufferBackedPrimitiveFloatList` is changed after
deserialization: `readPrimitiveFloatArray`, Fast Serializer won't
use the backed bytes because of the divergence.

Deserializer:
Use `reset` instead of `clear` when reusing `GenericArray` since `reset`
is cheaper than `clear` and the behavior difference is that `reset` won't
nullify the elements in current array, but just resize the array length to be 0.
Copy link
Collaborator

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (73acd3c) 45.77% compared to head (1f3c648) 45.78%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #517      +/-   ##
============================================
+ Coverage     45.77%   45.78%   +0.01%     
- Complexity     4438     4442       +4     
============================================
  Files           403      403              
  Lines         28070    28070              
  Branches       4622     4622              
============================================
+ Hits          12850    12853       +3     
  Misses        13664    13664              
+ Partials       1556     1553       -3     

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gaojieliu gaojieliu merged commit d661ed3 into linkedin:master Oct 5, 2023
2 checks passed
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.

4 participants