Add histograms to NetworkChangeNotifier to identify ways to improve interface

Add histograms to NetworkChangeNotifier to identify ways to improve interface
and narrow down offline false-positives.

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

[pauljensen, 2012-08-06 15:17:56]

I broke off this CL from 10843072 and addressed syzm's comments.  WRT the
concern over race conditions with |last_connection_change_|, I think this should
be okay as there is only one function (OnConnectionTypeChange) that writes to
this variable which I assume is only done from one thread; since I'm guessing
its larger than sig_atomic_t we could have problems with partial reads/writes
but the likelihood of this is very small (once every 2^32 microseconds, or once
every hour, it wraps, and offline packets should be fairly infrequent).  I could
see race conditions with |offline_packets_received_| which is written in two
different functions potentially running on two different threads, but the
likelihood seems really low as offline packets should be rare and we clear
|offline_packets_received_| on the online and offline transitions.  One way to
avoid this would be to have OnConnectionTypeChange make a copy of
|offline_packets_received_| into a new variable called
|offline_packets_received_at_last_transition_|, then we'd only have to worry
about the extremely unlikely case of wrapping.  Do you think its worth taking
more steps to avoid these potential inter-thread problems?

[szym, 2012-08-06 16:17:42]

On 2012/08/06 15:17:56, pauljensen wrote:
> I could
> see race conditions with |offline_packets_received_| which is written in two
> different functions potentially running on two different threads, but the
> likelihood seems really low as offline packets should be rare and we clear
> |offline_packets_received_| on the online and offline transitions. 

I suggest to make sure that it's always written on one thread. To do that, all
you need to do is ensure that |HistogramWatcher| is created on the network
thread (since |NotifyDataReceived| will only be called from there). You could do
that by exposing an |InitHistogramWatcher| to be called in
"chrome/browser/io_thread.cc". In |NotifyDataReceived| you'd add a restriction
that this can only be called from the same thread that called
|InitHistogramWatcher|. This approach would address all other races as well.

[pauljensen, 2012-08-06 20:06:32]

With my latest patch set it only runs on the I/O thread.

[szym, 2012-08-08 20:41:17]

http://codereview.chromium.org/10837123/diff/5/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10837123/diff/5/chrome/browser/io_thread.cc#ne...
chrome/browser/io_thread.cc:398: // Setup the HistogramWatcher to run on the I/O
thread.
maybe "IO thread" for consistency

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:59: void InitHistogramWatcher();
You could use base::NonThreadSafe to enforce the thread requirements. Just call
DetachFromThread in ~HistogramWatcher to avoid its complaints about destruction
order.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:66: SinceLast(&last_ip_address_change_));
I'm a bit uneasy about using a function call with side effects as a macro
parameter.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:79: // we can compare this sum with the sum
of NCN.OfflineDataRecv
nit: Format like a sentence, but maybe just remove.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:111: !(source.SchemeIs("http") ||
source.SchemeIs("https")))
nit: This might need braces.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:128: static base::TimeDelta
SinceLast(base::TimeTicks *last_time);
Method declarations should precede field declarations, but 
this could also be put in the unnamed namespace above it all.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:133: base::TimeDelta
HistogramWatcher::SinceLast(base::TimeTicks *last_time) {
Why is this implemented here rather than inline as everything else?

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:140: void
HistogramWatcher::InitHistogramWatcher() {
ditto: why implemented out-of-line?

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:287: histogram_watcher_.reset(new
HistogramWatcher());
nit: Put this in the initializer list.

http://codereview.chromium.org/10837123/diff/5/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10837123/diff/5/net/url_request/url_request.cc...
net/url_request/url_request.cc:883: // Notify NetworkChangeNotifier that we just
received network data
nit: add '.' at end of sentence. Perhaps explain that this is to measure
performance and add reference to the relevant bug?

[szym, 2012-08-08 20:57:56]

LGTM, after our offline discussion about interpreting the prospective results.

[pauljensen, 2012-08-13 15:42:03]

I think I addressed prior comments and nits as we had discussed offline.  If you
disagree with any of my comments/recollections of our discussion let me know and
I'll promptly address.  Since you gave approval, I'll commit it now.

http://codereview.chromium.org/10837123/diff/5/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10837123/diff/5/chrome/browser/io_thread.cc#ne...
chrome/browser/io_thread.cc:398: // Setup the HistogramWatcher to run on the I/O
thread.
On 2012/08/08 20:41:17, szym wrote:
> maybe "IO thread" for consistency

Done.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:59: void InitHistogramWatcher();
On 2012/08/08 20:41:17, szym wrote:
> You could use base::NonThreadSafe to enforce the thread requirements. Just
call
> DetachFromThread in ~HistogramWatcher to avoid its complaints about
destruction
> order.

As we discussed this would add so much complexity as to outweigh the benefit of
the checks.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:66: SinceLast(&last_ip_address_change_));
On 2012/08/08 20:41:17, szym wrote:
> I'm a bit uneasy about using a function call with side effects as a macro
> parameter.

I don't like it either.  I wish the histograms were done without macros.  The
histogram macros seem to be done right (they're so widespread I'd hope so)
though and only evaluate the sample argument once.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:79: // we can compare this sum with the sum
of NCN.OfflineDataRecv
On 2012/08/08 20:41:17, szym wrote:
> nit: Format like a sentence, but maybe just remove.

Done.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:111: !(source.SchemeIs("http") ||
source.SchemeIs("https")))
On 2012/08/08 20:41:17, szym wrote:
> nit: This might need braces.

