New Span-based C# parsing logic #7351

jtattermusch · 2020-04-02T14:37:57Z

A big refactor of C# parsing logic to allow parsing from ReadOnlySequence (which would allow parsing directly from the data as received on the wire without requiring to copy the buffers and thus reducing the GC pressure a lot).

Main changes:

refactor out the parsing logic to helper classes (to avoid any duplication of the parsing logic)
store all the parsing state into a struct (currently called ParserInternalState)
introduce a ParseContext ref struct that has public methods, but they are only meant to be invoked by the generated code
generate a InternalMergeFrom(ref ParseContext ctx) method, which has the actual parsing code (and is only supposed to be called by the generated code, never directly by the users.
allow easy and fast conversion from CodedInputStream into a ParseContext (because the parsing state is encapsulated in the ParserInternalState, so bringing the state between the two only requires copying a struct.
the original IMessage.MergeFrom(CodedInputStream cis) methods redirects to InternalMergeFrom(ref ParseContext). This transition has some (very small = copying a struct) overhead, but is basically only done once in the entire parsing tree, so that seem acceptable.
all the public APIs that take CodedInputStream still behave exactly the same.
compatibility with old generated code: if one uses old generated code against a newer runtime, everything will still work, but there might be some performance penalty (see benchmarks). Regenerating the code will speed things up.
the public API increase is kept as small as possible. Basically the only new method we provide is Parse.ParseFrom(ReadOnlySequence byte) as entry point for the more efficient parsing. There are a few other additions (parsing maps, repeated fields etc.), but they are only meant to be invoked by the generated code.
this PR only addresses memory-efficient parsing. Memory efficient serialization can be added in a followup PR.

This PR drops netstandard1.0 support (which doesn't support the Span type we need) and adds netstandard1.1 instead. The impact on supported platforms should be minimal (almost everyone supports netstandard2.0 these days anyway).

Basically this PR a new and simpler take on #5888 (which is much bigger, introduces much more code duplication and leads to bigger public API increase).

jtattermusch · 2020-04-02T14:40:01Z

CC @JamesNK @JunTaoLuo

this is still WIP and I'll probably do some rebasing, but all this seems to be the minimal PR that adds ReadOnlySequence parsing.

JamesNK · 2020-04-11T06:03:05Z

Changes to fix running on .NET Framework:

JamesNK@5d868ad

csharp/src/Google.Protobuf/ParseContext.cs

JamesNK · 2020-04-11T06:09:05Z

csharp/src/Google.Protobuf/IBufferMessage.cs

+        /// Internal implementation of merging data from given parse context into this message.
+        /// Users should never invoke this method directly.
+        /// </summary>        
+        void MergeFrom_Internal(ref ParseContext ctx);


I'm not a big fan of _Internal suffix.

I'd rather it just be MergeFrom, have this comment warning that this overload of MergeFrom shouldn't be used, and code generation should explicitly implement the interface method.

https://hackernoon.com/hiding-members-via-explicit-interface-implementation-in-c-2c5u3oc1

What are your plans for message writing? WriteContext and an equivalent method here?

I wonder if IBufferMessage is still the right name for this interface.

I think MergeFromInternal and InternalMergeFrom also works.
Explicitly implementing the interface in generated code sounds like a good idea - I'll try that.

For message writing I think we can use a similar approach (which will hopefully require way fewer code changes than parsing). I think serialization could go in as a follow-up PR to make this PR smaller (we can make sure we won't release the intermediate version)

Do you have suggestions for how IBufferMessage should be renamed?

JamesNK

Comments from looking through code with test coverage. Need to have some tests with legacy message generation to hit backwards compatibility paths.

csharp/src/Google.Protobuf/SegmentedBufferHelper.cs

JamesNK · 2020-04-11T08:42:07Z

csharp/src/Google.Protobuf/SegmentedBufferHelper.cs

+                }
+            }
+
+            state.totalBytesRetired += state.bufferSize;


FYI no tests reach here

csharp/src/Google.Protobuf/ParseContext.cs

JamesNK · 2020-04-11T08:44:28Z

csharp/src/Google.Protobuf/ParseContext.cs

+        /// </summary>
+        internal bool DiscardUnknownFields {
+            get { return state.DiscardUnknownFields; }
+            set { state.DiscardUnknownFields = value; }


~~Is this set ever used?~~ It is, just not used in tests

JamesNK · 2020-04-11T08:44:34Z

csharp/src/Google.Protobuf/ParseContext.cs

+        internal ExtensionRegistry ExtensionRegistry
+        {
+            get { return state.ExtensionRegistry; }
+            set { state.ExtensionRegistry = value; }


~~Is this set ever used?~~ It is, just not used in tests

csharp/src/Google.Protobuf/ParsingPrimitivesMessages.cs

JamesNK · 2020-04-11T08:49:36Z

csharp/src/Google.Protobuf/ParsingPrimitivesWrappers.cs

+            {
+                return 0F;
+            }
+            int finalBufferPos = state.totalBytesRetired + state.bufferPos + length;


FYI no tests reach here

JamesNK · 2020-04-11T08:49:52Z

csharp/src/Google.Protobuf/ParsingPrimitivesWrappers.cs

+            {
+                return 0D;
+            }
+            int finalBufferPos = state.totalBytesRetired + state.bufferPos + length;


FYI no tests reach here

csharp/src/Google.Protobuf/ParseContext.cs

csharp/src/Google.Protobuf/FieldCodec.cs

csharp/src/Google.Protobuf/MessageParser.cs

jtattermusch · 2020-04-24T17:21:28Z

@gerben-s sorry, the commentary on benchmark results was buried in the comment thread:
#7351 (comment)
#7351 (comment)

In short, the results seem to be pretty acceptable, especially because for C# we haven't really been too stringent about C# performance measurements until now (many of the benchmarks have only been added very recently) and we've demonstrated that being able to parse from ReadOnlySequence means a big improvement in terms of avoiding buffer copies and reducing GC pressure (so slight regression for some benchmarks would be trade off worth taking).

…ew_parsing

JamesNK · 2020-04-27T11:33:30Z

Note that one benefit not shown by the benchmark results is the cost of creating the array when using CodedInputStream. The input stream benchmarks are reading from an already initialized array. In the real world, e.g. gRPC, a framework will need to allocate an array and copy data to it before reading it with CodedInputStream.

Reading from a sequence (the newly added feature) allows parsing a third-party buffer of data, e.g. the request stream. That avoids overhead of allocations, copying and garbage collection.

gerben-s

I'm not the best person for c# review on code/style. However the high level structure seems very familiar and good to me. If the benchmarks are good as you say I think this is a great improvement

jtattermusch · 2020-04-29T13:49:56Z

@gerben-s Thanks for the review! James has already reviewed the code from the C# perspective. I'll see if @jskeet has any more comments and if not, I'll consider this ready to merge (upon green tests of course).

jtattermusch · 2020-04-29T13:52:45Z

@jskeet do you have any more comments beyond what's been said above? If you want to take some more time to review this, feel free to - there's no rush, just let me know.

jtattermusch · 2020-05-04T17:00:47Z

@jskeet please let me know if you're planning to review this or not.

jtattermusch · 2020-05-05T14:56:40Z

Test failures seem completely unrelated to C#.

jtattermusch · 2020-05-05T14:57:46Z

Thanks everyone for the reviews and for all the work to get things to this state.

jtattermusch · 2020-05-12T07:17:43Z

FTR @jskeet has discovered a problem with this PR.
Even though our TestProtos project is set to <LangVersion>3</LangVersion> to check if ref struct ParseContext is fine to use in the generated code, the compilation still seems to fail if we actually use an old compiler:

 it doesn't build with a real C# 5 compiler. <LangVersion>5</LangVersion> doesn't pick it up, but fortunately the C# 5 compiler (the last one before Roslyn) is still part of Windows, so I could test it reasonably simply:

> c:\Windows\Microsoft.NET\Framework\v4.0.30319\csc /r:..\Google.Protobuf\bin\debug\net45\google.protobuf.dll *.cs /main:Google.Protobuf.Examples.AddressBook.Program

Errors: 
Addressbook.cs(448,14): error CS0619: 'Google.Protobuf.ParseContext' is obsolete: 'Types with embedded references are not supported in this version of your compiler.'
Addressbook.cs(263,10): error CS0619: 'Google.Protobuf.ParseContext' is obsolete: 'Types with embedded references are not supported in this version of your compiler.'
Addressbook.cs(586,10): error CS0619: 'Google.Protobuf.ParseContext' is obsolete: 'Types with embedded references are not supported in this version of your compiler.'

(The "obsolete" message is unfortunate, but it's an artifact of the way Span/ParseContext and other ref struct are handled by the compiler. That's nothing we've done wrong.)

We have some ideas for how to fix this (it will require an #if) but we're still not sure it it's going to work out (or we will need to revert).

New Span-based serialization logic (followup for #7351)

…harp_new_parsing New Span-based C# parsing logic

…ffer_serialization New Span-based serialization logic (followup for protocolbuffers#7351)

googlebot added the cla: yes label Apr 2, 2020

jtattermusch added kokoro:run and removed cla: yes labels Apr 2, 2020

googlebot added the cla: yes label Apr 2, 2020

jtattermusch added the kokoro:force-run label Apr 3, 2020

jtattermusch marked this pull request as ready for review April 6, 2020 07:26

jtattermusch force-pushed the protobuf_csharp_new_parsing branch from 54f5533 to 4e01423 Compare April 6, 2020 07:40

protobuf-kokoro removed kokoro:run labels Apr 6, 2020

jtattermusch mentioned this pull request Apr 6, 2020

Refactor some Protobuf C# microbenchmarks #7360

Merged

JamesNK reviewed Apr 11, 2020

View reviewed changes

csharp/src/Google.Protobuf/MessageParser.cs Show resolved Hide resolved

csharp/src/Google.Protobuf/MessageParser.cs Show resolved Hide resolved

jtattermusch and others added 15 commits April 14, 2020 09:37

bring in new files

175c965

make everything build

ea60538

add ParseFrom(ReadOnlySequence) methods

6d5bc90

make things build after codegen change

220e7be

codegen changes

f6bdd7d

regenerate C# protos

3196ef9

make Google.Protobuf internals visible to benchmarks

b2d2915

ParseRawPrimitivesBenchmark: compare CodedInputStream vs ParseContext

5f83619

ParseMessageBenchmark: parsing from ReadOnlySequence

42eff9d

faster initialization of ParserInternalState

d17b511

optimize initialization of ParseContext

07182a8

Fix net45 tests

18bfd9e

remove newline

3936114

change MergeFrom_Internal to InternalMergeFrom

d7e2c83

codegen: InternalMergeFrom

92076f7

Merge remote-tracking branch 'upstream/master' into protobuf_csharp_n…

7134d25

…ew_parsing

jtattermusch mentioned this pull request Apr 28, 2020

C#: getting RecursionLimitExceeded with no way to configure grpc/grpc#22682

Closed

gerben-s approved these changes Apr 29, 2020

View reviewed changes

jtattermusch added the kokoro:run label Apr 29, 2020

protobuf-kokoro removed the kokoro:run label Apr 29, 2020

jtattermusch merged commit a690227 into protocolbuffers:master May 5, 2020

JamesNK mentioned this pull request May 10, 2020

HTTP2: Performance issues when many concurrent requests dotnet/runtime#35184

Open

This was referenced May 11, 2020

Add CodedInputReader and CodedOutputWriter #5888

Closed

Too many full GCs when receive big responses grpc/grpc#13236

Closed

This was referenced May 12, 2020

Overcome the ref struct limitation for pre-roslyn compilers by introducing GOOGLE_PROTOBUF_REFSTRUCT_COMPATIBILITY_MODE for generated code #7490

Merged

c# feature: new method AsSpan() on RepeatedField<T> #6538

Closed

jtattermusch mentioned this pull request May 29, 2020

New Span-based serialization logic (followup for #7351) #7576

Merged

jtattermusch mentioned this pull request Jun 24, 2020

3.12.3 might be wrongly tagged - created from master branch? (C#-release package lose IBufferMessage.cs) #7637

Closed

jtattermusch added a commit that referenced this pull request Jun 29, 2020

Merge pull request #7576 from jtattermusch/protobuf_buffer_serialization

206b973

New Span-based serialization logic (followup for #7351)

This was referenced Jul 9, 2020

DO NOT MERGE: add span_based_serialization design #6874

Closed

Use new protobuf serialization/deserialization API in C# marshallers grpc/grpc#23485

Merged

jtattermusch mentioned this pull request Oct 19, 2020

C# - support byte[]-segment deserializers **without** changing the calling API grpc/grpc#19460

Closed

JunTaoLuo mentioned this pull request Oct 30, 2020

Xamarin Forms and Grpc.Net grpc/grpc-dotnet#1097

Closed

This was referenced Nov 30, 2020

How to use the new Span API on the C# project #8084

Closed

[CSharp] Allow Span<byte>-based parsing in CodedInputStream #3431

Closed

bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023

Merge pull request protocolbuffers#7351 from jtattermusch/protobuf_cs…

ae8eed4

…harp_new_parsing New Span-based C# parsing logic

bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023

Merge pull request protocolbuffers#7576 from jtattermusch/protobuf_bu…

0a8fe81

…ffer_serialization New Span-based serialization logic (followup for protocolbuffers#7351)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Span-based C# parsing logic #7351

New Span-based C# parsing logic #7351

jtattermusch commented Apr 2, 2020 •

edited

jtattermusch commented Apr 2, 2020

JamesNK commented Apr 11, 2020

JamesNK Apr 11, 2020 •

edited

JamesNK Apr 11, 2020

jtattermusch Apr 14, 2020

JamesNK left a comment

JamesNK Apr 11, 2020

JamesNK Apr 11, 2020 •

edited

JamesNK Apr 11, 2020 •

edited

JamesNK Apr 11, 2020

JamesNK Apr 11, 2020

jtattermusch commented Apr 24, 2020

JamesNK commented Apr 27, 2020

gerben-s left a comment

jtattermusch commented Apr 29, 2020

jtattermusch commented Apr 29, 2020

jtattermusch commented May 4, 2020

jtattermusch commented May 5, 2020

jtattermusch commented May 5, 2020

jtattermusch commented May 12, 2020

New Span-based C# parsing logic #7351

New Span-based C# parsing logic #7351

Conversation

jtattermusch commented Apr 2, 2020 • edited

jtattermusch commented Apr 2, 2020

JamesNK commented Apr 11, 2020

JamesNK Apr 11, 2020 • edited

Choose a reason for hiding this comment

JamesNK Apr 11, 2020

Choose a reason for hiding this comment

jtattermusch Apr 14, 2020

Choose a reason for hiding this comment

JamesNK left a comment

Choose a reason for hiding this comment

JamesNK Apr 11, 2020

Choose a reason for hiding this comment

JamesNK Apr 11, 2020 • edited

Choose a reason for hiding this comment

JamesNK Apr 11, 2020 • edited

Choose a reason for hiding this comment

JamesNK Apr 11, 2020

Choose a reason for hiding this comment

JamesNK Apr 11, 2020

Choose a reason for hiding this comment

jtattermusch commented Apr 24, 2020

JamesNK commented Apr 27, 2020

gerben-s left a comment

Choose a reason for hiding this comment

jtattermusch commented Apr 29, 2020

jtattermusch commented Apr 29, 2020

jtattermusch commented May 4, 2020

jtattermusch commented May 5, 2020

jtattermusch commented May 5, 2020

jtattermusch commented May 12, 2020

jtattermusch commented Apr 2, 2020 •

edited

JamesNK Apr 11, 2020 •

edited

JamesNK Apr 11, 2020 •

edited

JamesNK Apr 11, 2020 •

edited