[Cronet] Initialize StatisticsRecorder early

[Cronet] Initialize StatisticsRecorder early

StatisticsRecorder must be initialized before histograms are emitted. Otherwise,
StatisticsRecorder::FactoryGet() leaks one histogram per call for a given name.

BUG=707836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2790063004
Cr-Commit-Position: refs/heads/master@{#462185}
Committed: https://chromium.googlesource.com/chromium/src/+/7e64a797f7b6b4fb54536bbbc4c809c4fc6da3e1

[xunjieli, 2017-04-03 18:33:48]

Description was changed from

==========
[Cronet] Initialize StatisticsRecorder early

StatisticsRecorder must be initialized before histograms are emitted. Otherwise,
StatisticsRecorder::FactoryGet() leaks one histogram per call for a given name.

BUG=707836
==========

to

==========
[Cronet] Initialize StatisticsRecorder early

StatisticsRecorder must be initialized before histograms are emitted. Otherwise,
StatisticsRecorder::FactoryGet() leaks one histogram per call for a given name.

BUG=707836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-04-03 18:39:39]

xunjieli@chromium.org changed reviewers:
+ pauljensen@chromium.org

[xunjieli, 2017-04-03 18:39:39]

Paul, could you do a first pass? Thanks!

https://codereview.chromium.org/2790063004/diff/20001/components/cronet/andro...
File components/cronet/android/cronet_library_loader.cc (right):

https://codereview.chromium.org/2790063004/diff/20001/components/cronet/andro...
components/cronet/android/cronet_library_loader.cc:106:
base::StatisticsRecorder::Initialize();
Doing this on the main UI thread because NetworkChangeNotifier will emit
histograms. Not sure if this impacts startup performance.

[xunjieli, 2017-04-04 16:32:04]

Patchset #4 (id:60001) has been deleted

[xunjieli, 2017-04-04 17:38:48]

I reverted the iOS portion because one of the iOS test is currently failing due
to https://codereview.chromium.org/2760073002/.

[pauljensen, 2017-04-04 19:11:13]

Is this going to slow things down for Cronet embedders that never call
CronetEngine.getGlobalMetricsDeltas()?  We document that the first call to
getGlobalMetricsDeltas() begins collection of deltas, so I wonder if not
collecting deltas makes the histogram code run faster.

https://codereview.chromium.org/2790063004/diff/100001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2790063004/diff/100001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:1209:
std::vector<uint8_t> data;
Perhaps add DCHECK(base::StatisticsRecorder::IsActive());

[xunjieli, 2017-04-04 19:39:56]

xunjieli@chromium.org changed reviewers:
+ asvitkine@chromium.org

[xunjieli, 2017-04-04 19:39:57]

+asvitkine@ could you comment on the question below?

> Is this going to slow things down for Cronet embedders that never call
> CronetEngine.getGlobalMetricsDeltas()?  We document that the first call to 
> getGlobalMetricsDeltas() begins collection of deltas, so I 
wonder if not
> collecting deltas makes the histogram code run faster.

If I understand correctly from the chromium-dev thread
(https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/70121e5f-c464-4...),
StatisticsRecorder is needed because we have histograms logging in native code.
We need to do it regardless whether developers cares about
CronetEngine.getGlobalMetricsDeltas(). Otherwise, we will have memory leaks. I
am not sure if there's a switch to turn histograms logging off. +asvitkine to
comment.

https://codereview.chromium.org/2790063004/diff/100001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2790063004/diff/100001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:1209:
std::vector<uint8_t> data;
On 2017/04/04 19:11:12, pauljensen wrote:
> Perhaps add DCHECK(base::StatisticsRecorder::IsActive());

Done.

[Alexei Svitkine (slow), 2017-04-04 21:31:31]

lgtm

[pauljensen, 2017-04-05 02:48:28]

Alexei, could you answer the question posed in message #6?
https://codereview.chromium.org/2790063004/#msg6

[Alexei Svitkine (slow), 2017-04-05 14:33:05]

"I am not sure if there's a switch to turn histograms logging off. +asvitkine to
comment."

There is no such switch. In general, histogram recording has pretty minimal
overhead, and often it's useful to get data from a production device locally for
debugging (in Chrome, via chrome://histograms, but each app could expose this in
its own way).

Still, I'm sympathetic to the use case of not wanting any overhead from
histograms for users who are not using the data. We don't have support for this
currently - although it could be added.

If we decided to go that route, the best way to achieve that (i.e. that would
remove the most overhead) would be as a compile-time option that makes all the
histogram macros no-ops (to save from binary size overhead from macros) and
making all the Histogram Factory calls return a dummy object that is a no-op
when logged to.

[pauljensen, 2017-04-05 15:16:44]

I was specifically asking about message #6
https://codereview.chromium.org/2790063004/#msg6
"Is this going to slow things down for Cronet embedders that never call
CronetEngine.getGlobalMetricsDeltas()?"

[pauljensen, 2017-04-05 15:18:07]

On 2017/04/05 15:16:44, pauljensen wrote:
> I was specifically asking about message #6
> https://codereview.chromium.org/2790063004/#msg6
> "Is this going to slow things down for Cronet embedders that never call
> CronetEngine.getGlobalMetricsDeltas()?"

Another way of phrasing this question:
Is always calling base::StatisticsRecorder::Initialize() going to slow down
cases where a user would never call base::StatisticsRecorder::Initialize()?

[Alexei Svitkine (slow), 2017-04-05 17:59:12]

No, I don't think there will be extra slow down because of this. In fact, it
should be faster - since there will be less memory allocated (i.e. the leak
stoppage) for the histograms that previously leaked. While all the other work
done by histograms would be unchanged.

[pauljensen, 2017-04-05 18:37:20]

lgtm

[xunjieli, 2017-04-05 18:46:37]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-04-05 18:46:57]

Thank you both. I will upload a CL to clean up iOS side.

[commit-bot: I haz the power, 2017-04-05 18:47:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 19:43:30]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491417997402670,
"parent_rev": "212d8b430e57da3710457d442bda0da823cce778", "commit_rev":
"7e64a797f7b6b4fb54536bbbc4c809c4fc6da3e1"}

[commit-bot: I haz the power, 2017-04-05 19:44:22]

Description was changed from

==========
[Cronet] Initialize StatisticsRecorder early

StatisticsRecorder must be initialized before histograms are emitted. Otherwise,
StatisticsRecorder::FactoryGet() leaks one histogram per call for a given name.

BUG=707836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Initialize StatisticsRecorder early

StatisticsRecorder must be initialized before histograms are emitted. Otherwise,
StatisticsRecorder::FactoryGet() leaks one histogram per call for a given name.

BUG=707836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2790063004
Cr-Commit-Position: refs/heads/master@{#462185}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e64a797f7b6b4fb54536bbbc4c8...
==========

[commit-bot: I haz the power, 2017-04-05 19:44:23]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/7e64a797f7b6b4fb54536bbbc4c8...