Improve online deserialization latency

[judahrand]

Signed-off-by: Judah Rand 17158624+judahrand@users.noreply.github.com

What this PR does / why we need it:
For _LIST type features this implementation is up to 7x faster in my testing.

For serializing and deserializing 10k rows of 3 columns DOUBLE, 3 columns FLOAT_LIST.

Old implementation:

New implementation:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

None

[judahrand]

@tsotnet Relevant for your benchmarking perhaps?

[achals]

This is great and much cleaner to read @judahrand - I'm curious where the speed up is coming from, is it just because we're avoiding the excess branching and if statements?

[judahrand]

This is great and much cleaner to read @judahrand - I'm curious where the speed up is coming from, is it just because we're avoiding the excess branching and if statements?

I think it is the branching and ifs but also for list types the old implementation was looping over each element of the list and type casting it. This implementation avoids the need for that by not using MessageToDict.

From what I can tell MessageToDict does some very odd things to types. Really it should probably be called MessageToJSONDict! Accessing the elements directly avoids the weirdness with types.

[judahrand]

I'd also add that this is relying much more on letting Protobuf sort out the conversions which might mean more of this is being done in C++ extensions?

Edit: actually I'm not sure this is true but I'll look into it.
Edit edit: I think it is probably true but only if the C++ implementation of Protobuf is also installed on the machine.

[judahrand]

This is an interesting comparison between master and these changes for both with and without the C++ extensions.

The long and short is... omg install the C++ extensions! I'm unclear whether this is done by default or not through pip 🤔

[judahrand]

Looks like most platform's wheels from pip do use the C++ extensions by default which is a relief!

[achals]

/lgtm

[feast-ci-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, judahrand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [achals]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[judahrand]

@achals Interestingly, there is the possibility of getting another small performance benefit by doing what is documented here: https://m.atthew.uk/posts/post.html

This is probably not worth the deploy nightmare, however.

[achals]

@achals Interestingly, there is the possibility of getting another small performance benefit by doing what is documented here: https://m.atthew.uk/posts/post.html

This is probably not worth the deploy nightmare, however.

Thanks for the detailed info! Yeah I'm inclined to opt for simplicity over speed in this particular case. I'm sure there's other avenues to speed things up that won't complicate our packaging and deploy story.