Skip to content

Restore pick-after-resolver-result logic from before #12297.#13070

Merged
markdroth merged 1 commit intogrpc:masterfrom
markdroth:client_channel_lb_policy_ref_fix
Oct 24, 2017
Merged

Restore pick-after-resolver-result logic from before #12297.#13070
markdroth merged 1 commit intogrpc:masterfrom
markdroth:client_channel_lb_policy_ref_fix

Conversation

@markdroth
Copy link
Copy Markdown
Member

@markdroth markdroth commented Oct 19, 2017

No description provided.

@markdroth markdroth requested a review from muxi October 19, 2017 18:05
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Oct 19, 2017 

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

libgrpc.so

     VM SIZE                                                             FILE SIZE
 ++++++++++++++ GROWING                                               ++++++++++++++
  +0.0%    +200 [None]                                                +1.15Ki  +0.0%
  +2.4%    +320 src/core/ext/filters/client_channel/client_channel.cc    +320  +2.4%
      +141%    +348 pick_after_resolver_result_done_locked                   +348  +141%
      [NEW]    +189 pick_after_resolver_result_start_locked                  +189  [NEW]
      +5.4%     +12 [Unmapped]                                                +12  +5.4%

  +0.0%    +520 TOTAL                                                 +1.46Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Oct 19, 2017 

[trickle] No significant performance differences

muxi
muxi approved these changes Oct 19, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@muxi muxi left a comment

Choose a reason for hiding this comment

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

LGTM.

src/core/ext/filters/client_channel/client_channel.cc Outdated
Copy link
Copy Markdown
Contributor

@muxi muxi Oct 19, 2017

Choose a reason for hiding this comment

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

I wonder if breakpad is able to make a report even if the program do not crash. If so we should probably set it up here.

Copy link
Copy Markdown
Member Author

@markdroth markdroth Oct 19, 2017

Choose a reason for hiding this comment

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

I have no idea. Can you investigate and tell me what, if anything, you need me to do?

Copy link
Copy Markdown
Contributor

@muxi muxi Oct 19, 2017

Choose a reason for hiding this comment

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

No. Current change is good as it is. I am just dumping idea for further investigation. Asked breakpad team about that and I'll investigate more.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Oct 19, 2017 

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__32k_2_0.counters.new: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__8_2_0.opt.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__2M_2_0.opt.new: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__0_2_0.opt.old: 4
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__64_2_0.opt.old: 4
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__2M_1_0.opt.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__0_2_0.counters.old: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__512_2_0.counters.new: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__512_2_0.counters.new: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__1_2_0.opt.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__2M_2_0.counters.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__64_2_0.counters.new: 3
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__1_2_0.counters.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__2M_1_0.counters.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__8_2_0.counters.old: 3
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__4k_2_0.counters.old: 4
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__8_2_0.counters.old: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__4k_2_0.opt.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__4k_2_0.opt.new: 7
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__4k_2_0.counters.new: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__512_2_0.opt.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__512_2_0.opt.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__1_2_0.opt.new: 6
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__0_2_0.counters.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__64_2_0.opt.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__8_2_0.opt.old: 5
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__512_2_0.opt.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__1_2_0.opt.old: 4
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__4k_2_0.opt.old: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__0_2_0.counters.new: 3
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__1_2_0.counters.old: 4
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__0_2_0.opt.new: 4
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__32k_2_0.counters.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__8_2_0.counters.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__64_2_0.opt.new: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__512_2_0.counters.old: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__512_2_0.opt.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__8_2_0.counters.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__1_2_0.counters.old: 3
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__32k_2_0.counters.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__0_2_0.counters.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__0_2_0.opt.new: 3
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__64_2_0.counters.old: 4
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__4k_2_0.counters.old: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__8_2_0.opt.new: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__1_2_0.counters.new: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__0_2_0.opt.old: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__64_2_0.opt.old: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__64_2_0.counters.new: 5
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__4k_2_0.opt.old: 2
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__4k_2_0.counters.new: 3
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__512_2_0.counters.old: 3


[microbenchmarks] No significant performance differences

@markdroth markdroth force-pushed the client_channel_lb_policy_ref_fix branch from 660ecf2 to 48be9de Compare October 23, 2017 19:27
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Oct 23, 2017 

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

libgrpc.so

     VM SIZE                                                             FILE SIZE
 ++++++++++++++ GROWING                                               ++++++++++++++
  +0.0%    +200 [None]                                                +1.14Ki  +0.0%
  +2.4%    +320 src/core/ext/filters/client_channel/client_channel.cc    +320  +2.4%
      +141%    +348 pick_after_resolver_result_done_locked                   +348  +141%
      [NEW]    +189 pick_after_resolver_result_start_locked                  +189  [NEW]
      +5.2%     +12 [Unmapped]                                                +12  +5.2%

  +0.0%    +520 TOTAL                                                 +1.45Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Oct 23, 2017 

[trickle] No significant performance differences

@markdroth
Copy link
Copy Markdown
Member Author

markdroth commented Oct 24, 2017

Known issues: #12913 #12510 #13081 #12886 #12918 #13132 #13131 #13085 #12932

The objc failures are fixed in #13094.

@markdroth markdroth merged commit c263204 into grpc:master Oct 24, 2017
@markdroth markdroth deleted the client_channel_lb_policy_ref_fix branch October 24, 2017 17:52
@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

@dgquintas dgquintas Awaiting requested review from dgquintas

+1 more reviewer

@muxi muxi muxi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@markdroth @grpc-testing @muxi @ctiller @googlebot