Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 3 months ago by honghaiz3 Modified:
5 years, 3 months ago Reviewers:
guoweis_webrtc, 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. |
Description1. Add receiving state as part of the connection sorting criteria. So if a connection's receiving state changes, it will re-select a better connection if there is any.
This will paves the way for continuous nomination lite and multi-networking.
2. Combined checking and pinging to remove some redundant checking and to make it switch to more frequent ping mode earlier.
Committed: https://crrev.com/a58ea7806a039c14e1f92a0757123062963b44b1
Cr-Commit-Position: refs/heads/master@{#10057}
Patch Set 1 : #
Total comments: 48
Patch Set 2 : #
Total comments: 11
Patch Set 3 : Merge and address comments #
Total comments: 12
Patch Set 4 : Dropping changes on pruning and write_stale state. #
Total comments: 16
Patch Set 5 : #Patch Set 6 : Merge with transport controller refractoring #Patch Set 7 : Merge with the head #
Created: 5 years, 3 months ago
Messages
Total messages: 72 (46 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:160001) has been deleted
honghaiz@webrtc.org changed reviewers: + guoweis@webrtc.org, pthatcher@webrtc.org
Working on the unittests. Will be great if you have any early comments.
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #2 (id:240001) has been deleted
This is going to need a lot of unit tests :). https://codereview.webrtc.org/1311433009/diff/220001/talk/app/webrtc/statstyp... File talk/app/webrtc/statstypes.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/talk/app/webrtc/statstyp... talk/app/webrtc/statstypes.cc:557: // TODO(honghaiz): renaming to googReceiving? No, let's just leave it googReadable. We can change it to something non-goog someday. But for now, let's just leave it. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:25: enum { MSG_SORT = 1, MSG_CHECK_AND_PING }; Why did you combine these? I think it made sense to be separate. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:34: static const uint32 SHORT_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000; // 50ms I think a good name for these would be WEAK_CONNECTION_PING_DELAY and STRONG_CONNECTION_PING_DELAY, and then comment that STRONG == writable && receiving, and WEAK == !writable || !receiving. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:75: // its writable state. It doesn't need to just be "backup". We could simply say that a receiving connection is better than a non-receiving one. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:85: return -1; So what's better? receiving-but-non-writable or writable-but-not-receiving? What you have is that receiving-but-non-writable is better. I think writable-but-not-receiving should be better because then if there are 2 seconds of no packets, we wouldn't switch to a non-writable connection. If I'm write, the receiving comparison should come after the write state comparison. What do you think? https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:174: // regardless of the nominate state. Otherwise, do not switch if |a_conn| is nominate => nominated https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:961: // not receiving but the top connection is not writable. It seems like, instead, that we should always go into "high speed pinging" whenever we are "weak" in our connectivity, rather than wait until we have a top connection to ping. I think that would be a lot more simple. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:971: next_connection_to_ping_ = top_connection; Shouldn't the faster pinging work fine without storing next_connection_to_ping_? https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1028: // in the network will time out and be deleted. I think this would be more clear as "once on connection is receiving, we begin pruning other connections on the same network". https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1037: if (!(primier && primier->receiving() && primier->connected())) { Why don't we want to keep it as writable()? Shouldn't we wait until a connection is writable before pruning other connections? https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1080: bool receiving = best_connection_ && best_connection_->receiving(); Shouldn't the logic here be the same as the previous logic: if *any* connection is receiving, the channel is? https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1155: // delay. not receiving or writable => not receiving or not writable https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1156: ping_delay_ = writable() && receiving() ? LONG_PING_DELAY : SHORT_PING_DELAY; We might even consider making a method bool weak() { return !writable() && !receiving(); } Then you'd have delay = weak() ? WEAK_CONNECTION_PING_DELAY : STRONG_CONNECTION_PING_DELAY; https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1158: if (now >= last_ping_sent_ + ping_delay_) { Might be more readable as: uint32 time_since_last_ping = rtc::Time() - last_ping_sent_; if (time_since_last_ping > delay) { ... } https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1165: thread()->PostDelayed(check_delay, this, MSG_CHECK_AND_PING); I think this was more clear before with two "loops" running. Was there a good reason to combine them? https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1186: Might be more readable as: // If the channel is weak, then ping pruned candidates. Otherwise, don't. return weak() || !conn->pruned(); https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:902: // If this connection received ping before, then pass along the packet. received ping => receive a ping https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:903: // If the remote side is ICE-LITE, it may never receive ping, but it needs receive ping => receive a ping https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:904: // to receive ping response before receiving data packet. receive ping => receive a ping receive data packet => receive a data packet https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:905: if (last_ping_received_ != 0 || last_ping_response_received_) { I don't understand why we even bother to check these. If we've receive a data packet form the remote side, why would we throw it away? Even if we are making this a refactoring with no behavior change, wouldn't we make it check only the last_ping_received_, not the last_ping_response_received_? https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1013: requests_.Clear(); There's a lot going on in one CL. Could we break this up into two CLs? 1. Remove readable state and use receiving instead. 2. Change behavior of write state, pruning, etc. I think that would make the diffs easier to understand, and make the CLs less risky. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1097: uint32 last_recv_time = last_received(); Might make sense to write it as: uint32 time_since_last_received = now - last_received(); if (receiving_ && time_since_last_received > receiving_timeout_) { set_receiving(false); } if (time_since_last_received > DEAD) { Destroy(); } https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:54: const uint32 CONNECTION_RECEIVE_LONG_TIMEOUT = 30 * 1000; // 30 seconds. I'd call these something like: WEAK_CONNECTION_RECEIVING_TIMEOUT DEAD_CONNECTION_RECEIVING_TIMEOUT https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/transpor... File webrtc/p2p/base/transportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/transpor... webrtc/p2p/base/transportchannel.cc:30: LOG(LS_INFO) << "XXX Set receiving " << receiving; XXX?
Patchset #2 (id:260001) has been deleted
Patchset #2 (id:280001) has been deleted
I created a new CL for the receiving state change only but this one will depend on that. I fixed code based on the comments. Please hold this for a while until the other is mostly approved. I send this in case you want to see my responses. Thanks! https://codereview.webrtc.org/1311433009/diff/220001/talk/app/webrtc/statstyp... File talk/app/webrtc/statstypes.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/talk/app/webrtc/statstyp... talk/app/webrtc/statstypes.cc:557: // TODO(honghaiz): renaming to googReceiving? On 2015/09/17 05:58:41, pthatcher1 wrote: > No, let's just leave it googReadable. We can change it to something non-goog > someday. But for now, let's just leave it. Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:25: enum { MSG_SORT = 1, MSG_CHECK_AND_PING }; On 2015/09/17 05:58:42, pthatcher1 wrote: > Why did you combine these? I think it made sense to be separate. There are two reasons. 1. When you do ping, you also check the connections states, which makes it sort of redundant for checking (except for the frequency). 2. When we check and fing the connection not receiving, we want to switch to a short ping period without waiting another ping coming. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:34: static const uint32 SHORT_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000; // 50ms On 2015/09/17 05:58:41, pthatcher1 wrote: > I think a good name for these would be WEAK_CONNECTION_PING_DELAY and > STRONG_CONNECTION_PING_DELAY, and then comment that STRONG == writable && > receiving, and WEAK == !writable || !receiving. Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:75: // its writable state. On 2015/09/17 05:58:42, pthatcher1 wrote: > It doesn't need to just be "backup". We could simply say that a receiving > connection is better than a non-receiving one. Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:174: // regardless of the nominate state. Otherwise, do not switch if |a_conn| is On 2015/09/17 05:58:42, pthatcher1 wrote: > nominate => nominated Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:961: // not receiving but the top connection is not writable. On 2015/09/17 05:58:41, pthatcher1 wrote: > It seems like, instead, that we should always go into "high speed pinging" > whenever we are "weak" in our connectivity, rather than wait until we have a top > connection to ping. I think that would be a lot more simple. Yes. That is what is done. Combining the comments, I am removing this extra part and sort connections based on write state first and then receiving. This is also safer. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:971: next_connection_to_ping_ = top_connection; On 2015/09/17 05:58:42, pthatcher1 wrote: > Shouldn't the faster pinging work fine without storing next_connection_to_ping_? Yes. We don't need the extra speedup of the ping request. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1028: // in the network will time out and be deleted. On 2015/09/17 05:58:42, pthatcher1 wrote: > I think this would be more clear as "once on connection is receiving, we begin > pruning other connections on the same network". Removed the comments as we change back to check writable. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1037: if (!(primier && primier->receiving() && primier->connected())) { On 2015/09/17 05:58:41, pthatcher1 wrote: > Why don't we want to keep it as writable()? Shouldn't we wait until a > connection is writable before pruning other connections? Yes. especially now that we put writable with higher priority then receiving https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1080: bool receiving = best_connection_ && best_connection_->receiving(); On 2015/09/17 05:58:42, pthatcher1 wrote: > Shouldn't the logic here be the same as the previous logic: if *any* connection > is receiving, the channel is? Done. I was thinking that we should use the best_connection's receiving to decide certain actions (notify upper layer and switch to the fast ping mode) but I also see the reasons for checking any connection. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1155: // delay. On 2015/09/17 05:58:41, pthatcher1 wrote: > not receiving or writable => not receiving or not writable Done with neither nor. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1156: ping_delay_ = writable() && receiving() ? LONG_PING_DELAY : SHORT_PING_DELAY; On 2015/09/17 05:58:41, pthatcher1 wrote: > We might even consider making a method > > bool weak() { return !writable() && !receiving(); } > > Then you'd have delay = weak() ? WEAK_CONNECTION_PING_DELAY : > STRONG_CONNECTION_PING_DELAY; Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1158: if (now >= last_ping_sent_ + ping_delay_) { On 2015/09/17 05:58:41, pthatcher1 wrote: > Might be more readable as: > > uint32 time_since_last_ping = rtc::Time() - last_ping_sent_; > if (time_since_last_ping > delay) { > ... > } Similar to the other comments. I try to avoid subtraction on two unsigned number. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1165: thread()->PostDelayed(check_delay, this, MSG_CHECK_AND_PING); On 2015/09/17 05:58:41, pthatcher1 wrote: > I think this was more clear before with two "loops" running. Was there a good > reason to combine them? For the two reasons I mentioned at the top. If you still feel better to use two loops, I will change it back. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1186: On 2015/09/17 05:58:41, pthatcher1 wrote: > Might be more readable as: > > // If the channel is weak, then ping pruned candidates. Otherwise, don't. > return weak() || !conn->pruned(); Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:902: // If this connection received ping before, then pass along the packet. On 2015/09/17 05:58:42, pthatcher1 wrote: > received ping => receive a ping Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:903: // If the remote side is ICE-LITE, it may never receive ping, but it needs On 2015/09/17 05:58:42, pthatcher1 wrote: > receive ping => receive a ping Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:904: // to receive ping response before receiving data packet. On 2015/09/17 05:58:42, pthatcher1 wrote: > receive ping => receive a ping > receive data packet => receive a data packet Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:905: if (last_ping_received_ != 0 || last_ping_response_received_) { On 2015/09/17 05:58:42, pthatcher1 wrote: > I don't understand why we even bother to check these. If we've receive a data > packet form the remote side, why would we throw it away? > > Even if we are making this a refactoring with no behavior change, wouldn't we > make it check only the last_ping_received_, not the > last_ping_response_received_? Yes the primary purpose is to make it have the same or similar behavior. For ICE_LITE, it set the read state to READABLE when receiving a ping response. So adding the last_ping_response_received is trying to have similar behavior. I agree the checking may not be necessary. But I guess it tries to enforce the protocol that other side must send stun ping to check the writability before sending data. See the bottom of the deleted code in this CL about ICE_LITE. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1013: requests_.Clear(); On 2015/09/17 05:58:42, pthatcher1 wrote: > There's a lot going on in one CL. Could we break this up into two CLs? > > 1. Remove readable state and use receiving instead. > 2. Change behavior of write state, pruning, etc. > > I think that would make the diffs easier to understand, and make the CLs less > risky. It is kind of convoluted. I created a new CL only changing the receiving only. But this CL will depend on that. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1097: uint32 last_recv_time = last_received(); On 2015/09/17 05:58:42, pthatcher1 wrote: > Might make sense to write it as: > > uint32 time_since_last_received = now - last_received(); > if (receiving_ && time_since_last_received > receiving_timeout_) { > set_receiving(false); > } if (time_since_last_received > DEAD) { > Destroy(); > } This saved an operation. But using subtraction on unsigned number may have unexpected overflow/underflow. If now < last_received, now - last_received becomes a big number. Although it should not happen here, it is probably better to just avoid subtraction at all for unsigned numbers. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:54: const uint32 CONNECTION_RECEIVE_LONG_TIMEOUT = 30 * 1000; // 30 seconds. On 2015/09/17 05:58:42, pthatcher1 wrote: > I'd call these something like: > > WEAK_CONNECTION_RECEIVING_TIMEOUT > DEAD_CONNECTION_RECEIVING_TIMEOUT > Done. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/transpor... File webrtc/p2p/base/transportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/transpor... webrtc/p2p/base/transportchannel.cc:30: LOG(LS_INFO) << "XXX Set receiving " << receiving; On 2015/09/17 05:58:42, pthatcher1 wrote: > XXX? Ah Sorry. It was removed in a later patch.
This will be easier to read once it's rebased on the one you split off. https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:971: next_connection_to_ping_ = top_connection; On 2015/09/17 19:47:56, honghaiz3 wrote: > On 2015/09/17 05:58:42, pthatcher1 wrote: > > Shouldn't the faster pinging work fine without storing > next_connection_to_ping_? > Yes. We don't need the extra speedup of the ping request. So are you going to remove next_connection_to_ping_? https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:38: static const uint32 WEAK_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000; Are the comments backwards? I thought WEAK was for when it's not writable nor receiving and STRONG is for when it is. https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:291: std::set<rtc::Network*> networks; This is more like networks_with_unpruned_connections now. https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1030: conn->Prune(true); What does "true" here mean? https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1069: } Can you use std::any_of? That might make this easier to read. Something like return std::any_of(connections_.begin(), connections_.end(), [] (const Connection* c) { return c->receiving() }); https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:173: bool weak() const; Needs a comment.
Patchset #3 (id:320001) has been deleted
Patchset #3 (id:340001) has been deleted
Patchset #3 (id:360001) has been deleted
Patchset #3 (id:380001) has been deleted
https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:38: static const uint32 WEAK_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000; On 2015/09/17 22:01:17, pthatcher1 wrote: > Are the comments backwards? I thought WEAK was for when it's not writable nor > receiving and STRONG is for when it is. WEAK means SHORT and STRONG means LONG. I feel using WEAK and STRONG to describe the delay may be confusing. It might be better to use LONG and SHORT and add comments to describe it. Let me know if you'd like to change it back (or something else). https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:291: std::set<rtc::Network*> networks; On 2015/09/17 22:01:17, pthatcher1 wrote: > This is more like networks_with_unpruned_connections now. Done. https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1030: conn->Prune(true); On 2015/09/17 22:01:17, pthatcher1 wrote: > What does "true" here mean? true means prune it. false means un-prune it. I think in some cases, we will need to unprune it. If ep1 has two networks and ep2 has one network. ep2 will prune the backup connection. When we switch to the backup connection, ep2 should un-prune the backup connection, so that it will ping it. https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1069: } On 2015/09/17 22:01:17, pthatcher1 wrote: > Can you use std::any_of? That might make this easier to read. > > Something like > > return std::any_of(connections_.begin(), connections_.end(), [] (const > Connection* c) { return c->receiving() }); Done. https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:173: bool weak() const; On 2015/09/17 22:01:17, pthatcher1 wrote: > Needs a comment. Done.
Patchset #3 (id:400001) has been deleted
Patchset #3 (id:420001) has been deleted
Patchset #3 (id:440001) has been deleted
Patchset #3 (id:460001) has been deleted
https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/300001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1030: conn->Prune(true); On 2015/09/21 22:53:41, honghaiz3 wrote: > On 2015/09/17 22:01:17, pthatcher1 wrote: > > What does "true" here mean? > > true means prune it. > false means un-prune it. > I think in some cases, we will need to unprune it. > If ep1 has two networks and ep2 has one network. > ep2 will prune the backup connection. > When we switch to the backup connection, ep2 should un-prune the backup > connection, so that it will ping it. I re-named these to Prune and UnPrune for better readability.
I think we need some more unit tests and some more clear documentation at the top of the file explaining the state transitions. https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:162: // |connections_| should not be changed from outside. from outside => from the outside. But if it's marked const, you can't anyway, right? https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:258: uint32 last_ping_sent_; Can you make it clear this is a timestamp, perhaps with last_ping_sent_ms_? Also, you can do this: uint32 last_ping_sent_ = 0; https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:915: Unprune(); if (!pruned_) Unprune()? Why would we unprune if we're not pruned? https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1054: // If the connection has not sent stun ping for a while (mostly because sent stun ping => sent a stun ping https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1162: 's', // STATE_WRITE_STALE Maybe a '?' would be good, since it means we don't know since we're not probing any more. https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:432: STATE_WRITE_TIMEOUT = 4, // we have had a large number of ping failures So the only difference between STALE and UNRELIABLE is whether we are sending them or not? I wonder if we can think of a better name here, especially when related to the STRONG/WEAK analogy with receiving.
Patchset #5 (id:520001) has been deleted
Patchset #5 (id:540001) has been deleted
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/1311433009/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311433009/560001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Patchset #5 (id:560001) has been deleted
Patchset #6 (id:590001) has been deleted
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/1311433009/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311433009/610001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
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/1311433009/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311433009/610001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run
Patchset #5 (id:580001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311433009/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311433009/650001
Patchset #5 (id:610001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Patchset #5 (id:630001) has been deleted
Patchset #4 (id:500001) has been deleted
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/1311433009/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311433009/650001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Removed the changes on pruning and write_stale state. The only thing left now is that we add receiving as part of the connection comparison and it will switch connections if the receiving state of the best connection changed. Added tests for the switching. Reverted the change of using any_of because it broke some unittests on some platform. Guess not all platforms support that function yet. https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:162: // |connections_| should not be changed from outside. On 2015/09/21 23:49:18, pthatcher1 wrote: > from outside => from the outside. > > But if it's marked const, you can't anyway, right? Right, don't need the comments. https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:258: uint32 last_ping_sent_; On 2015/09/21 23:49:18, pthatcher1 wrote: > Can you make it clear this is a timestamp, perhaps with last_ping_sent_ms_? > > Also, you can do this: > > uint32 last_ping_sent_ = 0; Done. https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:915: Unprune(); On 2015/09/21 23:49:18, pthatcher1 wrote: > if (!pruned_) Unprune()? > > Why would we unprune if we're not pruned? If it is not pruned, then you don't need to unprune it. This makes it similar to the original behavior that when a connection receives a data packet, it will be pulled out of the pruned state, so it will be available for pinging again. https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1054: // If the connection has not sent stun ping for a while (mostly because On 2015/09/21 23:49:18, pthatcher1 wrote: > sent stun ping => sent a stun ping Done. https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1162: 's', // STATE_WRITE_STALE On 2015/09/21 23:49:18, pthatcher1 wrote: > Maybe a '?' would be good, since it means we don't know since we're not probing > any more. Dropped this (saved for a separate CL). https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1311433009/diff/480001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:432: STATE_WRITE_TIMEOUT = 4, // we have had a large number of ping failures On 2015/09/21 23:49:18, pthatcher1 wrote: > So the only difference between STALE and UNRELIABLE is whether we are sending > them or not? I wonder if we can think of a better name here, especially when > related to the STRONG/WEAK analogy with receiving. Dropped this change now.
Mostly little things. I'm mostly worried about the loss in precision of when we notice that the connection is not receiving. Why not just make the check_and_ping frequency always 50ms? https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:87: return -1; Can you put a comment along the lines of "We prefer a receiving connection over a higher-priority non-receiving connection when choosing which connection to ping and which to switch to"? https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:178: // |a_conn| is nominated on the controlled side. Can you reword this as something like "We prefer to switch to a writable and receiving connection over a non-writable or non-receiving connection, even if the non-writable or non-receiving connection has been nominated by the controlling side"? https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1069: } I thought this was more readable using std::any_of. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1104: return !(best_connection_ && best_connection_->receiving() && writable()); Isn't the same as "!(best_connection_ && best_connection_->receiving() && best_connection_->writable());"? If so, it's more readable. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1161: } So we only check for receiving() every 500ms? Doesn't that make the receive timeout less precise? Shouldn't we check receiving() more often than that so that we know right after the timeout period that we're not receiving? https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1526: !best_connection1->receiving() && !best_connection2->receiving(), 3000); See my comments below and please change this place as well. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1578: !best_connection1->receiving() && !best_connection2->receiving(), 3000); Can you lower the receive timeout for the tests to make them run faster? And can you bump the expect timeout to make sure this test doesn't go flaky? https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1588: EXPECT_TRUE(best_connection2->writable()); Do you really need to check this? It doesn't matter right? And if a test bot gets really slow, this might be false by the time you get here.
Patchset #5 (id:670001) has been deleted
PTAL. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:87: return -1; On 2015/09/22 23:41:46, pthatcher1 wrote: > Can you put a comment along the lines of "We prefer a receiving connection over > a higher-priority non-receiving connection when choosing which connection to > ping and which to switch to"? Done. I do not see that the pinging order is related to the connection sorting based on this, so I did not include that part. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:178: // |a_conn| is nominated on the controlled side. On 2015/09/22 23:41:46, pthatcher1 wrote: > Can you reword this as something like "We prefer to switch to a writable and > receiving connection over a non-writable or non-receiving connection, even if > the non-writable or non-receiving connection has been nominated by the > controlling side"? Done. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1069: } On 2015/09/22 23:41:46, pthatcher1 wrote: > I thought this was more readable using std::any_of. Using that caused the compiling to break on some Mac platform. So I switched it back. See here http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/9633/ste... https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1104: return !(best_connection_ && best_connection_->receiving() && writable()); On 2015/09/22 23:41:46, pthatcher1 wrote: > Isn't the same as "!(best_connection_ && best_connection_->receiving() && > best_connection_->writable());"? If so, it's more readable. Done. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1161: } On 2015/09/22 23:41:46, pthatcher1 wrote: > So we only check for receiving() every 500ms? Doesn't that make the receive > timeout less precise? Shouldn't we check receiving() more often than that so > that we know right after the timeout period that we're not receiving? We took the minimum of the ping_delay and check_receiving_delay. So it is smaller than that. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1526: !best_connection1->receiving() && !best_connection2->receiving(), 3000); On 2015/09/22 23:41:46, pthatcher1 wrote: > See my comments below and please change this place as well. Done. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1578: !best_connection1->receiving() && !best_connection2->receiving(), 3000); On 2015/09/22 23:41:46, pthatcher1 wrote: > Can you lower the receive timeout for the tests to make them run faster? And > can you bump the expect timeout to make sure this test doesn't go flaky? I set the receive timeout to 1 second. Because of that, I think we do not need to increase the expect timeout any more. https://codereview.webrtc.org/1311433009/diff/650001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1588: EXPECT_TRUE(best_connection2->writable()); On 2015/09/22 23:41:46, pthatcher1 wrote: > Do you really need to check this? It doesn't matter right? And if a test bot > gets really slow, this might be false by the time you get here. Right. removed these two lines here and above.
lgtm, Although the TransportController CL was reverted again, so you'll have to wait until it lands again.
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/1311433009/730001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311433009/730001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/9521)
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/1311433009/#ps730001 (title: " Merge with the head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311433009/730001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311433009/730001
Message was sent while issue was closed.
Committed patchset #7 (id:730001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a58ea7806a039c14e1f92a0757123062963b44b1 Cr-Commit-Position: refs/heads/master@{#10057} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
