-
Notifications
You must be signed in to change notification settings - Fork 53
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
Provide "decode" code generation for the streaming variants for all other types #496
Conversation
A couple of things to the reviewers:
|
bf5fa66
to
a7893a9
Compare
1da7f69
to
6fa1b7c
Compare
bc1e0ce
to
80aca24
Compare
6fa1b7c
to
9042ba6
Compare
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.
Forgot to return on collection type mistmatch, but LGTM besides that
Also, check out gen/quick_test at some point. If we add another test to that test suite, it'll validate all generated types in the ThriftRW codebase. |
80aca24
to
f9f98e7
Compare
gen/container_test.go
Outdated
assertRoundTrip(t, &tt.p, tt.v, tt.desc) | ||
assert.True(t, tt.p.Equals(&tt.p), tt.desc) | ||
|
||
testRoundTripCombos(t, &tt.p, tt.v, tt.desc) | ||
assert.True(t, tt.p.Equals(&tt.p), tt.desc) |
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.
Doesn't testRoundTripCombos
essentially include assertRoundTrip
? From a brief look, it doesn't look like assertRoundTrip
checks for anything more than testRoundTripCombos
with false,false
.
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, we can remove that once all of this integration is done.
f467573
to
0d5251b
Compare
0053ede
to
91f4e59
Compare
Codecov Report
@@ Coverage Diff @@
## streamdev #496 +/- ##
=============================================
- Coverage 71.23% 68.81% -2.42%
=============================================
Files 129 129
Lines 18693 22736 +4043
=============================================
+ Hits 13316 15646 +2330
- Misses 3711 4066 +355
- Partials 1666 3024 +1358
Continue to review full report at Codecov.
|
…ther types Provide the streaming read generators for all of the other types. Doing so meant providing a `StreamGenerator` that plumbs into the existing generators. This `StreamGenerator` only provides the "decoding" mechanism, leveraging #491 for reading the raw Thrift encoding off of the wire. In addition to providing a `StreamGenerator`, the list, map, set, enum, typdef, and struct generators all added a `Decoder` method that will appropriately recurse and iterate to generate the proper, mirrored readers for the raw wire representation. The `Decode` and `DecodePtr` methods for `StreamGenerator` then hook into the templated generator itself, providing the templated `decode` and `decodePtr` calls that will be used where necessary. The existing tests were leveraged to make sure that the streaming reads were compatible with the current binary writes and that no data was lost. In doing so, a 'genericized' function that will perform the cross-products of encoding/writing and decoding/reading. A benchmark was also added to evaluate the new streaming reads. ``` name \ time/op old.txt new.txt stream.txt RoundTrip/PrimitiveOptionalStruct/Encode-8 1.72µs ± 5% 1.72µs ± 0% 1.76µs ± 6% RoundTrip/PrimitiveOptionalStruct/Decode-8 2.47µs ± 1% 2.75µs ± 0% 2.68µs ± 1% RoundTrip/Graph/Encode-8 3.18µs ± 2% 3.13µs ± 1% 3.13µs ± 1% RoundTrip/Graph/Decode-8 5.02µs ± 2% 8.34µs ± 2% 8.35µs ± 2% RoundTrip/ContainersOfContainers/Encode-8 19.5µs ± 3% 19.0µs ± 2% 19.3µs ± 2% RoundTrip/ContainersOfContainers/Decode-8 46.8µs ± 5% 104.8µs ± 1% 106.7µs ± 2% RoundTrip/PrimitiveOptionalStruct/StreamingRead-8 1.09µs ± 1% RoundTrip/Graph/StreamingRead-8 1.69µs ± 4% RoundTrip/ContainersOfContainers/StreamingRead-8 25.3µs ± 2% name \ alloc/op old.txt new.txt stream.txt RoundTrip/PrimitiveOptionalStruct/Encode-8 704B ± 0% 704B ± 0% 704B ± 0% RoundTrip/PrimitiveOptionalStruct/Decode-8 1.40kB ± 0% 1.46kB ± 0% 1.46kB ± 0% RoundTrip/Graph/Encode-8 1.70kB ± 0% 1.70kB ± 0% 1.70kB ± 0% RoundTrip/Graph/Decode-8 2.78kB ± 0% 3.57kB ± 0% 3.57kB ± 0% RoundTrip/ContainersOfContainers/Encode-8 1.30kB ± 0% 1.30kB ± 0% 1.30kB ± 0% RoundTrip/ContainersOfContainers/Decode-8 12.3kB ± 0% 28.6kB ± 0% 28.6kB ± 0% RoundTrip/PrimitiveOptionalStruct/StreamingRead-8 104B ± 0% RoundTrip/Graph/StreamingRead-8 216B ± 0% RoundTrip/ContainersOfContainers/StreamingRead-8 10.2kB ± 0% name \ allocs/op old.txt new.txt stream.txt RoundTrip/PrimitiveOptionalStruct/Encode-8 1.00 ± 0% 1.00 ± 0% 1.00 ± 0% RoundTrip/PrimitiveOptionalStruct/Decode-8 14.0 ± 0% 15.0 ± 0% 15.0 ± 0% RoundTrip/Graph/Encode-8 11.0 ± 0% 11.0 ± 0% 11.0 ± 0% RoundTrip/Graph/Decode-8 32.0 ± 0% 63.0 ± 0% 63.0 ± 0% RoundTrip/ContainersOfContainers/Encode-8 18.0 ± 0% 18.0 ± 0% 18.0 ± 0% RoundTrip/ContainersOfContainers/Decode-8 164 ± 0% 837 ± 0% 837 ± 0% RoundTrip/PrimitiveOptionalStruct/StreamingRead-8 11.0 ± 0% RoundTrip/Graph/StreamingRead-8 11.0 ± 0% RoundTrip/ContainersOfContainers/StreamingRead-8 147 ± 0% ``` "old" represents the original code, "new" represents the binary decoding utilizing the streaming decoder, and "stream" represents the benchmarks as it stands at this diff.
@@ -53,6 +53,29 @@ func (e *enumGenerator) Reader(g Generator, spec *compile.EnumSpec) (string, err | |||
return name, wrapGenerateError(spec.ThriftName(), err) | |||
} | |||
|
|||
func (e *enumGenerator) Decoder(g Generator, spec *compile.EnumSpec) (string, error) { |
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.
Be consistent with calling the function "Decode" like you've done with the fieldGenerator.
LGTM! |
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.
!!
642d2be
to
9888b65
Compare
# Commits The following comments are included in this release. Some of these cherry-picked and released in v1.28.0, but they appear again in the list above. - protocol: Add streaming interfaces (#485) - Move Stream-based interfaces into their own package - Make Streaming interfaces private to allow for safe experimentation (#488) - idl: Return structured ParseError from idl.Parse() (#492) - Add CHANGELOG entry for #492 (#494) - Support "<" in the templating language (#499) - idl: add a Position struct to wrap reported lines (#497) - Add streamwriter implementation (#490) - Add a "StreamReader" which implements "stream.Reader" - Use the "stream.Reader" in the "binary.Reader" - Add code generation for all wire types for stream encoding (#500) - Generate "Decode" for "enums" that will directly decode (#495) - Provide "decode" code generation for the streaming variants for all other types (#496) - idl: record document positions on constant nodes (#503) - ast: move idl.Position to the ast package (#504) - idl: replace internal.Position with ast.Position (#505) - Expose stream protocol method to close Writer (#506) - idl: add column numbers to parse error positions (#507) - idl: record full positions for constants (#508) - Mark assertParseCases() as a test helper (#509) - protocol/stream: Define enveloping interfaces (#511) - protocol/stream: Declare interface for encoding envelopes (#513) - binary/StreamWriter: Borrow => New; unexport Return (#515) - stream: add Close method, pool binary reader (#514) - binary/reader: Return to pool after ReadValue (#517) - binary/reader: Skip fixed width collections faster (#518) - binary/stream/reader: Fast-path offsetReader skips (#519) - binary: Move Responders and Protocol into package (#516) - benchmark: Refactor into a suite (#520) - Upgrade to Ragel version 6.10 (from 6.9) (#523) - Responder: Deduplicate interface (#524) - gen/quick_test: Add missing types (#525) - enum/json: Support rejecting unknown values (#502) - Back to development - Upgrade to golang.org/x/tools version 0.1.5 (#529) - ast: add column values to the AST nodes (#522) - stream: Implement Request and Response handling with Enveloping (#526) - offsetReader: Implement io.Seeker - binary/ReadRequest: Use io.Seeker if available - StreamReader: Use Seeker instead of offsetReader - protocol/stream: Unembed stream.Protocol from stream.RequestReader (#532) - thrifttest: Add mocks for streaming interfaces (#527) - streaming: Unembed iface.Private in streaming-based interfaces (#533) - Regenerate files for tests after merging `streamdev` - ast: formally declare CppInclude as a Node (#536) - ast: add Annotations(Node) []*Annotations (#537) - Preparing release v1.29.0 # API changes I ran apidiff on all packages in v1.28.0 and compared it with this release. Removing changes to gen/internal/tests, the result is: ``` --- go.uber.org/thriftrw/ast --- Compatible changes: - Annotation.Column: added - Annotations: added - BaseType.Column: added - Constant.Column: added - ConstantList.Column: added - ConstantMap.Column: added - ConstantMapItem.Column: added - ConstantReference.Column: added - CppInclude.Column: added - DefinitionInfo.Column: added - Enum.Column: added - EnumItem.Column: added - Field.Column: added - Function.Column: added - Include.Column: added - ListType.Column: added - MapType.Column: added - Namespace.Column: added - Position.Column: added - Position.String: added - Service.Column: added - ServiceReference.Column: added - SetType.Column: added - Struct.Column: added - TypeReference.Column: added - Typedef.Column: added --- go.uber.org/thriftrw/envelope/stream --- NEW PACKAGE --- go.uber.org/thriftrw/gen --- Compatible changes: - StreamGenerator: added --- go.uber.org/thriftrw/internal/envelope/exception --- Compatible changes: - (*ExceptionType).Decode: added - (*TApplicationException).Decode: added - (*TApplicationException).Encode: added - ExceptionType.Encode: added --- go.uber.org/thriftrw/plugin/api --- Compatible changes: - (*Argument).Decode: added - (*Argument).Encode: added - (*Feature).Decode: added - (*Function).Decode: added - (*Function).Encode: added - (*GenerateServiceRequest).Decode: added - (*GenerateServiceRequest).Encode: added - (*GenerateServiceResponse).Decode: added - (*GenerateServiceResponse).Encode: added - (*HandshakeRequest).Decode: added - (*HandshakeRequest).Encode: added - (*HandshakeResponse).Decode: added - (*HandshakeResponse).Encode: added - (*Module).Decode: added - (*Module).Encode: added - (*ModuleID).Decode: added - (*Plugin_Goodbye_Args).Decode: added - (*Plugin_Goodbye_Args).Encode: added - (*Plugin_Goodbye_Result).Decode: added - (*Plugin_Goodbye_Result).Encode: added - (*Plugin_Handshake_Args).Decode: added - (*Plugin_Handshake_Args).Encode: added - (*Plugin_Handshake_Result).Decode: added - (*Plugin_Handshake_Result).Encode: added - (*Service).Decode: added - (*Service).Encode: added - (*ServiceGenerator_Generate_Args).Decode: added - (*ServiceGenerator_Generate_Args).Encode: added - (*ServiceGenerator_Generate_Result).Decode: added - (*ServiceGenerator_Generate_Result).Encode: added - (*ServiceID).Decode: added - (*SimpleType).Decode: added - (*Type).Decode: added - (*Type).Encode: added - (*TypePair).Decode: added - (*TypePair).Encode: added - (*TypeReference).Decode: added - (*TypeReference).Encode: added - Feature.Encode: added - ModuleID.Encode: added - ServiceID.Encode: added - SimpleType.Encode: added --- go.uber.org/thriftrw/protocol --- Compatible changes: - BinaryStreamer: added --- go.uber.org/thriftrw/protocol/binary --- Compatible changes: - Default: added - EnvelopeV0Responder: added - EnvelopeV1Responder: added - NewStreamReader: added - NewStreamWriter: added - NoEnvelopeResponder: added - Protocol: added - Responder: added - StreamReader: added - StreamWriter: added --- go.uber.org/thriftrw/protocol/envelope --- NEW PACKAGE --- go.uber.org/thriftrw/protocol/stream --- NEW PACKAGE --- go.uber.org/thriftrw/thrifttest/streamtest --- NEW PACKAGE --- go.uber.org/thriftrw/version --- Incompatible changes: - Version: value changed from "1.28.0" to "1.29.0" ```
#495 provided the code generation for streaming reads of enums off of the wire.
Provide the streaming read generators for all of the other types.
Doing so meant providing a
StreamGenerator
that plumbs into the existinggenerators. This
StreamGenerator
only provides the "decoding" mechanism,leveraging #491 for reading the raw Thrift encoding off of the wire.
In addition to providing a
StreamGenerator
, the list, map, set, enum, typdef,and struct generators all added a
Decoder
method that will appropriatelyrecurse and iterate to generate the proper, mirrored readers for the raw wire
representation.
The
Decode
andDecodePtr
methods forStreamGenerator
then hook into thetemplated generator itself, providing the templated
decode
anddecodePtr
calls that will be used where necessary.
The existing tests were leveraged to make sure that the streaming reads were
compatible with the current binary writes and that no data was lost. In doing
so, a 'genericized' function that will perform the cross-products of
encoding/writing and decoding/reading.
A benchmark was also added to evaluate the new streaming reads. The following
represents benchmarks of the original code vs the dedup'd code (the binary
decode/encode leveraging the streaming implementations):
and the original code vs the streaming implementations (benchmarks renamed for
the purpose of benchstat-ing).