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

Add spans support to CodedInputStream #8

Conversation

mkosieradzki
Copy link

Supersedes #7

@@ -6,7 +6,7 @@
<AssemblyTitle>Google Protocol Buffers</AssemblyTitle>
<VersionPrefix>3.6.0</VersionPrefix>
<Authors>Google Inc.</Authors>
<TargetFrameworks>netstandard1.0;net45</TargetFrameworks>
Copy link
Owner

Choose a reason for hiding this comment

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

Fixed in #9. You can rebase and the diff should disappear from this this PR (btw, I think fixes like this should be in a separate commit - then it's easier to remove then once it has been fixed in the target branch).

Copy link
Author

Choose a reason for hiding this comment

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

Done

Exposed span-variants of CodedInputStream APIs
Added possibility to create CodedInputStream from ReadOnlySequence and ReadOnlySpan
Exposed Wrapped-version of Read APIs for future use by an updated Codegen
Exposed PushLimit/PopLimit/ReachedLimit/BeginReadNested/EndReadNested to allow more efficient inlining parsing loops by code generator
Changed ReadFloat to use Int32BitsToSingle approach
Resolved ambiguity in documentation crefs
@mkosieradzki mkosieradzki force-pushed the span_support_experimental branch from a54ed37 to 7907c6d Compare August 9, 2018 11:13
@jtattermusch
Copy link
Owner

@mkosieradzki I still have this PR on my backlog, but i got busy with fixing some unrelated gRPC C# issues recently. Sorry for delay!
I also spent some time prototyping what should become the new serialization/deserialization API for gRPC C# so that we can make use of the new protobuf API (and thus be able to process gRPC messages without performing any additional copies). The sketch is here:
grpc/grpc#16367 (the new (de)serialization API)
grpc/grpc#16371 (prototype of how accessing reading native segmented buffer can work in C# when receiving message)

@jtattermusch
Copy link
Owner

as a side note, once concern I still have with this PR (and I have trouble coming up with a better idea) is
the way we are exposing the ref ReadOnlySpan<byte> immediateBuffer in the API publicly - which basically exposes the internals of the implementation and presents a risk of breaking the API in the future.
My thoughts (i'm not leaning one way or another):
One option would be to just expose e.g. public long ReadInt64(ref ParsingContext context) where ParsingContext would be opaque to users, but that would in turn require declaring ParsingContext as a ref struct, which can be a problem for backwards compatibility IMHO.
Also the need for ref argument bothers me a bit, it just makes the API look a bit messy (and at the same time I know there's no easy way around it).

@jtattermusch
Copy link
Owner

CC @davidfowl in case you have opinions.

@kwaclaw
Copy link

kwaclaw commented Jan 13, 2019

Just curious: Is this issue still actively considered?

@davidfowl
Copy link

I think the approach here is solid. Do these methods all need to be exposed publicly?

@jskeet
Copy link
Collaborator

jskeet commented Jan 14, 2019

@davidfowl: Yes, I'd expect that they need to be public as they'll need to be called from generated code. Unfortunately I don't have time to review this carefully right now, due to other commitments.

@jtattermusch
Copy link
Owner

@davidfowl yes, they need to public which is the reason why I am hesitant with API as is (for internal API the same approach would be perfectly fine).

@prat0088
Copy link

This PR looked useful. Is it shelved?

@jtattermusch
Copy link
Owner

@prat0088 the plan is for fast Span-based parsing to land in one form or another (design discussion are in progress), but there is currently no ETA.

Another take on the Span-based parsing is protocolbuffers#5888 and this PR will probably be superseded by it. Stay tuned.

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.

6 participants