|
|
Created:
3 years, 9 months ago by Zach Stein Modified:
3 years, 9 months ago Reviewers:
Taylor Brandstetter CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMeasure packet loss so we can use it to select ICE candidate pairs.
BUG=webrtc:7028
Review-Url: https://codereview.webrtc.org/2722933002
Cr-Commit-Position: refs/heads/master@{#17313}
Committed: https://chromium.googlesource.com/external/webrtc/+/abbacbf4892b98d77a53d2a5a5ed58d2162afdc6
Patch Set 1 #Patch Set 2 : better comments and test names #
Total comments: 26
Patch Set 3 : Responding to Taylor's comments (don't divide by zero, simplify packet expiration, use an unordered… #Patch Set 4 : Move PacketLossEstimator to its own files. #Patch Set 5 : add forget_after, simplify the implementation and tests, and increase coverage slightly #Patch Set 6 : Call UpdateResponseRate from inside PacketLossEstimator ocasionally to avoid unbounded memory growt… #Patch Set 7 : test that UpdateResponseRate is called internally #
Total comments: 24
Patch Set 8 : Don't update the response rate when forgetting old requests + style feedback. #
Total comments: 2
Patch Set 9 : Add back tests that packets are forgotten when UpdateResponseRate is not called. Add a debugging me… #Patch Set 10 : Clean up some comments and add a test case where a response is received early. #
Total comments: 1
Patch Set 11 : Append ForTesting to TrackedPacketsString method name. #
Messages
Total messages: 17 (5 generated)
zstein@webrtc.org changed reviewers: + deadbeef@webrtc.org
Just the measurement code so far. Will look at other places to measure from (P2PTransportChannel::OnCheckAndPing?) and act on the measurements next. Thanks!
Looks good; and good idea making this a standalone CL. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:152: // 3) we never expected to see this id (something else went wrong). nit: Capitalization of "we" https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:178: static_cast<double>(responses_received_) / responses_expected_; Divide by zero? https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:939: packet_loss_estimator_(kPortTimeoutDelay), This value recently became very large (~45 seconds), so it probably won't be very useful. Maybe we should have two time thresholds? Threshold A (RTT and change): Response is counted as lost, but still remembered in case a response arrives later than expected. Threshold B (~45 seconds): Response completely forgotten. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:436: class PacketLossEstimator { Some comments around this class may be helpful. Such as: "Tracks the percentage of STUN pings that received a response. A response is counted as lost only after |expire_after| milliseconds." https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:438: struct PacketInfo { It looks like this struct can be private. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:446: PacketLossEstimator(int64_t expire_after); Doesn't have to be in this CL, but it may be useful to be able to set "expire after" after creation. For example, to set it to "RTT * 1.5", or "RTT + some factor of RTT variance" after RTT converges. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:454: // This is updated during ReceivedResponse and RemoveExpiredPackets. The code may be more readable if only "RemoveExpiredPackets" updates the rate. And actually, it could just be called "UpdateResponseRate" instead. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:462: int64_t get_responses_received() const { return responses_received_; } I'd prefer not having "for test" methods unless absolutely necessary; in this case I think you should be able to test everything just using get_response_rate(). https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:471: std::vector<PacketInfo> outstanding_packets_; nit: Probably doesn't matter, but would an unordered_map of IDs to times be more efficient? https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:475: }; I think we can move this to its own file; port.h already has a lot of stuff, and more files doesn't really hurt. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit... File webrtc/p2p/base/port_unittest.cc (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit... webrtc/p2p/base/port_unittest.cc:2889: class PacketLossEstimatorTest : public testing::Test {}; Could have a test for calling RemoveExpiredPackets/get_response_rate before ExpectResponse has been called. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit... webrtc/p2p/base/port_unittest.cc:2952: ple.ReceivedResponse("1", 7); // "1" should already be expired nit: Generally our style is to use full capitalization/punctuation for comments, even one-liners.
https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:152: // 3) we never expected to see this id (something else went wrong). On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > nit: Capitalization of "we" Done. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:178: static_cast<double>(responses_received_) / responses_expected_; On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > Divide by zero? Done. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:939: packet_loss_estimator_(kPortTimeoutDelay), On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > This value recently became very large (~45 seconds), so it probably won't be > very useful. > > Maybe we should have two time thresholds? > > Threshold A (RTT and change): Response is counted as lost, but still remembered > in case a response arrives later than expected. > > Threshold B (~45 seconds): Response completely forgotten. I like this idea. As is, we probably won't get a useful signal until long after a connection has been established. I would like to start with the simplest possible implementation so that we have something to measure though. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:436: class PacketLossEstimator { On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > Some comments around this class may be helpful. Such as: "Tracks the percentage > of STUN pings that received a response. A response is counted as lost only after > |expire_after| milliseconds." Done. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:438: struct PacketInfo { On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > It looks like this struct can be private. Don't need this now that outstanding_packets_ is an unordered_map (see below). https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:446: PacketLossEstimator(int64_t expire_after); On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > Doesn't have to be in this CL, but it may be useful to be able to set "expire > after" after creation. For example, to set it to "RTT * 1.5", or "RTT + some > factor of RTT variance" after RTT converges. I like this idea but will leave it for another CL. I renamed expire_after to consider_lost_after. Maybe we can differentiate between messages that are considered expired (we will still listen for the responses) and lost (we will stop listening for the responses) later. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:454: // This is updated during ReceivedResponse and RemoveExpiredPackets. On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > The code may be more readable if only "RemoveExpiredPackets" updates the rate. > And actually, it could just be called "UpdateResponseRate" instead. Good suggestion, I think this will be cleaner. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:462: int64_t get_responses_received() const { return responses_received_; } On 2017/03/01 02:26:13, Taylor Brandstetter wrote: > I'd prefer not having "for test" methods unless absolutely necessary; in this > case I think you should be able to test everything just using > get_response_rate(). I think this gives better coverage (ResponseLost/ResponseLate will fail if responses_expected_ is not incremented, but we wouldn't notice if we were only checking response_rate_), but I will delete them if you feel strongly. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:471: std::vector<PacketInfo> outstanding_packets_; On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > nit: Probably doesn't matter, but would an unordered_map of IDs to times be more > efficient? Maybe. Thinking about this did make me realize that I was assuming the vector would be sorted by sent_time in RemoveExpiredPackets (which isn't documented or necessarily true), so I will switch to an unordered_map. Hashing strings might be slower, but I agree it probably doesn't matter (though I have never seen profiles of any of this code). https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:475: }; On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > I think we can move this to its own file; port.h already has a lot of stuff, and > more files doesn't really hurt. Acknowledged. I will do this in a separate upload so the diffs are easier to read. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit... File webrtc/p2p/base/port_unittest.cc (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit... webrtc/p2p/base/port_unittest.cc:2889: class PacketLossEstimatorTest : public testing::Test {}; On 2017/03/01 02:26:13, Taylor Brandstetter wrote: > Could have a test for calling RemoveExpiredPackets/get_response_rate before > ExpectResponse has been called. Done. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port_unit... webrtc/p2p/base/port_unittest.cc:2952: ple.ReceivedResponse("1", 7); // "1" should already be expired On 2017/03/01 02:26:13, Taylor Brandstetter wrote: > nit: Generally our style is to use full capitalization/punctuation for comments, > even one-liners. Done.
https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:462: int64_t get_responses_received() const { return responses_received_; } On 2017/03/02 00:02:23, zstein wrote: > On 2017/03/01 02:26:13, Taylor Brandstetter wrote: > > I'd prefer not having "for test" methods unless absolutely necessary; in this > > case I think you should be able to test everything just using > > get_response_rate(). > > I think this gives better coverage (ResponseLost/ResponseLate will fail if > responses_expected_ is not incremented, but we wouldn't notice if we were only > checking response_rate_), but I will delete them if you feel strongly. ResponseLate and ResponseLost could be updated to send one request (or multiple) that succeeds, then one whose response is lost, and check that the percentage is what's expected. Maybe I'm a little opinionated about this, but my experience so far with "for_testing" stuff is usually negative; it's hard to maintain and often results in less overall test coverage. And almost always, you can get the same test coverage with a black box test. Maybe this CL isn't the best example, but we have lots of tests in this area of code that call internal event handler methods (like "OnReadPacket"). We'd like to change the implementation of these objects but it's made more difficult by all the tests depending on implementation details. Plus, in some cases they're only testing a small part of the normal code path. https://codereview.webrtc.org/2722933002/diff/20001/webrtc/p2p/base/port.h#ne... webrtc/p2p/base/port.h:475: }; On 2017/03/02 00:02:23, zstein wrote: > On 2017/03/01 02:26:12, Taylor Brandstetter wrote: > > I think we can move this to its own file; port.h already has a lot of stuff, > and > > more files doesn't really hurt. > > Acknowledged. I will do this in a separate upload so the diffs are easier to > read. Thanks; that does help a lot.
I think I responded to everything + more. Please take another look, thanks!
https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.cc (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.cc:2: * Copyright 2004 The WebRTC Project Authors. All rights reserved. nit: 2017 https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.cc:17: : consider_lost_after_(consider_lost_after), forget_after_(forget_after) {} Could add an "RTC_DCHECK_LT(consider_lost_after, forget_after)" (from webrtc/base/checks.h) https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.cc:22: UpdateResponseRateIfStale(sent_time); Is this called just to ensure the the map doesn't grow indefinitely if the client doesn't call "UpdateResponseRate"? If so I'd suggest making it *not* actually update the response rate since that may be an unexpected side effect. There could be a method called "ForgetOldRequests", for example, that only serves to clear things from the map and is called by both this and UpdateResponseRate. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.h (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:23: // |consider_lost_after| ms. If a responses is received after "response", singular https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:24: // |consider_lost_after| ms, it is still considered to be a response. Maybe "counted as" instead of "considered to be"? https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:26: // anymore. The response rate is initially assumed to be 1.0. If the current Paragraph break before "If"? https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:27: // time is 7, |forget_after| is 6, and |consider_lost_after| is 2, the response Since "forget_after" now means "forget both sent requests and received responses", maybe "window_size" would be a more clear name. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:32: // Wind: <-------> | I'm not sure this is accurate; if a request is sent at time 6 and received at time 7, won't it be counted? https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:64: int64_t consider_lost_after_; // In milliseconds. I forgot if I suggested this, but if these variables were named with "_ms_" at the end, the "In milleseconds" comment wouldn't be necessary. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator_unittest.cc (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator_unittest.cc:2: * Copyright 2004 The WebRTC Project Authors. All rights reserved. 2017 https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator_unittest.cc:98: TEST_F(PacketLossEstimatorTest, Lost3_Received1_Waiting1) { Since these two tests depend on the contents of "kFivePackets", could its definition be moved to right above these tests?
Thanks for all the feedback so far! Here is my next round of thinking. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.cc (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.cc:2: * Copyright 2004 The WebRTC Project Authors. All rights reserved. On 2017/03/07 18:16:31, Taylor Brandstetter wrote: > nit: 2017 Done. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.cc:17: : consider_lost_after_(consider_lost_after), forget_after_(forget_after) {} On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > Could add an "RTC_DCHECK_LT(consider_lost_after, forget_after)" (from > webrtc/base/checks.h) Done. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.cc:22: UpdateResponseRateIfStale(sent_time); On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > Is this called just to ensure the the map doesn't grow indefinitely if the > client doesn't call "UpdateResponseRate"? If so I'd suggest making it *not* > actually update the response rate since that may be an unexpected side effect. > > There could be a method called "ForgetOldRequests", for example, that only > serves to clear things from the map and is called by both this and > UpdateResponseRate. Having ForgetOldRequests not update the response rate does sound better. Do you have a preference for how to test this gets called from ReceivedResponse/UpdateResponseRate if we aren't updating the response rate? I don't see how to do it without either expanding the public interface or providing something like a protected getter just for the tests (though maybe that is more okay in this case, since we are explicitly testing something that isn't part of the public interface). https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.h (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:23: // |consider_lost_after| ms. If a responses is received after On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > "response", singular Done. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:24: // |consider_lost_after| ms, it is still considered to be a response. On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > Maybe "counted as" instead of "considered to be"? Done. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:26: // anymore. The response rate is initially assumed to be 1.0. If the current On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > Paragraph break before "If"? Done. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:27: // time is 7, |forget_after| is 6, and |consider_lost_after| is 2, the response On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > Since "forget_after" now means "forget both sent requests and received > responses", maybe "window_size" would be a more clear name. I don't really like the name forget after, but I would think of window size as forget_after - consider_lost_after (forget_after is relative to now, not the right/near end of the window). Using window_size and window_offset does sound a bit more formal though (forget_after = window_size + window_offset, consider_lost_after = window_offset). Do you like that better? https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:32: // Wind: <-------> | On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > I'm not sure this is accurate; if a request is sent at time 6 and received at > time 7, won't it be counted? It will be. I was trying to show the window where responses count against the response rate, but that is confusing. Maybe I should draw the window like this: // Time: 0 1 2 3 4 5 6 7 // Wind: <=======|---> And say that responses expected in the = region are counted against the loss rate but responses expected in the - region are not. Or we could just not count responses received in the - region yet, but that seems like it might be a useful signal. I could also define things a bit more mathematically (I need to double check inclusive/exclusive bounds): // response_rate = received_full_window / (expected_left + received_right) // received_full_window := #responses received for tracked IDs in [wind_left, now] // expected_left := #responses expected for tracked IDs sent in [wind_left, wind_right) // received_right := #responses received for tracked IDs from [wind_right, now] Where wind_left is < and wind_right is > in the window from the diff. I'll try to clarify this all and see where I end up. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:64: int64_t consider_lost_after_; // In milliseconds. On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > I forgot if I suggested this, but if these variables were named with "_ms_" at > the end, the "In milleseconds" comment wouldn't be necessary. Done.
lgtm after addressing comment about "PacketLossEstimatorInterface" https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.cc (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.cc:22: UpdateResponseRateIfStale(sent_time); On 2017/03/08 06:21:14, zstein wrote: > On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > > Is this called just to ensure the the map doesn't grow indefinitely if the > > client doesn't call "UpdateResponseRate"? If so I'd suggest making it *not* > > actually update the response rate since that may be an unexpected side effect. > > > > There could be a method called "ForgetOldRequests", for example, that only > > serves to clear things from the map and is called by both this and > > UpdateResponseRate. > > Having ForgetOldRequests not update the response rate does sound better. > > Do you have a preference for how to test this gets called from > ReceivedResponse/UpdateResponseRate if we aren't updating the response rate? I > don't see how to do it without either expanding the public interface or > providing something like a protected getter just for the tests (though maybe > that is more okay in this case, since we are explicitly testing something that > isn't part of the public interface). How to test that the map doesn't grow indefinitely, you mean? I haven't seen many examples of that kind of test, but that's a situation where it does seem appropriate to have a "tracked_request_count_for_testing()" (for example) method. We add the "for_testing" suffix to make it clear that the method is only used for testing, and I believe there's a presubmit check that complains if you call a "for_testing" method outside a test. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.h (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:27: // time is 7, |forget_after| is 6, and |consider_lost_after| is 2, the response On 2017/03/08 06:21:15, zstein wrote: > On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > > Since "forget_after" now means "forget both sent requests and received > > responses", maybe "window_size" would be a more clear name. > > I don't really like the name forget after, but I would think of window size as > forget_after - consider_lost_after (forget_after is relative to now, not the > right/near end of the window). Using window_size and window_offset does sound a > bit more formal though (forget_after = window_size + window_offset, > consider_lost_after = window_offset). Do you like that better? I guess "window size" is still ambiguous since there are two windows (window of responses being counted as received and window of responses being counted as lost). Let's just stick with forget_after. https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:32: // Wind: <-------> | On 2017/03/08 06:21:15, zstein wrote: > On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > > I'm not sure this is accurate; if a request is sent at time 6 and received at > > time 7, won't it be counted? > > It will be. I was trying to show the window where responses count against the > response rate, but that is confusing. Maybe I should draw the window like this: > > // Time: 0 1 2 3 4 5 6 7 > // Wind: <=======|---> > > And say that responses expected in the = region are counted against the loss > rate but responses expected in the - region are not. Or we could just not count > responses received in the - region yet, but that seems like it might be a useful > signal. > > I could also define things a bit more mathematically (I need to double check > inclusive/exclusive bounds): > > // response_rate = received_full_window / (expected_left + received_right) > // received_full_window := #responses received for tracked IDs in [wind_left, > now] > // expected_left := #responses expected for tracked IDs sent in [wind_left, > wind_right) > // received_right := #responses received for tracked IDs from [wind_right, now] > > Where wind_left is < and wind_right is > in the window from the diff. I'll try > to clarify this all and see where I end up. That diagram looks good (or, even just the sentence you added in the latest patchset is good). I wouldn't worry about it too much; someone can always just look a the code or tests to confirm the expected behavior. https://codereview.webrtc.org/2722933002/diff/140001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.h (right): https://codereview.webrtc.org/2722933002/diff/140001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:23: // TODO(zstein): The above comment describes a PacketLossEstimatorInterface. I don't think an interface is necessary until we have multiple implementations. Generally our policy is "only add something once you need it". If you add things you think you *might* need, half the time you don't end up needing them (or at least, that's my experience).
Would you mind taking one more quick look before I submit? Thanks! https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.cc (right): https://codereview.webrtc.org/2722933002/diff/120001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.cc:22: UpdateResponseRateIfStale(sent_time); On 2017/03/08 17:48:20, Taylor Brandstetter wrote: > On 2017/03/08 06:21:14, zstein wrote: > > On 2017/03/07 18:16:32, Taylor Brandstetter wrote: > > > Is this called just to ensure the the map doesn't grow indefinitely if the > > > client doesn't call "UpdateResponseRate"? If so I'd suggest making it *not* > > > actually update the response rate since that may be an unexpected side > effect. > > > > > > There could be a method called "ForgetOldRequests", for example, that only > > > serves to clear things from the map and is called by both this and > > > UpdateResponseRate. > > > > Having ForgetOldRequests not update the response rate does sound better. > > > > Do you have a preference for how to test this gets called from > > ReceivedResponse/UpdateResponseRate if we aren't updating the response rate? I > > don't see how to do it without either expanding the public interface or > > providing something like a protected getter just for the tests (though maybe > > that is more okay in this case, since we are explicitly testing something that > > isn't part of the public interface). > > How to test that the map doesn't grow indefinitely, you mean? I haven't seen > many examples of that kind of test, but that's a situation where it does seem > appropriate to have a "tracked_request_count_for_testing()" (for example) > method. We add the "for_testing" suffix to make it clear that the method is only > used for testing, and I believe there's a presubmit check that complains if you > call a "for_testing" method outside a test. Done. https://codereview.webrtc.org/2722933002/diff/140001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.h (right): https://codereview.webrtc.org/2722933002/diff/140001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:23: // TODO(zstein): The above comment describes a PacketLossEstimatorInterface. On 2017/03/08 17:48:21, Taylor Brandstetter wrote: > I don't think an interface is necessary until we have multiple implementations. > Generally our policy is "only add something once you need it". If you add things > you think you *might* need, half the time you don't end up needing them (or at > least, that's my experience). That's why I just put this in a comment :) I don't feel super strongly about this case, but in general I think there can be a lot of value in introducing interfaces early. One advantage in this case would be the separation of the intended client API and the implementation details. This is the whole interface: class IPacketLossEstimator { public: ExpectResponse(string id, int64_t sent_time); ReceivedResponse(string id, int64_t received_time); void UpdateResponseRate(int64_t now); double response_rate() const; } The details of what is considered lost, the private helpers and members, and methods for testing/debugging make it harder to understand the class and easier to depend more heavily on the class' internals. If I had made an interface from the beginning, I also might not have overwritten my previous implementation that used a vector (it could be a separate subclass instead). I could also have added a simpler version that just tracked the response rate without a window (which was my original plan). Having that would have allowed me to start getting the infrastructure in place to track this data somewhere real. Anyway, I'll just get rid of this TODO for now and decide what to do next later :)
lgtm https://codereview.webrtc.org/2722933002/diff/180001/webrtc/p2p/base/packetlo... File webrtc/p2p/base/packetlossestimator.h (right): https://codereview.webrtc.org/2722933002/diff/180001/webrtc/p2p/base/packetlo... webrtc/p2p/base/packetlossestimator.h:59: std::string TrackedPacketsString(std::size_t max) const; Append "ForTesting"?
The CQ bit was checked by zstein@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.webrtc.org/2722933002/#ps200001 (title: "Append ForTesting to TrackedPacketsString method name.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490029657905620, "parent_rev": "a5c18d73121f9fe3b877698f099dfaa47beea90b", "commit_rev": "abbacbf4892b98d77a53d2a5a5ed58d2162afdc6"}
Message was sent while issue was closed.
Description was changed from ========== Measure packet loss so we can use it to select ICE candidate pairs. BUG=webrtc:7028 ========== to ========== Measure packet loss so we can use it to select ICE candidate pairs. BUG=webrtc:7028 Review-Url: https://codereview.webrtc.org/2722933002 Cr-Commit-Position: refs/heads/master@{#17313} Committed: https://chromium.googlesource.com/external/webrtc/+/abbacbf4892b98d77a53d2a5a... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/abbacbf4892b98d77a53d2a5a... |