-
Notifications
You must be signed in to change notification settings - Fork 231
Implement experimental marshaler for OpenAPI V3 #416
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
Conversation
d5518e9 to
5f7f131
Compare
|
/assign @apelisse |
apelisse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks,
I've compared this to the v2 versino of the same PR and looked at each individual comments I got on that other PR. I checked all the fields to make sure nothing was missing. Looks good to me, two minor questions/comments.
alexzielenski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly lgtm with few nits/questions
| var x struct { | ||
| Ref string `json:"$ref,omitempty"` | ||
| spec.Extensions | ||
| ResponseProps `json:",inline"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These extra structs caused by Refable and VendorExtensible make me sad but do not see a way around them without breaking changes to the type.
| } | ||
| x.Ref = p.Refable.Ref.String() | ||
| x.Extensions = internal.SanitizeExtensions(p.Extensions) | ||
| x.ParameterProps = parameterPropsOmitZero(p.ParameterProps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could changing these parameterPropsOmitZero, operationsPropsOmitZero, etc. types to a pointer avoid a copy and boost performance at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried something like
var x struct {
Ref string `json:"$ref,omitempty"`
+ ParameterProps *parameterPropsOmitZero `json:",inline"`
spec.Extensions
}
...
+ tmp := parameterPropsOmitZero(p.ParameterProps)
+ x.ParameterProps = &tmp
...
and performance numbers look identical. Is this what you mean by changing them to pointers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Jordan called out the same thing in my PR for V2, and I ran the same experiment :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant using like this:
x.ParameterProps = (*parameterPropsOmitZero)(&p.ParameterProps)
In my other comment I had a toy example that seemed to compile at least. Doing the cast with the pointer this way may avoid a copy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Name | Before | After |
| (appsv1)OpenAPIV3Deserialize/jsonv2-24 | 13.68m ± 1% | 13.65m ± 1% |
| (authorizationv1)OpenAPIV3Deserialize/jsonv2#01-24 | 1.199m ± 1% | 1.192m ± 2% |
| (appsv1)OpenAPIV3Deserialize/jsonv2-24 | 5.260Mi ± 0% | 5.260Mi ± 0% |
| (authorizationv1)OpenAPIV3Deserialize/jsonv2#01-24 | 525.4Ki ± 0% | 525.4Ki ± 0% |
| (appsv1)OpenAPIV3Deserialize/jsonv2-24 | 26.89k ± 0% | 26.89k ± 0% |
| (authorizationv1)OpenAPIV3Deserialize/jsonv2#01-24 | 2.517k ± 0% | 2.517k ± 0% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah doesn’t seem to do anything
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, Jefftree 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:
Approvers can indicate their approval by writing |
Slightly less improvement than OpenAPI V2 which is somewhat expected because of the smaller size of OpenAPI V3.