Make inproc transport properly obey status ordering rules by vjpai · Pull Request #17096 · grpc/grpc

vjpai · 2018-11-05T09:13:05Z

The inproc transport was subtly violating the status ordering rules . This was not visible since only the C++ API was exposing the inproc transport, and the C++ API doesn't request status early (until the callback streaming client API came along). Technically, this was preventing the use of the inproc transport with, for example, Python that does request status early.

grpc-testing · 2018-11-05T09:18:24Z

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

libgrpc.so

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

 -------------- SHRINKING                                         --------------
  -0.0%     -88 [None]                                               -328  -0.0%
  -0.3%     -48 src/core/ext/transport/inproc/inproc_transport.cc     -48  -0.3%
      -2.3%     -84 op_state_machine                                      -84  -2.3%

  -0.0%    -136 TOTAL                                                -376  -0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-05T09:28:29Z

[trickle] No significant performance differences

grpc-testing · 2018-11-05T09:59:51Z

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

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,123,076      Total (<)     11,123,077

 No significant differences in binary sizes

grpc-testing · 2018-11-05T10:11:23Z

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] Performance differences noted:
Benchmark                                                                 locks_per_iteration
------------------------------------------------------------------------  ---------------------
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/0           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/1           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/2097152     -6%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/262144      -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/32768       -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/4096        -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/512         -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/64          -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/8           -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/0        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/1        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/2097152  -6%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/262144   -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/32768    -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/4096     -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/512      -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/64       -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/8        -7%

grpc-testing · 2018-11-05T12:22:52Z

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

libgrpc.so

     VM SIZE                                                         FILE SIZE
 ++++++++++++++ GROWING                                           ++++++++++++++
  +0.1%     +16 src/core/ext/transport/inproc/inproc_transport.cc     +16  +0.1%
      +1.2%     +32 perform_stream_op                                     +32  +1.2%
      +1.8%      +4 [Unmapped]                                             +4  +1.8%

 -+-+-+-+-+-+-+ MIXED                                             +-+-+-+-+-+-+-
  -0.0%     -88 [None]                                               +560  +0.0%

  -0.0%     -72 TOTAL                                                +576  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-05T12:30:32Z

[trickle] No significant performance differences

grpc-testing · 2018-11-05T13:04:13Z

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

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,123,085      Total (=)     11,123,085

 No significant differences in binary sizes

grpc-testing · 2018-11-05T13:16:55Z

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] Performance differences noted:
Benchmark                                                                 locks_per_iteration
------------------------------------------------------------------------  ---------------------
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/0           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/1           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/2097152     -4%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/262144      -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/32768       -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/4096        -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/512         -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/64          -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/8           -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/0        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/1        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/2097152  -5%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/262144   -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/32768    -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/4096     -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/512      -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/64       -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/8        -7%

ncteisen · 2018-11-05T13:46:09Z

"If it doesn't come with a test, is it a real PR?" --Vijay Pai, 2018

vjpai · 2018-11-05T14:25:55Z

Sometimes I wish I could just keep my mouth shut....

grpc-testing · 2018-11-05T18:01:26Z

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

libgrpc.so

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

 -+-+-+-+-+-+-+ MIXED  +-+-+-+-+-+-+-
  -0.0%     -64 [None]    +552  +0.0%

  -0.0%     -64 TOTAL     +552  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-05T18:09:41Z

[trickle] No significant performance differences

vjpai · 2018-11-05T18:25:19Z

Commit a3b803c adds a new test case. I have confirmed that without the inproc_transport.cc fix in place, this test fails, but with it in place, the test succeeds.

grpc-testing · 2018-11-05T18:25:48Z

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

libgrpc.so

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

 -+-+-+-+-+-+-+ MIXED  +-+-+-+-+-+-+-
  -0.0%     -64 [None]    +552  +0.0%

  -0.0%     -64 TOTAL     +552  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-05T18:29:38Z

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

libgrpc.so

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

 -+-+-+-+-+-+-+ MIXED  +-+-+-+-+-+-+-
  -0.0%     -64 [None]    +552  +0.0%

  -0.0%     -64 TOTAL     +552  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-05T18:36:28Z

[trickle] No significant performance differences

grpc-testing · 2018-11-05T18:42:08Z

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

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,180,937      Total (<)     11,180,946

 No significant differences in binary sizes

grpc-testing · 2018-11-05T18:42:56Z

[trickle] No significant performance differences

grpc-testing · 2018-11-05T18:54:21Z

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] Performance differences noted:
Benchmark                                                                 locks_per_iteration
------------------------------------------------------------------------  ---------------------
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/0           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/1           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/2097152     -6%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/262144      -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/32768       -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/4096        -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/512         -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/64          -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/8           -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/0        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/1        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/2097152  -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/262144   -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/32768    -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/4096     -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/512      -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/64       -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/8        -7%

grpc-testing · 2018-11-05T19:06:25Z

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

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,180,940      Total (>)     11,180,939

 No significant differences in binary sizes

grpc-testing · 2018-11-05T19:08:51Z

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

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,180,947      Total (>)     11,180,946

 No significant differences in binary sizes

grpc-testing · 2018-11-05T19:19:05Z

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] Performance differences noted:
Benchmark                                                                 locks_per_iteration
------------------------------------------------------------------------  ---------------------
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/0           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/1           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/2097152     -6%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/262144      -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/32768       -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/4096        -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/512         -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/64          -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/8           -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/0        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/1        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/2097152  -6%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/262144   -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/32768    -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/4096     -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/512      -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/64       -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/8        -7%

grpc-testing · 2018-11-05T19:23:00Z

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] Performance differences noted:
Benchmark                                                                 locks_per_iteration
------------------------------------------------------------------------  ---------------------
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/0           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/1           -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/2097152     -6%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/262144      -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/32768       -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/4096        -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/512         -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/64          -7%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/8           -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/0        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/1        -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/2097152  -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/262144   -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/32768    -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/4096     -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/512      -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/64       -7%
BM_StreamingPingPongMsgs<MinInProcess, NoOpMutator, NoOpMutator>/8        -7%

vjpai added the release notes: no Indicates if PR should not be in release notes label Nov 5, 2018

vjpai requested a review from ncteisen November 5, 2018 09:13

Make inproc transport properly obey status ordering rules

acd07f7

vjpai force-pushed the inproc_fix branch from f64dd84 to acd07f7 Compare November 5, 2018 17:56

Added another test case for status ordering

a3b803c

vjpai force-pushed the inproc_fix branch from 61a81a8 to a3b803c Compare November 5, 2018 18:24

ncteisen approved these changes Nov 5, 2018

View reviewed changes

vjpai merged commit 605e3bf into grpc:master Nov 5, 2018

vjpai deleted the inproc_fix branch November 5, 2018 20:58

lock bot locked as resolved and limited conversation to collaborators Feb 3, 2019

Conversation

vjpai commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

ncteisen commented Nov 5, 2018

Uh oh!

vjpai commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

vjpai commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

grpc-testing commented Nov 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants