Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1672513002:
Expose packet loss counts from QUIC to NetworkQualityEstimator (Closed)
|
Created:
4 years, 10 months ago by tbansal1 Modified:
4 years, 9 months ago Reviewers:
bengr CC:
chromium-reviews, cbentzel+watch_chromium.org, Randy Smith (Not in Mondays), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose packet loss counts from QUIC to
NetworkQualityEstimator.
Also, the packet loss observations are notified to the
all PacketLossObservers that have registered with NQE.
Next CL will register Cronet as a packet loss observer
with NQE.
BUG=548719
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed bengr comments #
Total comments: 26
Patch Set 3 : Addressed bengr comments #
Total comments: 70
Patch Set 4 : Addressed bengr comments #
Total comments: 4
Patch Set 5 : Addressed bengr comments, Rebased #
Total comments: 6
Patch Set 6 : Addressed rch comments #
Total comments: 10
Patch Set 7 : \ #
Total comments: 4
Patch Set 8 : Addressed rch comments #
Total comments: 2
Messages
Total messages: 49 (19 generated)
Description was changed from ========== w w w w w w w working but tests failing w w w w w w w w w w w w w w w w w nw w w w w w w w w w w w BUG= ========== to ========== Expose packet loss counts from QUIC to NetworkQualityEstimator. BUG= ==========
Description was changed from ========== Expose packet loss counts from QUIC to NetworkQualityEstimator. BUG= ========== to ========== Expose packet loss counts from QUIC to NetworkQualityEstimator. Next CL will expose the packet loss from NQE to Cronet. BUG=548719 ==========
Description was changed from ========== Expose packet loss counts from QUIC to NetworkQualityEstimator. Next CL will expose the packet loss from NQE to Cronet. BUG=548719 ========== to ========== Expose packet loss counts from QUIC to NetworkQualityEstimator. NQE now provides an estimated packet loss rate computed from recent observations (higher weight given to more recent observations). Also, the packet loss observations are notified to the all PacketLossObservers that have registered with NQE. Next CL will register Cronet as a packet loss observer with NQE. BUG=548719 ==========
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. The CL is big but it should be easy to review. I can break it if it helps.
At a high level, I see two issues with the design. First, the observer interface provides loss data in a format that is different from how you store it in the NQE. That's confusing. Second, if there's a burst of packets, you notify repeatedly, once for each packet. That seems less efficient than providing a summary (e.g., n out of m were lost). I'll do a more detailed review after we get the design right. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:122: const float NetworkQualityEstimator::kInvalidPacketLossRate = -1; -1.0f? https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:5: #ifndef NET_BASE_NETWORK_QUALITY_ESTIMATOR_H_ This file is too large. In a follow-up CL, please move each class to a separate file. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:67: // Unknown source. Why would the source ever be unknown? https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:115: virtual void OnPacketLossObservation( Is num_packets_received_in_order the number of consecutive packets that are received without a loss? What would the value be if we receive packets 1,2,3,5,6,7,8? Is it 3 or 4? I have no idea what num_packets_received_no_in_order means, so please explain. As for the duration, I find it a little strange that an observation is not self contained. I.e., it doesn't know when it began. I also find it strange that the observer interface does not expose observations as you define them below. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:170: // |packet_loss| should not be nullptr. |packet_loss| is always between 0.0 In a comment, I would say "should not be null." https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:454: bool GetWeightedAverage(const base::TimeTicks& begin_timestamp, If this is computed over observations then it is computed over uneven intervals. Is that representative? E.g., one interval could lose 1 out of 2 packets, and another could lose 1 out of 1000, but the reported loss rate would be ~1/2. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:598: const base::TimeTicks timestamp, Why not a const base::TimeTicks& ? https://codereview.chromium.org/1672513002/diff/1/net/base/socket_performance... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1672513002/diff/1/net/base/socket_performance... net/base/socket_performance_watcher.h:38: // Called when updated packet count at transport layer is available. when -> when an at -> at the
On 2016/02/09 15:41:03, bengr wrote: > At a high level, I see two issues with the design. First, the observer interface > provides loss data in a format that is different from how you store it in the > NQE. > That's confusing. Please see the comments inline. It would be ideal for raw observations and processed observations to have the same format, but it is not clear how we can do it (or even if it is necessary for them to have the same format). > Second, if there's a burst of packets, you notify > repeatedly, once for each packet. That seems less efficient than providing a > summary (e.g., n out of m were lost). Agreed, the current interface allows sending count of packets (m, n) etc. If the consumers feel that this is too spammy, we could think of ways to aggregate the readings. A simple way might be to notify only when there is a significant change in the observations.
bengr: ptal. Thanks! https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:122: const float NetworkQualityEstimator::kInvalidPacketLossRate = -1; On 2016/02/09 15:41:02, bengr wrote: > -1.0f? Done. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:5: #ifndef NET_BASE_NETWORK_QUALITY_ESTIMATOR_H_ On 2016/02/09 15:41:02, bengr wrote: > This file is too large. In a follow-up CL, please move each class to a separate > file. Acknowledged. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:67: // Unknown source. On 2016/02/09 15:41:03, bengr wrote: > Why would the source ever be unknown? Fixed the function definition, and eliminated UNKNOWN. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:115: virtual void OnPacketLossObservation( On 2016/02/09 15:41:03, bengr wrote: > Is num_packets_received_in_order the number of consecutive packets that are > received without a loss? What would the value be if we receive packets > 1,2,3,5,6,7,8? Is it 3 or 4? I have no idea what > num_packets_received_no_in_order means, so please explain. Added an example in socket_performance_watcher_factory.h to make it clearer. > > As for the duration, I find it a little strange that an observation is not self > contained. I.e., it doesn't know when it began. Removed that comment, it was unnecessary and not clear. > > I also find it strange that the observer interface does not expose observations > as you define them below. Agreed, but I am not sure what's the best way out. It would be ideal for raw observations and processed observations to have the same format, but it is not clear how we can do it (or even if it is necessary for them to have the same format). https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:170: // |packet_loss| should not be nullptr. |packet_loss| is always between 0.0 On 2016/02/09 15:41:02, bengr wrote: > In a comment, I would say "should not be null." Done. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:454: bool GetWeightedAverage(const base::TimeTicks& begin_timestamp, On 2016/02/09 15:41:03, bengr wrote: > If this is computed over observations then it is computed over uneven intervals. > Is that representative? E.g., one interval could lose 1 out of 2 packets, and > another could lose 1 out of 1000, but the reported loss rate would be ~1/2. I do not understand this comment. Two observations taken at the same time have same weight. Two observations may differ in weight only if they were taken at different times. Each observation corresponds to 1 packet received or 1 packet lost. https://codereview.chromium.org/1672513002/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.h:598: const base::TimeTicks timestamp, On 2016/02/09 15:41:02, bengr wrote: > Why not a const base::TimeTicks& ? Done. https://codereview.chromium.org/1672513002/diff/1/net/base/socket_performance... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1672513002/diff/1/net/base/socket_performance... net/base/socket_performance_watcher.h:38: // Called when updated packet count at transport layer is available. On 2016/02/09 15:41:03, bengr wrote: > when -> when an > at -> at the Done.
Patchset #2 (id:20001) has been deleted
bengr: ptal.
Here are a few more comments. I'll do a full pass later. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:563: return (*packet_loss != kInvalidPacketLossRate); Is any packet loss rate below 0 valid? https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:736: // |weighted_observations| may have a smaller size than |observations_| since similar size than -> size similar to https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:107: // Will be called when a new packet loss observation is available. Use complete sentences. "This method will.." https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:116: // consumers with more information. From the perspective of network quality, More information than what? How does network quality have perspective? :) https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:117: // loss of a packet indicates worse network quality than receival of an receival -> reception https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:118: // out-of-order packet. The latter in turns indicates worse network turns -> turn https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:119: // quality than receival of an in-order packet. receival -> reception https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:176: // and 1.0 with a higher value indicating higher packet loss rate. inclusive? https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:456: // unavailable, false is returned and |result| is not modified. Average Average -> The average https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:457: // value is unavailable if all the values in observation buffer are older all the -> all of the in observation -> in the observation https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:507: // packet was lost. What does a value of 0.5 mean? Why can't this be a bool? https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:524: static const float kInvalidPacketLossRate; It might be better to provide an is_valid() method. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:631: // Sets |observation_source| that corresponds to the given |protocol|, and Do you mean "Gets the?" The entire comment is awkward, so please rewrite.
Patchset #3 (id:60001) has been deleted
bengr: ptal. Thanks! https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:563: return (*packet_loss != kInvalidPacketLossRate); On 2016/02/16 17:02:01, bengr wrote: > Is any packet loss rate below 0 valid? No. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:736: // |weighted_observations| may have a smaller size than |observations_| since On 2016/02/16 17:02:01, bengr wrote: > similar size than -> size similar to Did you mean "smaller" instead of "similar"? https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:107: // Will be called when a new packet loss observation is available. On 2016/02/16 17:02:02, bengr wrote: > Use complete sentences. "This method will.." Done. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:116: // consumers with more information. From the perspective of network quality, On 2016/02/16 17:02:01, bengr wrote: > More information than what? > How does network quality have perspective? :) Done. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:117: // loss of a packet indicates worse network quality than receival of an On 2016/02/16 17:02:02, bengr wrote: > receival -> reception Removed that comment, it was confusing. Also, how to interpret the 3 counts depends on the consumers. So, that comment was unnecessary. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:118: // out-of-order packet. The latter in turns indicates worse network On 2016/02/16 17:02:02, bengr wrote: > turns -> turn Done. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:119: // quality than receival of an in-order packet. On 2016/02/16 17:02:02, bengr wrote: > receival -> reception Done. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:176: // and 1.0 with a higher value indicating higher packet loss rate. On 2016/02/16 17:02:02, bengr wrote: > inclusive? Done. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:456: // unavailable, false is returned and |result| is not modified. Average On 2016/02/16 17:02:02, bengr wrote: > Average -> The average Done. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:457: // value is unavailable if all the values in observation buffer are older On 2016/02/16 17:02:02, bengr wrote: > all the -> all of the > in observation -> in the observation Done. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:507: // packet was lost. On 2016/02/16 17:02:02, bengr wrote: > What does a value of 0.5 mean? Why can't this be a bool? This is packet loss rate observation from the reception of a single packet. Using a float makes it easier if in future, we record observation over multiple packets (say 10 packets). Also, the current implementation of ObservationBuffer computes the median/average with the same variable type as the observation itself. So, using bool would make it complicated to compute median/average. Renamed the variables a bit to make it clearer. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:524: static const float kInvalidPacketLossRate; On 2016/02/16 17:02:02, bengr wrote: > It might be better to provide an is_valid() method. Changed to use a min valid value. https://codereview.chromium.org/1672513002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:631: // Sets |observation_source| that corresponds to the given |protocol|, and On 2016/02/16 17:02:02, bengr wrote: > Do you mean "Gets the?" The entire comment is awkward, so please rewrite. Done.
https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:122: const float NetworkQualityEstimator::kMinimumValidPacketLossRate = 0.0f; I think you can get rid of this constant. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:470: base::HistogramBase* fastest_rtt_histogram = I'd save this for a different CL. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:478: base::HistogramBase* peak_kbps_histogram = Likewise, this has nothing to do with packet loss. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:480: 1000000); // UMA_HISTOGRAM_COUNTS To what does this comment refer? https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:487: DCHECK_LE(kMinimumValidPacketLossRate, packet_loss_rate); I would just do: float packet_loss_rate; if (GetPacketLossRateEstimate(&packet_loss_rate)) { DCHECK_LE(0.0, packet_loss_rate); DCHECK_GE(1.0, packet_loss_rate); ... https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:492: packet_loss_histogram->Add(packet_loss_rate * 100); Add(packet_loss_rate * 100.0f) https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:559: float* packet_loss) const { packet_loss_rate_estimate https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:562: *packet_loss = GetPacketLossRateEstimateInternal(base::TimeTicks()); It might be cleaner to not set packet_loss_rate_estimate if invalid. I.e.: float estimate = GetPacketLossRateEstimateInternal(base::TimeTicks()); if (estimate < 0.0 || estimate > 1.0) return false; *packet_loss_rate_estimate = estimate; Also, you might want to introduce a helper IsValidPacketLossRate(), but that might be overkill. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:638: packet_loss_rate_observations_.GetWeightedAverage(begin_timestamp, I would try to rewrite GetPacketLossRateEstimate, GetPacketLossRateEstimateInternal and GetWeightedAverage to eliminate this method. Ideally it would look like: bool NQE::GetPacketLossRateEstiamte(...) { return packet_loss_rate_observations_.GetWeightedAverage); } https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:722: // Stores WeightedObservation in increasing order of value, although for the Is the front or the back the highest value? https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:722: // Stores WeightedObservation in increasing order of value, although for the WeightedObservation -> weighted observations https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:726: // Total weight of all observations in |weighted_observations|. The sum of the weights of all observations in |weighted_observations|. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:941: const Protocol protocol, No need for const. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:117: // rate. ... for consumers of raw observations. Packet loss estimates do not provide this extra information. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:119: uint64_t num_packets_lost, Add concise examples, e.g.: // Last observation after packet #1. Packets #5 and #6 then received: // lost = 2, in_order = 2, not_in_order = 0 // Last observation after packet #6. Packet #3 received: // lost = 1 (is this right??), in_order = 0, not_in_order = 1 https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:173: // |packet_loss| should not be NULL. |packet_loss| is always between 0.0 should -> must https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:206: const Protocol protocol, No need for const. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:209: uint64_t num_packets_received_not_in_order) override; I'd prefer smaller types. 8 bytes seems like overkill. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:454: // value among all observations since |begin_timestamp|. If the value is among -> of Is that inclusive of begin_timestamp? https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:507: // lost. Currently, the observation period is set to 1 packet. 1 -> one https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:639: const Protocol protocol, No need for const. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator_unittest.cc:322: // Expecting packet number p, but received packet number p+9. 1 out of 10 p is a letter. :) https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator_unittest.cc:474: float packet_loss_rate = I don't think you need to initialize here, but if you do, just set it to -1.0f. https://codereview.chromium.org/1672513002/diff/80001/net/base/socket_perform... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/socket_perform... net/base/socket_performance_watcher.h:39: // |num_packets_lost| is the number of packets lost. The parameter definitions are tautologies. Consider adding more meaningful definitions. https://codereview.chromium.org/1672513002/diff/80001/net/base/socket_perform... File net/base/socket_performance_watcher_factory.h (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/socket_perform... net/base/socket_performance_watcher_factory.h:56: // As an example, assume that the packets with sequence number 1, 2 and 5 have I would be much more concise here. See my earlier comment on how to provide the example. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:541: const uint64_t diff = Move this to after 548 https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:548: if (socket_performance_watcher_) { if (!socket_performance_watcher) return; ... https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:550: // The received packet number has alower sequence number than the expected alower -> a lower https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:566: num_packets_received_not_in_order); I don't think you need "num_" but could be convinced you could replace it with the suffix "_count" I'd change "not_in" to "out_of" https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:815: float num_lost = largest_received_packet_number_ - num_packets_received_; I don't like "num." Maybe lost_count? Please remove "num_" everywhere. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... File net/quic/quic_connection_logger.h (right): https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.h:126: // Reports the counts of the packets received on the uplink (remote endpoint I think you just mean "reports packet receptions to the |socket_performance_watcher_|." Where do packet numbers come from? https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... File net/quic/quic_connection_logger_unittest.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger_unittest.cc:40: const base::TimeDelta& rtt) override {} Declare TimeDelta. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_network_t... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_network_t... net/quic/quic_network_transaction_unittest.cc:199: const base::TimeTicks& timestamp, Declare. https://codereview.chromium.org/1672513002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1672513002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31360: + Rough estimate of the packet loss rate multiplied by 100, before a Why is it rough? https://codereview.chromium.org/1672513002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31363: + This metric is recorded when a connectivity change is detected. This will Is this per network session? I.e., my desktop Chrome will only report once, but my Android Chrome will report several times a day? I think we should find a better interval. How about on every page load? Or on every connectivity change (including startup) and at least once every k hours?
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
bengr: ptal. thanks. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:122: const float NetworkQualityEstimator::kMinimumValidPacketLossRate = 0.0f; On 2016/02/19 00:45:08, bengr wrote: > I think you can get rid of this constant. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:470: base::HistogramBase* fastest_rtt_histogram = On 2016/02/19 00:45:07, bengr wrote: > I'd save this for a different CL. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:478: base::HistogramBase* peak_kbps_histogram = On 2016/02/19 00:45:07, bengr wrote: > Likewise, this has nothing to do with packet loss. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:480: 1000000); // UMA_HISTOGRAM_COUNTS On 2016/02/19 00:45:07, bengr wrote: > To what does this comment refer? Obsolete. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:487: DCHECK_LE(kMinimumValidPacketLossRate, packet_loss_rate); On 2016/02/19 00:45:07, bengr wrote: > I would just do: > > float packet_loss_rate; > if (GetPacketLossRateEstimate(&packet_loss_rate)) { > DCHECK_LE(0.0, packet_loss_rate); > DCHECK_GE(1.0, packet_loss_rate); > ... Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:492: packet_loss_histogram->Add(packet_loss_rate * 100); On 2016/02/19 00:45:08, bengr wrote: > Add(packet_loss_rate * 100.0f) Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:559: float* packet_loss) const { On 2016/02/19 00:45:07, bengr wrote: > packet_loss_rate_estimate Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:562: *packet_loss = GetPacketLossRateEstimateInternal(base::TimeTicks()); On 2016/02/19 00:45:07, bengr wrote: > It might be cleaner to not set packet_loss_rate_estimate if invalid. I.e.: > > float estimate = GetPacketLossRateEstimateInternal(base::TimeTicks()); > if (estimate < 0.0 || estimate > 1.0) > return false; > *packet_loss_rate_estimate = estimate; > Done. > Also, you might want to introduce a helper IsValidPacketLossRate(), but that > might be overkill. Right, I do not think this is needed. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:638: packet_loss_rate_observations_.GetWeightedAverage(begin_timestamp, On 2016/02/19 00:45:07, bengr wrote: > I would try to rewrite GetPacketLossRateEstimate, > GetPacketLossRateEstimateInternal and GetWeightedAverage to eliminate this > method. Ideally it would look like: > > bool NQE::GetPacketLossRateEstiamte(...) { > return packet_loss_rate_observations_.GetWeightedAverage); > } This is probably not possible. NQE exposes two methods: GetRTTEstimate() and GetRecentMedianRTT( const base::TimeTicks& begin_timestamp) Both of them use the same function internally: GetRTTEstimateInternal(const base::TimeTicks& begin_timestamp, int percentile). Same argument applies for bandwidth and packet loss. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:722: // Stores WeightedObservation in increasing order of value, although for the On 2016/02/19 00:45:08, bengr wrote: > Is the front or the back the highest value? Expanded the comment. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:722: // Stores WeightedObservation in increasing order of value, although for the On 2016/02/19 00:45:08, bengr wrote: > WeightedObservation -> weighted observations Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:726: // Total weight of all observations in |weighted_observations|. On 2016/02/19 00:45:07, bengr wrote: > The sum of the weights of all observations in |weighted_observations|. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:941: const Protocol protocol, On 2016/02/19 00:45:08, bengr wrote: > No need for const. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:117: // rate. On 2016/02/19 00:45:08, bengr wrote: > ... for consumers of raw observations. Packet loss estimates do not provide this > extra information. Removed that comment. It was not very useful. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:119: uint64_t num_packets_lost, On 2016/02/19 00:45:08, bengr wrote: > Add concise examples, e.g.: > > // Last observation after packet #1. Packets #5 and #6 then received: > // lost = 2, in_order = 2, not_in_order = 0 > > // Last observation after packet #6. Packet #3 received: > // lost = 1 (is this right??), in_order = 0, not_in_order = 1 Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:173: // |packet_loss| should not be NULL. |packet_loss| is always between 0.0 On 2016/02/19 00:45:08, bengr wrote: > should -> must Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:206: const Protocol protocol, On 2016/02/19 00:45:08, bengr wrote: > No need for const. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:209: uint64_t num_packets_received_not_in_order) override; On 2016/02/19 00:45:08, bengr wrote: > I'd prefer smaller types. 8 bytes seems like overkill. Using size_t which is the right type for counts. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:454: // value among all observations since |begin_timestamp|. If the value is On 2016/02/19 00:45:08, bengr wrote: > among -> of Done. > > Is that inclusive of begin_timestamp? Added comment. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:507: // lost. Currently, the observation period is set to 1 packet. On 2016/02/19 00:45:08, bengr wrote: > 1 -> one Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:639: const Protocol protocol, On 2016/02/19 00:45:08, bengr wrote: > No need for const. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator_unittest.cc:322: // Expecting packet number p, but received packet number p+9. 1 out of 10 On 2016/02/19 00:45:08, bengr wrote: > p is a letter. :) > Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/network_qualit... net/base/network_quality_estimator_unittest.cc:474: float packet_loss_rate = On 2016/02/19 00:45:08, bengr wrote: > I don't think you need to initialize here, but if you do, just set it to -1.0f. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/socket_perform... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/socket_perform... net/base/socket_performance_watcher.h:39: // |num_packets_lost| is the number of packets lost. On 2016/02/19 00:45:08, bengr wrote: > The parameter definitions are tautologies. Consider adding more meaningful > definitions. Done. https://codereview.chromium.org/1672513002/diff/80001/net/base/socket_perform... File net/base/socket_performance_watcher_factory.h (right): https://codereview.chromium.org/1672513002/diff/80001/net/base/socket_perform... net/base/socket_performance_watcher_factory.h:56: // As an example, assume that the packets with sequence number 1, 2 and 5 have On 2016/02/19 00:45:08, bengr wrote: > I would be much more concise here. See my earlier comment on how to provide the > example. Done. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:541: const uint64_t diff = On 2016/02/19 00:45:08, bengr wrote: > Move this to after 548 Done. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:548: if (socket_performance_watcher_) { On 2016/02/19 00:45:08, bengr wrote: > if (!socket_performance_watcher) > return; > ... Done. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:550: // The received packet number has alower sequence number than the expected On 2016/02/19 00:45:08, bengr wrote: > alower -> a lower Done. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:566: num_packets_received_not_in_order); On 2016/02/19 00:45:08, bengr wrote: > I don't think you need "num_" but could be convinced you could replace it with > the suffix "_count" > > I'd change "not_in" to "out_of" Done. This file uses num_ at multiple places. I was just going with the convention in this file. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.cc:815: float num_lost = largest_received_packet_number_ - num_packets_received_; On 2016/02/19 00:45:08, bengr wrote: > I don't like "num." Maybe lost_count? Please remove "num_" everywhere. num is already there in this code (also, this is not my code). I changed this line in this CL because I found the variable name to be wrong and confusing (num_received has almost opposite meaning of num_lost). Not done. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... File net/quic/quic_connection_logger.h (right): https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger.h:126: // Reports the counts of the packets received on the uplink (remote endpoint On 2016/02/19 00:45:09, bengr wrote: > I think you just mean "reports packet receptions to the > |socket_performance_watcher_|." > > Where do packet numbers come from? Done. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... File net/quic/quic_connection_logger_unittest.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_connectio... net/quic/quic_connection_logger_unittest.cc:40: const base::TimeDelta& rtt) override {} On 2016/02/19 00:45:09, bengr wrote: > Declare TimeDelta. Done. https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_network_t... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/1672513002/diff/80001/net/quic/quic_network_t... net/quic/quic_network_transaction_unittest.cc:199: const base::TimeTicks& timestamp, On 2016/02/19 00:45:09, bengr wrote: > Declare. Did not notice that file was not included. This class is already using base::Time::Now() function. https://codereview.chromium.org/1672513002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1672513002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31360: + Rough estimate of the packet loss rate multiplied by 100, before a On 2016/02/19 00:45:09, bengr wrote: > Why is it rough? Done. https://codereview.chromium.org/1672513002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31363: + This metric is recorded when a connectivity change is detected. This will On 2016/02/19 00:45:09, bengr wrote: > Is this per network session? I.e., my desktop Chrome will only report once, but > my Android Chrome will report several times a day? > > I think we should find a better interval. How about on every page load? Or on > every connectivity change (including startup) and at least once every k hours? Changed it to record on every page load.
Lgtm with nits. https://codereview.chromium.org/1672513002/diff/180001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1672513002/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:8: #include <stdint.h> Add a blank line after. https://codereview.chromium.org/1672513002/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:349: std::map<std::string, std::string> variation_params; #include <string>
tbansal@chromium.org changed reviewers: + rch@chromium.org
rch: PTAL at net/quic/* Thanks! https://codereview.chromium.org/1672513002/diff/180001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1672513002/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:8: #include <stdint.h> On 2016/02/26 23:07:04, bengr wrote: > Add a blank line after. Done. https://codereview.chromium.org/1672513002/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:349: std::map<std::string, std::string> variation_params; On 2016/02/26 23:07:04, bengr wrote: > #include <string> Done.
https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:563: packets_lost = diff - 1; This is a very aggressive definition of loss. Reordering in the network is expected, and this definition would end up confusing out of order deliver with loss. Are you sure this is the right metric?
https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:563: packets_lost = diff - 1; On 2016/03/01 18:20:32, Ryan Hamilton wrote: > This is a very aggressive definition of loss. Reordering in the network is > expected, and this definition would end up confusing out of order deliver with > loss. Are you sure this is the right metric? This API also reports p_r_out_of_order count. So, packets that were delivered later (due to reorder) would get reported to the callee (NQE). At the callee, value of packets_lost - p_r_out_of_order should give an estimate of how many packets are currently considered as lost. I did not put this logic here because missing packets may give an early indication of packet losses.
https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:563: packets_lost = diff - 1; On 2016/03/01 19:05:36, tbansal1 wrote: > On 2016/03/01 18:20:32, Ryan Hamilton wrote: > > This is a very aggressive definition of loss. Reordering in the network is > > expected, and this definition would end up confusing out of order deliver with > > loss. Are you sure this is the right metric? > > This API also reports p_r_out_of_order count. So, packets that were delivered > later (due to reorder) would get reported to the callee (NQE). At the callee, > value of packets_lost - p_r_out_of_order should give an estimate of how many > packets are currently considered as lost. If you have a variable called "packets_lost" but in order to compute the number of lost packets you need to subtract out_of_order from packets_lost, then packets_lost is the wrong name for that variable, I think. Perhaps you can come up with a better name? > I did not put this logic here because missing packets may give an early > indication of packet losses. Please do not do this. Reordering is common. This "early warning" is not advisable. Also, we have plans is QUIC for the sender to intentionally send gaps. For example after sending packets 1-100, the sender might jump to 500. (Long story). I'm concerned that your calculations here might do the wrong thing. If you wanted to measure up-stream packet loss, we can do that explicitly, of course.
https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:563: packets_lost = diff - 1; I will change the name, and send out the CL. In the meantime, question for you: If sender is going to skip sequence numbers, surely there must be some way control signal for receiver to be notified of that? Otherwise, how would the receiver report the list of missing_packets in QuicAckFrame to the sender? On 2016/03/02 04:26:49, Ryan Hamilton wrote: > On 2016/03/01 19:05:36, tbansal1 wrote: > > On 2016/03/01 18:20:32, Ryan Hamilton wrote: > > > This is a very aggressive definition of loss. Reordering in the network is > > > expected, and this definition would end up confusing out of order deliver > with > > > loss. Are you sure this is the right metric? > > > > This API also reports p_r_out_of_order count. So, packets that were delivered > > later (due to reorder) would get reported to the callee (NQE). At the callee, > > value of packets_lost - p_r_out_of_order should give an estimate of how many > > packets are currently considered as lost. > > If you have a variable called "packets_lost" but in order to compute the number > of lost packets you need to subtract out_of_order from packets_lost, then > packets_lost is the wrong name for that variable, I think. Perhaps you can come > up with a better name? > > > I did not put this logic here because missing packets may give an early > > indication of packet losses. > > Please do not do this. Reordering is common. This "early warning" is not > advisable. > > Also, we have plans is QUIC for the sender to intentionally send gaps. For > example after sending packets 1-100, the sender might jump to 500. (Long story). > I'm concerned that your calculations here might do the wrong thing. > > If you wanted to measure up-stream packet loss, we can do that explicitly, of > course.
https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:563: packets_lost = diff - 1; On 2016/03/03 02:39:05, tbansal1 wrote: > I will change the name, and send out the CL. In the meantime, question for you: > If sender is going to skip sequence numbers, surely there must be some way > control signal for receiver to be notified of that? Otherwise, how would the > receiver report the list of missing_packets in QuicAckFrame to the sender? The receiver will see packets 1, 2, ... 100, 500, 501, 502 and so it will report packets 101-499 as missing in the ack frame.
Patchset #6 (id:220001) has been deleted
rch: ptal. thanks. I rebased, and now it is using the uplink.
On 2016/03/03 04:24:24, Ryan Hamilton wrote: > https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... > File net/quic/quic_connection_logger.cc (right): > > https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... > net/quic/quic_connection_logger.cc:563: packets_lost = diff - 1; > On 2016/03/03 02:39:05, tbansal1 wrote: > > I will change the name, and send out the CL. In the meantime, question for > you: > > If sender is going to skip sequence numbers, surely there must be some way > > control signal for receiver to be notified of that? Otherwise, how would the > > receiver report the list of missing_packets in QuicAckFrame to the sender? > > The receiver will see packets 1, 2, ... 100, 500, 501, 502 > and so it will report packets 101-499 as missing in the ack frame. So, the receiver would never know that 101-499 were deliberately skipped? Would that not cause the receiver to get stuck, or unnecessarily keep reporting 101-499 as missing forever?
Patchset #6 (id:240001) has been deleted
On 2016/03/03 19:52:40, tbansal1 wrote: > On 2016/03/03 04:24:24, Ryan Hamilton wrote: > > > https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... > > File net/quic/quic_connection_logger.cc (right): > > > > > https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... > > net/quic/quic_connection_logger.cc:563: packets_lost = diff - 1; > > On 2016/03/03 02:39:05, tbansal1 wrote: > > > I will change the name, and send out the CL. In the meantime, question for > > you: > > > If sender is going to skip sequence numbers, surely there must be some way > > > control signal for receiver to be notified of that? Otherwise, how would the > > > receiver report the list of missing_packets in QuicAckFrame to the sender? > > > > The receiver will see packets 1, 2, ... 100, 500, 501, 502 > > and so it will report packets 101-499 as missing in the ack frame. > > So, the receiver would never know that 101-499 were deliberately skipped? Would > that not cause the receiver to get stuck, or unnecessarily keep reporting > 101-499 as missing forever? Also, would there be a minimum gap on the skipped sequence numbers? e.g., if there is a guarantee that the sequence numbers would jump by at least 100 when skipping, then I can filter those cases out. I am just interested in some packet loss events, not all. So, I do not mind if I lose an event where 100 packets got dropped at the same time.
rch: ptal. thanks! https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/200001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:563: packets_lost = diff - 1; On 2016/03/02 04:26:49, Ryan Hamilton wrote: > On 2016/03/01 19:05:36, tbansal1 wrote: > > On 2016/03/01 18:20:32, Ryan Hamilton wrote: > > > This is a very aggressive definition of loss. Reordering in the network is > > > expected, and this definition would end up confusing out of order deliver > with > > > loss. Are you sure this is the right metric? > > > > This API also reports p_r_out_of_order count. So, packets that were delivered > > later (due to reorder) would get reported to the callee (NQE). At the callee, > > value of packets_lost - p_r_out_of_order should give an estimate of how many > > packets are currently considered as lost. > > If you have a variable called "packets_lost" but in order to compute the number > of lost packets you need to subtract out_of_order from packets_lost, then > packets_lost is the wrong name for that variable, I think. Perhaps you can come > up with a better name? > > > I did not put this logic here because missing packets may give an early > > indication of packet losses. > > Please do not do this. Reordering is common. This "early warning" is not > advisable. > > Also, we have plans is QUIC for the sender to intentionally send gaps. For > example after sending packets 1-100, the sender might jump to 500. (Long story). > I'm concerned that your calculations here might do the wrong thing. > > If you wanted to measure up-stream packet loss, we can do that explicitly, of > course. Done.
https://codereview.chromium.org/1672513002/diff/260001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/260001/net/base/network_quali... net/base/network_quality_estimator.cc:1031: if (packets_received_in_order >= 3) { But this argument is really only going to ever be 0 or 1, right? https://codereview.chromium.org/1672513002/diff/260001/net/base/network_quali... net/base/network_quality_estimator.cc:1039: PacketLossRateObservation(0.0, now, observation_source)); You're computing packet loss rate per-packet? That seems unusual. https://codereview.chromium.org/1672513002/diff/260001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1672513002/diff/260001/net/base/socket_perfor... net/base/socket_performance_watcher.h:50: void OnUpdatedPacketCountAvailable( Perhaps s/Updated/Incremental/? https://codereview.chromium.org/1672513002/diff/260001/net/base/socket_perfor... net/base/socket_performance_watcher.h:51: size_t packets_missing, I was hoping to see a comment here about how out_of_order and missing interact to determine the packet loss rate. Can you add one? https://codereview.chromium.org/1672513002/diff/260001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/260001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:559: // as lost. "Lost" is probably not what you mean to say. How about "currently missing, but they may be received later and will be counted as out of order"
Patchset #7 (id:280001) has been deleted
Patchset #7 (id:300001) has been deleted
Patchset #7 (id:320001) has been deleted
rch: ptal. thanks. https://codereview.chromium.org/1672513002/diff/260001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/260001/net/base/network_quali... net/base/network_quality_estimator.cc:1031: if (packets_received_in_order >= 3) { On 2016/03/07 20:02:10, Ryan Hamilton wrote: > But this argument is really only going to ever be 0 or 1, right? Obsolete. https://codereview.chromium.org/1672513002/diff/260001/net/base/network_quali... net/base/network_quality_estimator.cc:1039: PacketLossRateObservation(0.0, now, observation_source)); On 2016/03/07 20:02:10, Ryan Hamilton wrote: > You're computing packet loss rate per-packet? That seems unusual. Obsolete. https://codereview.chromium.org/1672513002/diff/260001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1672513002/diff/260001/net/base/socket_perfor... net/base/socket_performance_watcher.h:50: void OnUpdatedPacketCountAvailable( On 2016/03/07 20:02:10, Ryan Hamilton wrote: > Perhaps s/Updated/Incremental/? Done. https://codereview.chromium.org/1672513002/diff/260001/net/base/socket_perfor... net/base/socket_performance_watcher.h:51: size_t packets_missing, On 2016/03/07 20:02:10, Ryan Hamilton wrote: > I was hoping to see a comment here about how out_of_order and missing interact > to determine the packet loss rate. Can you add one? There are 2 use cases for this: (1) Report the raw incremental packet count numbers to the observers (One app based on Cronet is waiting for this CL. They plan to run their own logic on the raw data). (2) Compute the packet loss rate for use within Chromium. This would require figuring out how the 3 counts interact with each other. I added some more comments in nqe.cc::OnIncrementalPacketCountAvailable() on how this can be done. I have removed the code for this from this CL since at this point, it is not clear if it is really needed. https://codereview.chromium.org/1672513002/diff/260001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/260001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:559: // as lost. On 2016/03/07 20:02:10, Ryan Hamilton wrote: > "Lost" is probably not what you mean to say. How about "currently missing, but > they may be received later and will be counted as out of order" Done.
Description was changed from ========== Expose packet loss counts from QUIC to NetworkQualityEstimator. NQE now provides an estimated packet loss rate computed from recent observations (higher weight given to more recent observations). Also, the packet loss observations are notified to the all PacketLossObservers that have registered with NQE. Next CL will register Cronet as a packet loss observer with NQE. BUG=548719 ========== to ========== Expose packet loss counts from QUIC to NetworkQualityEstimator. Also, the packet loss observations are notified to the all PacketLossObservers that have registered with NQE. Next CL will register Cronet as a packet loss observer with NQE. BUG=548719 ==========
rch@chromium.org changed reviewers: + jri@chromium.org
I'm including Jana on this because he's pretty familiar with the issues around client-side estimation of packet loss. https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... net/base/network_quality_estimator.cc:454: default: nit: If there are only two value, you can omit the default: case, I think. https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... net/base/network_quality_estimator.h:110: // currently missing, but they may be received later and will be counted s/currently missing/newly missing/
On 2016/03/10 00:29:53, Ryan Hamilton wrote: > I'm including Jana on this because he's pretty familiar with the issues around > client-side estimation of packet loss. I am going to fix other errors, but in the meantime just wanted to comment that this CL is no longer doing packet loss estimation or detection (I will do it in some CL in future). This CL is just passing packet counts to the observer (Cronet would be one of the observers). > > https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... > File net/base/network_quality_estimator.cc (right): > > https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... > net/base/network_quality_estimator.cc:454: default: > nit: If there are only two value, you can omit the default: case, I think. > > https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... > File net/base/network_quality_estimator.h (right): > > https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... > net/base/network_quality_estimator.h:110: // currently missing, but they may be > received later and will be counted > s/currently missing/newly missing/
https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... net/base/network_quality_estimator.cc:454: default: On 2016/03/10 00:29:53, Ryan Hamilton wrote: > nit: If there are only two value, you can omit the default: case, I think. Done. https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/340001/net/base/network_quali... net/base/network_quality_estimator.h:110: // currently missing, but they may be received later and will be counted On 2016/03/10 00:29:53, Ryan Hamilton wrote: > s/currently missing/newly missing/ Done.
ping!
Apologies for not getting to this earlier. A couple of high-order thoughts -- I am happy to chat over VC next week (if that's easier) about what API might make sense in the context of a QUIC receiver. (You can't get this for TCP, by the way; is that a concern?) On the mechanism, the missing packet detection scheme is simplistic and any signal coming out of this machinery is likely to be noisy. QUIC has more than one loss detection algorithm (at the sender), and we are continually playing with changing how we detect loss within QUIC, largely as a response to reordering that we commonly see in the network. Loss detection is usually tuned in the context of what happens in response... I have no idea how this information that is being exposed will be used, which makes it hard to evaluate this loss detector. What is the proposed use case for this signal? It should not be used without careful filtering (and I'm not sure how this filtering can be done for the missing/reordering signals that are being delivered), but I am concerned about exposing such low-level noisy signals to arbitrary consumers. https://codereview.chromium.org/1672513002/diff/360001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1672513002/diff/360001/net/base/network_quali... net/base/network_quality_estimator.h:117: // missing = 3, in_order = 2, out_of_order = 0. Inferring missing packets is something that QUIC/TCP's loss detection mechanisms do, and simply inferring 2, 3, and 4 as missing here would be aggressive, and may not be in line with what the transport underneath is doing. For instance, what if immediately after 6 is received, 2, 3, 4 are received? https://codereview.chromium.org/1672513002/diff/360001/net/quic/quic_connecti... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/1672513002/diff/360001/net/quic/quic_connecti... net/quic/quic_connection_logger.cc:828: base::TimeDelta::FromMicroseconds(rtt.ToMicroseconds())); this rtt signal will be quite noisy.
On 2016/03/12 03:11:28, Jana wrote: > Apologies for not getting to this earlier. A couple of high-order thoughts -- I > am happy to chat over VC next week (if that's easier) about what API might make > sense in the context of a QUIC receiver. (You can't get this for TCP, by the > way; is that a concern?) Sure, I will set up a meeting. I also wanted to get your feedback on the bandwidth doc. VC would be super helpful. > > On the mechanism, the missing packet detection scheme is simplistic and any > signal coming out of this machinery is likely to be noisy. QUIC has more than > one loss detection algorithm (at the sender), and we are continually playing > with changing how we detect loss within QUIC, largely as a response to > reordering that we commonly see in the network. > > Loss detection is usually tuned in the context of what happens in response... I > have no idea how this information that is being exposed will be used, which > makes it hard to evaluate this loss detector. What is the proposed use case for > this signal? It should not be used without careful filtering (and I'm not sure > how this filtering can be done for the missing/reordering signals that are being > delivered), but I am concerned about exposing such low-level noisy signals to > arbitrary consumers. > > https://codereview.chromium.org/1672513002/diff/360001/net/base/network_quali... > File net/base/network_quality_estimator.h (right): > > https://codereview.chromium.org/1672513002/diff/360001/net/base/network_quali... > net/base/network_quality_estimator.h:117: // missing = 3, in_order = 2, > out_of_order = 0. > Inferring missing packets is something that QUIC/TCP's loss detection mechanisms > do, and simply inferring 2, 3, and 4 as missing here would be aggressive, and > may not be in line with what the transport underneath is doing. For instance, > what if immediately after 6 is received, 2, 3, 4 are received? > > https://codereview.chromium.org/1672513002/diff/360001/net/quic/quic_connecti... > File net/quic/quic_connection_logger.cc (right): > > https://codereview.chromium.org/1672513002/diff/360001/net/quic/quic_connecti... > net/quic/quic_connection_logger.cc:828: > base::TimeDelta::FromMicroseconds(rtt.ToMicroseconds())); > this rtt signal will be quite noisy.
I'm happy to defer to Jana. Let me know if you'd like me to chime back in.
I am going to close this CL for now. I agree with jri and rch that this may not be a good idea to pursue.
Message was sent while issue was closed.
tbansal@chromium.org changed reviewers: - bengr@chromium.org, jri@chromium.org, rch@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
