Removing old histograms for measuring RTT vs Kilobytes of data

Previously i was recording number of RTT's vs Kilobytes of data downloaded per http transaction. This was generating lots of histograms.
Removing them for efficiency.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98562

[jar (doing other things), 2011-08-23 23:01:35]

http://codereview.chromium.org/7714020/diff/1/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/7714020/diff/1/net/http/http_basic_stream.cc#n...
net/http/http_basic_stream.cc:146: VLOG(2) << StringPrintf("%s\nrtt =
%f\tnum_rtt = %f\t"
What does VLOG do?

IMO, this should all be debug code, as it probably has no significant use.  The
call on line 142 to acquire a duration seems like a waste (central call to get a
locked resource, which is Now()).

Have you considered getting rid of this whole function?.... or worst case,
making it only produce output when some debug flag is set?

For instance, the entire body of this function could be conditioned away with
 if (!VLOG_IS_ON(2)) return;
on line 130.

[gagansingh, 2011-08-24 00:03:29]

http://codereview.chromium.org/7714020/diff/1/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/7714020/diff/1/net/http/http_basic_stream.cc#n...
net/http/http_basic_stream.cc:146: VLOG(2) << StringPrintf("%s\nrtt =
%f\tnum_rtt = %f\t"
On 2011/08/23 23:01:35, jar wrote:
> What does VLOG do?
> 
> IMO, this should all be debug code, as it probably has no significant use. 
The
> call on line 142 to acquire a duration seems like a waste (central call to get
a
> locked resource, which is Now()).
> 
> Have you considered getting rid of this whole function?.... or worst case,
> making it only produce output when some debug flag is set?
> 
> For instance, the entire body of this function could be conditioned away with
>  if (!VLOG_IS_ON(2)) return;
> on line 130.

This log was very helpful in the initial development phase.
i had missed a read in tcp socket and was getting lesser bytes read than the
resource size, and this was useful then. probably useless now.
plz let me know if youd like this removed, for now ive just enabled this with
VLOG_IS_ON(2)

[jar (doing other things), 2011-08-25 02:28:09]

IF it is really useless, we should yank it ('cause we'll never get back to yank
it later).  If you find it useful, we can keep it.  Note that we always have the
svn repository if we need to resurrect it ;-) .

My preference is to yank, but I'll give you an LGTM, and let you decide.

thanks,

Jim

[gagansingh, 2011-08-25 16:27:45]

Thanks Jim. Code removed.

Chris, can you please take a look. Owners approval needed.

Thanks
Gagan

[jar (doing other things), 2011-08-25 16:52:35]

LGTM

[gagansingh, 2011-08-26 19:03:21]

+ cbentzel, ahendrickson, wtc

Please take a look.

Thanks
Gagan




On Thu, Aug 25, 2011 at 10:22 PM, <jar@chromium.org> wrote:

> LGTM
>
>
>
http://codereview.chromium.**org/7714020/<http://codereview.chromium.org/7714...
>

[ahendrickson, 2011-08-26 20:51:56]

I'm not familiar with that portion of the code.

[jar (doing other things), 2011-08-26 21:21:51]

On 2011/08/26 20:51:56, ahendrickson wrote:
> I'm not familiar with that portion of the code.

This CL is removing histogramming code.  The code being removed should never
have had any side-effects, other than populating a histogram.  As a result, the
change should be localized.

New code (not shown here) records significant stats in a single histogram,
rather than the roughly 600 histograms this is creating.  The 600 histograms is
almost doubling the total number of histograms produced by the browser.  That in
turn is causing a bunch of problems with back-ends display code.

I think gagan was asking for your approval because you are an OWNER in net.

Thanks in advance.

[wtc, 2011-08-26 21:26:19]

LGTM.

[gagansingh, 2011-08-27 07:02:17]

Thanks guys.

Gagan




On Sat, Aug 27, 2011 at 2:56 AM, <wtc@chromium.org> wrote:

> LGTM.
>
>
>
http://codereview.chromium.**org/7714020/<http://codereview.chromium.org/7714...
>

[commit-bot: I haz the power, 2011-08-27 08:18:55]

Change committed as 98562