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

Optimize field set for oneof case #385

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yanivshaked
Copy link

The current implementation initializes an empty List with n fields for a one-of case.
This means, that while one of of the elements in the list will actually be populated, the entire list is initialized taking n times the amount of memory than required.
The suggested change uses a Map instead of a List for the one-of case, reducing amount of memory drastically if one-of has many types to choose from (which is mostly the case).

Copy link
Author

@yanivshaked yanivshaked left a comment

Choose a reason for hiding this comment

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

Updated files according to test results

@osa1 osa1 self-assigned this May 6, 2022
@osa1
Copy link
Member

osa1 commented Aug 4, 2022

The problem is we represent oneofs as message even though they have nothing in common.

We should have a separate value class for oneofs (similar to ProtobufEnum, which I wish we called PbEnum for consistency with PbList and PbMap) instead of optimizing message code (_FieldSet) to support this use case.

Message values are like product types (tuples, records, structs) and they're represented as GeneratedMessage.

Enum values are just integers with a mapping from the integers to names, and represented as ProtobufEnum.

Oneofs are sums (e.g. enhanced enums in Dart), and we don't have a way to represent sums efficiently currently. Instead we try to use GeneratedMessage which as demonstrated in this PR is not a good idea as it wastes O(n) space where n is the number of variants of the oneof.

@sigurdm
Copy link
Collaborator

sigurdm commented Aug 4, 2022

Oneofs are sums (e.g. enhanced enums in Dart), and we don't have a way to represent sums efficiently currently. Instead we try to use GeneratedMessage which as demonstrated in this PR is not a good idea as it wastes O(n) space where n is the number of variants of the oneof.

I guess generating a class with a single dynamic field would be the easiest solution here without proper language support for sum-types.

We could generate the appropriate getters that will do the cast....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants