Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1406423008:
Stop a session when a new connection becomes writable. (Closed)
|
Created:
5 years, 2 months ago by honghaiz3 Modified:
5 years, 1 month ago Reviewers:
pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, juberti1 Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionStop a session when a new connection becomes writable.
We cannot do it at the end of sorting because it may stop a session too early.
Also remove was_writable_, which is not useful.
BUG=webrtc:5119
Committed: https://crrev.com/9b66957b4f7dfe40c17eed597826b3ed0faf5439
Cr-Commit-Position: refs/heads/master@{#10511}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #Patch Set 3 : Merge and changed variable name #Patch Set 4 : Removing the state variable has_stopped_sessions_ #
Total comments: 8
Patch Set 5 : Address comments #
Messages
Total messages: 25 (11 generated)
Description was changed from ========== Stop a session when a new connection becomes writable. We cannot do it at the end of sorting because it may stop a session too early. BUG= ========== to ========== Stop a session when a new connection becomes writable. We cannot do it at the end of sorting because it may stop a session too early. Also remove was_writable_, which is not useful. BUG=webrtc:5119 ==========
Patchset #1 (id:1) has been deleted
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
Patchset #1 (id:20001) has been deleted
https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1074: void P2PTransportChannel::MaybeStopSessions() { Should be MaybeStopPortAllocatorSessions https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1078: is_getting_ports_ = false; How is is_getting_ports_ going to be different than session->IsGettingPorts()? If it's the same, then this method doesn't do anything because it checks if !session->IsGettingPorts(). So we wouldn't need is_getting_ports_. https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1289: if (!connection->weak()) { Can you document more clearly why connection->writable() isn't enough? https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2163: TEST_F(P2PTransportChannelPingTest, TestStopSessions) { Can you leave a comment about what this is testing, and perhaps make "TestStopSessions"? Basically, explain that if an ICE restart happens while we're still writable, we need to stop gathering.
PTAL. https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1074: void P2PTransportChannel::MaybeStopSessions() { On 2015/10/24 00:39:04, pthatcher1 wrote: > Should be MaybeStopPortAllocatorSessions Done. https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1078: is_getting_ports_ = false; On 2015/10/24 00:39:04, pthatcher1 wrote: > How is is_getting_ports_ going to be different than session->IsGettingPorts()? > > If it's the same, then this method doesn't do anything because it checks if > !session->IsGettingPorts(). So we wouldn't need is_getting_ports_. It is pretty much the same except for the last session which is not stopped but cleared. For correctness, is_geeting_ports_ is not needed. But it stopped the rest from being called multiple times (every time a connection becomes strong). Its effect is similar to the check of (writable()) in the original code. If it is removed, it will call ClearGettingPorts on the last session every time (if continual gathering is enabled), although it does not do much except for wasting some computing cycles for the current code. https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1289: if (!connection->weak()) { On 2015/10/24 00:39:04, pthatcher1 wrote: > Can you document more clearly why connection->writable() isn't enough? Done. https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2163: TEST_F(P2PTransportChannelPingTest, TestStopSessions) { On 2015/10/24 00:39:04, pthatcher1 wrote: > Can you leave a comment about what this is testing, and perhaps make > "TestStopSessions"? Basically, explain that if an ICE restart happens while > we're still writable, we need to stop gathering. Done. Not sure what do you mean by "make TestStopSessions".
The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406423008/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406423008/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/8919) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/697) mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/4829)
https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1078: is_getting_ports_ = false; On 2015/10/26 16:47:46, honghaiz3 wrote: > On 2015/10/24 00:39:04, pthatcher1 wrote: > > How is is_getting_ports_ going to be different than session->IsGettingPorts()? > > > > If it's the same, then this method doesn't do anything because it checks if > > !session->IsGettingPorts(). So we wouldn't need is_getting_ports_. > It is pretty much the same except for the last session which is not stopped but > cleared. > For correctness, is_geeting_ports_ is not needed. > But it stopped the rest from being called multiple times (every time a > connection becomes strong). > Its effect is similar to the check of (writable()) in the original code. > If it is removed, it will call ClearGettingPorts on the last session every time > (if continual gathering is enabled), although it does not do much except for > wasting some computing cycles for the current code. OK, could we make this a little cleaner by doing something like this? bool P2pTransportChannel::IsGettingPorts() { for (PortAllocatorSession* session : allocator_sessions_) { if (session->IsGettingPorts()) { return true; } } } void P2PTransportChannel::MaybeStopSessions() { if (!IsGettingPorts()) { return; } ... }
Patchset #3 (id:80001) has been deleted
https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1078: is_getting_ports_ = false; On 2015/10/27 07:24:04, pthatcher1 wrote: > On 2015/10/26 16:47:46, honghaiz3 wrote: > > On 2015/10/24 00:39:04, pthatcher1 wrote: > > > How is is_getting_ports_ going to be different than > session->IsGettingPorts()? > > > > > > If it's the same, then this method doesn't do anything because it checks if > > > !session->IsGettingPorts(). So we wouldn't need is_getting_ports_. > > It is pretty much the same except for the last session which is not stopped > but > > cleared. > > For correctness, is_geeting_ports_ is not needed. > > But it stopped the rest from being called multiple times (every time a > > connection becomes strong). > > Its effect is similar to the check of (writable()) in the original code. > > If it is removed, it will call ClearGettingPorts on the last session every > time > > (if continual gathering is enabled), although it does not do much except for > > wasting some computing cycles for the current code. > > OK, could we make this a little cleaner by doing something like this? > > bool P2pTransportChannel::IsGettingPorts() { > for (PortAllocatorSession* session : allocator_sessions_) { > if (session->IsGettingPorts()) { > return true; > } > } > } > > void P2PTransportChannel::MaybeStopSessions() { > if (!IsGettingPorts()) { > return; > } > ... > } The problem is that whenever Continual Gathering is enabled, IsGettingPorts will always return true on the last session. So we will keep on running StopSession() for every session whenever a new connection becomes writable. The purpose of the variable is to remember whether we have already attempted to stop sessions. Changed the variable name and hopefully make it less confusing.
On 2015/10/27 16:59:42, honghaiz3 wrote: > https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... > File webrtc/p2p/base/p2ptransportchannel.cc (right): > > https://codereview.webrtc.org/1406423008/diff/40001/webrtc/p2p/base/p2ptransp... > webrtc/p2p/base/p2ptransportchannel.cc:1078: is_getting_ports_ = false; > On 2015/10/27 07:24:04, pthatcher1 wrote: > > On 2015/10/26 16:47:46, honghaiz3 wrote: > > > On 2015/10/24 00:39:04, pthatcher1 wrote: > > > > How is is_getting_ports_ going to be different than > > session->IsGettingPorts()? > > > > > > > > If it's the same, then this method doesn't do anything because it checks > if > > > > !session->IsGettingPorts(). So we wouldn't need is_getting_ports_. > > > It is pretty much the same except for the last session which is not stopped > > but > > > cleared. > > > For correctness, is_geeting_ports_ is not needed. > > > But it stopped the rest from being called multiple times (every time a > > > connection becomes strong). > > > Its effect is similar to the check of (writable()) in the original code. > > > If it is removed, it will call ClearGettingPorts on the last session every > > time > > > (if continual gathering is enabled), although it does not do much except for > > > wasting some computing cycles for the current code. > > > > OK, could we make this a little cleaner by doing something like this? > > > > bool P2pTransportChannel::IsGettingPorts() { > > for (PortAllocatorSession* session : allocator_sessions_) { > > if (session->IsGettingPorts()) { > > return true; > > } > > } > > } > > > > void P2PTransportChannel::MaybeStopSessions() { > > if (!IsGettingPorts()) { > > return; > > } > > ... > > } > The problem is that whenever Continual Gathering is enabled, IsGettingPorts will > always return true on the last session. > So we will keep on running StopSession() for every session whenever a new > connection becomes writable. The purpose of the variable is to remember whether > we have already attempted to stop sessions. Changed the variable name and > hopefully make it less confusing. Have a second thought. The overhead of calling the second half of MaybeStopSessions may not be that big. So I created patch #4 following your suggestions, except that we only need to check the last allocator session. It should never happen that the last session was stopped and any previous one was not. Please let me know which looks better, patch #3 or #4.
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Note, both patch 3 and 4 should work. Patch 3 added a variable to check if the sessions have been stopped and patch 4 did not have that variable but may need more checking and computation (especially if CG is enabled).
lgtm, with comment nits https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1296: // May stop the session after at least one connection becomes strong the session => allocator sessions https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1296: // May stop the session after at least one connection becomes strong strong => strongly connected https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1298: // becomes writable because the connection may be changing from writable => weakly connected https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2211: // It should stop getting ports after a new connection becomes strong. strong => strongly connected
Thanks! https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1296: // May stop the session after at least one connection becomes strong On 2015/11/04 15:06:14, pthatcher1 wrote: > strong => strongly connected Done. https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1296: // May stop the session after at least one connection becomes strong On 2015/11/04 15:06:14, pthatcher1 wrote: > strong => strongly connected Done. https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1298: // becomes writable because the connection may be changing from On 2015/11/04 15:06:14, pthatcher1 wrote: > writable => weakly connected Done. https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1406423008/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2211: // It should stop getting ports after a new connection becomes strong. On 2015/11/04 15:06:14, pthatcher1 wrote: > strong => strongly connected Done.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1406423008/#ps180001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1406423008/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1406423008/180001
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9b66957b4f7dfe40c17eed597826b3ed0faf5439 Cr-Commit-Position: refs/heads/master@{#10511} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
