Implement UMA and internal data structure for tracking EWOULDBLOCK.

content/renderer/p2p/ipc_socket_factory.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/p2p/ipc_socket_factory.h"
 
 #include <algorithm>
 #include <deque>
 
 #include "base/compiler_specific.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/metrics/histogram.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/non_thread_safe.h"
 #include "content/renderer/media/webrtc_logging.h"
 #include "content/renderer/p2p/host_address_request.h"
 #include "content/renderer/p2p/socket_client_delegate.h"
 #include "content/renderer/p2p/socket_client_impl.h"
 #include "content/renderer/p2p/socket_dispatcher.h"
 #include "jingle/glue/utils.h"
 #include "third_party/webrtc/base/asyncpacketsocket.h"
 
@@ skipping 83 matching lines @@
 
  private:
   enum InternalState {
     IS_UNINITIALIZED,
     IS_OPENING,
     IS_OPEN,
     IS_CLOSED,
     IS_ERROR,
   };
 
+  // Keep track of the max discarded byte sequence in the IpcPacketSocket

[jamesr, 2014/10/28 00:24:15]

what does 'max' mean here? maximum length? maximum value?

[guoweis2, 2014/10/28 04:44:10]

Rephrased this.

+  // session.
+  void IncrementDiscardCounters(uint32 bytes_discarded);
+
+  // Reset cur_discard_bytes_sequence_ to 0.

[jamesr, 2014/10/28 00:24:15]

not a useful comment. if this just sets one variable to 0, why is it even a
function?

[guoweis2, 2014/10/28 04:44:09]

Removed this function.

+  void RestartCurrentDiscardCounter();
+
   // Update trace of send throttling internal state. This should be called
   // immediately after any changes to |send_bytes_available_| and/or
   // |in_flight_packet_sizes_|.
   void TraceSendThrottlingState() const;
 
   void InitAcceptedTcp(P2PSocketClient* client,
                        const rtc::SocketAddress& local_address,
                        const rtc::SocketAddress& remote_address);
 
   int DoSetOption(P2PSocketOption option, int value);
@@ skipping 27 matching lines @@
   std::deque<size_t> in_flight_packet_sizes_;
 
   // Set to true once EWOULDBLOCK was returned from Send(). Indicates that the
   // caller expects SignalWritable notification.
   bool writable_signal_expected_;
 
   // Current error code. Valid when state_ == IS_ERROR.
   int error_;
   int options_[P2P_SOCKET_OPT_MAX];
 
+  // Track the maximum & current consecutive bytes discarded due to not enough

[jamesr, 2014/10/28 00:24:15]

use words in comments, not symbols.

[guoweis2, 2014/10/28 04:44:09]

Done.

+  // send_bytes_available_.
+  uint32 max_discard_bytes_sequence_;

[jamesr, 2014/10/28 00:24:15]

we use uint32_t, etc, not the _t-less versions

if this is representing a number of bytes it should be size_t.  it's unclear if
that's what it means, however

[guoweis2, 2014/10/28 04:44:10]

Done.

+  uint32 cur_discard_bytes_sequence_;

[jamesr, 2014/10/28 00:24:15]

don't use abbreviations. if 'cur' means current, say 'current'

[guoweis2, 2014/10/28 04:44:09]

Done.

+
+  // Track total count of packet drops and attempt of packet sends.

[jamesr, 2014/10/28 00:24:15]

this isn't very grammatical, could you try to rephrase? ideally this would be a
sentence

[guoweis2, 2014/10/28 04:44:09]

Done.

+  uint32 packets_discarded_;
+  uint32 total_packets_;
+
   DISALLOW_COPY_AND_ASSIGN(IpcPacketSocket);
 };
 
 // Simple wrapper around P2PAsyncAddressResolver. The main purpose of this
 // class is to send SignalDone, after OnDone callback from
 // P2PAsyncAddressResolver. Libjingle sig slots are not thread safe. In case
 // of MT sig slots clients must call disconnect. This class is to make sure
 // we destruct from the same thread on which is created.
 class AsyncAddressResolverImpl :  public base::NonThreadSafe,
                                   public rtc::AsyncResolverInterface {
@@ skipping 14 matching lines @@
   int port_;   // Port number in |addr| from Start() method.
   std::vector<rtc::IPAddress> addresses_;  // Resolved addresses.
 };
 
 IpcPacketSocket::IpcPacketSocket()
     : type_(P2P_SOCKET_UDP),
       message_loop_(base::MessageLoop::current()),
       state_(IS_UNINITIALIZED),
       send_bytes_available_(kMaximumInFlightBytes),
       writable_signal_expected_(false),
