Randomly enable leak detector based on probability parameter

Randomly enable leak detector based on probability parameter

Read in probability parameters from the variations system. Get a random
double within the range [0, 1) and check it against the probability to
decide whether to enable the leak detector for each browser and renderer
process. By default, the probability is 0 (turned off).

Also enables the sending of params to renderer process and receiving of
leak reports from renderer process.

BUG=chromium:615223
TEST=Run Chrome with variations seed, check that the proper params are
read from variations system. Set probabilities to 50%. Add logging to
browser and renderer process' LeakDetector modules and make sure that
they are only enabled about half the time.

Committed: https://crrev.com/ebddf6df6607c9574adc4e78600bfd98ca5265c9
Cr-Commit-Position: refs/heads/master@{#410462}

[Simon Que, 2016-07-28 02:01:37]

Description was changed from

==========
Randomly enable leak detector based on probability parameter

Read in a probability parameter from the variations system. Get a random
double within the range [0, 1) and check it against the probability to
decide whether to enable the leak detector.

By default, the probability is 100% (1.0).

The randomization will eventually be extended to each renderer process
as well.

BUG=chromium:615223
==========

to

==========
Randomly enable leak detector based on probability parameter

Read in probability parameters from the variations system. Get a random
double within the range [0, 1) and check it against the probability to
decide whether to enable the leak detector for each browser and renderer
process. By default, the probability is 0 (turned off).

Also enables the sending of params to renderer process and receiving of
leak reports from renderer process.

BUG=chromium:615223
TEST=Run Chrome with variations seed, check that the proper params are
read from variations system. Set probabilities to 50%. Add logging to
browser and renderer process' LeakDetector modules and make sure that
they are only enabled about half the time.
==========

[Simon Que, 2016-07-28 02:04:05]

sque@chromium.org changed reviewers:
+ wfh@chromium.org

[Simon Que, 2016-07-28 02:05:18]

On 2016/07/28 02:04:05, Simon Que wrote:

I wonder if the changes in chrome/renderer should be in a separate CL. Those
changes do the actual instantiation of the renderer-side client, which is
independent of the randomization code.

[Simon Que, 2016-07-30 00:14:55]

On 2016/07/28 02:05:18, Simon Que wrote:
> On 2016/07/28 02:04:05, Simon Que wrote:
> 
> I wonder if the changes in chrome/renderer should be in a separate CL. Those
> changes do the actual instantiation of the renderer-side client, which is
> independent of the randomization code.

Ping.

[Simon Que, 2016-08-01 23:46:16]

sque@chromium.org changed reviewers:
+ isherman@chromium.org

[Simon Que, 2016-08-01 23:46:16]

+isherman since wfh is out.

[Simon Que, 2016-08-02 21:50:06]

On 2016/08/01 23:46:16, Simon Que wrote:
> +isherman since wfh is out.

Here's an updated version of an older presentation that shows the order of
operations with this change:
https://docs.google.com/presentation/d/1G2LAXWt2hlLUmqG2pHNOeJXaAUqHjASJvjo-G...

[Will Harris, 2016-08-03 00:14:51]

sorry for delay. lgtm

[Simon Que, 2016-08-03 00:27:53]

sque@chromium.org changed reviewers:
+ jochen@chromium.org

[Simon Que, 2016-08-03 00:27:54]

+jochen to review chrome/renderer

[jochen (gone - plz use gerrit), 2016-08-03 15:26:17]

do you have test coverage for how this impacts system health metrics when
enabled?

[Simon Que, 2016-08-03 17:04:24]

On 2016/08/03 15:26:17, jochen wrote:
> do you have test coverage for how this impacts system health metrics when
> enabled?

Not yet. We will enable it using the variations system for CrOS dev and canary
channels only, at first. That way we can get system performance data back from
the field before enabling it on the other channels.

[Simon Que, 2016-08-05 01:10:14]

On 2016/08/03 17:04:24, Simon Que wrote:
> On 2016/08/03 15:26:17, jochen wrote:
> > do you have test coverage for how this impacts system health metrics when
> > enabled?
> 
> Not yet. We will enable it using the variations system for CrOS dev and canary
> channels only, at first. That way we can get system performance data back from
> the field before enabling it on the other channels.

So far, we have used the variations system to enable it on the browser process
only. We have not seen a statistically significant drop in UMA performance
metrics. In some cases, the metrics even improved, which indicates that the
actual performance impact is lower than the noise. I will use similarly low
numbers for leak detection on the renderer process, and continue to monitor the
performance numbers.

[jochen (gone - plz use gerrit), 2016-08-05 11:52:54]

well well, chrome/renderer lgtm, but please wait for isherman to look at the
metrics stuff

[Simon Que, 2016-08-05 21:24:46]

The CQ bit was checked by sque@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 21:25:08]

