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

[wip] add support for foreign types #94

Closed
wants to merge 2 commits into from
Closed

[wip] add support for foreign types #94

wants to merge 2 commits into from

Conversation

vmg
Copy link
Member

@vmg vmg commented Sep 12, 2023

As discussed in #93 (comment), this is the third approach to supporting fast serialization of well-known types:

The idea is adding a flag, foreign to the compiler, which forces code generation to write free-standing functions (i.e. functions where the ProtoBuf message being (de)serialized is an argument, and not a method receiver). This allows the functions to operate on ProtoBuf objects from foreign packages.

As @biosvs points out on the other issue, generating free-standing functions is going to be very hard without adding a lot of if indirections. I had some time today so I sketched out a possible solution that abstracts method definitions and function calls behind a generic API. I managed to get foreign functions generation working for marshal+size -- all the other generation kinds are not implemented.

@biosvs: I would very much like to hear your thoughts on this approach. I think it's reasonably clean given the complexity that foreign types add. I'm out of time for this week, so I'll pick this up again next week -- if you have any suggestions or ways to improve on this design please feel free to push them to your own branch and I'll merge them here. Thanks again for your help with this feature!

Signed-off-by: Vicent Marti <[email protected]>
@biosvs
Copy link
Contributor

biosvs commented Sep 14, 2023

Hi @vmg

First, let me say that in general I like the idea of having generator.Signature/generator.Call. Seems like a good approach to deal with an increased complexity.

But I'm not sure if I get the whole idea of the new feature:

  1. Is it internal feature (for wkt only) or it'll be available for any messages?
  2. How it should be used? There are generated marshal/unmarshal functions, but now they should to be called from other marshal/unmarshal functions.

My concerns are:

  1. If it's internal feature, it's not good that main code of plugin is affected. Even from this PR it's obvious that complexity increases, it's harder to read, test and maintain.
  2. If it's public feature, what to do with the chains of composition? Like m1.m2.m3.m4.field, where m3 has only "foreign" marshaler?

I'm trying to say, that such features are not only solution for particular problem, but also commitment for the future (and responsibility for those who use plugin/feature), new tech debt, complexity, potential bugs and inconsistencies between other features, etc. So, it's nice to have design and justification behind any new features.

@vmg
Copy link
Member Author

vmg commented Sep 20, 2023

Hey @biosvs! Thanks for your feedback! I agree with you that the feature is a big change and a lot to maintain down the road. My line of thought was this: since we have to change the codegen to support foreign types, we might as well expose this as a public-facing feature. By allowing vtprotobuf to generate free-standing functions, we can let people opt-in to faster ProtoBuf (de)serialization even for types they don't control because they're part of a third party dependency (e.g. for the Kubernetes API in their project).

Is this feature generally useful enough? I don't know! But it seems like we need the extra codegen to be able to support the well-known types, so we might as well make expose it to the users in case they find a use for it. I just can't think of a simpler way to generate free-standing functions for the Well Known Types without modifying all the codegen for Marshal and Unmarshal. Can you?

@biosvs
Copy link
Contributor

biosvs commented Sep 21, 2023

There was a suggestion about alternative approach: #80 (comment). As you already accepted loss of unknownFields, proposed solution with an alias may be a good approach. It'll allow to have interface implementation checks (like _, ok := m.(vtMarshaller)).

Another approach, suitable for both types and standalone functions, is to have a global registry (filled with init()). Registry may be used to find correct function/method to marshal/unmarshal/size particular type.

@vmg
Copy link
Member Author

vmg commented Oct 2, 2023

Merged an alternative implementation here: #99

Thanks for the help and the insights @biosvs!

@vmg vmg closed this Oct 2, 2023
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.

2 participants