Amortize data usage using TrafficStats on Android.

Amortize data usage using TrafficStats on Android.

This CL adds a DataUseAmortizer interface to the data_usage component,
and makes the DataUseAggregator use it to amortize data usage byte
counts on supported platforms.

This CL also adds an implementation of the DataUseAmortizer based on
TrafficStats for Android.

TBR=brettw@chromium.org,mmenke@chromium.org
BUG=518051
Committed: https://crrev.com/c441f78fcb48ef54a880e32fc2b43996958d3eb9
Cr-Commit-Position: refs/heads/master@{#359198}

[sclittle, 2015-10-10 08:35:01]

sclittle@chromium.org changed reviewers:
+ bengr@chromium.org, tbansal@chromium.org

[sclittle, 2015-10-10 08:35:02]

PTAL for sanity check.

Thanks in advance!

[tbansal1, 2015-10-12 21:23:20]

https://codereview.chromium.org/1390993005/diff/1/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/1390993005/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate_unittest.cc:104: .Pass()),
nit: don't think Pass() is needed

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.cc:16: int64_t
AmortizeByRatio(int64_t bytes, double ratio, double* remainder) {
please add comments.

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.cc:19: double intpart;
may be initialize to -1, than DCHECK_GE() below would ensure that it is >=0

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.cc:69:
static_cast<double>(total_unamortized_tx_delta_bytes);
can this give divide by 0 error? It probably can depending on how fast traffic
stats updates.

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.cc:80: }
DCHECK that tx_remainder and rx_remainder < 1 and >=0?

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.cc:87: return
net::android::traffic_stats::GetCurrentUidTxBytes(tx_bytes);
DCHECK(thread_checker_...)

[bengr, 2015-10-12 21:33:08]

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.cc:51:
RefreshTrafficStats();
Say more. How far behind is TrafficStats?

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.cc:69:
static_cast<double>(total_unamortized_tx_delta_bytes);
On 2015/10/12 21:23:20, tbansal1 wrote:
> can this give divide by 0 error? It probably can depending on how fast traffic
> stats updates.

Agreed.

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.cc:98:
GetTrafficStatsTxBytes(&traffic_stats_tx_bytes_) &&
Can tx_bytes_ or rx_bytes_ be 0?

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
File components/data_usage/android/traffic_stats_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/1/components/data_usage/andro...
components/data_usage/android/traffic_stats_amortizer.h:44: bool
are_traffic_stats_available_;
were_... ?

[sclittle, 2015-11-10 10:25:41]

Patchset #3 (id:40001) has been deleted

[sclittle, 2015-11-10 10:26:28]

PTAL, I'm still ironing out some tests but the production code's all there.

[bengr, 2015-11-10 18:12:51]

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:613: data_use_amortizer.reset(new
data_usage::android::TrafficStatsAmortizer());
It would be better if you didn't specialize for Android here, and instead passed
this in via the IOThread constructor or Globals. Likewise for the
external_data_use_observer_.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:39: const size_t
kDefaultMaxDataUseBufferSize = 128;
So does this mean we will lose accuracy whenever there are more than 128 reads
in a half second?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:46:
std::modf(static_cast<double>(bytes) * ratio + (*remainder), &intpart);
Are there any assumptions about ratio? E.g., >= 1.0? What happens when bytes is
INT64_MAX? Or when bytes * ratio overflows double?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:100:
kDefaultMaxDataUseBufferSize) {}
Since there should only be one instance, would it make sense to set a static
bool?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:163: if
(!is_amortization_run_in_progress_) {
I'd just call this is_amortization_in_progress_.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:176:
traffic_stats_query_timer_->Stop();
Add UMA (or a TODO for UMA).

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:190: // timer was
already set, then this overrides the previous delay.
So the point of this is that we will hopefully amortize when the network is
idle?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:199: int64_t
current_traffic_stats_tx_bytes = -1;
What's the significance of -1?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:242: // Pass
post-amortization DataUse to their respective callbacks.
DataUse -> DataUse objects

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:69: void
AddPreAmortizationBytes(int64_t tx_bytes, int64_t rx_bytes);
Please comment on what this and AmortizeNow() do.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:78: // One-shot timer
for polling TrafficStats byte counts until they stabilize.
Please document the algorithm for taking TrafficStats measurements.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:83: // TrafficStats byte
counts to stabilize.
What does "stabilize" mean?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:91: // elements start to
be passed through without performing any amortization.
Why is this needed?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer_unittest.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:159: //
Convenience function for creating an ExpectDataUse callback.
// Creates an ExpectDataUse callback, as a convenience.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:177: // Weak,
owned by |amortizer_|.
Add blank line above.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:179: // Weak,
owned by |amortizer_|.
Add blank line above.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:180:
MockTimerWithTickClock* mock_timer_;
Add blank line below.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:267:
amortizer()->AmortizeDataUse(CreateDataUse(50, 500),
Also add cases where sent or received bytes is at the int64_t max, and where
ratio is the int64_t max (e.g., with sent/received = 1).

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:284:
amortizer()->AmortizeDataUse(CreateDataUse(0, 0),
Add cases where sent or received bytes is 0.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:305: //
Simulate 9 cases of extra bytes being reported, each before TrafficStats
Why 9? Do you mean kMaxBufferSize + 1?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:307: const
base::TimeDelta kSmallDelay = kMaxAmortizationDelay / 10;
Why / 10?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
File components/data_usage/core/data_use.h (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
components/data_usage/core/data_use.h:52: // TODO in this CL: remove these debug
members.
Remove them.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:68: // same callback every
time.
This sounds like a good idea.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
File components/data_usage/core/data_use_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
components/data_usage/core/data_use_amortizer.h:32: // Notifies the
DataUseAmortizer that some extra bytes have happened that
Be clearer than "have happened."

[tbansal1, 2015-11-10 18:53:55]

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:613: data_use_amortizer.reset(new
data_usage::android::TrafficStatsAmortizer());
Why can this not be constructed in DataUseAggregator constructor (instead of
IOthread constructing it and passing it)?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:7: #include <algorithm>
 // For std::min.
Is algorithm not banned in Chromium?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:67: if
(pre_amortization_total_tx_bytes != 0) {
if |pre_amortization_total_tx_bytes| is 0 but
|desired_post_amortization_total_tx_bytes| != 0, then I would simply distribute
|desired_post_amortization_total_tx_bytes| equally among all data_use objects.
WDYT?

Same for rx case.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:141:
is_amortization_run_in_progress_(false),
Is it possible to get rid of is_amortization_run_in_progress_?
Seems like is_amortization_run_in_progress_ is false iff
current_amortization_run_start_time_ is null?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:191:
traffic_stats_query_timer_->Start(
does this post immediately if the delay is negative?

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:43: void
AmortizeDataUse(scoped_ptr<DataUse> data_use,
Method comments please.

[sclittle, 2015-11-11 02:10:08]

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:613: data_use_amortizer.reset(new
data_usage::android::TrafficStatsAmortizer());
On 2015/11/10 18:12:50, bengr wrote:
> It would be better if you didn't specialize for Android here, and instead
passed
> this in via the IOThread constructor or Globals. Likewise for the
> external_data_use_observer_.

I don't think I understand what you mean. It has to be specialized for Android
somewhere, and if it's specialized here, then no where else has to hold on to a
scoped_ptr<DataUseAmortizer>.

Plus, the aggregator takes ownership of the amortizer, so it would be an empty
scoped_ptr<DataUseAmortizer> in Globals/IOThread most of the time.

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:613: data_use_amortizer.reset(new
data_usage::android::TrafficStatsAmortizer());
On 2015/11/10 18:53:55, tbansal1 wrote:
> Why can this not be constructed in DataUseAggregator constructor (instead of
> IOthread constructing it and passing it)?

Because the DataUseAggregator doesn't have visibility into the
components/data_usage/android directory, since that would be a cyclical
dependency.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:7: #include <algorithm>
 // For std::min.
On 2015/11/10 18:53:55, tbansal1 wrote:
> Is algorithm not banned in Chromium?

I don't think it's banned, lots of stuff uses it, e.g.
https://code.google.com/p/chromium/codesearch#search/&q=%22include%20%3Calgor...

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:39: const size_t
kDefaultMaxDataUseBufferSize = 128;
On 2015/11/10 18:12:50, bengr wrote:
> So does this mean we will lose accuracy whenever there are more than 128 reads
> in a half second?

Yes, this is true. I doubt it will be a problem in practice though, especially
if we combine reads for the same request with the same callback.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:46:
std::modf(static_cast<double>(bytes) * ratio + (*remainder), &intpart);
On 2015/11/10 18:12:50, bengr wrote:
> Are there any assumptions about ratio? E.g., >= 1.0? What happens when bytes
is
> INT64_MAX? Or when bytes * ratio overflows double?

Added DCHECKs and comments. |ratio| can be any double, so long as the final
scaled and truncated result is between [0, INT64_MAX].

It's impossible for those overflows to happen, since |ratio| is the quotient of
two other non-negative int64_t numbers, and even if |remainder| is non-zero in
this case, then |remainder| would still be truncated anyways.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:67: if
(pre_amortization_total_tx_bytes != 0) {
On 2015/11/10 18:53:55, tbansal1 wrote:
> if |pre_amortization_total_tx_bytes| is 0 but
> |desired_post_amortization_total_tx_bytes| != 0, then I would simply
distribute
> |desired_post_amortization_total_tx_bytes| equally among all data_use objects.
> WDYT?
> 
> Same for rx case.

Added a TODO to handle this case gracefully.

I see what you mean, but I don't think it really makes sense to do that. Not all
DataUse objects are equal (there could be multiple tiny DataUse objects for the
same request, and one big DataUse for a different request), and how would we
distribute them among reported extra bytes (e.g. off-the-record)?

It seems easier in these cases to just give up on the TrafficStats values and
fall back to just using the byte counts from the net stack, since those are at
least a decent guess of the data usage.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:100:
kDefaultMaxDataUseBufferSize) {}
On 2015/11/10 18:12:50, bengr wrote:
> Since there should only be one instance, would it make sense to set a static
> bool?

On second thought, that's not exactly true, I've reworded that comment on the
TrafficStatsAmortizer class. It's OK for multiple TrafficStatsAmortizers to
exist, so long as every amortizer is notified of every byte, e.g. the same
DataUse object or a corresponding number of extra bytes must be reported to all
the amortizers.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:141:
is_amortization_run_in_progress_(false),
On 2015/11/10 18:53:55, tbansal1 wrote:
> Is it possible to get rid of is_amortization_run_in_progress_?
> Seems like is_amortization_run_in_progress_ is false iff
> current_amortization_run_start_time_ is null?

The null TimeTicks (i.e. base::TimeTicks()) is actually a valid time that could
come up for real, so I don't want to have special casing for that.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:163: if
(!is_amortization_run_in_progress_) {
On 2015/11/10 18:12:50, bengr wrote:
> I'd just call this is_amortization_in_progress_.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:176:
traffic_stats_query_timer_->Stop();
On 2015/11/10 18:12:50, bengr wrote:
> Add UMA (or a TODO for UMA).

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:190: // timer was
already set, then this overrides the previous delay.
On 2015/11/10 18:12:50, bengr wrote:
> So the point of this is that we will hopefully amortize when the network is
> idle?

Yes, added comment.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:191:
traffic_stats_query_timer_->Start(
On 2015/11/10 18:53:55, tbansal1 wrote:
> does this post immediately if the delay is negative?

Yes, see
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.c...

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:199: int64_t
current_traffic_stats_tx_bytes = -1;
On 2015/11/10 18:12:50, bengr wrote:
> What's the significance of -1?

It's just for debug purposes. If something goes wrong somehow, then it's easier
to recognize a -1 here then some arbitrary uninitialized memory value.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:242: // Pass
post-amortization DataUse to their respective callbacks.
On 2015/11/10 18:12:50, bengr wrote:
> DataUse -> DataUse objects

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:43: void
AmortizeDataUse(scoped_ptr<DataUse> data_use,
On 2015/11/10 18:53:55, tbansal1 wrote:
> Method comments please.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:69: void
AddPreAmortizationBytes(int64_t tx_bytes, int64_t rx_bytes);
On 2015/11/10 18:12:50, bengr wrote:
> Please comment on what this and AmortizeNow() do.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:78: // One-shot timer
for polling TrafficStats byte counts until they stabilize.
On 2015/11/10 18:12:50, bengr wrote:
> Please document the algorithm for taking TrafficStats measurements.

Done. This comment was wrong.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:83: // TrafficStats byte
counts to stabilize.
On 2015/11/10 18:12:50, bengr wrote:
> What does "stabilize" mean?

Oops, I forgot to update this and a few other comments here. Fixed.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.h:91: // elements start to
be passed through without performing any amortization.
On 2015/11/10 18:12:50, bengr wrote:
> Why is this needed?

Added comment. It's here so that we don't hog memory with the buffer - DataUse
objects could easily take around 1 KB in memory each, so that could get pretty
big if we're not careful.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer_unittest.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:159: //
Convenience function for creating an ExpectDataUse callback.
On 2015/11/10 18:12:50, bengr wrote:
> // Creates an ExpectDataUse callback, as a convenience.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:177: // Weak,
owned by |amortizer_|.
On 2015/11/10 18:12:50, bengr wrote:
> Add blank line above.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:179: // Weak,
owned by |amortizer_|.
On 2015/11/10 18:12:50, bengr wrote:
> Add blank line above.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:180:
MockTimerWithTickClock* mock_timer_;
On 2015/11/10 18:12:50, bengr wrote:
> Add blank line below.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:267:
amortizer()->AmortizeDataUse(CreateDataUse(50, 500),
On 2015/11/10 18:12:50, bengr wrote:
> Also add cases where sent or received bytes is at the int64_t max, and where
> ratio is the int64_t max (e.g., with sent/received = 1).

Done. Note that these are weird since floating point error figures in at these
magnitudes.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:284:
amortizer()->AmortizeDataUse(CreateDataUse(0, 0),
On 2015/11/10 18:12:50, bengr wrote:
> Add cases where sent or received bytes is 0.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:305: //
Simulate 9 cases of extra bytes being reported, each before TrafficStats
On 2015/11/10 18:12:50, bengr wrote:
> Why 9? Do you mean kMaxBufferSize + 1?

Reworded comment. I meant that we'll keep reporting extra bytes until the
kMaxAmortizationDelay is hit.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer_unittest.cc:307: const
base::TimeDelta kSmallDelay = kMaxAmortizationDelay / 10;
On 2015/11/10 18:12:50, bengr wrote:
> Why / 10?

Added comments.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
File components/data_usage/core/data_use.h (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
components/data_usage/core/data_use.h:52: // TODO in this CL: remove these debug
members.
On 2015/11/10 18:12:50, bengr wrote:
> Remove them.

Done.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
File components/data_usage/core/data_use_aggregator.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
components/data_usage/core/data_use_aggregator.cc:68: // same callback every
time.
On 2015/11/10 18:12:51, bengr wrote:
> This sounds like a good idea.

Ack. Yeah, it'll save on a bunch of memory and allocations since callbacks are
all ref-counted.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
File components/data_usage/core/data_use_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/c...
components/data_usage/core/data_use_amortizer.h:32: // Notifies the
DataUseAmortizer that some extra bytes have happened that
On 2015/11/10 18:12:51, bengr wrote:
> Be clearer than "have happened."

Done.

[sclittle, 2015-11-11 02:15:48]

sclittle@chromium.org changed reviewers:
+ mmenke@chromium.org

[sclittle, 2015-11-11 02:15:49]

mmenke: io_thread.cc, chrome_network_delegate_unittest.cc

Thanks in advance!

[bengr, 2015-11-11 15:41:38]

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:613: data_use_amortizer.reset(new
data_usage::android::TrafficStatsAmortizer());
On 2015/11/11 02:10:07, sclittle wrote:
> On 2015/11/10 18:53:55, tbansal1 wrote:
> > Why can this not be constructed in DataUseAggregator constructor (instead of
> > IOthread constructing it and passing it)?
> 
> Because the DataUseAggregator doesn't have visibility into the
> components/data_usage/android directory, since that would be a cyclical
> dependency.

I was hoping that you could set up the platform specialization in
ChromeBrowserMainParts::PreCreateThreadsImpl() or somewhere nearby, but it
doesn't look like there's an established pattern.

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/60001/components/data_usage/a...
components/data_usage/android/traffic_stats_amortizer.cc:199: int64_t
current_traffic_stats_tx_bytes = -1;
On 2015/11/11 02:10:07, sclittle wrote:
> On 2015/11/10 18:12:50, bengr wrote:
> > What's the significance of -1?
> 
> It's just for debug purposes. If something goes wrong somehow, then it's
easier
> to recognize a -1 here then some arbitrary uninitialized memory value.

Please add a comment.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.cc:107: // Amortize the
difference between |desired_post_amortization_total_tx_bytes|
Amortize -> Amortizes

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
File components/data_usage/android/traffic_stats_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:32: // Class that uses
Android TrafficStats to amortize any unincluded overhead into
It's worth mentioning that TrafficStats has been available in Android since 2.2.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:37: // TrafficStats
bytes for those as overhead.
Please list the sources of this overhead: network layer, TLS, DNS. Anything
else?

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:49: // Notifies the
DataUseAmortizer that some extra bytes have happened that
have happened -> have been transferred

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:51: void
OnExtraBytes(int64_t extra_tx_bytes, int64_t extra_rx_bytes) override;
This reports bytes that are measured, but that are not classified as overhead.
"ExtraBytes" doesn't capture that, but I don't have a better suggestion.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:66: // Query the total
transmitted and received bytes for the application from
Query -> Queries

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:69: // TrafficStats,
otherwise returns false.
Say that tx_bytes and rx_bytes must not be null.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:74: // Adds |tx_bytes|
and |rx_bytes| as pre-amortization bytes, and schedules
What are "pre-amortization bytes?" Please spell out the answer in the comment.
This file should spell out that there are three inputs: the pre-amortization
byte counts in DataUse objects, pre-amortization byte counts for transfers that
aren't captured by DataUse objects, and overhead as measured by TrafficStats
(less all measurements of data use inside Chrome).

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:119: // object. Nothing
but |buffered_data_use_| is allowed to hold linked_ptrs to
Nothing but ... is allowed to -> Only ... may

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:126: bool
are_last_amortization_traffic_stats_available_;
Add blank line below.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:129: int64_t
last_amortization_traffic_stats_tx_bytes_;
Add blank line below.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:149: }  // namespace
android
Add blank line below.

[sclittle, 2015-11-11 20:02:21]

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.cc:107: // Amortize the
difference between |desired_post_amortization_total_tx_bytes|
On 2015/11/11 15:41:38, bengr wrote:
> Amortize -> Amortizes

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
File components/data_usage/android/traffic_stats_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:32: // Class that uses
Android TrafficStats to amortize any unincluded overhead into
On 2015/11/11 15:41:38, bengr wrote:
> It's worth mentioning that TrafficStats has been available in Android since
2.2.

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:37: // TrafficStats
bytes for those as overhead.
On 2015/11/11 15:41:38, bengr wrote:
> Please list the sources of this overhead: network layer, TLS, DNS. Anything
> else?

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:49: // Notifies the
DataUseAmortizer that some extra bytes have happened that
On 2015/11/11 15:41:38, bengr wrote:
> have happened -> have been transferred

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:51: void
OnExtraBytes(int64_t extra_tx_bytes, int64_t extra_rx_bytes) override;
On 2015/11/11 15:41:38, bengr wrote:
> This reports bytes that are measured, but that are not classified as overhead.
> "ExtraBytes" doesn't capture that, but I don't have a better suggestion.

Ack, I'll keep it as it is then.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:66: // Query the total
transmitted and received bytes for the application from
On 2015/11/11 15:41:38, bengr wrote:
> Query -> Queries

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:69: // TrafficStats,
otherwise returns false.
On 2015/11/11 15:41:38, bengr wrote:
> Say that tx_bytes and rx_bytes must not be null.

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:74: // Adds |tx_bytes|
and |rx_bytes| as pre-amortization bytes, and schedules
On 2015/11/11 15:41:38, bengr wrote:
> What are "pre-amortization bytes?" Please spell out the answer in the comment.
> This file should spell out that there are three inputs: the pre-amortization
> byte counts in DataUse objects, pre-amortization byte counts for transfers
that
> aren't captured by DataUse objects, and overhead as measured by TrafficStats
> (less all measurements of data use inside Chrome).

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:119: // object. Nothing
but |buffered_data_use_| is allowed to hold linked_ptrs to
On 2015/11/11 15:41:38, bengr wrote:
> Nothing but ... is allowed to -> Only ... may

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:126: bool
are_last_amortization_traffic_stats_available_;
On 2015/11/11 15:41:38, bengr wrote:
> Add blank line below.

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:129: int64_t
last_amortization_traffic_stats_tx_bytes_;
On 2015/11/11 15:41:38, bengr wrote:
> Add blank line below.

Done.

https://codereview.chromium.org/1390993005/diff/100001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:149: }  // namespace
android
On 2015/11/11 15:41:38, bengr wrote:
> Add blank line below.

Done.

[sclittle, 2015-11-11 20:04:55]

sclittle@chromium.org changed reviewers:
+ thestig@chromium.org
- mmenke@chromium.org

[sclittle, 2015-11-11 20:04:55]

thestig: chrome/browser/*

Switching out mmenke@ with thestig@ since mmenke@ is travelling.

[Lei Zhang, 2015-11-11 20:08:19]

On 2015/11/11 20:04:55, sclittle wrote:
> thestig: chrome/browser/*
> 
> Switching out mmenke@ with thestig@ since mmenke@ is travelling.

lgtm

[bengr, 2015-11-11 22:52:53]

lgtm with a nit.

https://codereview.chromium.org/1390993005/diff/120001/components/data_usage/...
File components/data_usage/android/traffic_stats_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/120001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:38: // TrafficStats have
been available in Android since API level 8 (Android 2.2).
nit: "The TrafficStats API has..."

[sclittle, 2015-11-11 22:56:25]

https://codereview.chromium.org/1390993005/diff/120001/components/data_usage/...
File components/data_usage/android/traffic_stats_amortizer.h (right):

https://codereview.chromium.org/1390993005/diff/120001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.h:38: // TrafficStats have
been available in Android since API level 8 (Android 2.2).
On 2015/11/11 22:52:53, bengr wrote:
> nit: "The TrafficStats API has..."

Done.

[sclittle, 2015-11-11 22:58:18]

Description was changed from

==========
Amortize data usage using TrafficStats on Android.

This CL adds a DataUseAmortizer interface to the data_usage component,
and makes the DataUseAggregator use it to amortize data usage byte
counts on supported platforms.

This CL also adds an implementation of the DataUseAmortizer based on
TrafficStats for Android.

BUG=518051
==========

to

==========
Amortize data usage using TrafficStats on Android.

This CL adds a DataUseAmortizer interface to the data_usage component,
and makes the DataUseAggregator use it to amortize data usage byte
counts on supported platforms.

This CL also adds an implementation of the DataUseAmortizer based on
TrafficStats for Android.

TBR=brettw@chromium.org,mmenke@chromium.org
BUG=518051
==========

[sclittle, 2015-11-11 22:59:55]

TBR-ing to brettw@ and mmenke@ for "url/" and "net/" added to DEPS respectively.

[sclittle, 2015-11-11 23:00:08]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-11-11 23:00:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/1390993005/#ps140001
(title: "Fixed nit")

[commit-bot: I haz the power, 2015-11-11 23:01:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390993005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390993005/140001

[commit-bot: I haz the power, 2015-11-12 01:12:21]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-11-12 20:02:16]

Patchset 7 (id:??) landed as
https://crrev.com/c441f78fcb48ef54a880e32fc2b43996958d3eb9
Cr-Commit-Position: refs/heads/master@{#359198}

[mmenke, 2015-11-12 20:14:50]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2015-11-12 20:14:51]

https://codereview.chromium.org/1390993005/diff/140001/components/data_usage/...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/140001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.cc:78: int64_t
desired_post_amortization_total) {
So you're adding data use from all monitored URLRequests and then amortizing
based on aggregate data use on Android?  What about URLRequestContexts that
don't use a ChromeNetworkDelegate, or things that directly use a sockets?  (GCM
does that, wouldn't be surprised if other stuff does as well).

[bengr, 2015-11-12 20:42:03]

https://codereview.chromium.org/1390993005/diff/140001/components/data_usage/...
File components/data_usage/android/traffic_stats_amortizer.cc (right):

https://codereview.chromium.org/1390993005/diff/140001/components/data_usage/...
components/data_usage/android/traffic_stats_amortizer.cc:78: int64_t
desired_post_amortization_total) {
On 2015/11/12 20:14:50, mmenke (OOO Nov 11 - Nov 15) wrote:
> So you're adding data use from all monitored URLRequests and then amortizing
> based on aggregate data use on Android?  What about URLRequestContexts that
> don't use a ChromeNetworkDelegate, or things that directly use a sockets? 
(GCM
> does that, wouldn't be surprised if other stuff does as well).

This is a source of noise in the accounting. We hope to characterize this noise
and to improve on accuracy.

[mmenke, 2015-11-12 22:02:57]

On 2015/11/12 20:42:03, bengr wrote:
>
https://codereview.chromium.org/1390993005/diff/140001/components/data_usage/...
> File components/data_usage/android/traffic_stats_amortizer.cc (right):
> 
>
https://codereview.chromium.org/1390993005/diff/140001/components/data_usage/...
> components/data_usage/android/traffic_stats_amortizer.cc:78: int64_t
> desired_post_amortization_total) {
> On 2015/11/12 20:14:50, mmenke (OOO Nov 11 - Nov 15) wrote:
> > So you're adding data use from all monitored URLRequests and then amortizing
> > based on aggregate data use on Android?  What about URLRequestContexts that
> > don't use a ChromeNetworkDelegate, or things that directly use a sockets? 
> (GCM
> > does that, wouldn't be surprised if other stuff does as well).
> 
> This is a source of noise in the accounting. We hope to characterize this
noise
> and to improve on accuracy.

Great, just wanted to make sure it was on your radar.