Skip to content

Immediately run write closures for failed stream#16179

Merged
srini100 merged 1 commit intogrpc:v1.14.xfrom
kpayson64:fix_again
Jul 31, 2018
Merged

Immediately run write closures for failed stream#16179
srini100 merged 1 commit intogrpc:v1.14.xfrom
kpayson64:fix_again

Conversation

@kpayson64
Copy link
Copy Markdown
Contributor

@kpayson64 kpayson64 commented Jul 30, 2018

Fixes #15889

This change was present in an initial iteration of the bug fix, but later got removed after review + rework.

@kpayson64 kpayson64 requested a review from mehrdada July 30, 2018 18:41
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

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

libgrpc.so

     VM SIZE                                                                   FILE SIZE
 ++++++++++++++ GROWING                                                     ++++++++++++++
  +0.1%     +16 src/core/ext/transport/chttp2/transport/chttp2_transport.cc     +16  +0.1%
      +2.6%     +16 grpc_chttp2_complete_closure_step                               +16  +2.6%

 -+-+-+-+-+-+-+ MIXED                                                       +-+-+-+-+-+-+-
  -0.0%     -16 [None]                                                          +32  +0.0%

  [ = ]       0 TOTAL                                                           +48  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@kpayson64 kpayson64 changed the base branch from master to v1.14.x July 30, 2018 18:45
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

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

libgrpc.so

     VM SIZE                                                                   FILE SIZE
 ++++++++++++++ GROWING                                                     ++++++++++++++
  +0.1%     +16 src/core/ext/transport/chttp2/transport/chttp2_transport.cc     +16  +0.1%
      +2.6%     +16 grpc_chttp2_complete_closure_step                               +16  +2.6%

 -+-+-+-+-+-+-+ MIXED                                                       +-+-+-+-+-+-+-
  -0.0%     -16 [None]                                                          +32  +0.0%

  [ = ]       0 TOTAL                                                           +48  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

[trickle] No significant performance differences

1 similar comment
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,976,662      Total (>)      1,976,654

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,775,741      Total (>)     10,775,735

 No significant differences in binary sizes

@srini100 srini100 added lang/core release notes: yes Indicates if PR needs to be in release notes labels Jul 30, 2018
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

[microbenchmarks] Performance differences noted:
Benchmark                                                              cpu_time    real_time
---------------------------------------------------------------------  ----------  -----------
BM_StreamingPingPong<MinInProcess, NoOpMutator, NoOpMutator>/262144/1  -4%         -4%

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

[microbenchmarks] Performance differences noted:
Benchmark                                                                                 cpu_time    real_time
----------------------------------------------------------------------------------------  ----------  -----------
BM_StreamingPingPong<InProcess, NoOpMutator, NoOpMutator>/262144/1                        +10%        +10%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/262144/1/0  +4%         +4%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/262144/2/1  +5%         +5%

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

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

libgrpc.so

     VM SIZE                                                                   FILE SIZE
 ++++++++++++++ GROWING                                                     ++++++++++++++
  +0.1%     +16 src/core/ext/transport/chttp2/transport/chttp2_transport.cc     +16  +0.1%
      +2.6%     +16 grpc_chttp2_complete_closure_step                               +16  +2.6%

 -+-+-+-+-+-+-+ MIXED                                                       +-+-+-+-+-+-+-
  -0.0%     -16 [None]                                                          +32  +0.0%

  [ = ]       0 TOTAL                                                           +48  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

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

libgrpc.so

     VM SIZE                                                                   FILE SIZE
 ++++++++++++++ GROWING                                                     ++++++++++++++
  +0.1%     +16 src/core/ext/transport/chttp2/transport/chttp2_transport.cc     +16  +0.1%
      +2.6%     +16 grpc_chttp2_complete_closure_step                               +16  +2.6%

 -+-+-+-+-+-+-+ MIXED                                                       +-+-+-+-+-+-+-
  -0.0%     -16 [None]                                                          +32  +0.0%

  [ = ]       0 TOTAL                                                           +48  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 30, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 31, 2018 

[microbenchmarks] No significant performance differences

@srini100 srini100 requested a review from yashykt July 31, 2018 00:48
@yashykt
Copy link
Copy Markdown
Member

yashykt commented Jul 31, 2018

LGTM

@yashykt yashykt closed this Jul 31, 2018
@yashykt yashykt reopened this Jul 31, 2018
yashykt
yashykt approved these changes Jul 31, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

LGTM

@srini100
Copy link
Copy Markdown
Contributor

srini100 commented Jul 31, 2018

Verified that this fixes the issue #15889

@srini100
Copy link
Copy Markdown
Contributor

srini100 commented Jul 31, 2018

Known MacOS failures due to no space left on device #16156

@srini100 srini100 merged commit dd5caf0 into grpc:v1.14.x Jul 31, 2018
@srini100 srini100 mentioned this pull request Aug 1, 2018
Closed
@ncteisen ncteisen mentioned this pull request Aug 3, 2018
Merged
ncteisen added a commit to ncteisen/grpc that referenced this pull request Aug 3, 2018
@ncteisen
reverts grpc#16179
2526c2f
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yashykt yashykt yashykt approved these changes

+1 more reviewer

@mehrdada mehrdada mehrdada approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lang/core 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.

6 participants

@kpayson64 @grpc-testing @yashykt @srini100 @mehrdada @kokoro-team