-      error_(0) {
+      error_(0),
+      max_discard_bytes_sequence_(0),
+      cur_discard_bytes_sequence_(0),
+      packets_discarded_(0),
+      total_packets_(0) {
   COMPILE_ASSERT(kMaximumInFlightBytes > 0, would_send_at_zero_rate);
   std::fill_n(options_, static_cast<int> (P2P_SOCKET_OPT_MAX),
               kDefaultNonSetOptionValue);
 }
 
 IpcPacketSocket::~IpcPacketSocket() {
   if (state_ == IS_OPENING || state_ == IS_OPEN ||
       state_ == IS_ERROR) {
     Close();
   }
+
+  UMA_HISTOGRAM_COUNTS_10000("WebRTC.ApplicationMaxConsecutiveBytesDiscard",
+                             max_discard_bytes_sequence_);
+
+  if (total_packets_ > 0) {
+    UMA_HISTOGRAM_PERCENTAGE("WebRTC.ApplicationPercenetPacketsDiscarded",

[jamesr, 2014/10/28 00:24:15]

'Percenet' - you probably mean Percent right?

[guoweis2, 2014/10/28 04:44:10]

Thanks for catching this. My test should have caught this but I'll double check
why it doesn't.

+                             (packets_discarded_ * 100) / total_packets_);
+  }
 }
 
 void IpcPacketSocket::TraceSendThrottlingState() const {
   TRACE_COUNTER_ID1("p2p", "P2PSendBytesAvailable", local_address_.port(),
                     send_bytes_available_);
   TRACE_COUNTER_ID1("p2p", "P2PSendPacketsInFlight", local_address_.port(),
                     in_flight_packet_sizes_.size());
 }
 
+void IpcPacketSocket::IncrementDiscardCounters(uint32 bytes_discarded) {
+  cur_discard_bytes_sequence_ += bytes_discarded;
+  packets_discarded_++;
+
+  if (cur_discard_bytes_sequence_ > max_discard_bytes_sequence_) {
+    max_discard_bytes_sequence_ = cur_discard_bytes_sequence_;
+  }
+}
+
+void IpcPacketSocket::RestartCurrentDiscardCounter() {
+  cur_discard_bytes_sequence_ = 0;
+}
+
 bool IpcPacketSocket::Init(P2PSocketType type,
                            P2PSocketClientImpl* client,
                            const rtc::SocketAddress& local_address,
                            const rtc::SocketAddress& remote_address) {
   DCHECK_EQ(base::MessageLoop::current(), message_loop_);
   DCHECK_EQ(state_, IS_UNINITIALIZED);
 
   type_ = type;
   client_ = client;
   local_address_ = local_address;
@@ skipping 81 matching lines @@
     case IS_OPEN:
       // Continue sending the packet.
       break;
   }
 
   if (data_size == 0) {
     NOTREACHED();
     return 0;
   }
 
+  total_packets_++;
+
   if (data_size > send_bytes_available_) {
     TRACE_EVENT_INSTANT1("p2p", "MaxPendingBytesWouldBlock",
                          TRACE_EVENT_SCOPE_THREAD,
                          "id",
                          client_->GetSocketID());
     if (!writable_signal_expected_) {
       WebRtcLogMessage(base::StringPrintf(
           "IpcPacketSocket: sending is blocked. %d packets_in_flight.",
           static_cast<int>(in_flight_packet_sizes_.size())));
 
       writable_signal_expected_ = true;
     }
 
     error_ = EWOULDBLOCK;
+    IncrementDiscardCounters(data_size);
     return -1;
+  } else {
+    RestartCurrentDiscardCounter();
   }
 
   net::IPEndPoint address_chrome;
   if (!jingle_glue::SocketAddressToIPEndPoint(address, &address_chrome)) {
     NOTREACHED();
     error_ = EINVAL;
     return -1;
   }
 
   send_bytes_available_ -= data_size;
@@ skipping 326 matching lines @@
 }
 
 rtc::AsyncResolverInterface*
 IpcPacketSocketFactory::CreateAsyncResolver() {
   scoped_ptr<AsyncAddressResolverImpl> resolver(
     new AsyncAddressResolverImpl(socket_dispatcher_));
   return resolver.release();
 }
 
 }  // namespace content