Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2093623004:
Add config to prune TURN ports (Closed)
DescriptionAdd config to prune low-priority TURN ports for creating connections
When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned).
This effectively reduces the number of TURN candidates and connections created by TURN ports.
BUG=
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/b9e7b4ad66a0466b83545273ad99f030c24bea7f
Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973
Cr-Original-Commit-Position: refs/heads/master@{#13335}
Cr-Commit-Position: refs/heads/master@{#13354}
Patch Set 1 : Merge with head #Patch Set 2 : . #Patch Set 3 : Add all tests and fix a bug to set port type #
Total comments: 30
Patch Set 4 : Address comments #Patch Set 5 : Minor fixes #Patch Set 6 : Merge branch 'master' into turn_port_exposer #
Total comments: 30
Patch Set 7 : Address comments #Patch Set 8 : Minor fix #
Total comments: 10
Patch Set 9 : . #
Total comments: 4
Patch Set 10 : . #
Total comments: 16
Patch Set 11 : Address Taylor's comments #Patch Set 12 : Initialized prune_turn_ports_ and removed an unused method #Patch Set 13 : Merge branch 'master' into turn_port_exposer #Patch Set 14 : Partially disable the test TestEachInterfaceHasItsOwnTurnPorts #
Messages
Total messages: 70 (37 generated)
Description was changed from ========== Do not use low-priority TURN ports to gather candidates. BUG= ========== to ========== Add config with disabling low-priority TURN ports to gather candidates. This effectively reduces the number of candidates and connections created by TURN ports. BUG= ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add config with disabling low-priority TURN ports to gather candidates. This effectively reduces the number of candidates and connections created by TURN ports. BUG= ========== to ========== Add config with disabling low-priority TURN ports to gather candidates. This effectively reduces the number of candidates and connections created by TURN ports. Plus, also do not create a port if the local address family and the server address family are not compatible. BUG= ==========
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
This is the CL to not use low-priority TURN ports. You may want to take a look. Still need to add tests yet.
Description was changed from ========== Add config with disabling low-priority TURN ports to gather candidates. This effectively reduces the number of candidates and connections created by TURN ports. Plus, also do not create a port if the local address family and the server address family are not compatible. BUG= ========== to ========== Add config to not use low-priority TURN ports for creating connections This effectively reduces the number of TURN candidates and connections created by TURN ports. Plus, also do not create a port if the local address family and the server address family are not compatible. BUG= ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Added tests. This includes a bug fix CL https://codereview.webrtc.org/2099023002/ in oder for the tests to pass. If you would like to shorten this CL, you can review the other one first.
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14507)
https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1653: if (session != allocator_session()) { Can you comment on why we need this? Will this happen for an allocator session other than the latest? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:181: sigslot::signal2<PortAllocatorSession*, PortInterface*> SignalPortDeprecated; Can you comment on what it means for a port to be "deprecated"? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:251: bool disable_low_priority_turn_ports); Can you comment on what it means to be "low priority" and what "disable" means? Should we call, instead, something like prune_turn_ports_? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:64: } Is this just for sorting? If so, instead of making an emum for address family, it seems like we should just make it part a int GetAddressFamilyPriority(int ip_family). UNSPEC -> 0, IPV4 -> 1, IPV6 -> 2. Actually, why are we preferring ipv4 over ipv6? Shouldn't be the other way around? Or should we just prefer which ever TURN candidate works first (lowest RTT)? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:630: MaybeSignalPortReady(port); If this is only called once, can we just inline it? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:641: MaybeSignalCandidatesReady(port, candidates); Since this is only called once, can you inline it? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:652: turn_ports_in_use_by_network_names_[port->Network()->name()]; Shouldn't we do a find() just in case it's not in the map? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:671: } Correct me if I'm wrong, but it looks like turn_ports are always equal. In that case, you don't need to check all of them. Just check the first. So all you have to do is something like this: int cmp = ComparePorts(port, turn_ports[0]); if (cmp > 0) { // Ignore the port; It's worse than ones we already have. return; } if (cmp < 0) { // Stop using all the rest; They are worse than the new one we have. for (Port* deprecated_port : turn_ports) { SignalPortDeprecated(this, deprecated_port); } turn_ports.Clear(); } turn_ports.push_back(port); SignalPortReady(port); https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:672: if (use_port) { Actually, if we just store all ports in ports_ and mark them deprecated, then this would be much more simple. Just add this one, then use std::max to find the best relay port. Then, go through ports_ and mark all relay ports worse than the best one as deprecated. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:674: SignalPortReady(this, port); I think we need a better pair of names than SignalPortReady/SignalPortDeprecated. How about SignalPortPairable/SignalPortUnpairable? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:704: turn_ports.end(); If we store all port in ports_ and just mark them deprecated, then this would be as simple as finding the port in ports_ and then looking at the state. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:1198: } Should this be a separate CL? It seems like a bug fix. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.h (right): https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:198: typedef std::vector<Port*> PortArray; I'd prefer not to have a typedef and just have std::map<std::string, std::vector<Port*>> https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:200: std::map<std::string, PortArray> turn_ports_in_use_by_network_names_; Why not just turn_ports_by_network_name? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:201: Actually, instead of having a separate map for turn ports that we are using or not by network name, why not just use the ports inside of ports_? And if we deprecate a port, set the state in PortData of that port to STATE_DEPRECATED?
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
PTAL. Thanks! https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1653: if (session != allocator_session()) { On 2016/06/27 20:35:54, pthatcher1 wrote: > Can you comment on why we need this? Will this happen for an allocator session > other than the latest? Perhaps this is not needed. If this is from the previous session, it should have been removed anyway. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:181: sigslot::signal2<PortAllocatorSession*, PortInterface*> SignalPortDeprecated; On 2016/06/27 20:35:54, pthatcher1 wrote: > Can you comment on what it means for a port to be "deprecated"? Done. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:251: bool disable_low_priority_turn_ports); On 2016/06/27 20:35:54, pthatcher1 wrote: > Can you comment on what it means to be "low priority" and what "disable" means? > > Should we call, instead, something like prune_turn_ports_? Changed to prune_turn_ports and used SignalPortPruned instead of SignalPortDeprecated https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:64: } On 2016/06/27 20:35:55, pthatcher1 wrote: > Is this just for sorting? If so, instead of making an emum for address family, > it seems like we should just make it part a int GetAddressFamilyPriority(int > ip_family). UNSPEC -> 0, IPV4 -> 1, IPV6 -> 2. You are right. Make it return an integer. > > Actually, why are we preferring ipv4 over ipv6? Shouldn't be the other way > around? Or should we just prefer which ever TURN candidate works first (lowest > RTT)? I am giving IPv6 higher priority than Ipv4 already. If we choose whichever candidate works first, it will essentially choose the first port becoming ready. I think we discussed that and we don't want that approach, although not particular about IpV4 vs IPv6. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:630: MaybeSignalPortReady(port); On 2016/06/27 20:35:55, pthatcher1 wrote: > If this is only called once, can we just inline it? A separate method will make it more readable here. Plus it allows early return in the method to be called. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:641: MaybeSignalCandidatesReady(port, candidates); On 2016/06/27 20:35:54, pthatcher1 wrote: > Since this is only called once, can you inline it? Done. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:652: turn_ports_in_use_by_network_names_[port->Network()->name()]; On 2016/06/27 20:35:55, pthatcher1 wrote: > Shouldn't we do a find() just in case it's not in the map? If it is not in the map, we create a new vector and add it to the map. That's what we want. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:671: } On 2016/06/27 20:35:54, pthatcher1 wrote: > Correct me if I'm wrong, but it looks like turn_ports are always equal. In that > case, you don't need to check all of them. Just check the first. So all you > have to do is something like this: > > int cmp = ComparePorts(port, turn_ports[0]); > if (cmp > 0) { > // Ignore the port; It's worse than ones we already have. > return; > } > if (cmp < 0) { > // Stop using all the rest; They are worse than the new one we have. > for (Port* deprecated_port : turn_ports) { > SignalPortDeprecated(this, deprecated_port); > } > turn_ports.Clear(); > } > turn_ports.push_back(port); > SignalPortReady(port); You are right, although we have removed the map now. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:672: if (use_port) { On 2016/06/27 20:35:55, pthatcher1 wrote: > Actually, if we just store all ports in ports_ and mark them deprecated, then > this would be much more simple. Just add this one, then use std::max to find > the best relay port. Then, go through ports_ and mark all relay ports worse > than the best one as deprecated. Done. Did not use std::max as it is not really much more easier than find the max directly. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:674: SignalPortReady(this, port); On 2016/06/27 20:35:54, pthatcher1 wrote: > I think we need a better pair of names than > SignalPortReady/SignalPortDeprecated. How about > SignalPortPairable/SignalPortUnpairable? SignalPortReady is used elsewhere by downstream code. Changing this will break the other usage. Should we keep it (until we want to change the name everywhere) and perhaps just change the name of the newly added signal? https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:704: turn_ports.end(); On 2016/06/27 20:35:54, pthatcher1 wrote: > If we store all port in ports_ and just mark them deprecated, then this would be > as simple as finding the port in ports_ and then looking at the state. Done. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:1198: } On 2016/06/27 20:35:54, pthatcher1 wrote: > Should this be a separate CL? It seems like a bug fix. Done. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.h (right): https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:198: typedef std::vector<Port*> PortArray; On 2016/06/27 20:35:55, pthatcher1 wrote: > I'd prefer not to have a typedef and just have > > std::map<std::string, std::vector<Port*>> Done. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:200: std::map<std::string, PortArray> turn_ports_in_use_by_network_names_; On 2016/06/27 20:35:55, pthatcher1 wrote: > Why not just turn_ports_by_network_name? Removed this. https://codereview.webrtc.org/2093623004/diff/100001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:201: On 2016/06/27 20:35:55, pthatcher1 wrote: > Actually, instead of having a separate map for turn ports that we are using or > not by network name, why not just use the ports inside of ports_? And if we > deprecate a port, set the state in PortData of that port to STATE_DEPRECATED? Done.
Description was changed from ========== Add config to not use low-priority TURN ports for creating connections This effectively reduces the number of TURN candidates and connections created by TURN ports. Plus, also do not create a port if the local address family and the server address family are not compatible. BUG= ========== to ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= ==========
https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:181: // A TURN port is pruned if there is a higher-priority TURN port becomes if there is a => if a https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:75: } I think it makes sense to prefer UDP > TCP, but I still don't see what we gain by preferring ipv4 over ipv6 (or vice versa). https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:254: if (IsPortInUse(data)) { Since the existing port is ReadyPorts, why not calls this PortIsReady()? And, in fact, why not just put it on PortData as .ready()? https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:318: // If all allocated ports are in one of the terminal states, session must have Could say "if all allocated ports are no longer gathering". https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:321: if (std::all_of(ports_.begin(), ports_.end(), Could be "std::none_of(ports_.... port.gathering();} )" https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:326: return false; Instead of if(foo) return true; return false; Why not return foo; ? https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:396: if (!it->terminated()) { it->gathering() ? https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:618: OnPortPairable(data); We should keep has_pairable_candidate and set_has_pairable_candidate(true) next to each other. So perhaps make this: if (CandidatePairable(c, port)) { OnPortPairable(data); } And then: void BasicPortAllocatorSession::OnPortPairable(PortData* port_data) { if (port_data->has_pairable_candidate()) { return; } port_data->set_has_pairable_candidate(true); https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:634: void BasicPortAllocatorSession::OnPortPairable(PortData* port_data) { We could call it "data" or "port_data" consistenly throughout the file. I'm fine either way, but please make it consistent. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:659: } Can you move this into a helper method that can be called like this? Port* best_turn_port_on_same_network = GetBestTurnPortForNetwork(port->Network()->name()); https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:679: } Can you move this into a helper method that can be called like this? PrunePortsWorseThan(best_turn_port_on_same_network); https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:681: MaybeSignalCandidatesAllocationDone(); Shouldn't these just be part of the if block above? if (ComparePort(port, best_turn_port_on_same_network) >= 0) { SignalPortReady(this, port); } else { port_data.set_pruned(); SignalPortPruned(this, port); MaybeSignalCandidatesAllocationDone(); } Also, shouldn't we "MaybeSignalCandidatesAllocationDone()" when we data.set_pruned() in the for loop? https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.h (right): https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:128: bool terminated() const { return state_ != STATE_INPROGRESS; } Why not have bool gathering() cosnt { return state_ == STATE_INPROGRESS; } and use !gathering() at all the call sites? https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator_unittest.cc (right): https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:263: static int FindPorts(const std::vector<PortInterface*>& ports, This should be called CountPorts. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:274: return found_ports; This could use std::count_if.
PTAL. Thanks! https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:181: // A TURN port is pruned if there is a higher-priority TURN port becomes On 2016/06/28 21:01:41, pthatcher1 wrote: > if there is a => if a Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:75: } On 2016/06/28 21:01:41, pthatcher1 wrote: > I think it makes sense to prefer UDP > TCP, but I still don't see what we gain > by preferring ipv4 over ipv6 (or vice versa). keep this based on our discussion. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:254: if (IsPortInUse(data)) { On 2016/06/28 21:01:41, pthatcher1 wrote: > Since the existing port is ReadyPorts, why not calls this PortIsReady()? And, > in fact, why not just put it on PortData as .ready()? Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:318: // If all allocated ports are in one of the terminal states, session must have On 2016/06/28 21:01:41, pthatcher1 wrote: > Could say "if all allocated ports are no longer gathering". Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:321: if (std::all_of(ports_.begin(), ports_.end(), On 2016/06/28 21:01:41, pthatcher1 wrote: > Could be "std::none_of(ports_.... port.gathering();} )" Done used any_of ... May be more readable. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:326: return false; On 2016/06/28 21:01:41, pthatcher1 wrote: > Instead of > > if(foo) > return true; > return false; > > Why not > > return foo; > > ? Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:396: if (!it->terminated()) { On 2016/06/28 21:01:41, pthatcher1 wrote: > it->gathering() > > ? Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:618: OnPortPairable(data); On 2016/06/28 21:01:41, pthatcher1 wrote: > We should keep has_pairable_candidate and set_has_pairable_candidate(true) next > to each other. So perhaps make this: > > if (CandidatePairable(c, port)) { > OnPortPairable(data); > } > > And then: > > void BasicPortAllocatorSession::OnPortPairable(PortData* port_data) { > if (port_data->has_pairable_candidate()) { > return; > } > port_data->set_has_pairable_candidate(true); > Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:634: void BasicPortAllocatorSession::OnPortPairable(PortData* port_data) { On 2016/06/28 21:01:41, pthatcher1 wrote: > We could call it "data" or "port_data" consistenly throughout the file. I'm > fine either way, but please make it consistent. Now that I have used helper methods below, it is possible to just use data without getting the name shadowed. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:659: } On 2016/06/28 21:01:41, pthatcher1 wrote: > Can you move this into a helper method that can be called like this? > > Port* best_turn_port_on_same_network = > GetBestTurnPortForNetwork(port->Network()->name()); Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:679: } On 2016/06/28 21:01:41, pthatcher1 wrote: > Can you move this into a helper method that can be called like this? > > PrunePortsWorseThan(best_turn_port_on_same_network); Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:681: MaybeSignalCandidatesAllocationDone(); On 2016/06/28 21:01:41, pthatcher1 wrote: > Shouldn't these just be part of the if block above? > > if (ComparePort(port, best_turn_port_on_same_network) >= 0) { > SignalPortReady(this, port); > } else { > port_data.set_pruned(); > SignalPortPruned(this, port); > MaybeSignalCandidatesAllocationDone(); > } Yes we can do some optimization like that, although we don't need to signal if the current port is pruned. > > Also, shouldn't we "MaybeSignalCandidatesAllocationDone()" when we > data.set_pruned() in the for loop? If we do that way, We may end up calling MaybeSignalCandidatesAllocationDone multiple times, which is not necessary, I think. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.h (right): https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:128: bool terminated() const { return state_ != STATE_INPROGRESS; } On 2016/06/28 21:01:41, pthatcher1 wrote: > Why not have bool gathering() cosnt { return state_ == STATE_INPROGRESS; } and > use !gathering() at all the call sites? Done. I used the name inprogress to match the variable name. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator_unittest.cc (right): https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:263: static int FindPorts(const std::vector<PortInterface*>& ports, On 2016/06/28 21:01:42, pthatcher1 wrote: > This should be called CountPorts. Done. https://codereview.webrtc.org/2093623004/diff/200001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:274: return found_ports; On 2016/06/28 21:01:42, pthatcher1 wrote: > This could use std::count_if. Done.
Just a few more readability suggestions. This is complex enough that I'd really like to make this as readable as possible. Thank you for you patience. https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:50: // Gets address family priority: Unspecified < IPv4 < IPv6. Can you write this as IPv6 > IPv4 > Unspecified? https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:60: RTC_DCHECK(false); Shouldn't we have RTC_DCHECK for AF_UNSPEC as well? https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:61: return 0; Can you put highest first? case AF_INET6: return 2; case AF_INET: return 1; ... https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:634: data->set_has_pairable_candidate(true); Actually, this makes it seem like the method should be called OnPairableCandidateReady, and that makes me think we should move the has_pairable_candidate/set_has_pairable_candidate thing back to OnCandidateReady and call this method OnFirstPairableCandidateReady, like so: if (CandidatePairable(c, port) && data->has_pairable_candidate()) { data->set_has_pairable_candidate(true); OnFirstPairableCandidateReady(); } Sorry for the churn on this one. https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:669: } I feel like we could also be more readable by sticking this in a separate method. I'm trying to think of a good name, though. Perhaps something like this: PruneTurnPortsForNetwork(port->Network()->name()); if (!data->pruned()) { SignalPortReady(this, port); } And PruneTurnPortsForNetwork could just be be: Port* best_port = GetBestTurnPortForNetwork(name); PruneTurnPortsWorseThan(best_port); MaybeSignalCandidatesAllocationDone(); Then this whole method could just be: if (port->Type() == RELAY_PORT_TYPE && allocator_->prune_turn_ports()) { PruneTurnPortsForNetwork(port->Network()->name()); } if (!data->pruned()) { SignalPortReady(this, port); } And then we could perhaps just inline it after all.
Patchset #9 (id:260001) has been deleted
Patchset #9 (id:280001) has been deleted
This won't send unnecessary SignalPortPruned message. PTAL. https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:50: // Gets address family priority: Unspecified < IPv4 < IPv6. On 2016/06/29 00:33:11, pthatcher1 wrote: > Can you write this as IPv6 > IPv4 > Unspecified? Done. https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:60: RTC_DCHECK(false); On 2016/06/29 00:33:11, pthatcher1 wrote: > Shouldn't we have RTC_DCHECK for AF_UNSPEC as well? Agree. put it together with default. https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:61: return 0; On 2016/06/29 00:33:11, pthatcher1 wrote: > Can you put highest first? > > case AF_INET6: > return 2; > case AF_INET: > return 1; > ... Done. https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:634: data->set_has_pairable_candidate(true); On 2016/06/29 00:33:11, pthatcher1 wrote: > Actually, this makes it seem like the method should be called > OnPairableCandidateReady, and that makes me think we should move the > has_pairable_candidate/set_has_pairable_candidate thing back to OnCandidateReady > and call this method OnFirstPairableCandidateReady, like so: > > if (CandidatePairable(c, port) && data->has_pairable_candidate()) { > data->set_has_pairable_candidate(true); > OnFirstPairableCandidateReady(); > } > > > Sorry for the churn on this one. Done. https://codereview.webrtc.org/2093623004/diff/240001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:669: } On 2016/06/29 00:33:11, pthatcher1 wrote: > I feel like we could also be more readable by sticking this in a separate > method. I'm trying to think of a good name, though. Perhaps something like > this: > > PruneTurnPortsForNetwork(port->Network()->name()); > if (!data->pruned()) { > SignalPortReady(this, port); > } > > > And PruneTurnPortsForNetwork could just be be: > > Port* best_port = GetBestTurnPortForNetwork(name); > PruneTurnPortsWorseThan(best_port); > MaybeSignalCandidatesAllocationDone(); > > > Then this whole method could just be: > > if (port->Type() == RELAY_PORT_TYPE && allocator_->prune_turn_ports()) { > PruneTurnPortsForNetwork(port->Network()->name()); > } > if (!data->pruned()) { > SignalPortReady(this, port); > } > > > And then we could perhaps just inline it after all. Done and inlined it.
lgtm I like that a lot. Thank you. Just two minor improvement suggestions. Taylor, are you going to review this? https://codereview.webrtc.org/2093623004/diff/300001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/300001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:615: bool pruned_port = PruneTurnPortsWith(port); Can you move the "if (!allocator_->prune_turn_ports() || port->Type() != RELAY_PORT_TYPE)" here so it reads like this? if (port->Type() == RELAY_PORT_TYPE && allocator_->prune_turn_ports()) { PruneTurnPorts(port); } https://codereview.webrtc.org/2093623004/diff/300001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:651: bool BasicPortAllocatorSession::PruneTurnPortsWith(Port* port) { I'd suggest calling this PruneTurnPorts and calling the passed-in variable newly_pairable_tunr_port.
On 2016/06/29 17:36:14, pthatcher1 wrote: > > Taylor, are you going to review this? Yes, just got a little behind with sheriff duties and investigating bugs this week. But I should catch up on reviews today.
https://codereview.webrtc.org/2093623004/diff/300001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/300001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:615: bool pruned_port = PruneTurnPortsWith(port); On 2016/06/29 17:36:14, pthatcher1 wrote: > Can you move the "if (!allocator_->prune_turn_ports() || port->Type() != > RELAY_PORT_TYPE)" here so it reads like this? > > if (port->Type() == RELAY_PORT_TYPE && allocator_->prune_turn_ports()) { > PruneTurnPorts(port); > } Done. https://codereview.webrtc.org/2093623004/diff/300001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:651: bool BasicPortAllocatorSession::PruneTurnPortsWith(Port* port) { On 2016/06/29 17:36:14, pthatcher1 wrote: > I'd suggest calling this PruneTurnPorts and calling the passed-in variable > newly_pairable_tunr_port. Done.
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14632)
https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1633: if (RemovePort(port)) { If a port is pruned and it doesn't have any connections, should it just be destroyed? https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:185: sigslot::signal2<PortAllocatorSession*, PortInterface*> SignalPortPruned; I realize this may be too large of a change to the CL, but I don't think the port pruning logic should go in BasicPortAllocator. This means if someone wants to use their own PortAllocator, they have to implement their own pruning logic. It would be better in my opinion if PortAllocator just gathers ports/candidates, and P2PTransportChannel decides which ones to use. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:252: if (data.ready()) { To me this is a little confusing because it's not clear what "ready" means, and I have to look at the header file. Just a matter of preference, but I would be fine with the whole boolean expression written out everywhere necessary. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:616: if (allocator_->prune_turn_ports() && port->Type() == RELAY_PORT_TYPE) { It's not safe to access allocator_->prune_turn_ports() directly. If I do: allocator.SetConfiguration(..., prune=true); session1 = allocator.CreateSession(); allocator.SetConfiguration(..., prune=false); session2 = allocator.CreateSession(); I would expect session2 to prune TURN ports and session1 to not. A session should store the configuration from the time when it was created. This is a problem I fixed recently with the candidate filter. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:625: MaybeSignalCandidatesAllocationDone(); Does this mean we may signal candidate allocation done before we signal the candidates below? That seems like a problem. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator_unittest.cc (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:263: static int CountPorts(const std::vector<PortInterface*>& ports, nit: CountPortsWithAddress? https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:1237: // Otherwise there will be only 2 candidates. This seems less than optimal. I understand keeping the candidates/connections when a port is removed because the network is inactive, but if a port is removed because it's pruned, it seems safe to remove the candidates/connections (unless the connection is nominated on the controlled side). Do you have any plans to do this or something similar?
https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:185: sigslot::signal2<PortAllocatorSession*, PortInterface*> SignalPortPruned; On 2016/06/29 20:57:35, Taylor Brandstetter wrote: > I realize this may be too large of a change to the CL, but I don't think the > port pruning logic should go in BasicPortAllocator. This means if someone wants > to use their own PortAllocator, they have to implement their own pruning logic. > It would be better in my opinion if PortAllocator just gathers ports/candidates, > and P2PTransportChannel decides which ones to use. Actually I take this back. It may be fine as is. One advantage of having the PortAllocator filter ports is that it can stop allocating ports it realizes it no longer needs, saving packets. And if this is standardized, it very well may be an option on the IceGatherer, not the IceTransport. So it's probably fine to leave it the way it is. Hopefully you haven't already started changing it, or I'd feel really bad...
https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1633: if (RemovePort(port)) { On 2016/06/29 20:57:35, Taylor Brandstetter wrote: > If a port is pruned and it doesn't have any connections, should it just be > destroyed? Since the candidates of the port may have been sent, there is a potential risk that the remote-side (especially if that is the controlling side) may create connection and use that connection for a short-period of time. So I think it is safer just to let the port destroy itself after having no connection for a period of time. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:185: sigslot::signal2<PortAllocatorSession*, PortInterface*> SignalPortPruned; On 2016/06/29 20:57:35, Taylor Brandstetter wrote: > I realize this may be too large of a change to the CL, but I don't think the > port pruning logic should go in BasicPortAllocator. This means if someone wants > to use their own PortAllocator, they have to implement their own pruning logic. > It would be better in my opinion if PortAllocator just gathers ports/candidates, > and P2PTransportChannel decides which ones to use. I haven't done that yet. I think there is benefit to do it here as you pointed out. Plus, it is more natural to do it here because PortAllocator manages ports. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:252: if (data.ready()) { On 2016/06/29 20:57:35, Taylor Brandstetter wrote: > To me this is a little confusing because it's not clear what "ready" means, and > I have to look at the header file. Just a matter of preference, but I would be > fine with the whole boolean expression written out everywhere necessary. It basically means the port has a pairable candidate and has not failed or pruned. I think putting that in a function would increase the code reuse as it is used multiple times. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:616: if (allocator_->prune_turn_ports() && port->Type() == RELAY_PORT_TYPE) { On 2016/06/29 20:57:35, Taylor Brandstetter wrote: > It's not safe to access allocator_->prune_turn_ports() directly. If I do: > > allocator.SetConfiguration(..., prune=true); > session1 = allocator.CreateSession(); > allocator.SetConfiguration(..., prune=false); > session2 = allocator.CreateSession(); > > I would expect session2 to prune TURN ports and session1 to not. A session > should store the configuration from the time when it was created. This is a > problem I fixed recently with the candidate filter. Good observation. Done. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:625: MaybeSignalCandidatesAllocationDone(); On 2016/06/29 20:57:35, Taylor Brandstetter wrote: > Does this mean we may signal candidate allocation done before we signal the > candidates below? That seems like a problem. Done. Move this after the SignalCandidatesReady. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator_unittest.cc (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:263: static int CountPorts(const std::vector<PortInterface*>& ports, On 2016/06/29 20:57:36, Taylor Brandstetter wrote: > nit: CountPortsWithAddress? Ideally we may want CountPortsWithTypeProtocolAndAddress, more like the IOS function names. But with C++ style, it is perhaps not necessary to include all arguments in the method name, right? I add a comment. https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:1237: // Otherwise there will be only 2 candidates. On 2016/06/29 20:57:36, Taylor Brandstetter wrote: > This seems less than optimal. I understand keeping the candidates/connections > when a port is removed because the network is inactive, but if a port is removed > because it's pruned, it seems safe to remove the candidates/connections (unless > the connection is nominated on the controlled side). Do you have any plans to do > this or something similar? We do have a plan to remove candidates for v1.1 as Justin suggested (this is v1.0). I do agree it is fairly easy to remove the candidates, except that we need to make sure that the other side will handle it correctly (otherwise it should be a no-op). Regarding removing the connection. It is pretty hard to know whether a connection may be used by the other (controlling) side in the near future. So I think we'd just leave it there until they are used or removed.
lgtm https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator_unittest.cc (right): https://codereview.webrtc.org/2093623004/diff/320001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator_unittest.cc:1237: // Otherwise there will be only 2 candidates. On 2016/06/29 22:53:30, honghaiz3 wrote: > On 2016/06/29 20:57:36, Taylor Brandstetter wrote: > > This seems less than optimal. I understand keeping the candidates/connections > > when a port is removed because the network is inactive, but if a port is > removed > > because it's pruned, it seems safe to remove the candidates/connections > (unless > > the connection is nominated on the controlled side). Do you have any plans to > do > > this or something similar? > We do have a plan to remove candidates for v1.1 as Justin suggested (this is > v1.0). > I do agree it is fairly easy to remove the candidates, except that we need to > make sure that the other side will handle it correctly (otherwise it should be a > no-op). > > Regarding removing the connection. It is pretty hard to know whether a > connection may be used by the other (controlling) side in the near future. So I > think we'd just leave it there until they are used or removed. Well in theory, if a connection has not been nominated it should be safe to assume it's not being used. But I'm fine with doing this further optimization later.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2093623004/#ps340001 (title: "Address Taylor's comments")
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
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/2093623004/#ps360001 (title: "Initialized prune_turn_ports_ and removed an unused method")
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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14649)
Description was changed from ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= ========== to ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/17aac053f585e892114974d2e... ==========
Message was sent while issue was closed.
Description was changed from ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= ========== to ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 Cr-Commit-Position: refs/heads/master@{#13335} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 Cr-Commit-Position: refs/heads/master@{#13335}
Message was sent while issue was closed.
Committed patchset #12 (id:360001) manually as 17aac053f585e892114974d2eb248e05ad37f973 (presubmit successful).
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:360001) has been created in https://codereview.webrtc.org/2111663003/ by danilchap@webrtc.org. The reason for reverting is: Breaks Win32/Win64 Debug bots in client.webrtc waterfall.
Message was sent while issue was closed.
Description was changed from ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 Cr-Commit-Position: refs/heads/master@{#13335} ========== to ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 Cr-Commit-Position: refs/heads/master@{#13335} ==========
Patchset #13 (id:380001) has been deleted
Patchset #14 (id:420001) has been deleted
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/2093623004/#ps440001 (title: "Fix the test failure on win_dbg and win_x64_dbg")
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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14709)
Patchset #14 (id:440001) has been deleted
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/2093623004/#ps460001 (title: ".")
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2016/07/01 03:12:59, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/... It turns out that the test failure (TestEachNetworkHasItsOwnTurnPorts) on win_x64_dbg and win_dbg is not caused by this CL. The error message is about some comparator errors. But we did not introduce or use any comparator in this CL. And I verified the test has the same error even if I set prune_turn_ports as false, or completely revert the CL but keep the test itself. So I am going to partially disable the test and land the CL, and filed a bug to track the issue: webrtc:6068.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14721)
Description was changed from ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 Cr-Commit-Position: refs/heads/master@{#13335} ========== to ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 Cr-Commit-Position: refs/heads/master@{#13335} Committed: https://chromium.googlesource.com/external/webrtc/+/b9e7b4ad66a0466b83545273a... ==========
Message was sent while issue was closed.
Description was changed from ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 Cr-Commit-Position: refs/heads/master@{#13335} ========== to ========== Add config to prune low-priority TURN ports for creating connections When the flag prune_turn_ports is set, When a high-priority turn port becomes available, it will prune low-priority ones. The pruned port will not be used for creating connections locally and its candidates will not be sent over to the remove side (unless they have been sent before being pruned). This effectively reduces the number of TURN candidates and connections created by TURN ports. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/b9e7b4ad66a0466b83545273ad99f030c24bea7f Committed: https://crrev.com/17aac053f585e892114974d2eb248e05ad37f973 Cr-Original-Commit-Position: refs/heads/master@{#13335} Cr-Commit-Position: refs/heads/master@{#13354} ==========
Message was sent while issue was closed.
Committed patchset #14 (id:460001) manually as b9e7b4ad66a0466b83545273ad99f030c24bea7f (presubmit successful).
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b9e7b4ad66a0466b83545273ad99f030c24bea7f Cr-Commit-Position: refs/heads/master@{#13354} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
