Protobuf: protoc for c# generates slightly inconsistent code: Direct use of "new" instead of "parser.CreateTemplate()" to create new instances for singular message fields

[ManuelKugelmann]

What version of protobuf and what language are you using?
Version: master/v3.7.0 but looks like 3.9.0 is the same
Language: C#

What operating system and version?
Windows 10

What runtime / compiler are you using
VS2017

What did you do?
Steps to reproduce the behavior:

1. use protoc to generate c# code
2. look at code

What did you expect to see
use of "parser.CreateTemplate()" to create new instances for singular message fields

What did you see instead?
direct use of "new T()"

• inconsistent with RepeatedFields codecs
• limits customizability through factory method and message parser

Anything else we should know about your project / environment
Implemented pooling of message objects by setting a new static MessageParser with a pool aware factory method. Now running mostly garbage collection free. Code could be cleaner if this issue was addressed.

[owt5008137]

We meet the same problem. Is there any plan to support custom factory?

[ManuelKugelmann]

@owt5008137 Apply the patch from my pull request and add a partial class for each message that should have a custom factory:

   partial class MyMessage
   {
      static MyMessage factory()
      {
         <...>
      }
      static MyMessage()
      {
         _parser = new MessageParser<MyMessage>(factory);
      }
   }

[jtattermusch]

Does the trick from https://github.com/protocolbuffers/protobuf/issues/6401#issuecomment-659395435 really work?
The _parser field is readonly as far as I can tell. Also it's not immediatelly clear that the static intitializer beats the generated _parser assignment.
If so, I think the trick works, but feels a bit hacky as you're forcibly overwriting a field that intentionally private.

On the other hand, being able to support pooling for messages sounds interesting, if done right.
The problems I see

• you have no easy way to return rented messages to the pool (e.g. there is not Dispose() or Return() method)
• I think a big portion of allocations actually comes from allocating fields like string, ByteString, RepeatedFields, Maps etc., where you can't simply avoid the allocations (and if the pooling only works in very special cases, it's less useful).

[jtattermusch]

CC @JamesNK as this sounds like an interesting idea. It's a long haul but maybe this could be the start of being able to do high-performance allocation-free gRPC services.

[JamesNK]

For pooling to be generally useful there needs to be a way to return a hierarchy of messages, and reset all the message properties while you're at it.

I agree with @jtattermusch. I'm not sure how useful it would be. The overhead of renting/resetting/returning a hierarchy of messages is probably more work than GCing them. And usually messages are relatively small compared to the bigger string and byte array allocations.

• I like making CreateTemplate public. With that method being public you can create messages without reflection. I had to use Activator.CreateInstance in some places where I needed to create messages.
• Is there noticeable perf overhead of using CreateTemplate vs the ctor?

[owt5008137]

@ ManuelKugelmann Nice work. But I wonder if it's better to provide a way to modify _parser of all generated messages once for all?

[ManuelKugelmann]

@JamesNK @jtattermusch

I have a mostly GC free protobuf running - and as you said, I had to generate/add additional code for instance cleanup and returning them to the pool.

In my setup I got a large message (full scene desctiption via OpenSimulationInterface) every frame - the messages contain very many nested message fields (e.g. positions vectors). GC was not happy - pooling was definitely faster and produced no GC spikes.

CreateTemplate was already used in RepeatedFields - I do not believe that it is an performance issue.

I'll look into cleaning up my Pooling code and posting it here for discussion. An upside to the pooling is, that you get to keep the wrapper objects for e.g. RepeatedFields, Maps, etc...

@owt5008137 I had the code to replace the parser generated along with the instance cleanup code :)

[jtattermusch]

@JamesNK @jtattermusch

I have a mostly GC free protobuf running - and as you said, I had to generate/add additional code for instance cleanup and returning them to the pool.

In my setup I got a large message (full scene desctiption via OpenSimulationInterface) every frame - the messages contain very many nested message fields (e.g. positions vectors). GC was not happy - pooling was definitely faster and produced no GC spikes.

CreateTemplate was already used in RepeatedFields - I do not believe that it is an performance issue.

It's always best to check - we have a pretty good set of microbenchmarks now, so it would be good if you could run the before and after state and double checked your assertions (please post the results you got on the PR).

I'll look into cleaning up my Pooling code and posting it here for discussion. An upside to the pooling is, that you get to keep the wrapper objects for e.g. RepeatedFields, Maps, etc...

@owt5008137 I had the code to replace the parser generated along with the instance cleanup code :)