Done.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:128: static base::TimeDelta
SinceLast(base::TimeTicks *last_time);
On 2012/08/08 20:41:17, szym wrote:
> Method declarations should precede field declarations, but 
> this could also be put in the unnamed namespace above it all.

Done.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:133: base::TimeDelta
HistogramWatcher::SinceLast(base::TimeTicks *last_time) {
On 2012/08/08 20:41:17, szym wrote:
> Why is this implemented here rather than inline as everything else?

Done.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:140: void
HistogramWatcher::InitHistogramWatcher() {
On 2012/08/08 20:41:17, szym wrote:
> ditto: why implemented out-of-line?

Done.

http://codereview.chromium.org/10837123/diff/5/net/base/network_change_notifi...
net/base/network_change_notifier.cc:287: histogram_watcher_.reset(new
HistogramWatcher());
On 2012/08/08 20:41:17, szym wrote:
> nit: Put this in the initializer list.

It must appear after |g_network_change_notifier = this;| which unfortunately
cannot be moved to the initializer list without removing the
|DCHECK(!g_network_change_notifier)| line which does server a purpose.

http://codereview.chromium.org/10837123/diff/5/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10837123/diff/5/net/url_request/url_request.cc...
net/url_request/url_request.cc:883: // Notify NetworkChangeNotifier that we just
received network data
On 2012/08/08 20:41:17, szym wrote:
> nit: add '.' at end of sentence. Perhaps explain that this is to measure
> performance and add reference to the relevant bug?

Done.

[commit-bot: I haz the power, 2012-08-13 15:45:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/10837123/10001

[commit-bot: I haz the power, 2012-08-13 15:45:13]

Presubmit check for 10837123-10001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.5s to calculate.

[pauljensen, 2012-08-13 17:30:51]

Ben,

Could you please approve the one line of code in chrome/browser/io_thread.cc?

Thanks,
Paul

[pauljensen, 2012-08-13 17:38:17]

Oops, wrong Ben, my apologies.

On 2012/08/13 17:30:51, pauljensen wrote:
> Ben,
> 
> Could you please approve the one line of code in chrome/browser/io_thread.cc?
> 
> Thanks,
> Paul

[pauljensen, 2012-08-13 17:38:38]

Ben,

Could you please approve the one line of code in chrome/browser/io_thread.cc?

Thanks,
Paul

[Ben Goodger (Google), 2012-08-13 17:48:21]

lgtm

[commit-bot: I haz the power, 2012-08-13 18:00:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/10837123/10001

[commit-bot: I haz the power, 2012-08-13 19:20:53]

Change committed as 151325