Dry run: 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, 2016-08-05 22:38:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 22:38:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Simon Que, 2016-08-06 08:14:21]

On 2016/08/05 11:52:54, jochen (OOO until Aug 22) wrote:
> well well, chrome/renderer lgtm, but please wait for isherman to look at the
> metrics stuff

wfh is an owner of chrome/browser/metrics/leak_detector so his LGTM will
suffice.

[Simon Que, 2016-08-06 08:14:34]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2016-08-06 08:14:48]

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, 2016-08-06 09:23:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 09:23:30]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Simon Que, 2016-08-06 18:35:06]

The CQ bit was checked by sque@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 18:35:20]

Dry run: 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, 2016-08-06 19:36:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 19:36:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Simon Que, 2016-08-07 05:25:00]

sque@chromium.org changed reviewers:
+ sadrul@chromium.org

[Simon Que, 2016-08-07 05:25:01]

+sadrul to review RenderThreadObserver changes.

[Simon Que, 2016-08-07 05:27:24]

The CQ bit was checked by sque@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-07 05:27:32]

Dry run: 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, 2016-08-07 06:10:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-07 06:10:02]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-08-08 20:20:06]

lgtm

https://codereview.chromium.org/2062743002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2062743002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:447:
thread->AddObserver(leak_detector_remote_client_.get());
I was going to comment that there isn't a corresponding RemoveObserver, but
looks like that's the pattern for the rest of the observers added here (and
Remove isn't necessary right now either since the observer is destroyed only
when the thread itself is also destroyed).

[Simon Que, 2016-08-08 20:21:25]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-08-08 20:21:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2062743002/#ps160001
(title: "Use RenderThreadObserver interface to shutdown remote client on same
thread")

[commit-bot: I haz the power, 2016-08-08 20:21:46]

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, 2016-08-08 21:18:37]

Description was changed from

==========
Randomly enable leak detector based on probability parameter

Read in probability parameters from the variations system. Get a random
double within the range [0, 1) and check it against the probability to
decide whether to enable the leak detector for each browser and renderer
process. By default, the probability is 0 (turned off).

Also enables the sending of params to renderer process and receiving of
leak reports from renderer process.

BUG=chromium:615223
TEST=Run Chrome with variations seed, check that the proper params are
read from variations system. Set probabilities to 50%. Add logging to
browser and renderer process' LeakDetector modules and make sure that
they are only enabled about half the time.
==========

to

==========
Randomly enable leak detector based on probability parameter

Read in probability parameters from the variations system. Get a random
double within the range [0, 1) and check it against the probability to
decide whether to enable the leak detector for each browser and renderer
process. By default, the probability is 0 (turned off).

Also enables the sending of params to renderer process and receiving of
leak reports from renderer process.

BUG=chromium:615223
TEST=Run Chrome with variations seed, check that the proper params are
read from variations system. Set probabilities to 50%. Add logging to
browser and renderer process' LeakDetector modules and make sure that
they are only enabled about half the time.
==========

[commit-bot: I haz the power, 2016-08-08 21:18:38]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-08 21:20:53]

Description was changed from

==========
Randomly enable leak detector based on probability parameter

Read in probability parameters from the variations system. Get a random
double within the range [0, 1) and check it against the probability to
decide whether to enable the leak detector for each browser and renderer
process. By default, the probability is 0 (turned off).

Also enables the sending of params to renderer process and receiving of
leak reports from renderer process.

BUG=chromium:615223
TEST=Run Chrome with variations seed, check that the proper params are
read from variations system. Set probabilities to 50%. Add logging to
browser and renderer process' LeakDetector modules and make sure that
they are only enabled about half the time.
==========

to

==========
Randomly enable leak detector based on probability parameter

Read in probability parameters from the variations system. Get a random
double within the range [0, 1) and check it against the probability to
decide whether to enable the leak detector for each browser and renderer
process. By default, the probability is 0 (turned off).

Also enables the sending of params to renderer process and receiving of
leak reports from renderer process.

BUG=chromium:615223
TEST=Run Chrome with variations seed, check that the proper params are
read from variations system. Set probabilities to 50%. Add logging to
browser and renderer process' LeakDetector modules and make sure that
they are only enabled about half the time.

Committed: https://crrev.com/ebddf6df6607c9574adc4e78600bfd98ca5265c9
Cr-Commit-Position: refs/heads/master@{#410462}
==========

[commit-bot: I haz the power, 2016-08-08 21:20:54]

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