Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
180833004:
Gather histogram for received packets indicating losses. (Closed)
|
Created:
6 years, 9 months ago by jar (doing other things) Modified:
6 years, 9 months ago Reviewers:
Ryan Hamilton CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGather histogram for received packets indicating losses.
Gather separate histograms for for first 100 received, and for missing (NACKed) packets.
We should be able to take the ratio, and see any correlation of packet loss with position
in the server transmitted stream.
r=rch
BUG=349314
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255986
Patch Set 1 #
Total comments: 14
Patch Set 2 : Vary the histogram depending on connection type. #Patch Set 3 : Provide prose in histograms.xml #Patch Set 4 : Respond to comments, and add small optimization #
Total comments: 8
Patch Set 5 : Respond to comments #Patch Set 6 : fix arm build; using size_t rather than uint64 for const static initialized in header #
Total comments: 10
Patch Set 7 : Fix a few nits #Patch Set 8 : Spell WiFi with capitalization in prose. #Patch Set 9 : fix Linux ASAN compile issues #
Messages
Total messages: 21 (0 generated)
https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... File net/quic/quic_connection_logger.cc (right): https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:29: const size_t MAX_PACKET_STATUS = 100; nit: kMaxPacketStatus, I think, right? https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:241: const size_t last_index = std::min<size_t>(MAX_PACKET_STATUS - 1, nit: We typically just say min(foo, bar); Is the <size_t> required here? https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:246: UMA_HISTOGRAM_ENUMERATION("Net.QuicSessionPacketReceivedAck", i - 1, Interesting! I didn't realize ENUMERATION was applicable without actual enums. I'll have to remember this for next time. https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:352: packets_received_[largest_received_packet_sequence_number_] = true; nit: i think the style guide says that DCHECK is only for situations that will NEVER happen, and so handling the condition where the DCHECK is not satisfied is prohibited. If you want to use a DCHECK, then I think you should do use largest_received_packet_sequence_number_ - 1 as the array index. If you're afraid of dealing with 0, then I would use an explicit if statement to deal with this case. https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:352: packets_received_[largest_received_packet_sequence_number_] = true; It looks like this code only records packets that arrive in order because of the "if" on line 336. Is that what you intended? https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... File net/quic/quic_connection_logger.h (right): https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.h:84: std::vector<bool> packets_received_; You could also consider using a std::bitset, if you'd like, since it's optimized to hold single bits.
Ryan: PTAL The good news is that this should generally provide the histograms broken down by connection type. The bad news is that the functionality on Windows was not completely implemented (but I assume(?) it was more implemented on Android). I'll work to complete the Windows implementation... but for now, it only returns CONNECITON_UNKNOWN (whether it is WiFi, or Etherenet, :-( ). https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... File net/quic/quic_connection_logger.cc (right): https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:29: const size_t MAX_PACKET_STATUS = 100; On 2014/03/05 03:48:00, Ryan Hamilton wrote: > nit: kMaxPacketStatus, I think, right? Chromium style guide seems to ask for this format: http://www.chromium.org/developers/coding-style https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:241: const size_t last_index = std::min<size_t>(MAX_PACKET_STATUS - 1, On 2014/03/05 03:48:00, Ryan Hamilton wrote: > nit: We typically just say min(foo, bar); Is the <size_t> required here? I think the code may have been wrong, since it was handling uint64 as well as size_t. I wrote it out (with one explicit cast) so that there is no chance for ambiguity. https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:246: UMA_HISTOGRAM_ENUMERATION("Net.QuicSessionPacketReceivedAck", i - 1, On 2014/03/05 03:48:00, Ryan Hamilton wrote: > Interesting! I didn't realize ENUMERATION was applicable without actual enums. > I'll have to remember this for next time. Yes... it works nicely when what you really want is a linear histogram, with buckets at every integer. I ended up changing this to have to call for a LinearHistogram manually... since I now dynamically form the name. https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:352: packets_received_[largest_received_packet_sequence_number_] = true; On 2014/03/05 03:48:00, Ryan Hamilton wrote: > nit: i think the style guide says that DCHECK is only for situations that will > NEVER happen, and so handling the condition where the DCHECK is not satisfied is > prohibited. If you want to use a DCHECK, then I think you should do use > largest_received_packet_sequence_number_ - 1 as the array index. If you're > afraid of dealing with 0, then I would use an explicit if statement to deal with > this case. I moved around the DCHECK, and it now just checks when I'm recording things into the histogram. I think this may comply with the style rules. I don't handle the case where the DCHECK is false. https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:352: packets_received_[largest_received_packet_sequence_number_] = true; On 2014/03/05 03:48:00, Ryan Hamilton wrote: > It looks like this code only records packets that arrive in order because of the > "if" on line 336. Is that what you intended? No. That was not intended... I messed this up in my refactors. I fixed it to record any packet arrival. It might be interesting to look for "skipped" packets with another bitset... but I'll leave that for future exploration. https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... File net/quic/quic_connection_logger.h (right): https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.h:84: std::vector<bool> packets_received_; On 2014/03/05 03:48:00, Ryan Hamilton wrote: > You could also consider using a std::bitset, if you'd like, since it's optimized > to hold single bits. Done.
https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... File net/quic/quic_connection_logger.cc (right): https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:29: const size_t MAX_PACKET_STATUS = 100; On 2014/03/06 03:34:17, jar wrote: > On 2014/03/05 03:48:00, Ryan Hamilton wrote: > > nit: kMaxPacketStatus, I think, right? > > Chromium style guide seems to ask for this format: > http://www.chromium.org/developers/coding-style Do you mean this line: Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. Continue to use this style for consistency. That seems to be for enum not static variables. Or are you referring to some other section of the guide? https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... File net/quic/quic_connection_logger.cc (right): https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... net/quic/quic_connection_logger.cc:335: packets_received_[header.packet_sequence_number] = true; Since you're comparing with <, and not indexing by sequence_number - 1, I think this means that you'll actually only end up recoding 99 packets, not 100. Seems like indexing by -1 would be ideal. https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... net/quic/quic_connection_logger.cc:484: prefix + "Ack_" + suffix, 1, MAX_PACKET_STATUS, MAX_PACKET_STATUS + 1, Can you extract this logic into a single method that's something like: base::HistogramBase* GetAckHistogram("Ack_" | "Nack_"); Then you can say: base::HistogramBase* ack_histogram = GetAckHistogram("Ack_"); base::HistogramBase* nack_histogram = GetAckHistogram("Nack_"); https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... net/quic/quic_connection_logger.cc:487: // NACK histogram when we need it (just in time). Given that this code runs extremely infrequently, it seems unlikely that it's performance sensitive at all. Consequently, I don't think that it's worth the complexity of doing this just-in-time initialization. https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... net/quic/quic_connection_logger.cc:494: static_cast<size_t>(largest_received_packet_sequence_number_); I don't think I understand why doing this manually is "easier", since it requires 4 lines to implement and a 2 line comment to explain :> It would seem like this code should work just fine: const size_t last_index = min(MAX_PACKET_STATUS - 1, static_cast<size_t>(largest_received_packet_sequence_number_)); Though I think the min<size_t> you had before would also work just fine. Alternatively, how about just making MAX_PACKET_STATUS a unit64 which avoids all casting problems. (Or just #define. Ok, not really :>)
On Wed, Mar 5, 2014 at 7:34 PM, <jar@chromium.org> wrote: > > The bad news is that the functionality on Windows was not completely > implemented > (but I assume(?) it was more implemented on Android). > > I'll work to complete the Windows implementation... but for now, it only > returns > CONNECITON_UNKNOWN (whether it is WiFi, or Etherenet, :-( ). > Do you know if it's implemented on OS X or Chrome OS? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... File net/quic/quic_connection_logger.cc (right): https://chromiumcodereview.appspot.com/180833004/diff/1/net/quic/quic_connect... net/quic/quic_connection_logger.cc:29: const size_t MAX_PACKET_STATUS = 100; On 2014/03/06 04:47:43, Ryan Hamilton wrote: > On 2014/03/06 03:34:17, jar wrote: > > On 2014/03/05 03:48:00, Ryan Hamilton wrote: > > > nit: kMaxPacketStatus, I think, right? > > > > Chromium style guide seems to ask for this format: > > http://www.chromium.org/developers/coding-style > > Do you mean this line: > > Though the Google C++ Style Guide now says to use kConstantNaming for enums, > Chromium was written using MACRO_STYLE naming. Continue to use this style for > consistency. > > That seems to be for enum not static variables. Or are you referring to some > other section of the guide? > Done. https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... File net/quic/quic_connection_logger.cc (right): https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... net/quic/quic_connection_logger.cc:335: packets_received_[header.packet_sequence_number] = true; On 2014/03/06 04:47:43, Ryan Hamilton wrote: > Since you're comparing with <, and not indexing by sequence_number - 1, I think > this means that you'll actually only end up recoding 99 packets, not 100. Seems > like indexing by -1 would be ideal. Fixed per discussion. https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... net/quic/quic_connection_logger.cc:484: prefix + "Ack_" + suffix, 1, MAX_PACKET_STATUS, MAX_PACKET_STATUS + 1, On 2014/03/06 04:47:43, Ryan Hamilton wrote: > Can you extract this logic into a single method that's something like: > > base::HistogramBase* GetAckHistogram("Ack_" | "Nack_"); > > Then you can say: > > base::HistogramBase* ack_histogram = GetAckHistogram("Ack_"); > base::HistogramBase* nack_histogram = GetAckHistogram("Nack_"); Done. https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... net/quic/quic_connection_logger.cc:487: // NACK histogram when we need it (just in time). On 2014/03/06 04:47:43, Ryan Hamilton wrote: > Given that this code runs extremely infrequently, it seems unlikely that it's > performance sensitive at all. Consequently, I don't think that it's worth the > complexity of doing this just-in-time initialization. Done. https://chromiumcodereview.appspot.com/180833004/diff/20002/net/quic/quic_con... net/quic/quic_connection_logger.cc:494: static_cast<size_t>(largest_received_packet_sequence_number_); On 2014/03/06 04:47:43, Ryan Hamilton wrote: > I don't think I understand why doing this manually is "easier", since it > requires 4 lines to implement and a 2 line comment to explain :> > > It would seem like this code should work just fine: > > const size_t last_index = min(MAX_PACKET_STATUS - 1, > static_cast<size_t>(largest_received_packet_sequence_number_)); > > Though I think the min<size_t> you had before would also work just fine. > > Alternatively, how about just making MAX_PACKET_STATUS a unit64 which avoids all > casting problems. (Or just #define. Ok, not really :>) Per discussion... I changed to consistent typedefs that get uint64, ... and I'm ignoring the cost for now. ...but that broke the arm build.... so I ended up using size_t for the const static member, and explicitly selecting the template type here.
LGTM modulo a few minor nits. https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... File net/quic/quic_connection_logger.cc (left): https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... net/quic/quic_connection_logger.cc:25: nit: can you add this blank line back? https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... net/quic/quic_connection_logger.cc:478: std::string prefix("Net.QuicSession.PacketReceived_"); nit: since we're "using std::string", I think you can drop std:: https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... File net/quic/quic_connection_logger.h (right): https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... net/quic/quic_connection_logger.h:96: // that offset is never really used. nit: s/really// https://codereview.chromium.org/180833004/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/180833004/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:34533: + <group name="CONNECTION_UNKNOWN" label="Connections via WiFi are tallied."/> nit: I'm not sure that "are tallied" adds any value here since the tallying is pretty much what a histogram does. Your call though. https://codereview.chromium.org/180833004/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:34540: + <group name="CONNECTION_2G" label="Connections mobile 2G are tallied."/> nit: did you mean to say "*via* mobile" here and below?
https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... File net/quic/quic_connection_logger.cc (left): https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... net/quic/quic_connection_logger.cc:25: On 2014/03/07 17:47:05, Ryan Hamilton wrote: > nit: can you add this blank line back? Done. https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... net/quic/quic_connection_logger.cc:478: std::string prefix("Net.QuicSession.PacketReceived_"); On 2014/03/07 17:47:05, Ryan Hamilton wrote: > nit: since we're "using std::string", I think you can drop std:: Done. https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... File net/quic/quic_connection_logger.h (right): https://codereview.chromium.org/180833004/diff/80001/net/quic/quic_connection... net/quic/quic_connection_logger.h:96: // that offset is never really used. On 2014/03/07 17:47:05, Ryan Hamilton wrote: > nit: s/really// Done. https://codereview.chromium.org/180833004/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/180833004/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:34533: + <group name="CONNECTION_UNKNOWN" label="Connections via WiFi are tallied."/> On 2014/03/07 17:47:05, Ryan Hamilton wrote: > nit: I'm not sure that "are tallied" adds any value here since the tallying is > pretty much what a histogram does. Your call though. This line of text is added to the "prose" section that provides a description of the histogram. As a result, I try to use full sentences here (unlike an enum, which provides a super-terse description of a bucket). As a result, I have to say something like: This histogram only includes data about WiFi connections. I was looking for a short sentence to convey that.... but I can go longer if you have a better sentence in mind. https://codereview.chromium.org/180833004/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:34540: + <group name="CONNECTION_2G" label="Connections mobile 2G are tallied."/> On 2014/03/07 17:47:05, Ryan Hamilton wrote: > nit: did you mean to say "*via* mobile" here and below? Done.
The CQ bit was checked by jar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/180833004/110001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/180833004/110001
The CQ bit was unchecked by jar@chromium.org
The CQ bit was checked by jar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/180833004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by jar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/180833004/120001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/180833004/120001
FYI: Linux ASAN was not happy with even the size_t initialization in the header... so I went a different route. Comments welcome... but I'd probably only change things in a follow-up CL.
Message was sent while issue was closed.
Change committed as 255986 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
