Skip to content

Flow control fixes#13388

Merged
y-zeng merged 3 commits intogrpc:masterfrom
y-zeng:flow_control
Nov 16, 2017
Merged

Flow control fixes#13388
y-zeng merged 3 commits intogrpc:masterfrom
y-zeng:flow_control

Conversation

@y-zeng
Copy link
Copy Markdown
Contributor

@y-zeng y-zeng commented Nov 14, 2017

  • Handle negative remote_window in AnyOutgoing
  • Limit the imput dt of PID to 100ms in TransportFlowControl::SmoothLogBdp

@y-zeng y-zeng requested a review from ctiller November 14, 2017 19:41
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 14, 2017 

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

libgrpc.so

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

 -------------- SHRINK --------------
  [ = ]       0 [None]     -40  -0.0%

  [ = ]       0 TOTAL      -40  -0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 14, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 14, 2017 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 14, 2017 

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

libgrpc.so

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

 -------------- SHRINK --------------
  [ = ]       0 [None]     -24  -0.0%

  [ = ]       0 TOTAL      -24  -0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 15, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 15, 2017 

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_unary_ping_pong.BM_UnaryPingPong_InProcessCHTTP2_NoOpMutator_NoOpMutator__8_0.counters.old: 1


[microbenchmarks] No significant performance differences

ctiller
ctiller approved these changes Nov 15, 2017
View reviewed changes
src/core/ext/transport/chttp2/transport/flow_control.cc Outdated
const double dt = (double)(now - last_pid_update_) * 1e-3;
double dt = (double)(now - last_pid_update_) * 1e-3;
// Limit dt to 100ms
if (dt > 0.1) {
Copy link
Copy Markdown
Member

@ctiller ctiller Nov 15, 2017

Choose a reason for hiding this comment

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

Let's have a constant instead of repeating the number

Copy link
Copy Markdown
Contributor Author

@y-zeng y-zeng Nov 16, 2017

Choose a reason for hiding this comment

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

Done. Thanks for the review!

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

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

libgrpc.so

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

 -------------- SHRINK --------------
  [ = ]       0 [None]      -8  -0.0%

  [ = ]       0 TOTAL       -8  -0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

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

libgrpc.so

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

 -------------- SHRINK --------------
  [ = ]       0 [None]      -8  -0.0%

  [ = ]       0 TOTAL       -8  -0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

[microbenchmarks] No significant performance differences

@y-zeng
Copy link
Copy Markdown
Contributor Author

y-zeng commented Nov 16, 2017 •
edited
Loading

Basic Tests MacOS [dbg] (internal CI): #13085
Basic Tests MacOS [opt] (internal CI): #13085
Asan C++ (internal CI): #13122
Interop Cloud-to-Cloud Tests (internal CI): #13421
Tsan C++ (internal CI): #13178
Interop Cloud-to-Cloud Tests (internal CI): #13426

@y-zeng y-zeng merged commit 6d66369 into grpc:master Nov 16, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ctiller ctiller ctiller approved these changes

Assignees

@ctiller ctiller

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@y-zeng @grpc-testing @ctiller @kokoro-team