Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by honghaiz3 Modified:
4 years, 1 month ago Reviewers:
Taylor Brandstetter, pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPing the premier connection on each network with higher priority.
When the selected connection becomes not receiving and there are many connections,
If we use a round-robin fashion to ping all connections, none of the connections will
be in receiving state for sufficient long time to ensure switching connections promptly.
Triggered check will help in this situation to some extent but it may still fail to switch promptly when there are a lot of connections.
With this CL, if the selected connection is weak, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state.
Plus, if the selected connection is weak, we choose a shorter ping interval (900ms) for all writable connections.
BUG=b/32022719
Committed: https://crrev.com/7252a00b9f2c5cb8b0a0557386ea3a8c29b26b9c
Cr-Commit-Position: refs/heads/master@{#14991}
Patch Set 1 : . #
Total comments: 24
Patch Set 2 : Merge #Patch Set 3 : Address Taylor's comments #
Total comments: 2
Patch Set 4 : Add TODO for re-thinking prioritizing connections for ping requests. #
Total comments: 26
Patch Set 5 : . #
Total comments: 19
Patch Set 6 : Address Peter's comments #Patch Set 7 : Merge and Minor change #Patch Set 8 : Rearrange FindNextPingableConnection #
Total comments: 12
Patch Set 9 : Address Both comments #Patch Set 10 : Merge branch 'master' into failover_quickly #Patch Set 11 : Merge again #Patch Set 12 : Merge to head #Patch Set 13 : Add a header file <iterator> #
Created: 4 years, 1 month ago
Messages
Total messages: 71 (40 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 #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Description was changed from ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. BUG= ========== to ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there is random network delay. BUG= ==========
Patchset #1 (id:200001) has been deleted
https://codereview.webrtc.org/2369963004/diff/220001/webrtc/base/firewallsock... File webrtc/base/firewallsocketserver.cc (right): https://codereview.webrtc.org/2369963004/diff/220001/webrtc/base/firewallsock... webrtc/base/firewallsocketserver.cc:45: FirewallProtocol protocol = (type_ == SOCK_DGRAM) ? FP_UDP : FP_TCP; This is needed so that TCP traffic can be filtered in the middle of a call. https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2024: virtual_socket_server()->set_delay_stddev(100); The random delay is needed so that the test may fail without the source code change in this CL. When there is no random delay, triggered check will help keep a non-best connection in the receiving state. And once the non-best connection was switched, the best connection will be found after a short while. With random delay, the test will fail randomly with some chance even with the triggered check.
Description was changed from ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there is random network delay. BUG= ========== to ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there is random network delay. To resolve this issue, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. We also changed the pruning algorithms to prune connections based on the network interface name. So if an ipv4 and an ipv6 connection have the same interface name, if the ipv6 connection becomes writable, it will prune the ipv4 connection (provided that all other parts of the priority metric for the two connections are the same). BUG= ==========
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
As far I can tell, this CL is making the following changes: 1. Prune based on network name, rather than the Network pointers matching. 2. Ping weak connections at a higher rate (the "stabilizing interval") 3. If a network is "weak", ping all the connections on that network (previously this only occurred if the entire channel was "weak"). 4. Prioritize "premier" connections over others. Although these all help fix the bug in question, they seem like mostly independent changes. So I think it would be a good idea to separate them into separate CLs. And although the test you added is a good high-level regression test for the issue, there should probably be more granular tests for individual changes (using P2PTransportChannelPingTest?). Also, the CL description is mostly a description of the issue the CL is solving. Could you create a bug in the WebRTC tracker with this explanation, and link the bug to the CL? https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1548: // If the premier connection is weakly connected, ping all connections on Suggest changing this to "If the premier connection on a network is weakly connected..." to make it clear there are multiple premier connections. https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1579: premier_connection->state() == Connection::STATE_FAILED) { Why did the "failed" check not exist before? https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: return (!weak() && conn->stable(now)) ? stable_interval : stablizing_interval; So this is now the "stabilizing or weak" interval? I think that needs to be made clear. Maybe a new interval should be introduced. https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1758: LOG_J(LS_INFO, this) << "Removed connection " << std::hex << connection Why not log connection->ToString()? https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:259: bool IsPremierConnectionPingable(Connection* conn, int64_t now); Can you explain in a comment somewhere what's meant by "premier connection"? https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:379: std::map<std::string, Connection*> premier_connection_by_network_name_; Can't this be determined algorithmically from connections_? If so I'd prefer not storing a separate map. https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2002: int delay = kMinimumStepDelay; nit: Why is this variable needed? https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2024: virtual_socket_server()->set_delay_stddev(100); On 2016/10/03 21:28:33, honghaiz3 wrote: > The random delay is needed so that the test may fail without the source code > change in this CL. When there is no random delay, triggered check will help keep > a non-best connection in the receiving state. And once the non-best connection > was switched, the best connection will be found after a short while. With random > delay, the test will fail randomly with some chance even with the triggered > check. If the test will only fail randomly, then if a regression is introduced, it will be more difficult for the person submitting the CL or the sheriff who notices the flakiness to react. Could the test be written without any randomness? Maybe you could blackhole all traffic for a while (so that no triggered checks are queued), then un-blackhole all but the Wi-Fi traffic? https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2033: 3000); Can you use a constant instead of 3000? https://codereview.webrtc.org/2369963004/diff/220001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2041: LOG(LS_INFO) << "Failing over..."; Is this log still necessary?
Description was changed from ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there is random network delay. To resolve this issue, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. We also changed the pruning algorithms to prune connections based on the network interface name. So if an ipv4 and an ipv6 connection have the same interface name, if the ipv6 connection becomes writable, it will prune the ipv4 connection (provided that all other parts of the priority metric for the two connections are the same). BUG= ========== to ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there is random network delay. To resolve this issue, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. We also changed the pruning algorithms to prune connections based on the network interface name. So if an ipv4 and an ipv6 connection have the same interface name, if the ipv6 connection becomes writable, it will prune the ipv4 connection (provided that all other parts of the priority metric for the two connections are the same). BUG=b/32022719 ==========
Patchset #3 (id:260001) has been deleted
Patchset #3 (id:280001) has been deleted
Description was changed from ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there is random network delay. To resolve this issue, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. We also changed the pruning algorithms to prune connections based on the network interface name. So if an ipv4 and an ipv6 connection have the same interface name, if the ipv6 connection becomes writable, it will prune the ipv4 connection (provided that all other parts of the priority metric for the two connections are the same). BUG=b/32022719 ========== to ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there are a lot of connections. With this CL, if the selected connection is weak, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. BUG=b/32022719 ==========
Patchset #3 (id:300001) has been deleted
Description was changed from ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there are a lot of connections. With this CL, if the selected connection is weak, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. BUG=b/32022719 ========== to ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there are a lot of connections. With this CL, if the selected connection is weak, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. Plus, if the selected connection is weak, we choose a shorter ping interval (900ms) for all writable connections. BUG=b/32022719 ==========
PTAL. Out of the four changes, I removed 1 and 3. #2 and #4 are best to be together in one CL as #2 only has impact when we prioritize premier connections. I add tests to check the ping interval for premier connections. It is pretty nontrivial to test that in a simpler setting because if there is only a few connections, it will fall back to the mode that if the selected connection is weak, every connection is pingable. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1548: // If the premier connection is weakly connected, ping all connections on On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > Suggest changing this to "If the premier connection on a network is weakly > connected..." to make it clear there are multiple premier connections. Done. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1579: premier_connection->state() == Connection::STATE_FAILED) { On 2016/10/04 02:08:09, Taylor Brandstetter wrote: > Why did the "failed" check not exist before? It is logical to include this although it does not matter here because whenever we set a connection to STATE_FAILED, we also mark it as WRITE_TIMEOUT. Revert this change to avoid the confusion. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1625: return (!weak() && conn->stable(now)) ? stable_interval : stablizing_interval; On 2016/10/04 02:08:09, Taylor Brandstetter wrote: > So this is now the "stabilizing or weak" interval? I think that needs to be made > clear. Maybe a new interval should be introduced. Changed to weak_or_stablizing_interval. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1758: LOG_J(LS_INFO, this) << "Removed connection " << std::hex << connection On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > Why not log connection->ToString()? connection->ToString() needs to access the members in the respective port. There is potential risk that the port may also be in the process of deleting itself and the members are partially released. Plus, it is probably sufficient to know which connection is removed at this point. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.h:259: bool IsPremierConnectionPingable(Connection* conn, int64_t now); On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > Can you explain in a comment somewhere what's meant by "premier connection"? Done. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.h:379: std::map<std::string, Connection*> premier_connection_by_network_name_; On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > Can't this be determined algorithmically from connections_? If so I'd prefer not > storing a separate map. Done. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2002: int delay = kMinimumStepDelay; On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > nit: Why is this variable needed? Removed https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2024: virtual_socket_server()->set_delay_stddev(100); On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > On 2016/10/03 21:28:33, honghaiz3 wrote: > > The random delay is needed so that the test may fail without the source code > > change in this CL. When there is no random delay, triggered check will help > keep > > a non-best connection in the receiving state. And once the non-best connection > > was switched, the best connection will be found after a short while. With > random > > delay, the test will fail randomly with some chance even with the triggered > > check. > > If the test will only fail randomly, then if a regression is introduced, it will > be more difficult for the person submitting the CL or the sheriff who notices > the flakiness to react. Could the test be written without any randomness? Maybe > you could blackhole all traffic for a while (so that no triggered checks are > queued), then un-blackhole all but the Wi-Fi traffic? I Find a way to remove the randomness by using longer delay. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2033: 3000); On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > Can you use a constant instead of 3000? Done. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2041: LOG(LS_INFO) << "Failing over..."; On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > Is this log still necessary? This log is informational. The similar log is also in other tests.
Patchset #3 (id:320001) has been deleted
Patchset #3 (id:340001) has been deleted
lgtm. Though I feel the logic for deciding how to prioritize connections and how often to ping them is getting somewhat complex, and we should be thinking about how the code could be simplified (while having the same end result). https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1758: LOG_J(LS_INFO, this) << "Removed connection " << std::hex << connection On 2016/10/20 23:47:58, honghaiz3 wrote: > On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > > Why not log connection->ToString()? > connection->ToString() needs to access the members in the respective port. There > is potential risk that the port may also be in the process of deleting itself > and the members are partially released. > Plus, it is probably sufficient to know which connection is removed at this > point. Does that risk really exist? I thought a Port is only destroyed asynchronously, after all its connections are destroyed. https://codereview.chromium.org/2369963004/diff/360001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.chromium.org/2369963004/diff/360001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2303: WEAK_OR_STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL + SCHEDULING_DELAY); This condition seems a little too specific for this high-level test. It would feel more at home in a P2PTranpsortChannelPingTest, to me.
I agree it is getting pretty complicated on prioritizing the ping requests on connections. Add TODO for simplifying the process. https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.chromium.org/2369963004/diff/220001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1758: LOG_J(LS_INFO, this) << "Removed connection " << std::hex << connection On 2016/10/21 20:18:49, Taylor Brandstetter wrote: > On 2016/10/20 23:47:58, honghaiz3 wrote: > > On 2016/10/04 02:08:10, Taylor Brandstetter wrote: > > > Why not log connection->ToString()? > > connection->ToString() needs to access the members in the respective port. > There > > is potential risk that the port may also be in the process of deleting itself > > and the members are partially released. > > Plus, it is probably sufficient to know which connection is removed at this > > point. > > Does that risk really exist? I thought a Port is only destroyed asynchronously, > after all its connections are destroyed. It probably does not although it would require careful analysis of the code. I once got caught when I tried to print out the whole information of a Connection when it is about to be deleted and it caused some tests flaky after the CL was landed. It is not super important to print out the complete information here. It is probably sufficient to know which connection was deleted at this point for the purpose of debugging. https://codereview.chromium.org/2369963004/diff/360001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.chromium.org/2369963004/diff/360001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2303: WEAK_OR_STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL + SCHEDULING_DELAY); On 2016/10/21 20:18:49, Taylor Brandstetter wrote: > This condition seems a little too specific for this high-level test. It would > feel more at home in a P2PTranpsortChannelPingTest, to me. That would require very similar setup (including multiple networks, many connections) to trigger the conditions to happen. Plus, Connection setup and ping request/response are manually done in P2PTransportChannelPingTest, which makes it more desirable doing it here.
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/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by honghaiz@webrtc.org
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.chromium.org/2369963004/#ps380001 (title: "Add TODO for re-thinking prioritizing connections for ping requests.")
The CQ bit was unchecked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
On 2016/10/21 23:28:37, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build > URL) > android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build > URL) > android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build > URL) > android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build > URL) > android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) > linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) It needs an owner review on the webrtc/base dir.
https://codereview.chromium.org/2369963004/diff/380001/webrtc/base/firewallso... File webrtc/base/firewallsocketserver.cc (left): https://codereview.chromium.org/2369963004/diff/380001/webrtc/base/firewallso... webrtc/base/firewallsocketserver.cc:45: if (!server_->Check(FP_UDP, GetLocalAddress(), addr)) { It was a bug that it did not check the TCP packet for the Firewall socket. This is needed for the test in this CL.
The code could be a little more clear. https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1312: std::map<rtc::Network*, Connection*> You don't need to return a map. You could instead return just a list of connections, since you can get the network from the connection. https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1313: P2PTransportChannel::GetPremierConnectionsByNetwork() const { I'm not really a fan of the word "premier", since it sounds just like "best". I think we need to be clear that we're getting the best connection for each network. So perhaps just call this method "GetBestConnectionOfEachNetwork" https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1325: premier_connections_by_network.insert(std::make_pair(network, conn)); Instead of inserting in the map, you could just iterate over the list of connections already added and see if the network is in there. Since the vector will be very small, it would (2? 3?), it will most likely be faster than a map. And you can even allocate the number of connections to return to the number of networks. https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1345: premier_connections_by_network[conn->port()->Network()]; If GetBestConnectionForEachNetwork returns a vector, you can just test if the connection is in the vector, which should be very fast. And the variable could be "bool best_for_this_network". https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1593: Connection* P2PTransportChannel::FindBestPremierConnectionToPing(int64_t now) { Why not just put all of this logic into FindNextPingableConnection? That would avoid this weird name (FindBestPremierX) which sounds like FindBestBestX. https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1598: // If the selected connection is strongly connected, don't bother to ping bother to ping => bother pinging https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1610: for (auto kv : premier_connections_by_network) { If GetBestConnectionForEachNetwork returned a vector, you just need to do std::max(best_connection_for_each_network) with a proper "old" metric. https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( Can't this just be IsConnectionPingable? https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.chromium.org/2369963004/diff/380001/webrtc/p2p/base/p2ptra... webrtc/p2p/base/p2ptransportchannel.h:329: // Finds the best premier connection for pinging. I find this confusing since "best" and "premier" almost mean the same thing. What we actually have here is a "best per network" and "best across networks". Can we have "best" mean "best across networks" and "best for network" mean "best per network"? For example, this method could just be "FindBestConnectionToPing"? And the method above could be GetBestConnectionForNetwork().
PTAL. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1312: std::map<rtc::Network*, Connection*> On 2016/10/24 20:49:05, pthatcher1 wrote: > You don't need to return a map. You could instead return just a list of > connections, since you can get the network from the connection. I wonder what was the concern about using a map. A std::map is based on red-black tree which should not be much slower than a vector if its size is small. Later we will need to find the best connection based on the network, using a map will be more readable. If you are worried about the cost of copying and moving the map, we can pass in a map pointer as an argument. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1313: P2PTransportChannel::GetPremierConnectionsByNetwork() const { On 2016/10/24 20:49:05, pthatcher1 wrote: > I'm not really a fan of the word "premier", since it sounds just like "best". I > think we need to be clear that we're getting the best connection for each > network. So perhaps just call this method "GetBestConnectionOfEachNetwork" Done. Changed Premier to best. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1345: premier_connections_by_network[conn->port()->Network()]; On 2016/10/24 20:49:05, pthatcher1 wrote: > If GetBestConnectionForEachNetwork returns a vector, you can just test if the > connection is in the vector, which should be very fast. And the variable could > be "bool best_for_this_network". Actually that is not enough. We will need to find the best connection on the same network and than compare the best connection and this connection. If this connection has higher cost or lower priority, then prune it. To find the best connection on the same network, we will need to iterate over the list of best connections if it is represented by a vector. This is primary place where I think map would be more readable than a vector. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1593: Connection* P2PTransportChannel::FindBestPremierConnectionToPing(int64_t now) { On 2016/10/24 20:49:05, pthatcher1 wrote: > Why not just put all of this logic into FindNextPingableConnection? That would > avoid this weird name (FindBestPremierX) which sounds like FindBestBestX. Done. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1598: // If the selected connection is strongly connected, don't bother to ping On 2016/10/24 20:49:05, pthatcher1 wrote: > bother to ping => bother pinging Done. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( On 2016/10/24 20:49:05, pthatcher1 wrote: > Can't this just be IsConnectionPingable? No. In this method, it is critical to ping connection once every XX (1 or 2.5) seconds. Otherwise, the set of best connections will utilize all slots for ping transmissions. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:329: // Finds the best premier connection for pinging. On 2016/10/24 20:49:05, pthatcher1 wrote: > I find this confusing since "best" and "premier" almost mean the same thing. > What we actually have here is a "best per network" and "best across networks". > > Can we have "best" mean "best across networks" and "best for network" mean "best > per network"? For example, this method could just be > "FindBestConnectionToPing"? And the method above could be > GetBestConnectionForNetwork(). Removed this method base on another suggestion.
https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1312: std::map<rtc::Network*, Connection*> On 2016/10/24 23:35:08, honghaiz3 wrote: > On 2016/10/24 20:49:05, pthatcher1 wrote: > > You don't need to return a map. You could instead return just a list of > > connections, since you can get the network from the connection. > I wonder what was the concern about using a map. A std::map is based on > red-black tree which should not be much slower than a vector if its size is > small. > Later we will need to find the best connection based on the network, using a map > will be more readable. > If you are worried about the cost of copying and moving the map, we can pass in > a map pointer as an argument. I thought it might be more readable. But what you have here now is good. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1345: premier_connections_by_network[conn->port()->Network()]; On 2016/10/24 23:35:09, honghaiz3 wrote: > On 2016/10/24 20:49:05, pthatcher1 wrote: > > If GetBestConnectionForEachNetwork returns a vector, you can just test if the > > connection is in the vector, which should be very fast. And the variable > could > > be "bool best_for_this_network". > Actually that is not enough. We will need to find the best connection on the > same network and than compare the best connection and this connection. If this > connection has higher cost or lower priority, then prune it. To find the best > connection on the same network, we will need to iterate over the list of best > connections if it is represented by a vector. This is primary place where I > think map would be more readable than a vector. OK, I see the logic now. https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( On 2016/10/24 23:35:09, honghaiz3 wrote: > On 2016/10/24 20:49:05, pthatcher1 wrote: > > Can't this just be IsConnectionPingable? > No. In this method, it is critical to ping connection once every XX (1 or 2.5) > seconds. Otherwise, the set of best connections will utilize all slots for ping > transmissions. What I mean is that since you're already passing in a connection, there's nothing about the method specific to "premier" connections. It's logic that can apply to any connection. The logic in the method really "a connection is pingable if we can send a ping and we haven't sent a ping very recently", which seems to apply to any connection. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1327: return best_connections_by_network; best_connection_by_network (makes it clear there is one per network). https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1340: auto best_connections_by_network = GetBestConnectionsByNetwork(); here too https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1603: return per_network_best_connection->last_ping_sent() + interval <= now; I think you could just call this variable "connection". https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1641: // If the selected connection is strongly connected, don't bother pinging Now that you have this as if (weak()), you should probably make the comment say "if not strongly connected, ping the best connection of each network at a higher priority". Either that or use an early return. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1660: } Can we do something like this? auto oldest_best_connection_by_network = std::min(best_connection_by_network.values(), [](Connection* conn1, Connection* conn2) { // Push unpingable connections to the max end to make pingable connections be at the min end. return !IsConnectionPingable(conn2) || conn1->last_ping_sent() < conn2->last_ping_sent(); }); if (IsConnectionPingable(oldest_best_connection_by_network)) { return oldest_best_connection_by_network; } https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:265: bool IsPerNetworkBestConnectionPingable(Connection* best_conn, int64_t now); I still think you could just say "IsConnectionPingable" and have the comment say "We can send a ping and we haven't sent a ping recently". https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:321: // Gets all best connections on every network. Better phrased as "Get the best connection for each network" https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:322: std::map<rtc::Network*, Connection*> GetBestConnectionsByNetwork() const; GetBestConnectionByNetwork (makes it clear there is one per network).
Patchset #6 (id:420001) has been deleted
PTAL. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1327: return best_connections_by_network; On 2016/10/25 17:00:06, pthatcher1 wrote: > best_connection_by_network (makes it clear there is one per network). Done. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1340: auto best_connections_by_network = GetBestConnectionsByNetwork(); On 2016/10/25 17:00:06, pthatcher1 wrote: > here too Done. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1603: return per_network_best_connection->last_ping_sent() + interval <= now; On 2016/10/25 17:00:06, pthatcher1 wrote: > I think you could just call this variable "connection". Done. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1641: // If the selected connection is strongly connected, don't bother pinging On 2016/10/25 17:00:06, pthatcher1 wrote: > Now that you have this as if (weak()), you should probably make the comment say > "if not strongly connected, ping the best connection of each network at a higher > priority". Either that or use an early return. Done. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1660: } On 2016/10/25 17:00:06, pthatcher1 wrote: > Can we do something like this? > > auto oldest_best_connection_by_network = > std::min(best_connection_by_network.values(), > [](Connection* conn1, Connection* conn2) { > // Push unpingable connections to the max end to make pingable connections > be at the min end. > return !IsConnectionPingable(conn2) || conn1->last_ping_sent() < > conn2->last_ping_sent(); > }); > if (IsConnectionPingable(oldest_best_connection_by_network)) { > return oldest_best_connection_by_network; > } It will need to be something like this: auto oldest_best_connection_by_network = std::min(best_connection_by_network.values(), [](Connection* conn1, Connection* conn2) { // Push unpingable connections to the max end to make pingable connections be at the min end. return IsPerNetworkBestConnectionPingable(conn1) < IsPerNetworkBestConnectionPingable(conn2) || conn1->last_ping_sent() < conn2->last_ping_sent(); }); if (IsPerNetworkBestConnectionPingable(oldest_best_connection_by_network)) { return oldest_best_connection_by_network; } I do not feel this is much more readable. It needs to check IsPerNetworkBestConnectionPingable more times. Plus, The comparing logic could easily get wrong. So I would prefer the current one in the code. It might be a few more lines, but it is hard to get wrong. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:265: bool IsPerNetworkBestConnectionPingable(Connection* best_conn, int64_t now); On 2016/10/25 17:00:06, pthatcher1 wrote: > I still think you could just say "IsConnectionPingable" and have the comment say > "We can send a ping and we haven't sent a ping recently". There is a method above called IsPingable, which is kind of IsConnectionPingable. It will be confusing to have another method called IsConnectionPingable. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:321: // Gets all best connections on every network. On 2016/10/25 17:00:06, pthatcher1 wrote: > Better phrased as "Get the best connection for each network" Done. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:322: std::map<rtc::Network*, Connection*> GetBestConnectionsByNetwork() const; On 2016/10/25 17:00:06, pthatcher1 wrote: > GetBestConnectionByNetwork (makes it clear there is one per network). Done.
https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( On 2016/10/25 17:00:06, pthatcher1 wrote: > On 2016/10/24 23:35:09, honghaiz3 wrote: > > On 2016/10/24 20:49:05, pthatcher1 wrote: > > > Can't this just be IsConnectionPingable? > > No. In this method, it is critical to ping connection once every XX (1 or 2.5) > > seconds. Otherwise, the set of best connections will utilize all slots for > ping > > transmissions. > > What I mean is that since you're already passing in a connection, there's > nothing about the method specific to "premier" connections. It's logic that can > apply to any connection. > > The logic in the method really "a connection is pingable if we can send a ping > and we haven't sent a ping very recently", which seems to apply to any > connection. Except there's a completely different method, "IsPingable" for determining if a "normal" connection should be pinged. This method is really "Should this premier connection be pinged in place of a normal connection, since the channel is currently weakly connected?" When I mentioned that this logic is getting confusing, this is the kind of thing I meant. It's spread across many methods and it's hard to tell how an individual method is used given just its name.
https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( On 2016/10/25 18:29:28, Taylor Brandstetter wrote: > On 2016/10/25 17:00:06, pthatcher1 wrote: > > On 2016/10/24 23:35:09, honghaiz3 wrote: > > > On 2016/10/24 20:49:05, pthatcher1 wrote: > > > > Can't this just be IsConnectionPingable? > > > No. In this method, it is critical to ping connection once every XX (1 or > 2.5) > > > seconds. Otherwise, the set of best connections will utilize all slots for > > ping > > > transmissions. > > > > What I mean is that since you're already passing in a connection, there's > > nothing about the method specific to "premier" connections. It's logic that > can > > apply to any connection. > > > > The logic in the method really "a connection is pingable if we can send a ping > > and we haven't sent a ping very recently", which seems to apply to any > > connection. > > Except there's a completely different method, "IsPingable" for determining if a > "normal" connection should be pinged. This method is really "Should this premier > connection be pinged in place of a normal connection, since the channel is > currently weakly connected?" > > When I mentioned that this logic is getting confusing, this is the kind of thing > I meant. It's spread across many methods and it's hard to tell how an individual > method is used given just its name. IsPremierConnectionPingable (now it is called IsPerNetworkBestConnectionPingable) is specifically used for PerNetworkBestConnections. IsPingable is used for a normal connection everywhere else. We have to differentiate these two as I explained, similar to the differentiation we had for IsSelectedConnectionPingable vs IsPingable.
https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( On 2016/10/25 22:01:24, honghaiz3 wrote: > On 2016/10/25 18:29:28, Taylor Brandstetter wrote: > > On 2016/10/25 17:00:06, pthatcher1 wrote: > > > On 2016/10/24 23:35:09, honghaiz3 wrote: > > > > On 2016/10/24 20:49:05, pthatcher1 wrote: > > > > > Can't this just be IsConnectionPingable? > > > > No. In this method, it is critical to ping connection once every XX (1 or > > 2.5) > > > > seconds. Otherwise, the set of best connections will utilize all slots for > > > ping > > > > transmissions. > > > > > > What I mean is that since you're already passing in a connection, there's > > > nothing about the method specific to "premier" connections. It's logic that > > can > > > apply to any connection. > > > > > > The logic in the method really "a connection is pingable if we can send a > ping > > > and we haven't sent a ping very recently", which seems to apply to any > > > connection. > > > > Except there's a completely different method, "IsPingable" for determining if > a > > "normal" connection should be pinged. This method is really "Should this > premier > > connection be pinged in place of a normal connection, since the channel is > > currently weakly connected?" > > > > When I mentioned that this logic is getting confusing, this is the kind of > thing > > I meant. It's spread across many methods and it's hard to tell how an > individual > > method is used given just its name. > IsPremierConnectionPingable (now it is called > IsPerNetworkBestConnectionPingable) is specifically used for > PerNetworkBestConnections. IsPingable is used for a normal connection everywhere > else. We have to differentiate these two as I explained, similar to the > differentiation we had for IsSelectedConnectionPingable vs IsPingable. I agree with Taylor that this is confusing. IsPingable has clear meaning. ShouldBePingedWithHigherPriorityBecauseWereWeaklyConnected is more awkward. Let's step back a bit and realize that this is all just to implement FindNextPingableConnection(). In fact we now have several methods which just exist to implement FindNextPingableConnection: FindConnectionToPing FindOldestConnectionNeedingTriggeredCheck SelectMostPingableConnection FindBestPremierConnectionToPing IsPremierConnectionPingable Perhaps we should rethink how we want FindNextPingableConnection to work. Perhaps we don't need all these separate methods after all and can simplify between them. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1660: } On 2016/10/25 18:01:09, honghaiz3 wrote: > On 2016/10/25 17:00:06, pthatcher1 wrote: > > Can we do something like this? > > > > auto oldest_best_connection_by_network = > > std::min(best_connection_by_network.values(), > > [](Connection* conn1, Connection* conn2) { > > // Push unpingable connections to the max end to make pingable connections > > be at the min end. > > return !IsConnectionPingable(conn2) || conn1->last_ping_sent() < > > conn2->last_ping_sent(); > > }); > > if (IsConnectionPingable(oldest_best_connection_by_network)) { > > return oldest_best_connection_by_network; > > } > It will need to be something like this: > auto oldest_best_connection_by_network = > std::min(best_connection_by_network.values(), > [](Connection* conn1, Connection* conn2) { > // Push unpingable connections to the max end to make pingable connections > be at the min end. > return IsPerNetworkBestConnectionPingable(conn1) < > IsPerNetworkBestConnectionPingable(conn2) || conn1->last_ping_sent() < > conn2->last_ping_sent(); > }); > if (IsPerNetworkBestConnectionPingable(oldest_best_connection_by_network)) { > return oldest_best_connection_by_network; > } > I do not feel this is much more readable. It needs to check > IsPerNetworkBestConnectionPingable more times. Plus, The comparing logic could > easily get wrong. So I would prefer the current one in the code. It might be a > few more lines, but it is hard to get wrong. You don't need to call IsPerNetworkBestConnectionPingable twice. Once should be enough. We're just taking the min, but not sorting. But you bring up a good point that it would be more clear to do something like this: auto oldest_pingable_best_connection = std::min(std::filter(best_connection_by_network.values(), ShouldPingWithHigherPriority), [](Connection* conn1, Connection* conn2) { conn1->last_ping_sent() < conn2->last_ping_sent();})); I don't know if there is such a std::filter, though. If there is, I think that would be more readable. But maybe it doesn't matter, because if we cleanup GetNextPingableConnection from the top, we might not need this after all.
https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( On 2016/10/26 22:37:50, pthatcher1 wrote: > On 2016/10/25 22:01:24, honghaiz3 wrote: > > On 2016/10/25 18:29:28, Taylor Brandstetter wrote: > > > On 2016/10/25 17:00:06, pthatcher1 wrote: > > > > On 2016/10/24 23:35:09, honghaiz3 wrote: > > > > > On 2016/10/24 20:49:05, pthatcher1 wrote: > > > > > > Can't this just be IsConnectionPingable? > > > > > No. In this method, it is critical to ping connection once every XX (1 > or > > > 2.5) > > > > > seconds. Otherwise, the set of best connections will utilize all slots > for > > > > ping > > > > > transmissions. > > > > > > > > What I mean is that since you're already passing in a connection, there's > > > > nothing about the method specific to "premier" connections. It's logic > that > > > can > > > > apply to any connection. > > > > > > > > The logic in the method really "a connection is pingable if we can send a > > ping > > > > and we haven't sent a ping very recently", which seems to apply to any > > > > connection. > > > > > > Except there's a completely different method, "IsPingable" for determining > if > > a > > > "normal" connection should be pinged. This method is really "Should this > > premier > > > connection be pinged in place of a normal connection, since the channel is > > > currently weakly connected?" > > > > > > When I mentioned that this logic is getting confusing, this is the kind of > > thing > > > I meant. It's spread across many methods and it's hard to tell how an > > individual > > > method is used given just its name. > > IsPremierConnectionPingable (now it is called > > IsPerNetworkBestConnectionPingable) is specifically used for > > PerNetworkBestConnections. IsPingable is used for a normal connection > everywhere > > else. We have to differentiate these two as I explained, similar to the > > differentiation we had for IsSelectedConnectionPingable vs IsPingable. > > I agree with Taylor that this is confusing. IsPingable has clear meaning. > ShouldBePingedWithHigherPriorityBecauseWereWeaklyConnected is more awkward. > > Let's step back a bit and realize that this is all just to implement > FindNextPingableConnection(). In fact we now have several methods which just > exist to implement FindNextPingableConnection: > > FindConnectionToPing > FindOldestConnectionNeedingTriggeredCheck > SelectMostPingableConnection > FindBestPremierConnectionToPing > IsPremierConnectionPingable > > > Perhaps we should rethink how we want FindNextPingableConnection to work. > Perhaps we don't need all these separate methods after all and can simplify > between them. > > > > FindNextPingableConnection has the following steps in order: 1. If the selected connection is pingable, ping it. 2. If weak, and the premier connection (PerNetworkBestConnection) is pingable, ping it 3. If there is ping needed for triggered check, then ping it. 4. Ping every connection (using normal order or finding the connection that is most likely to work). Only step 2 (and it is sort of similar to step 1) was added in this CL. I don't see an easy way to simplify them without behavior changing. Maybe we can do something like assigning each connection a ping score and then find the one with the highest score. But we will still need to decide the score based on all four steps above. Because we have the 64 bit timestamp as part of the score, we will need something like tuple to represent the score. We can potentially merge some steps (possibly 3 and 4) with some behavior changing. But if we just want to merge 3 and 4, we should leave it in a separate CL. We can discuss more on this. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1660: } On 2016/10/26 22:37:50, pthatcher1 wrote: > On 2016/10/25 18:01:09, honghaiz3 wrote: > > On 2016/10/25 17:00:06, pthatcher1 wrote: > > > Can we do something like this? > > > > > > auto oldest_best_connection_by_network = > > > std::min(best_connection_by_network.values(), > > > [](Connection* conn1, Connection* conn2) { > > > // Push unpingable connections to the max end to make pingable > connections > > > be at the min end. > > > return !IsConnectionPingable(conn2) || conn1->last_ping_sent() < > > > conn2->last_ping_sent(); > > > }); > > > if (IsConnectionPingable(oldest_best_connection_by_network)) { > > > return oldest_best_connection_by_network; > > > } > > It will need to be something like this: > > auto oldest_best_connection_by_network = > > std::min(best_connection_by_network.values(), > > [](Connection* conn1, Connection* conn2) { > > // Push unpingable connections to the max end to make pingable connections > > be at the min end. > > return IsPerNetworkBestConnectionPingable(conn1) < > > IsPerNetworkBestConnectionPingable(conn2) || conn1->last_ping_sent() < > > conn2->last_ping_sent(); > > }); > > if (IsPerNetworkBestConnectionPingable(oldest_best_connection_by_network)) { > > return oldest_best_connection_by_network; > > } > > I do not feel this is much more readable. It needs to check > > IsPerNetworkBestConnectionPingable more times. Plus, The comparing logic could > > easily get wrong. So I would prefer the current one in the code. It might be a > > few more lines, but it is hard to get wrong. > > > You don't need to call IsPerNetworkBestConnectionPingable twice. Once should be > enough. We're just taking the min, but not sorting. But you bring up a good > point that it would be more clear to do something like this: > > auto oldest_pingable_best_connection = > std::min(std::filter(best_connection_by_network.values(), > ShouldPingWithHigherPriority), [](Connection* conn1, Connection* conn2) { > conn1->last_ping_sent() < conn2->last_ping_sent();})); > > I don't know if there is such a std::filter, though. If there is, I think that > would be more readable. > > But maybe it doesn't matter, because if we cleanup GetNextPingableConnection > from the top, we might not need this after all. We can do that. Will defer this until the other, main comment is resolved.
https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( On 2016/10/27 19:20:58, honghaiz3 wrote: > On 2016/10/26 22:37:50, pthatcher1 wrote: > > On 2016/10/25 22:01:24, honghaiz3 wrote: > > > On 2016/10/25 18:29:28, Taylor Brandstetter wrote: > > > > On 2016/10/25 17:00:06, pthatcher1 wrote: > > > > > On 2016/10/24 23:35:09, honghaiz3 wrote: > > > > > > On 2016/10/24 20:49:05, pthatcher1 wrote: > > > > > > > Can't this just be IsConnectionPingable? > > > > > > No. In this method, it is critical to ping connection once every XX (1 > > or > > > > 2.5) > > > > > > seconds. Otherwise, the set of best connections will utilize all slots > > for > > > > > ping > > > > > > transmissions. > > > > > > > > > > What I mean is that since you're already passing in a connection, > there's > > > > > nothing about the method specific to "premier" connections. It's logic > > that > > > > can > > > > > apply to any connection. > > > > > > > > > > The logic in the method really "a connection is pingable if we can send > a > > > ping > > > > > and we haven't sent a ping very recently", which seems to apply to any > > > > > connection. > > > > > > > > Except there's a completely different method, "IsPingable" for determining > > if > > > a > > > > "normal" connection should be pinged. This method is really "Should this > > > premier > > > > connection be pinged in place of a normal connection, since the channel is > > > > currently weakly connected?" > > > > > > > > When I mentioned that this logic is getting confusing, this is the kind of > > > thing > > > > I meant. It's spread across many methods and it's hard to tell how an > > > individual > > > > method is used given just its name. > > > IsPremierConnectionPingable (now it is called > > > IsPerNetworkBestConnectionPingable) is specifically used for > > > PerNetworkBestConnections. IsPingable is used for a normal connection > > everywhere > > > else. We have to differentiate these two as I explained, similar to the > > > differentiation we had for IsSelectedConnectionPingable vs IsPingable. > > > > I agree with Taylor that this is confusing. IsPingable has clear meaning. > > ShouldBePingedWithHigherPriorityBecauseWereWeaklyConnected is more awkward. > > > > Let's step back a bit and realize that this is all just to implement > > FindNextPingableConnection(). In fact we now have several methods which > just > > exist to implement FindNextPingableConnection: > > > > FindConnectionToPing > > FindOldestConnectionNeedingTriggeredCheck > > SelectMostPingableConnection > > FindBestPremierConnectionToPing > > IsPremierConnectionPingable > > > > > > Perhaps we should rethink how we want FindNextPingableConnection to work. > > Perhaps we don't need all these separate methods after all and can simplify > > between them. > > > > > > > > > FindNextPingableConnection has the following steps in order: > 1. If the selected connection is pingable, ping it. > 2. If weak, and the premier connection (PerNetworkBestConnection) is pingable, > ping it > 3. If there is ping needed for triggered check, then ping it. > 4. Ping every connection (using normal order or finding the connection that is > most likely to work). > Only step 2 (and it is sort of similar to step 1) was added in this CL. > I don't see an easy way to simplify them without behavior changing. Maybe we can > do something like assigning each connection a ping score and then find the one > with the highest score. But we will still need to decide the score based on all > four steps above. Because we have the 64 bit timestamp as part of the score, we > will need something like tuple to represent the score. > We can potentially merge some steps (possibly 3 and 4) with some behavior > changing. But if we just want to merge 3 and 4, we should leave it in a separate > CL. > We can discuss more on this. I had an idea similar to the "ping score" concept. Basically, break things apart into the following phases: 1. For each connection, check if it's eligible to be pinged (based on state, type of connection, when it was last pinged, etc.). 2. Use a sorting function to sort the eligible connections according to how badly we want to ping them. 3. Ping the topmost one. However, any of the refactoring being discussed would only make sense in a standalone CL, so I think this is fine as-is; it at least matches the current conventions.
https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/380001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1625: bool P2PTransportChannel::IsPremierConnectionPingable( On 2016/10/27 21:04:54, Taylor Brandstetter wrote: > On 2016/10/27 19:20:58, honghaiz3 wrote: > > On 2016/10/26 22:37:50, pthatcher1 wrote: > > > On 2016/10/25 22:01:24, honghaiz3 wrote: > > > > On 2016/10/25 18:29:28, Taylor Brandstetter wrote: > > > > > On 2016/10/25 17:00:06, pthatcher1 wrote: > > > > > > On 2016/10/24 23:35:09, honghaiz3 wrote: > > > > > > > On 2016/10/24 20:49:05, pthatcher1 wrote: > > > > > > > > Can't this just be IsConnectionPingable? > > > > > > > No. In this method, it is critical to ping connection once every XX > (1 > > > or > > > > > 2.5) > > > > > > > seconds. Otherwise, the set of best connections will utilize all > slots > > > for > > > > > > ping > > > > > > > transmissions. > > > > > > > > > > > > What I mean is that since you're already passing in a connection, > > there's > > > > > > nothing about the method specific to "premier" connections. It's > logic > > > that > > > > > can > > > > > > apply to any connection. > > > > > > > > > > > > The logic in the method really "a connection is pingable if we can > send > > a > > > > ping > > > > > > and we haven't sent a ping very recently", which seems to apply to any > > > > > > connection. > > > > > > > > > > Except there's a completely different method, "IsPingable" for > determining > > > if > > > > a > > > > > "normal" connection should be pinged. This method is really "Should this > > > > premier > > > > > connection be pinged in place of a normal connection, since the channel > is > > > > > currently weakly connected?" > > > > > > > > > > When I mentioned that this logic is getting confusing, this is the kind > of > > > > thing > > > > > I meant. It's spread across many methods and it's hard to tell how an > > > > individual > > > > > method is used given just its name. > > > > IsPremierConnectionPingable (now it is called > > > > IsPerNetworkBestConnectionPingable) is specifically used for > > > > PerNetworkBestConnections. IsPingable is used for a normal connection > > > everywhere > > > > else. We have to differentiate these two as I explained, similar to the > > > > differentiation we had for IsSelectedConnectionPingable vs IsPingable. > > > > > > I agree with Taylor that this is confusing. IsPingable has clear meaning. > > > ShouldBePingedWithHigherPriorityBecauseWereWeaklyConnected is more awkward. > > > > > > > Let's step back a bit and realize that this is all just to implement > > > FindNextPingableConnection(). In fact we now have several methods which > > just > > > exist to implement FindNextPingableConnection: > > > > > > FindConnectionToPing > > > FindOldestConnectionNeedingTriggeredCheck > > > SelectMostPingableConnection > > > FindBestPremierConnectionToPing > > > IsPremierConnectionPingable > > > > > > > > > Perhaps we should rethink how we want FindNextPingableConnection to work. > > > Perhaps we don't need all these separate methods after all and can simplify > > > between them. > > > > > > > > > > > > > > FindNextPingableConnection has the following steps in order: > > 1. If the selected connection is pingable, ping it. > > 2. If weak, and the premier connection (PerNetworkBestConnection) is pingable, > > ping it > > 3. If there is ping needed for triggered check, then ping it. > > 4. Ping every connection (using normal order or finding the connection that is > > most likely to work). > > Only step 2 (and it is sort of similar to step 1) was added in this CL. > > I don't see an easy way to simplify them without behavior changing. Maybe we > can > > do something like assigning each connection a ping score and then find the one > > with the highest score. But we will still need to decide the score based on > all > > four steps above. Because we have the 64 bit timestamp as part of the score, > we > > will need something like tuple to represent the score. > > We can potentially merge some steps (possibly 3 and 4) with some behavior > > changing. But if we just want to merge 3 and 4, we should leave it in a > separate > > CL. > > We can discuss more on this. > > I had an idea similar to the "ping score" concept. Basically, break things apart > into the following phases: > > 1. For each connection, check if it's eligible to be pinged (based on state, > type of connection, when it was last pinged, etc.). > 2. Use a sorting function to sort the eligible connections according to how > badly we want to ping them. > 3. Ping the topmost one. > > However, any of the refactoring being discussed would only make sense in a > standalone CL, so I think this is fine as-is; it at least matches the current > conventions. Note a few details: 1. The eligibility of a connection (i.e. Pingable) may be different depending on the stage. For example, as a premier connection or selected connection, it may not be pingable if a connection was pinged in the past second. This prevents a few premier connections including the selected connection from saturating all transmission slots. But as a normal connection, it is pingable as long as the current selected connection is weak so that it can use all allocated transmission slots to find a better connection when the present connection is weak. 2. If our goal is to find the topmost connection, we don't need sort for this and for finding the selected connection. Using std::max/min should be sufficient. These details can be further discussed in a separate CL. https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/400001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1660: } On 2016/10/27 19:20:58, honghaiz3 wrote: > On 2016/10/26 22:37:50, pthatcher1 wrote: > > On 2016/10/25 18:01:09, honghaiz3 wrote: > > > On 2016/10/25 17:00:06, pthatcher1 wrote: > > > > Can we do something like this? > > > > > > > > auto oldest_best_connection_by_network = > > > > std::min(best_connection_by_network.values(), > > > > [](Connection* conn1, Connection* conn2) { > > > > // Push unpingable connections to the max end to make pingable > > connections > > > > be at the min end. > > > > return !IsConnectionPingable(conn2) || conn1->last_ping_sent() < > > > > conn2->last_ping_sent(); > > > > }); > > > > if (IsConnectionPingable(oldest_best_connection_by_network)) { > > > > return oldest_best_connection_by_network; > > > > } > > > It will need to be something like this: > > > auto oldest_best_connection_by_network = > > > std::min(best_connection_by_network.values(), > > > [](Connection* conn1, Connection* conn2) { > > > // Push unpingable connections to the max end to make pingable > connections > > > be at the min end. > > > return IsPerNetworkBestConnectionPingable(conn1) < > > > IsPerNetworkBestConnectionPingable(conn2) || conn1->last_ping_sent() < > > > conn2->last_ping_sent(); > > > }); > > > if (IsPerNetworkBestConnectionPingable(oldest_best_connection_by_network)) { > > > return oldest_best_connection_by_network; > > > } > > > I do not feel this is much more readable. It needs to check > > > IsPerNetworkBestConnectionPingable more times. Plus, The comparing logic > could > > > easily get wrong. So I would prefer the current one in the code. It might be > a > > > few more lines, but it is hard to get wrong. > > > > > > You don't need to call IsPerNetworkBestConnectionPingable twice. Once should > be > > enough. We're just taking the min, but not sorting. But you bring up a good > > point that it would be more clear to do something like this: > > > > auto oldest_pingable_best_connection = > > std::min(std::filter(best_connection_by_network.values(), > > ShouldPingWithHigherPriority), [](Connection* conn1, Connection* conn2) { > > conn1->last_ping_sent() < conn2->last_ping_sent();})); > > > > I don't know if there is such a std::filter, though. If there is, I think > that > > would be more readable. > > > > But maybe it doesn't matter, because if we cleanup GetNextPingableConnection > > from the top, we might not need this after all. > We can do that. Will defer this until the other, main comment is resolved. copy_if is the c++ filter but unfortunately, c++ map does not have the value method to get all values in this map (i.e., connections). I do not feel it would be worth iterating the map to find the list of connections, apply filter and then apply std::min to get the minimum.
Patchset #8 (id:480001) has been deleted
I rearranged the FindNextPingableConnection method as suggested by Peter. PTAL.
lgtm. This restructuring looks really good! Especially with the detailed comments. I think it would have been better in a standalone CL, but it would be too much work to split them apart now. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1646: // connection per network be pinged frequently enough in order for it to be be -> is https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1648: // Rule 2.1: Among such connections, pick the one with the smallest smallest -> earliest https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1661: return conn1->last_ping_sent() < This isn't a new issue, but this comparison doesn't handle time wraparound. We may want to file a bug if we don't have one. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1687: pinged_connections_.end()); This is still a confusing part (pinged connections being added to unpinged_connections_), but should be changed in a later CL if at all.
Much easier to read. But I think we might need to do "writable" then "best" instead of "writable" then "best", although for backup connections, it might not matter. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1323: for (Connection* conn : connections_) { Should put a TODO to get off of connections_ being sorted. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1334: for (auto kv : GetBestConnectionByNetwork()) { If we have two connections on a network and the best one isn't writable, this won't pick any. Don't we want it to pick the best out of the writable ones?
PTAL. Thanks! https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1323: for (Connection* conn : connections_) { On 2016/10/31 18:21:27, pthatcher1 wrote: > Should put a TODO to get off of connections_ being sorted. Done. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1334: for (auto kv : GetBestConnectionByNetwork()) { On 2016/10/31 18:21:27, pthatcher1 wrote: > If we have two connections on a network and the best one isn't writable, this > won't pick any. Don't we want it to pick the best out of the writable ones? I do not think so. If a connection is not writable, it is not clear whether we should pin on that connection yet. This has the same logic as the selected connection. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1646: // connection per network be pinged frequently enough in order for it to be On 2016/10/29 00:27:45, Taylor Brandstetter wrote: > be -> is Done. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1648: // Rule 2.1: Among such connections, pick the one with the smallest On 2016/10/29 00:27:45, Taylor Brandstetter wrote: > smallest -> earliest Done. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1661: return conn1->last_ping_sent() < On 2016/10/29 00:27:45, Taylor Brandstetter wrote: > This isn't a new issue, but this comparison doesn't handle time wraparound. We > may want to file a bug if we don't have one. We are using 64 bit time now. I think won't need to worry about the time wraparound for at least hundreds of years. https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1687: pinged_connections_.end()); On 2016/10/29 00:27:45, Taylor Brandstetter wrote: > This is still a confusing part (pinged connections being added to > unpinged_connections_), but should be changed in a later CL if at all. I agree. I think we can just use a state pinged_ in the connection instead of maintaining two separate vectors. But that belongs to a separate CL.
On 2016/10/31 19:17:30, honghaiz3 wrote: > PTAL. Thanks! > > https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... > File webrtc/p2p/base/p2ptransportchannel.cc (right): > > https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1323: for (Connection* conn : > connections_) { > On 2016/10/31 18:21:27, pthatcher1 wrote: > > Should put a TODO to get off of connections_ being sorted. > > Done. > > https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1334: for (auto kv : > GetBestConnectionByNetwork()) { > On 2016/10/31 18:21:27, pthatcher1 wrote: > > If we have two connections on a network and the best one isn't writable, this > > won't pick any. Don't we want it to pick the best out of the writable ones? > I do not think so. If a connection is not writable, it is not clear whether we > should pin on that connection yet. > This has the same logic as the selected connection. > > https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1646: // connection per network be pinged > frequently enough in order for it to be > On 2016/10/29 00:27:45, Taylor Brandstetter wrote: > > be -> is > > Done. > > https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1648: // Rule 2.1: Among such > connections, pick the one with the smallest > On 2016/10/29 00:27:45, Taylor Brandstetter wrote: > > smallest -> earliest > > Done. > > https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1661: return conn1->last_ping_sent() < > On 2016/10/29 00:27:45, Taylor Brandstetter wrote: > > This isn't a new issue, but this comparison doesn't handle time wraparound. We > > may want to file a bug if we don't have one. > We are using 64 bit time now. I think won't need to worry about the time > wraparound for at least hundreds of years. > > https://codereview.webrtc.org/2369963004/diff/500001/webrtc/p2p/base/p2ptrans... > webrtc/p2p/base/p2ptransportchannel.cc:1687: pinged_connections_.end()); > On 2016/10/29 00:27:45, Taylor Brandstetter wrote: > > This is still a confusing part (pinged connections being added to > > unpinged_connections_), but should be changed in a later CL if at all. > I agree. I think we can just use a state pinged_ in the connection instead of > maintaining two separate vectors. But that belongs to a separate CL. Peter, can you take another look?
lgtm
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2369963004/#ps580001 (title: "Merge to head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2958)
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2369963004/#ps600001 (title: "Add a header file <iterator>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there are a lot of connections. With this CL, if the selected connection is weak, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. Plus, if the selected connection is weak, we choose a shorter ping interval (900ms) for all writable connections. BUG=b/32022719 ========== to ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there are a lot of connections. With this CL, if the selected connection is weak, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. Plus, if the selected connection is weak, we choose a shorter ping interval (900ms) for all writable connections. BUG=b/32022719 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there are a lot of connections. With this CL, if the selected connection is weak, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. Plus, if the selected connection is weak, we choose a shorter ping interval (900ms) for all writable connections. BUG=b/32022719 ========== to ========== Ping the premier connection on each network with higher priority. When the selected connection becomes not receiving and there are many connections, If we use a round-robin fashion to ping all connections, none of the connections will be in receiving state for sufficient long time to ensure switching connections promptly. Triggered check will help in this situation to some extent but it may still fail to switch promptly when there are a lot of connections. With this CL, if the selected connection is weak, once we find a writable connection on a network we start to ping it with a higher priority to keep it in receiving state. Plus, if the selected connection is weak, we choose a shorter ping interval (900ms) for all writable connections. BUG=b/32022719 Committed: https://crrev.com/7252a00b9f2c5cb8b0a0557386ea3a8c29b26b9c Cr-Commit-Position: refs/heads/master@{#14991} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/7252a00b9f2c5cb8b0a0557386ea3a8c29b26b9c Cr-Commit-Position: refs/heads/master@{#14991} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
