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

Avoid C# GC when deserializing large messages #13440

Closed
wants to merge 4 commits into from

Conversation

jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Nov 17, 2017

Progress towards fixing #13236.

  • add new SegmentDeserializer field into marshaller (that takes ArraySegment instead of byte[]). the backward compatibility is maintained as the new deserializer fallbacks to older byte[] (with some overhead though) based serializer if not defined explicitly
  • when deserializing messages, don't always allocate a new byte array, but instead reuse a thread-local byte array that is allocated for that purpose. That byte array is always-growing, but it's a weak reference, so the runtime can free the memory if it grows too big and memory is needed (the expectation is users shouldn't really use messages larger >X MB or so at that makes little practical sense - they could always use streaming).
  • new ParseFrom(buffer, offset, count) API from Protobuf 3.5+ is used to allow parsing messages from byte array that is potentially larger than the message size. Users would need to regenerate their .proto generate code to benefit from this optimization.

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@jtattermusch jtattermusch changed the title WIP: Avoid C# GC when deserializing large messages Avoid C# GC when deserializing large messages Dec 5, 2017
@jtattermusch
Copy link
Contributor Author

CC @azyobuzin

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

}
// the cached buffer size only grows here, but that's fine because GC can collect the weak reference
// if memory pressure increases.
if (buffer == null || buffer.Length <= minSize)

Choose a reason for hiding this comment

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

buffer.Length <= minSize should be buffer.Length < minSize

@azyobuzin
Copy link

azyobuzin commented Dec 6, 2017

It looks great as to deserializing mesages and works as I expect (with #13440 (comment) ). Thank you.

@apolcyn
Copy link
Contributor

apolcyn commented Feb 7, 2018

@jtattermusch is this waiting on review?

@jtattermusch
Copy link
Contributor Author

@apolcyn I temporarily interrupted work on this PR to take care of other things (cmake & test flakiness). I'll get back to it when I'm back from vacation.

@jtattermusch
Copy link
Contributor Author

Superseded by #18865 which provides a better solution.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants