Skip to content

Better slice management for windows reads#17377

Closed
jtattermusch wants to merge 2 commits intogrpc:masterfrom
jtattermusch:windows_oom_fix
Closed

Better slice management for windows reads#17377
jtattermusch wants to merge 2 commits intogrpc:masterfrom
jtattermusch:windows_oom_fix

Conversation

@jtattermusch
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch commented Dec 3, 2018

Based on #17355.

Fixes #16464.

tcp_windows is much more basic implementation than tcp_posix (which supports resource quota, counters and a few other things missing in tcp_windows).

The scenario from #16464:

  1. there is a one-way stream of messages sending as fast as it can
  2. for every read on windows, tcp_windows allocates a new slice (8KB in size) and but the actual size of the read is small, so there is a lot of allocated memory.
  3. the slices only get disposed once the messages are processed by the C# layers.
  4. there seems to be a large flow control window (because it's a high speed connection), so many messages will be in flight, each taking 8KB (regardless of its actual size).
  5. In the example described in C#: Crash in grpc_csharp_ext.x86.dll (out of memory) #16464 there is multiple channels, each one keeping a lot of slice around, to the memory usage grows like crazy and the thing eventually OOMs.

To fix, I basically used the same logic that is currently used by tcp_posix.cc -> a "leftover" buffer is kept between reads (buffer_trim is used to split the actual read data and the leftover), so all the slices allocated are fully utilized.
I've verified with an example that the memory consumption per channel is now much more reasonable (reduction from ~500MB to ~35MB for ty example app).

Please refer to tcp_posix when reviewing, as the logic used is almost identical
https://github.com/grpc/grpc/blob/master/src/core/lib/iomgr/tcp_posix.cc

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 3, 2018 •
edited
Loading

The fix is important enough to be backported to v1.17.x
CC @muxi

@jtattermusch jtattermusch added the release notes: yes Indicates if PR needs to be in release notes label Dec 3, 2018
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 3, 2018 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@jtattermusch jtattermusch mentioned this pull request Dec 3, 2018
Closed
@jtattermusch jtattermusch mentioned this pull request Dec 3, 2018
Merged
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 3, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 3, 2018 

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,410      Total (=)      2,020,410

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,174,076      Total (<)     11,174,080

 No significant differences in binary sizes

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 3, 2018 

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 4, 2018

I think it might be better to merge #17378 first and then upmerge to master, rather than merging in both branches? @muxi @nicolasnoble

@muxi
Copy link
Copy Markdown
Contributor

muxi commented Dec 4, 2018

I thought upmerge is not performed until before next release, so merging both PR right now sounds better by helping reduce the conflicts when we upmerge later.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 5, 2018

@muxi I think that's wrong. The upmerge should be performed as soon as possible after stuff gets merged to a release branch (because if there's an important fix in the release branch, we can be 99.9% sure that the same fix will be needed in the master branch as well). I think upmerges should be frequent (which makes them simple) and should not be delayed unnecessarily.

I think merging the same PR twice is not a good idea in general (especially if the PR is non-trivial and has more than 1 commit), merging it just once to the release branch and then merging the branch with master makes it easier to track what fixes got merged where.

AspirinSJL
AspirinSJL approved these changes Dec 6, 2018
View reviewed changes
src/core/lib/iomgr/tcp_windows.cc
if (info->bytes_transfered != 0 && !tcp->shutting_down) {
sub = grpc_slice_sub_no_ref(tcp->read_slice, 0, info->bytes_transfered);
grpc_slice_buffer_add(tcp->read_slices, sub);
GPR_ASSERT((size_t)info->bytes_transfered <= tcp->read_slices->length);
Copy link
Copy Markdown
Contributor

@AspirinSJL AspirinSJL Dec 5, 2018

Choose a reason for hiding this comment

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

Please use C++-style cast.

Similar through this file.

src/core/lib/iomgr/tcp_windows.cc
grpc_closure* write_cb;
grpc_slice read_slice;

/* garbage after the last read */
Copy link
Copy Markdown
Contributor

@AspirinSJL AspirinSJL Dec 6, 2018

Choose a reason for hiding this comment

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

Not sure why it's called "garbage". But I'm okay with this since it's used elsewhere before this PR.

Copy link
Copy Markdown
Contributor

@nicolasnoble nicolasnoble Dec 6, 2018

Choose a reason for hiding this comment

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

"leftover".

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 6, 2018

The PR has already been merged into v1.17.x as #17378.
Because the review comments are only code-style changes, I'll just wait for the upmerge of #17378 to master and then fix the style in a followup PR on master.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 6, 2018

This was upmerged to master as part of #17423

@lock lock bot locked as resolved and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@nicolasnoble nicolasnoble nicolasnoble approved these changes

@AspirinSJL AspirinSJL AspirinSJL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AspirinSJL AspirinSJL

@nicolasnoble nicolasnoble

Labels

lang/core priority/P0/RELEASE BLOCKER release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

C#: Crash in grpc_csharp_ext.x86.dll (out of memory)

5 participants

@jtattermusch @grpc-testing @muxi @AspirinSJL @nicolasnoble