Skip to content

Fix uses of resource quota in UV TCP code#13433

Merged
murgatroid99 merged 2 commits intogrpc:masterfrom
murgatroid99:uv_resource_quota_fixes
Nov 17, 2017
Merged

Fix uses of resource quota in UV TCP code#13433
murgatroid99 merged 2 commits intogrpc:masterfrom
murgatroid99:uv_resource_quota_fixes

Conversation

@murgatroid99
Copy link
Copy Markdown
Member

@murgatroid99 murgatroid99 commented Nov 17, 2017

This fixes #12932.

The root cause for that issue appears to be that the optional_on_done argument to grpc_resource_user_alloc isn't really optional (anymore), and failing to pass it can result in mishandling allocation failures and/or using memory while the allocation is pending. The use of that code in tcp_uv.cc directly and through grpc_resource_user_slice_malloc had not been updated to account for that fact.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

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

libgrpc.so

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

 -------------- SHRINKING                            --------------
  -0.0%    -141 [None]                                  -677  -0.0%
  -1.6%    -115 src/core/lib/iomgr/resource_quota.cc    -115  -1.6%
      [DEL]    -114 grpc_resource_user_slice_malloc         -114  [DEL]
      -0.3%      -1 [Unmapped]                                -1  -0.3%

  -0.0%    -256 TOTAL                                   -792  -0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

[microbenchmarks] Performance differences noted:
Benchmark                  cpu_time    real_time
-------------------------  ----------  -----------
BM_MetadataRefUnrefStatic  -4%         -4%

@murgatroid99 murgatroid99 mentioned this pull request Nov 17, 2017
Closed
@nicolasnoble
Copy link
Copy Markdown
Contributor

nicolasnoble commented Nov 17, 2017

cc @dgquintas

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

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

libgrpc.so

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

 -------------- SHRINKING                            --------------
  -0.0%    -141 [None]                                  -677  -0.0%
  -1.6%    -115 src/core/lib/iomgr/resource_quota.cc    -115  -1.6%
      [DEL]    -114 grpc_resource_user_slice_malloc         -114  [DEL]
      -0.3%      -1 [Unmapped]                                -1  -0.3%

  -0.0%    -256 TOTAL                                   -792  -0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

[microbenchmarks] Performance differences noted:
Benchmark                  cpu_time    real_time
-------------------------  ----------  -----------
BM_MetadataRefUnrefStatic  -4%         -4%

@murgatroid99 murgatroid99 mentioned this pull request Nov 17, 2017
Merged
@sreecha sreecha mentioned this pull request Nov 17, 2017
Merged
@murgatroid99
Copy link
Copy Markdown
Member Author

murgatroid99 commented Nov 17, 2017

Known failures: #13445, #13148, #13381, #13426, #13446, #13404, #13122.

The interop Cloud-to-Cloud tests won't pass until the corresponding submodule update is merged into grpc-node.

@murgatroid99 murgatroid99 merged commit dbca3d9 into grpc:master Nov 17, 2017
This was referenced Nov 17, 2017
Closed
Closed
Closed
@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

@sreecha sreecha Awaiting requested review from sreecha

@ctiller ctiller Awaiting requested review from ctiller

1 more reviewer

@nicolasnoble nicolasnoble nicolasnoble approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/core kind/bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Node failure on macos

3 participants

@murgatroid99 @grpc-testing @nicolasnoble