Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1944003002:
Increase the stun ping interval. (Closed)
DescriptionIncrease the stun ping interval.
Writable connections are pinged at a slower rate.
The function IsPingable will filter out the writable connections.
The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration.
BUG=webrtc:1161
Committed: https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0
Committed: https://crrev.com/435264a183e16572c99e8dd46171163b6fd33d59
Cr-Original-Commit-Position: refs/heads/master@{#12736}
Cr-Commit-Position: refs/heads/master@{#13241}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Changes based on the code review. #
Total comments: 4
Patch Set 3 : Remove the hard code in IsPingable(); Renaming. #Patch Set 4 : typo #
Total comments: 1
Patch Set 5 : Remove useless comments. #Patch Set 6 : First Serveral pings are sent at faster rate. #
Total comments: 9
Patch Set 7 : Merge with Issue 2009763002. #
Total comments: 17
Patch Set 8 : Modified IsPingable(). #Patch Set 9 : Minor format change. #
Total comments: 4
Patch Set 10 : Add a helper function to calculate ping interval. #
Total comments: 4
Patch Set 11 : Add unit test with fake clock. #
Total comments: 2
Patch Set 12 : Modified TestInitialPingInterval. #
Total comments: 12
Patch Set 13 : Merge the unit tests. Add stable() function to Connection. #Patch Set 14 : Fix minor mistakes. #
Total comments: 2
Patch Set 15 : Fix naming issue in unit test before committing. #
Messages
Total messages: 60 (19 generated)
zhihuang@webrtc.org changed reviewers: + deadbeef@webrtc.org, honghaiz@webrtc.org, pthatcher@webrtc.org
https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:1285: if (rtc::TimeMillis() >= last_ping_sent_ms_ + weak_ping_interval_) { Now after I see this, I realize it will ping all connections with a fast rate until either they time out or become writable, which is good and bad. This prevents the issue of missing finding a high-priority connection although it may ping an un-connectable high-priority connection too often. https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:1331: if (conn->writable()) { You may need to move this after the backupconnection check. Otherwise, once a backup connection is writable, it will keep on being pinged at strong_ping_interval.
https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:1285: if (rtc::TimeMillis() >= last_ping_sent_ms_ + weak_ping_interval_) { On 2016/05/03 19:15:22, honghaiz3 wrote: > Now after I see this, I realize it will ping all connections with a fast rate > until either they time out or become writable, which is good and bad. > This prevents the issue of missing finding a high-priority connection although > it may ping an un-connectable high-priority connection too often. I think you remove the if condition here and take the check_and_ping delay as a min of weak_ping_interval_ and checking_receiving_interval_.
https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:215: static const int STRONG_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 200; I don't think the strong ping interval should change. That means that if one connection becomes writable, other connections will only be pinged every ~5 seconds. https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:223: static const int MAX_CURRENT_STRONG_INTERVAL = 4500; // ms This can probably change to 2500 though. https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:1285: if (rtc::TimeMillis() >= last_ping_sent_ms_ + weak_ping_interval_) { I think the strong ping interval (if it stays ~500ms) should still be used here. https://codereview.webrtc.org/1944003002/diff/1/webrtc/p2p/base/p2ptransportc... webrtc/p2p/base/p2ptransportchannel.cc:1332: return now >= conn->last_ping_sent() + STRONG_PING_INTERVAL; If STRONG_PING_INTERVAL is changed back to 500ms, this should use MAX_CURRENT_STRONG_INTERVAL, or some other define that's also 2500ms.
Some changes based on the comments.
Description was changed from ========== Increase the stun ping interval. The strong ping interval is now 2400ms, the max current strong interval is 4500ms. All the connections are supposed to be pinged every 48ms. The function IsPingable will filter out the writable connections which can be pinged in a slower rate. BUG=webrtc:1161 ========== to ========== Increase the stun ping interval. The max current strong interval is 2500ms. The function IsPingable will filter out the writable connections which can be pinged in a slower rate. BUG=webrtc:1161 ==========
https://codereview.webrtc.org/1944003002/diff/20001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:221: // also try hard to make sure it is pinged at this rate. This comment isn't accurate any more, because this rate is now not only used for the current best connection, but also for all writable connections. I like the idea of just saying "Writable connections use this ping interval. Unwritable connections use the faster interval." But in that case, we should rename the constant to WRITABLE_CONNECTION_PING_INTERVAL, and the settable config_ value to config_.writable_connection_ping_interval. https://codereview.webrtc.org/1944003002/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1338: return now >= conn->last_ping_sent() + MAX_CURRENT_STRONG_INTERVAL; This should use config_.max_strong_interval (or, better, config_.writable_connection_ping_interval), since the constant is just a default that can be changed with SetIceConfig.
https://codereview.webrtc.org/1944003002/diff/20001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:221: // also try hard to make sure it is pinged at this rate. On 2016/05/05 20:46:13, pthatcher1 wrote: > This comment isn't accurate any more, because this rate is now not only used for > the current best connection, but also for all writable connections. > > I like the idea of just saying "Writable connections use this ping interval. > Unwritable connections use the faster interval." But in that case, we should > rename the constant to WRITABLE_CONNECTION_PING_INTERVAL, and the settable > config_ value to config_.writable_connection_ping_interval. > Done. https://codereview.webrtc.org/1944003002/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:1338: return now >= conn->last_ping_sent() + MAX_CURRENT_STRONG_INTERVAL; On 2016/05/05 20:46:13, pthatcher1 wrote: > This should use config_.max_strong_interval (or, better, > config_.writable_connection_ping_interval), since the constant is just a default > that can be changed with SetIceConfig. > Agree. Hardcode is indeed inappropriate here.
lgtm, with a nit https://codereview.webrtc.org/1944003002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:251: WRITABLE_CONNECTION_PING_INTERVAL /* max_strong_interval */) { The comment doesn't match. And the comment provides no value. Perhaps just delete it.
Description was changed from ========== Increase the stun ping interval. The max current strong interval is 2500ms. The function IsPingable will filter out the writable connections which can be pinged in a slower rate. BUG=webrtc:1161 ========== to ========== Increase the stun ping interval. Introduce the WRITABLE_CONNECTION_PING_INTERVAL(2500ms). The function IsPingable will filter out the writable connections which can be pinged in a slower rate. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL and can be set with the configuration. BUG=webrtc:1161 ==========
Description was changed from ========== Increase the stun ping interval. Introduce the WRITABLE_CONNECTION_PING_INTERVAL(2500ms). The function IsPingable will filter out the writable connections which can be pinged in a slower rate. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL and can be set with the configuration. BUG=webrtc:1161 ========== to ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL and can be set with the configuration. BUG=webrtc:1161 ==========
Description was changed from ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL and can be set with the configuration. BUG=webrtc:1161 ========== to ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 ==========
The CQ bit was checked by zhihuang@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.chromium.org/1944003002/#ps80001 (title: "Remove useless comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1944003002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1944003002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13200)
The CQ bit was checked by zhihuang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1944003002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1944003002/80001
Message was sent while issue was closed.
Description was changed from ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 ========== to ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 ========== to ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 Committed: https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0 Cr-Commit-Position: refs/heads/master@{#12736} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0 Cr-Commit-Position: refs/heads/master@{#12736}
Message was sent while issue was closed.
not lgtm If we do this, I think we need to update the RTT calculation algorithm to converge faster. Otherwise it will take 5x as long to converge (and it is already very slow).
Message was sent while issue was closed.
On 2016/05/13 19:33:35, juberti2 wrote: > not lgtm > > If we do this, I think we need to update the RTT calculation algorithm to > converge faster. Otherwise it will take 5x as long to converge (and it is > already very slow). simulations indicate it will take 45s for our rtt estimate to converge, with this change. I think we should roll back until we can account for this.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.webrtc.org/1979573003/ by zhihuang@webrtc.org. The reason for reverting is: This will take longer time to converge. Need to update the RTT calculation algorithm if doing this. .
Message was sent while issue was closed.
On 2016/05/13 19:50:14, juberti2 wrote:
> On 2016/05/13 19:33:35, juberti2 wrote:
> > not lgtm
> >
> > If we do this, I think we need to update the RTT calculation algorithm to
> > converge faster. Otherwise it will take 5x as long to converge (and it is
> > already very slow).
>
> simulations indicate it will take 45s for our rtt estimate to converge, with
> this change.
>
> I think we should roll back until we can account for this.
That's a good point. I can think of two ways to account for this:
1. Calculate RTT differently.
2. Send pings at a fast rate for the first N (= 10?) pings. So instead of "if
writable() { go_slow() }", it would be "if writable() and pings_sent() > 10 {
go_slow() }".
#2 seems easy enough. What do you think Justin?
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.webrtc.org/1982713002/ by deadbeef@webrtc.org. The reason for reverting is: This will take longer time for the RTT to converge. Need to update the RTT calculation algorithm if doing this. .
Message was sent while issue was closed.
Description was changed from ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 Committed: https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0 Cr-Commit-Position: refs/heads/master@{#12736} ========== to ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 Committed: https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0 Cr-Commit-Position: refs/heads/master@{#12736} ==========
First N pings are sent with a faster rate so that it will not take too long for rtt to converge.
Taylor, can you re-review this? https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1332: // The best_connection need to ping at a fast rate. need => needs https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1343: now); This would be more clear with early returns and an interval parameter. if (!best_connection_ || !best_connection_->connected() || !best_connection_->writable()) { return false; } int interval = best_connection_->NeedToPingFast() ? MAX_CURRENT_STRONG_INTERVAL : config_.writable_connection_ping_interval; return best_connection_->last_ping_sent() + interval <= rtc::TimeMillis(); https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:594: // If the connection need to send Ping at fast rate. Ping => pings at fast rate => at a fast rate https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:595: bool NeedToPingFast(); I'd prefer to simply expose pings_sent() as a method and then let p2ptransportchannel do the logic of choosing fast vs. slow. That way all the logic is in one place.
On 2016/05/27 17:36:32, pthatcher1 wrote: > Taylor, can you re-review this? > > https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... > File webrtc/p2p/base/p2ptransportchannel.cc (right): > > https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1332: // The best_connection need to ping > at a fast rate. > need => needs > > https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1343: now); > This would be more clear with early returns and an interval parameter. > > if (!best_connection_ || !best_connection_->connected() || > !best_connection_->writable()) { > return false; > } > > int interval = best_connection_->NeedToPingFast() ? MAX_CURRENT_STRONG_INTERVAL > : config_.writable_connection_ping_interval; > return best_connection_->last_ping_sent() + interval <= rtc::TimeMillis(); > > https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h > File webrtc/p2p/base/port.h (right): > > https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h#n... > webrtc/p2p/base/port.h:594: // If the connection need to send Ping at fast rate. > Ping => pings > at fast rate => at a fast rate > > https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h#n... > webrtc/p2p/base/port.h:595: bool NeedToPingFast(); > I'd prefer to simply expose pings_sent() as a method and then let > p2ptransportchannel do the logic of choosing fast vs. slow. That way all the > logic is in one place. Zhi, can you merge the CL with the one I recently landed which also counts the number of pings sent on each connection? Plus, do you need to create a new issue (using a different git branch) for this? I am afraid you won't be able to land the CL twice on the same git branch.
https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:222: // connection may get starved. "connection" -> "connections" Also, may want to mention that the first several pings are sent at a faster rate in order for the RTT to converge faster. https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1381: // 2. The connection is not sending first several pings "The connect" -> "The connection" "is not sending first several pings" -> "has sent enough initial pings for the RTT to converge". https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:594: // If the connection need to send Ping at fast rate. On 2016/05/27 17:36:31, pthatcher1 wrote: > Ping => pings > at fast rate => at a fast rate also need => needs https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:595: bool NeedToPingFast(); On 2016/05/27 17:36:31, pthatcher1 wrote: > I'd prefer to simply expose pings_sent() as a method and then let > p2ptransportchannel do the logic of choosing fast vs. slow. That way all the > logic is in one place. I agree with this. P2PTransportChannel can have a helper function "NeedToPingFast(Connection*)".
zhihuang@chromium.org changed reviewers: + zhihuang@chromium.org
https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1343: now); On 2016/05/27 17:36:31, pthatcher1 wrote: > This would be more clear with early returns and an interval parameter. > > if (!best_connection_ || !best_connection_->connected() || > !best_connection_->writable()) { > return false; > } > > int interval = best_connection_->NeedToPingFast() ? MAX_CURRENT_STRONG_INTERVAL > : config_.writable_connection_ping_interval; > return best_connection_->last_ping_sent() + interval <= rtc::TimeMillis(); > > writable_connection_ping_interval may be smaller than the default value MAX_CURRENT_STRONG_INTERVAL. So I changed it to: int interval = best_connection_->NeedToPingFast() ? min(MAX_CURRENT_STRONG_INTERVAL, config_.writable_connection_ping_interval) : config_.writable_connection_ping_interval;
Taylor and Honghai, Can you both review the code I just wrote in a comment for IsPingable and make sure my logic is accurate? I think it would be more simple, but it is still a little bit complex and I want to make sure it's correct. https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:220: // If the current best connection is both writable and receiving, first several first several pings => the first several pings https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:221: // pings will be sent at this rate by default in order for the RTT to converge This is for all connections, not just the best connection, right? Perhaps we could say "Writable connections at ping at a higher rate while stabilizing" https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:222: // faster. If it pings too fast, other connections may get starved. Doesn't this apply to all connections, not just the best? If so, I think the comment should say something like "Writable connections are pinged at a faster rate while stabilizing." https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:223: static const int MAX_CURRENT_STRONG_INTERVAL = 900; // ms This needs to be renamed now. Perhaps: STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL And the other one could be: STABLE_WRITABLE_CONNECTION_PING_INTERVAL https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:225: // Writable connections are pinged at a slower rate. once stablized https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1380: bool P2PTransportChannel::NeedToPingFast(Connection* conn) { This the code I wrote below, I think we don't need this method. In fact, I don't think we need IsBestConnectionPingable. We could just have: best_connection_ && IsPingable(best_connection_) https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1414: I think instead of having NeedsToPingFast, we can just roll the logic into here. From here in this method and below, the logic could be thus: // Don't ping inactive non-backup connections. if (!conn->active()) { return false; } // Do ping unwritable, active connections. if (!conn->writable()) { return true; } // Ping writable, active connections if it's been long enough since the last ping. int ping_interval = config_.writable_connection_ping_interval; // A stabilizing connection gets a faster ping interval. bool stable = conn->num_pings_sent() <= MIN_PINGS_TO_STABILIZE; if (!stable) { ping_interval = std::min(ping_interval, STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); } return (now >= conn->last_ping_sent() + ping_interval); https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:43: static const int MIN_PINGS_FOR_RTT_CONVERGENCE = 10; I think a better name would be something like MIN_PINGS_TO_STABILIZE
https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1380: bool P2PTransportChannel::NeedToPingFast(Connection* conn) { On 2016/06/01 23:22:02, pthatcher1 wrote: > This the code I wrote below, I think we don't need this method. In fact, I > don't think we need IsBestConnectionPingable. We could just have: > > best_connection_ && IsPingable(best_connection_) Cannot do this. If the best_connection becomes unwritable, it will ping the best connection forever. https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1414: On 2016/06/01 23:22:02, pthatcher1 wrote: > I think instead of having NeedsToPingFast, we can just roll the logic into here. > From here in this method and below, the logic could be thus: > > // Don't ping inactive non-backup connections. > if (!conn->active()) { > return false; > } > > // Do ping unwritable, active connections. > if (!conn->writable()) { > return true; > } > > // Ping writable, active connections if it's been long enough since the last > ping. > int ping_interval = config_.writable_connection_ping_interval; > > // A stabilizing connection gets a faster ping interval. > bool stable = conn->num_pings_sent() <= MIN_PINGS_TO_STABILIZE; > if (!stable) { > ping_interval = std::min(ping_interval, > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); > } > return (now >= conn->last_ping_sent() + ping_interval); look good to me. This is just int ping_interval = stable ? config_.writable_connection_ping_interval : STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:78: please remove this change.
https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1414: On 2016/06/02 00:03:57, honghaiz3 wrote: > On 2016/06/01 23:22:02, pthatcher1 wrote: > > I think instead of having NeedsToPingFast, we can just roll the logic into > here. > > From here in this method and below, the logic could be thus: > > > > // Don't ping inactive non-backup connections. > > if (!conn->active()) { > > return false; > > } > > > > // Do ping unwritable, active connections. > > if (!conn->writable()) { > > return true; > > } > > > > // Ping writable, active connections if it's been long enough since the last > > ping. > > int ping_interval = config_.writable_connection_ping_interval; > > > > // A stabilizing connection gets a faster ping interval. > > bool stable = conn->num_pings_sent() <= MIN_PINGS_TO_STABILIZE; > > if (!stable) { > > ping_interval = std::min(ping_interval, > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); > > } > > return (now >= conn->last_ping_sent() + ping_interval); > look good to me. This is just > int ping_interval = stable ? config_.writable_connection_ping_interval : > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; As I thought more about it, I think it may not be good to set the ping interval to 2.5 seconds when it is stable and writable, because writable state will be affected by the ping frequency. If a connection suddenly becomes bad (from a writable state) and if we ping once every 2.5 seconds since it is writable, the connection won't change to unwritable after 12.5 seconds because the state change from writable to unwritable need 5 un-acked stun pings and at least 5 seconds. So we need to check if the last stun ping has been responded and if yes use the 2.5 seconds interval. Otherwise use the 1.0 seconds interval. It would be something like that int ping_interval = stable && conn->LastPingWasResponded() ? config_.writable_connection_ping_interval : STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; and perhaps change the name of the constant STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL By doing so, we may still wait for up 6.5 seconds before detecting a connection is not writable (compare to 5 seconds today). This is because we may have waited for 2.5 seconds to send the next ping and found it failed and from then on, it needs to send 4 more stun pings before switching to unwritable state.
https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1380: bool P2PTransportChannel::NeedToPingFast(Connection* conn) { On 2016/06/02 00:03:57, honghaiz3 wrote: > On 2016/06/01 23:22:02, pthatcher1 wrote: > > This the code I wrote below, I think we don't need this method. In fact, I > > don't think we need IsBestConnectionPingable. We could just have: > > > > best_connection_ && IsPingable(best_connection_) > > Cannot do this. > If the best_connection becomes unwritable, it will ping the best connection > forever. Good catch. How about best_connection_ && best_connection_->connected() && best_connection->writable() && IsPingable(best_connection_)? That's basically what we have now, except replacing "(best_connection_->last_ping_sent() + config_.max_strong_interval <= now)" with "IsPingable(best_connection_)". https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1414: On 2016/06/02 00:03:57, honghaiz3 wrote: > On 2016/06/01 23:22:02, pthatcher1 wrote: > > I think instead of having NeedsToPingFast, we can just roll the logic into > here. > > From here in this method and below, the logic could be thus: > > > > // Don't ping inactive non-backup connections. > > if (!conn->active()) { > > return false; > > } > > > > // Do ping unwritable, active connections. > > if (!conn->writable()) { > > return true; > > } > > > > // Ping writable, active connections if it's been long enough since the last > > ping. > > int ping_interval = config_.writable_connection_ping_interval; > > > > // A stabilizing connection gets a faster ping interval. > > bool stable = conn->num_pings_sent() <= MIN_PINGS_TO_STABILIZE; > > if (!stable) { > > ping_interval = std::min(ping_interval, > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); > > } > > return (now >= conn->last_ping_sent() + ping_interval); > look good to me. This is just > int ping_interval = stable ? config_.writable_connection_ping_interval : > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; Yes, except you still need the std::min in there in case config_.writable_connection_ping_interval is less than STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL (you don't want STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL to slow down pinging in that case). https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1414: On 2016/06/02 17:06:25, honghaiz3 wrote: > On 2016/06/02 00:03:57, honghaiz3 wrote: > > On 2016/06/01 23:22:02, pthatcher1 wrote: > > > I think instead of having NeedsToPingFast, we can just roll the logic into > > here. > > > From here in this method and below, the logic could be thus: > > > > > > // Don't ping inactive non-backup connections. > > > if (!conn->active()) { > > > return false; > > > } > > > > > > // Do ping unwritable, active connections. > > > if (!conn->writable()) { > > > return true; > > > } > > > > > > // Ping writable, active connections if it's been long enough since the last > > > ping. > > > int ping_interval = config_.writable_connection_ping_interval; > > > > > > // A stabilizing connection gets a faster ping interval. > > > bool stable = conn->num_pings_sent() <= MIN_PINGS_TO_STABILIZE; > > > if (!stable) { > > > ping_interval = std::min(ping_interval, > > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); > > > } > > > return (now >= conn->last_ping_sent() + ping_interval); > > look good to me. This is just > > int ping_interval = stable ? config_.writable_connection_ping_interval : > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; > > As I thought more about it, I think it may not be good to set the ping interval > to 2.5 seconds when it is stable and writable, because writable state will be > affected by the ping frequency. If a connection suddenly becomes bad (from a > writable state) and if we ping once every 2.5 seconds since it is writable, the > connection won't change to unwritable after 12.5 seconds because the state > change from writable to unwritable need 5 un-acked stun pings and at least 5 > seconds. So we need to check if the last stun ping has been responded and if yes > use the 2.5 seconds interval. Otherwise use the 1.0 seconds interval. > It would be something like that > int ping_interval = stable && conn->LastPingWasResponded() ? > config_.writable_connection_ping_interval : > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; > and perhaps change the name of the constant > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL > > By doing so, we may still wait for up 6.5 seconds before detecting a connection > is not writable (compare to 5 seconds today). This is because we may have waited > for 2.5 seconds to send the next ping and found it failed and from then on, it > needs to send 4 more stun pings before switching to unwritable state. Yes, that's the tradeoff with low-frequency keep-alive, and we have two possible mitigations: 1. We could mark a connection as "unstable" if it hasn't received anything for 3 seconds, and then start pinging it at a higher rate. This would allow us to get 5 stun pings within 5 seconds if we don't receive anything. 2. We can move away from using writability as the signal for "still connected" and use receiving as the signal for "still connected" instead. I think what you're saying is that we should not submit this CL without at least one of this mitigations. Is that correct?
https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1380: bool P2PTransportChannel::NeedToPingFast(Connection* conn) { On 2016/06/02 18:03:02, pthatcher1 wrote: > On 2016/06/02 00:03:57, honghaiz3 wrote: > > On 2016/06/01 23:22:02, pthatcher1 wrote: > > > This the code I wrote below, I think we don't need this method. In fact, I > > > don't think we need IsBestConnectionPingable. We could just have: > > > > > > best_connection_ && IsPingable(best_connection_) > > > > Cannot do this. > > If the best_connection becomes unwritable, it will ping the best connection > > forever. > > Good catch. How about best_connection_ && best_connection_->connected() && > best_connection->writable() && IsPingable(best_connection_)? > > That's basically what we have now, except replacing > "(best_connection_->last_ping_sent() + config_.max_strong_interval <= now)" > with "IsPingable(best_connection_)". That would work. It is more readable although at the price of checking writable() and connected() twice for the best connection. https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1414: On 2016/06/02 18:03:02, pthatcher1 wrote: > On 2016/06/02 00:03:57, honghaiz3 wrote: > > On 2016/06/01 23:22:02, pthatcher1 wrote: > > > I think instead of having NeedsToPingFast, we can just roll the logic into > > here. > > > From here in this method and below, the logic could be thus: > > > > > > // Don't ping inactive non-backup connections. > > > if (!conn->active()) { > > > return false; > > > } > > > > > > // Do ping unwritable, active connections. > > > if (!conn->writable()) { > > > return true; > > > } > > > > > > // Ping writable, active connections if it's been long enough since the last > > > ping. > > > int ping_interval = config_.writable_connection_ping_interval; > > > > > > // A stabilizing connection gets a faster ping interval. > > > bool stable = conn->num_pings_sent() <= MIN_PINGS_TO_STABILIZE; > > > if (!stable) { > > > ping_interval = std::min(ping_interval, > > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); > > > } > > > return (now >= conn->last_ping_sent() + ping_interval); > > look good to me. This is just > > int ping_interval = stable ? config_.writable_connection_ping_interval : > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; > > Yes, except you still need the std::min in there in case > config_.writable_connection_ping_interval is less than > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL (you don't want > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL to slow down pinging in that case). > I did not realize that we may have a case of writable_connection_ping_interval smaller than the STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL. Perhaps we should enforce the former is always bigger than the latter. Yes. I think not land the current CL as is. Other possible mitigations: 3. Start ping at a higher rate (once per second, not that high) if there are ping pending and the last ping response was received more than 1 second ago. This is similar to what I have suggested in the previous comments. 4. Start ping at a higher rate (once per second) if the connection is not receiving (the receiving timeout is adjustable). This is similar to your mitigation 1 but using our receiving state and the timeout. I hope we don't introduce new states or new timeout values.
> https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... > File webrtc/p2p/base/p2ptransportchannel.cc (right): > > https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1380: bool > P2PTransportChannel::NeedToPingFast(Connection* conn) { > On 2016/06/02 18:03:02, pthatcher1 wrote: > > On 2016/06/02 00:03:57, honghaiz3 wrote: > > > On 2016/06/01 23:22:02, pthatcher1 wrote: > > > > This the code I wrote below, I think we don't need this method. In fact, > I > > > > don't think we need IsBestConnectionPingable. We could just have: > > > > > > > > best_connection_ && IsPingable(best_connection_) > > > > > > Cannot do this. > > > If the best_connection becomes unwritable, it will ping the best connection > > > forever. > > > > Good catch. How about best_connection_ && best_connection_->connected() && > > best_connection->writable() && IsPingable(best_connection_)? > > > > That's basically what we have now, except replacing > > "(best_connection_->last_ping_sent() + config_.max_strong_interval <= now)" > > with "IsPingable(best_connection_)". > > That would work. It is more readable although at the price of checking > writable() and connected() twice for the best connection. I think the performance impact is inconsequential. In fact, I wouldn't be surprised if the compiler removes it entirely. > > https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1414: > On 2016/06/02 18:03:02, pthatcher1 wrote: > > On 2016/06/02 00:03:57, honghaiz3 wrote: > > > On 2016/06/01 23:22:02, pthatcher1 wrote: > > > > I think instead of having NeedsToPingFast, we can just roll the logic into > > > here. > > > > From here in this method and below, the logic could be thus: > > > > > > > > // Don't ping inactive non-backup connections. > > > > if (!conn->active()) { > > > > return false; > > > > } > > > > > > > > // Do ping unwritable, active connections. > > > > if (!conn->writable()) { > > > > return true; > > > > } > > > > > > > > // Ping writable, active connections if it's been long enough since the > last > > > > ping. > > > > int ping_interval = config_.writable_connection_ping_interval; > > > > > > > > // A stabilizing connection gets a faster ping interval. > > > > bool stable = conn->num_pings_sent() <= MIN_PINGS_TO_STABILIZE; > > > > if (!stable) { > > > > ping_interval = std::min(ping_interval, > > > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); > > > > } > > > > return (now >= conn->last_ping_sent() + ping_interval); > > > look good to me. This is just > > > int ping_interval = stable ? config_.writable_connection_ping_interval : > > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL; > > > > Yes, except you still need the std::min in there in case > > config_.writable_connection_ping_interval is less than > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL (you don't want > > STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL to slow down pinging in that > case). > > > > I did not realize that we may have a case of writable_connection_ping_interval > smaller than the STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL. Perhaps we should > enforce the former is always bigger than the latter. I don't think it's worth enforcing. It will probably never happen, and even if it did, the negative impact is small. Even the use of std::min here is probably being too careful. > > Yes. I think not land the current CL as is. > Other possible mitigations: > 3. Start ping at a higher rate (once per second, not that high) if there are > ping pending and the last ping response was received more than 1 second ago. > This is similar to what I have suggested in the previous comments. Yeah, that sounds like a good idea. In other words, the idea of "stable" could be more than "has sent more than N ping" but also "has sent more than N pings, and has received a response in the last N seconds". > 4. Start ping at a higher rate (once per second) if the connection is not > receiving (the receiving timeout is adjustable). This is similar to your > mitigation 1 but using our receiving state and the timeout. > I hope we don't introduce new states or new timeout values. Yeah, that's what I meant for #1. In other words, the idea of "stable" could be "has sent more than N pings, and has received packets in the last N seconds". We could do both, although obviously if the N for #3 is less than the N for #1/#4, then the N for #1/#4 doesn't matter.
zhihuang@webrtc.org changed reviewers: - zhihuang@chromium.org
On 2016/06/03 00:13:23, pthatcher1 wrote: > > > https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... > > File webrtc/p2p/base/p2ptransportchannel.cc (right): > > > > > https://codereview.webrtc.org/1944003002/diff/120001/webrtc/p2p/base/p2ptrans... > > webrtc/p2p/base/p2ptransportchannel.cc:1380: bool > > P2PTransportChannel::NeedToPingFast(Connection* conn) { > > On 2016/06/02 18:03:02, pthatcher1 wrote: > > > On 2016/06/02 00:03:57, honghaiz3 wrote: > > > > On 2016/06/01 23:22:02, pthatcher1 wrote: > > > > > This the code I wrote below, I think we don't need this method. In > fact, > > I > > > > > don't think we need IsBestConnectionPingable. We could just have: > > > > > > > > > > best_connection_ && IsPingable(best_connection_) > > > > > > > > Cannot do this. > > > > If the best_connection becomes unwritable, it will ping the best > connection > > > > forever. > > > > > > Good catch. How about best_connection_ && best_connection_->connected() && > > > best_connection->writable() && IsPingable(best_connection_)? > > > > > > That's basically what we have now, except replacing > > > "(best_connection_->last_ping_sent() + config_.max_strong_interval <= now)" > > > with "IsPingable(best_connection_)". > > > > That would work. It is more readable although at the price of checking > > writable() and connected() twice for the best connection. > > I think the performance impact is inconsequential. In fact, I wouldn't be > surprised if the compiler removes it entirely. I think we still cannot do this because when best connection is not receiving, the transportchannel will be weak, and we will always ping the best connection. SO I keep the IsBestConnectionPingable().
https://codereview.webrtc.org/1944003002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1430: config_.stable_writable_connection_ping_interval) Should this be the same as the ping_interval calculation above? https://codereview.webrtc.org/1944003002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1432: return best_connection_->last_ping_sent() + interval <= rtc::TimeMillis(); You could pass in now from the parameter to avoid reading the clock again. Probably better to use a common method to calculate the ping interval and decide IsPingable to avoid the code duplicate.
https://codereview.webrtc.org/1944003002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1430: config_.stable_writable_connection_ping_interval) On 2016/06/03 22:54:07, honghaiz3 wrote: > Should this be the same as the ping_interval calculation above? Yes, it should be. https://codereview.webrtc.org/1944003002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1432: return best_connection_->last_ping_sent() + interval <= rtc::TimeMillis(); On 2016/06/03 22:54:07, honghaiz3 wrote: > You could pass in now from the parameter to avoid reading the clock again. > Probably better to use a common method to calculate the ping interval and decide > IsPingable to avoid the code duplicate. Done.
I like the mitigations. It's not mostly style/readability. And don't we need to add quite a few unit tests to make sure we ping at the right times? I bet you could use Taylor's new FakeClock to get good timing tests without being flaky and without taking a long time. https://codereview.webrtc.org/1944003002/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1398: // Each connection ping sufficiently. A better comment would be: "Ping each connection at a higher rate at least this many times" https://codereview.webrtc.org/1944003002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1401: } Should this be part of CalculatePingInterval? if (conn->num_pings_sent() <= MIN_PINGS_AT_WEAK_PING_INTERVAL) { return weak_ping_interval_; } https://codereview.webrtc.org/1944003002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1405: int ping_interval = CalculatePingInterval(conn, now); Should this be called CalculateActiveWritablePingInterval just to be clear? https://codereview.webrtc.org/1944003002/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1433: return ping_interval; Might be more clear as: int stable_interval = config_.stable_writable_connection_ping_interval; int stabilizing_interval = std::min(stable_interval, STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); if (!stable) { return stablizing_interval; } return stable_interval; Or, with my other comment: if (conn->num_pings_sent() <= MIN_PINGS_AT_WEAK_PING_INTERVAL) { return weak_ping_interval_; } int stable_interval = config_.stable_writable_connection_ping_interval; int stabilizing_interval = std::min(stable_interval, STABLIZING_WRITABLE_CONNECTION_PING_INTERVAL); ... if (!stable) { return stablizing_interval; } return stable_interval;
https://codereview.chromium.org/1944003002/diff/200001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.chromium.org/1944003002/diff/200001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2097: EXPECT_TRUE(duration >= 100 && duration <= 200); The duration is not expected to be exactly the same as the intervals (stabilizing / stabilized etc.) because the ping is scheduled based on the ping interval and the check receiving interval.
OK, really close :) https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1436: } If we use the Connection::stable(), then we can do this: return conn->stable() ? stable_interval : stablizing_interval; https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2100: duration); If you use X * cricket::WEAK_PING_INTERVAL * kNumNanosecsPerMillisec and let duration by in nanos this would be a bit more readable. https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2119: // The connection become strong but not stabilized. The ping interval is not stabilized because we haven't been able to converge the RTT. https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2125: stablilizing_timeout, clock); Just use 2000 here :) https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2127: EXPECT_TRUE(duration >= 900 && duration <= 1100); EXPECT_GT(STABLIZING_INTERVAL, duration); EXPECT_LT(STABLIZING_INTERVAL + SCHEDULING_RANGE, duration); https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2147: conn->ReceivedPingResponse(); You'll need to call this 4 times now to become stable. And maybe AdvanceTime in between to get a non-zero RTT. https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2153: EXPECT_TRUE(duration >= 2500 && duration <= 2600); Can you combine the two tests above? https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2182: stablilizing_timeout, clock); Just use clock.AdvanceTime(2*conn->rtt()); https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2184: EXPECT_TRUE(duration >= 900 && duration <= 1100); Actually, can you combine all three tests? https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:590: } We don't need this if we do the thing below. https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:634: int rtt_; Can we also store this? int rtt_samples_ = 0; And then in Connection::OnConnectionRequestResponse do this? rtt_ = (RTT_RATIO * rtt_ + rtt) / (RTT_RATIO + 1); rtt_samples_++; And then: private: bool rtt_converged() { return rtt_samples_ > (RTT_RATIO + 1); } And could we also calculate this? public: bool stable(int64_t now) { // A connection is stable if it's RTT has converged and it isn't missing any responses. We should send pings at a higher rate until the RTT converges and whenever a ping response is missing (so that we can detect unwritability faster) return rtt_converged() && !missing_responses(now); } private: bool missing_responses(int64_t now) { if (ping_since_last_response.empty()) { return false; } int64_t waiting = now - ping_since_last_response[0].sent_time; return waiting > 2*rtt(); } https://codereview.webrtc.org/1944003002/diff/220001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:635: int64_t last_ping_sent_; // last time we sent a ping to the other side And could we also calculate this? bool stable() { return rtt_converged && !missing_responses(); } bool missing_responses(int64_t now) { if (ping_since_last_response.empty()) { return false; } int64_t waiting = now - ping_since_last_response[0].sent_time; return waiting > 2*rtt(); }
zhihuang@webrtc.org changed reviewers: - deadbeef@webrtc.org
https://codereview.webrtc.org/1944003002/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1944003002/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2097: EXPECT_TRUE(duration >= 100 && duration <= 200); On 2016/06/10 20:39:02, Zhi Huang wrote: > The duration is not expected to be exactly the same as the intervals > (stabilizing / stabilized etc.) because the ping is scheduled based on the ping > interval and the check receiving interval. We can get the exact ping interval for initial pings.
lgtm https://codereview.webrtc.org/1944003002/diff/260001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1944003002/diff/260001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2114: // converge the RTT. become => becomes https://codereview.webrtc.org/1944003002/diff/260001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2901: conn3->ReceivedPingResponse(0); 0 RTT? Why not make it a more reasonable value. Say, 20? And could you give it named constant to make it more readable, like LOW_RTT?
The CQ bit was checked by zhihuang@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/1944003002/#ps280001 (title: "Fix naming issue in unit test before committing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1944003002/280001
Message was sent while issue was closed.
Description was changed from ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 Committed: https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0 Cr-Commit-Position: refs/heads/master@{#12736} ========== to ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 Committed: https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0 Cr-Commit-Position: refs/heads/master@{#12736} ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 Committed: https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0 Cr-Commit-Position: refs/heads/master@{#12736} ========== to ========== Increase the stun ping interval. Writable connections are pinged at a slower rate. The function IsPingable will filter out the writable connections. The interval for slower ping rate by default is WRITABLE_CONNECTION_PING_INTERVAL(2500ms) and can be set with the configuration. BUG=webrtc:1161 Committed: https://crrev.com/8f7a5aad55a64f0d81b6436a22ffbdfcdcde91e0 Committed: https://crrev.com/435264a183e16572c99e8dd46171163b6fd33d59 Cr-Original-Commit-Position: refs/heads/master@{#12736} Cr-Commit-Position: refs/heads/master@{#13241} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/435264a183e16572c99e8dd46171163b6fd33d59 Cr-Commit-Position: refs/heads/master@{#13241} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
