From 2551ddda4bcc0a1604e2e0bcbc23e68902e8128f Mon Sep 17 00:00:00 2001 From: rkaplow Date: Tue, 31 Mar 2015 14:09:33 -0700 Subject: [PATCH] Remove the ScopedBandwidthMetrics class and associated Net.DownloadBandwith metric. Also had to fix a couple missing includes. BUG=469288 Review URL: https://codereview.chromium.org/1045003002 Cr-Commit-Position: refs/heads/master@{#323108} --- net/base/bandwidth_metrics.cc | 36 ------ net/base/bandwidth_metrics.h | 136 ----------------------- net/http/http_stream_factory_impl_job.cc | 1 + net/net.gypi | 2 - net/spdy/spdy_stream.cc | 6 +- net/spdy/spdy_stream.h | 3 - tools/metrics/histograms/histograms.xml | 3 + 7 files changed, 5 insertions(+), 182 deletions(-) delete mode 100644 net/base/bandwidth_metrics.cc delete mode 100644 net/base/bandwidth_metrics.h diff --git a/net/base/bandwidth_metrics.cc b/net/base/bandwidth_metrics.cc deleted file mode 100644 index 0644122d7ddc7d..00000000000000 --- a/net/base/bandwidth_metrics.cc +++ /dev/null @@ -1,36 +0,0 @@ -// Copyright (c) 2011 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 "base/lazy_instance.h" -#include "net/base/bandwidth_metrics.h" - -static base::LazyInstance g_bandwidth_metrics = - LAZY_INSTANCE_INITIALIZER; - -namespace net { - -ScopedBandwidthMetrics::ScopedBandwidthMetrics() - : started_(false) { -} - -ScopedBandwidthMetrics::~ScopedBandwidthMetrics() { - if (started_) - g_bandwidth_metrics.Get().StopStream(); -} - -void ScopedBandwidthMetrics::StartStream() { - started_ = true; - g_bandwidth_metrics.Get().StartStream(); -} - -void ScopedBandwidthMetrics::StopStream() { - started_ = false; - g_bandwidth_metrics.Get().StopStream(); -} - -void ScopedBandwidthMetrics::RecordBytes(int bytes) { - g_bandwidth_metrics.Get().RecordBytes(bytes); -} - -} // namespace net diff --git a/net/base/bandwidth_metrics.h b/net/base/bandwidth_metrics.h deleted file mode 100644 index 679d65a673b6b1..00000000000000 --- a/net/base/bandwidth_metrics.h +++ /dev/null @@ -1,136 +0,0 @@ -// Copyright (c) 2011 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. - -#ifndef NET_BASE_BANDWIDTH_METRICS_H_ -#define NET_BASE_BANDWIDTH_METRICS_H_ - -#include - -#include "base/logging.h" -#include "base/metrics/histogram.h" -#include "base/time/time.h" - -namespace net { - -// Tracks statistics about the bandwidth metrics over time. In order to -// measure, this class needs to know when individual streams are in progress, -// so that it can know when to discount idle time. The BandwidthMetrics -// is unidirectional - it should only be used to record upload or download -// bandwidth, but not both. -// -// Note, the easiest thing to do is to just measure each stream and average -// them or add them. However, this does not work. If multiple streams are in -// progress concurrently, you have to look at the aggregate bandwidth at any -// point in time. -// -// Example: -// Imagine 4 streams opening and closing with overlapping time. -// We can't measure bandwidth by looking at any individual stream. -// We can only measure actual bandwidth by looking at the bandwidth -// across all open streams. -// -// Time ---------------------------------------> -// s1 +----------------+ -// s2 +----------------+ -// s3 +--------------+ -// s4 +--------------+ -// -// Example usage: -// -// BandwidthMetrics tracker; -// -// // When a stream is created -// tracker.StartStream(); -// -// // When data is transferred on any stream -// tracker.RecordSample(bytes); -// -// // When the stream is finished -// tracker.StopStream(); -// -// NOTE: This class is not thread safe. -// -class BandwidthMetrics { - public: - BandwidthMetrics() - : num_streams_in_progress_(0), - num_data_samples_(0), - data_sum_(0.0), - bytes_since_last_start_(0) { - } - - // Get the bandwidth. Returns Kbps (kilo-bits-per-second). - double bandwidth() const { - return data_sum_ / num_data_samples_; - } - - // Record that we've started a stream. - void StartStream() { - // If we're the only stream, we've finished some idle time. Record a new - // timestamp to indicate the start of data flow. - if (++num_streams_in_progress_ == 1) { - last_start_ = base::TimeTicks::Now(); - bytes_since_last_start_ = 0; - } - } - - // Track that we've completed a stream. - void StopStream() { - if (--num_streams_in_progress_ == 0) { - // We don't use small streams when tracking bandwidth because they are not - // precise; imagine a 25 byte stream. The sample is too small to make - // a good measurement. - // 20KB is an arbitrary value. We might want to use a lesser value. - static const int64 kRecordSizeThreshold = 20 * 1024; - if (bytes_since_last_start_ < kRecordSizeThreshold) - return; - - base::TimeDelta delta = base::TimeTicks::Now() - last_start_; - double ms = delta.InMillisecondsF(); - if (ms > 0.0) { - double kbps = static_cast(bytes_since_last_start_) * 8 / ms; - ++num_data_samples_; - data_sum_ += kbps; - VLOG(1) << "Bandwidth: " << kbps - << "Kbps (avg " << bandwidth() << "Kbps)"; - int kbps_int = static_cast(kbps); - UMA_HISTOGRAM_COUNTS_10000("Net.DownloadBandwidth", kbps_int); - } - } - } - - // Add a sample of the number of bytes read from the network into the tracker. - void RecordBytes(int bytes) { - DCHECK(num_streams_in_progress_); - bytes_since_last_start_ += static_cast(bytes); - } - - private: - int num_streams_in_progress_; // The number of streams in progress. - // TODO(mbelshe): Use a rolling buffer of 30 samples instead of an average. - int num_data_samples_; // The number of samples collected. - double data_sum_; // The sum of all samples collected. - int64 bytes_since_last_start_; // Bytes tracked during this "session". - base::TimeTicks last_start_; // Timestamp of the begin of this "session". -}; - -// A utility class for managing the lifecycle of a measured stream. -// It is important that we not leave unclosed streams, and this class helps -// ensure we always stop them. -class ScopedBandwidthMetrics { - public: - ScopedBandwidthMetrics(); - ~ScopedBandwidthMetrics(); - - void StartStream(); - void StopStream(); - void RecordBytes(int bytes); - - private: - bool started_; -}; - -} // namespace net - -#endif // NET_BASE_BANDWIDTH_METRICS_H_ diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc index e1a547efe9ecbc..b539ac23bc140c 100644 --- a/net/http/http_stream_factory_impl_job.cc +++ b/net/http/http_stream_factory_impl_job.cc @@ -10,6 +10,7 @@ #include "base/bind.h" #include "base/bind_helpers.h" #include "base/logging.h" +#include "base/metrics/histogram_macros.h" #include "base/profiler/scoped_tracker.h" #include "base/stl_util.h" #include "base/strings/string_util.h" diff --git a/net/net.gypi b/net/net.gypi index b06535ba66355b..f8e848c3eb49de 100644 --- a/net/net.gypi +++ b/net/net.gypi @@ -187,8 +187,6 @@ 'base/address_tracker_linux.h', 'base/backoff_entry.cc', 'base/backoff_entry.h', - 'base/bandwidth_metrics.cc', - 'base/bandwidth_metrics.h', 'base/cache_type.h', 'base/chunked_upload_data_stream.cc', 'base/chunked_upload_data_stream.h', diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc index 85b20165a2ddee..1fac26a7738944 100644 --- a/net/spdy/spdy_stream.cc +++ b/net/spdy/spdy_stream.cc @@ -8,6 +8,7 @@ #include "base/compiler_specific.h" #include "base/logging.h" #include "base/message_loop/message_loop.h" +#include "base/metrics/histogram_macros.h" #include "base/strings/string_number_conversions.h" #include "base/strings/stringprintf.h" #include "base/values.h" @@ -427,8 +428,6 @@ int SpdyStream::OnInitialResponseHeadersReceived( break; } - metrics_.StartStream(); - DCHECK_NE(io_state_, STATE_IDLE); response_time_ = response_time; @@ -477,7 +476,6 @@ void SpdyStream::OnDataReceived(scoped_ptr buffer) { pending_recv_data_.push_back(buffer.release()); } else { pending_recv_data_.push_back(NULL); - metrics_.StopStream(); // Note: we leave the stream open in the session until the stream // is claimed. } @@ -496,7 +494,6 @@ void SpdyStream::OnDataReceived(scoped_ptr buffer) { CHECK(!IsClosed()); if (!buffer) { - metrics_.StopStream(); if (io_state_ == STATE_OPEN) { io_state_ = STATE_HALF_CLOSED_REMOTE; } else if (io_state_ == STATE_HALF_CLOSED_LOCAL) { @@ -518,7 +515,6 @@ void SpdyStream::OnDataReceived(scoped_ptr buffer) { } // Track our bandwidth. - metrics_.RecordBytes(length); recv_bytes_ += length; recv_last_byte_time_ = base::TimeTicks::Now(); diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h index 26da667470f07a..e0b3a2d08a442a 100644 --- a/net/spdy/spdy_stream.h +++ b/net/spdy/spdy_stream.h @@ -14,7 +14,6 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/scoped_vector.h" #include "base/memory/weak_ptr.h" -#include "net/base/bandwidth_metrics.h" #include "net/base/io_buffer.h" #include "net/base/net_export.h" #include "net/base/net_log.h" @@ -508,8 +507,6 @@ class NET_EXPORT_PRIVATE SpdyStream { int32 recv_window_size_; int32 unacked_recv_window_bytes_; - ScopedBandwidthMetrics metrics_; - const base::WeakPtr session_; // The transaction should own the delegate. diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index fafdab84303c31..87270d9871c8e1 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -17786,6 +17786,9 @@ Therefore, the affected-histogram name has to have at least one dot in it. + + Deprecated as of 03/2015. No longer generated. + Please list the metric's owners. Add more owner tags as needed. Kbps on download streams exceeding 25KB. Measures from the beginning of the