Skip to content

Revert arena size fix#15524

Merged
AspirinSJL merged 1 commit intogrpc:masterfrom
AspirinSJL:revert_arena_fix
May 23, 2018
Merged

Revert arena size fix#15524
AspirinSJL merged 1 commit intogrpc:masterfrom
AspirinSJL:revert_arena_fix

Conversation

@AspirinSJL
Copy link
Copy Markdown
Contributor

@AspirinSJL AspirinSJL commented May 23, 2018 •
edited by jtattermusch
Loading

because data race #15457.

Reverts #15402.

@AspirinSJL AspirinSJL requested a review from jtattermusch May 23, 2018 19:36
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 23, 2018 

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

libgrpc.so

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

 -------------- SHRINKING                               --------------
  -0.0%    -144 [None]                                     -232  -0.0%
  -0.1%     -16 src/core/lib/surface/call.cc                -16  -0.1%
      -2.6%     -10 [Unmapped]                                  -10  -2.6%
      [DEL]      -6 grpc_call_get_initial_size_estimate          -6  [DEL]

  -0.0%    -160 TOTAL                                      -248  -0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 23, 2018 

[trickle] No significant performance differences

@AspirinSJL
Copy link
Copy Markdown
Contributor Author

AspirinSJL commented May 23, 2018

I've verified this reversion fixes the data race. Still don't understand why though.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented May 23, 2018 

[microbenchmarks] Performance differences noted:
Benchmark                                                                                    atm_cas_per_iteration
-------------------------------------------------------------------------------------------  -----------------------
BM_StreamingPingPongWithCoalescingApi<InProcess, NoOpMutator, NoOpMutator>/134217728/1/0     +4%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/134217728/1/1  +4%
BM_UnaryPingPong<InProcess, NoOpMutator, NoOpMutator>/134217728/134217728                    +4%
BM_UnaryPingPong<MinInProcess, NoOpMutator, NoOpMutator>/134217728/134217728                 +4%

jtattermusch
jtattermusch approved these changes May 23, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM.

@AspirinSJL AspirinSJL merged commit c387458 into grpc:master May 23, 2018
@AspirinSJL AspirinSJL deleted the revert_arena_fix branch May 23, 2018 23:54
@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented May 24, 2018

I did a quick run on upstream/master and after reverting the issue is gone.

@AspirinSJL AspirinSJL restored the revert_arena_fix branch May 25, 2018 00:10
@AspirinSJL AspirinSJL deleted the revert_arena_fix branch May 25, 2018 00:10
@AspirinSJL AspirinSJL mentioned this pull request May 25, 2018
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jtattermusch jtattermusch jtattermusch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AspirinSJL @grpc-testing @jtattermusch