Add memory ablation experiment.

Add memory ablation experiment.

BUG=710048
Review-Url: https://codereview.chromium.org/2810833002
Cr-Commit-Position: refs/heads/master@{#464136}
Committed: https://chromium.googlesource.com/chromium/src/+/950fbc86a6623aebe69b471dfa156504b8f4467b

[DmitrySkiba, 2017-04-10 17:15:42]

dskiba@chromium.org changed reviewers:
+ mariakhomenko@chromium.org, primiano@chromium.org

[DmitrySkiba, 2017-04-10 17:15:42]

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:2611:
flag_descriptions::kEnableZipArchiverOnFileManagerDescription,
This was formatted by "git cl format" along with my changes.

[DmitrySkiba, 2017-04-10 17:52:13]

dskiba@chromium.org changed reviewers:
+ brettw@chromium.org, isherman@chromium.org

[DmitrySkiba, 2017-04-10 17:52:14]

Ilya Sherman: please take a look at histograms.xml

[Ilya Sherman, 2017-04-10 17:55:23]

histograms.xml lgtm

[DmitrySkiba, 2017-04-10 18:02:54]

Description was changed from

==========
Add memory ablation experiment.

BUG=
==========

to

==========
Add memory ablation experiment.

BUG=710048
==========

[Primiano Tucci (use gerrit), 2017-04-10 18:05:04]

Maybe link an actual BUG=?

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:844: {"5 MiB", kMemoryAblation5MiB,
arraysize(kMemoryAblation5MiB), nullptr},
I think 2 vs 5 isn't going to be a huge difference, maybe these should be a bit
more esponential, like, 1, 5, 10, 100 ?

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
File chrome/browser/experiments/memory_ablation_experiment.cc (right):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.cc:6: 
do you need a class at all here?
It seems all this code could be just a function, something like

MaybeStartMemoryAblationExperiment() {
  int size = GetFieldTrialParamByFeatureAsInt(....);
  if (!size <= 0)
   return;
 std::unique_ptr mem(new uint8_t[size]);
 for(i = 0 ....)  
   touch memory
  PostDelayedTask(FROM_HERE, Bind([] (std::unique_ptr<uint8_t>) {}, delay )
}

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.cc:59: memory_[i]++;
two things here:
1. touching uninitialized memory is undefined behavior. clang is very aggressive
on undefined behavior and I've seen it allowing to just insert deliberate
crashes even on things that shouln't crash in any case.
See crbug.com/681218 where wfh was doing something very similar to this and
ended up with clang generating a crash instead.

2. Nothing is doing anything with memory_, which allows the compiler to just
omit this loop completely (if didn't emit a trap as above).

What I would do here instead is:
1. Just write without reading, i.e. memory[i] = static_cast<uint8_t>(i).
if you want to be really paranoid, cast it to volatile uint8_t*, that should
make the compiler even less conservative.
2. Add a call to base::debug::Alias(memory_), which will stop the compiler form
making escape analysis on this pointer

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
File chrome/browser/experiments/memory_ablation_experiment.h (right):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.h:14: #include
"base/sequenced_task_runner.h"
can just forward declare this

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.h:40:
std::unique_ptr<uint8_t[], decltype(free)*> memory_;
this one seems over-complicated for the need.
Can you just have a std::unique_ptr<uint8_t[]> and just initialize that as

memory_(new uint8_t[size]) ?

using new vs delete shouldn't make any difference vs malloc. If you do to avoid
large zero-initializaiton, memory shouldn'be initialize unless you do new
(uint8_t[size]().

[DmitrySkiba, 2017-04-10 19:31:00]

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:844: {"5 MiB", kMemoryAblation5MiB,
arraysize(kMemoryAblation5MiB), nullptr},
On 2017/04/10 18:05:04, Primiano Tucci wrote:
> I think 2 vs 5 isn't going to be a huge difference, maybe these should be a
bit
> more esponential, like, 1, 5, 10, 100 ?

Done. Removed 2, added 50. These values are just for the chrome://flags, we can
use any values in the experiment.

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
File chrome/browser/experiments/memory_ablation_experiment.cc (right):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.cc:6: 
On 2017/04/10 18:05:04, Primiano Tucci wrote:
> do you need a class at all here?
> It seems all this code could be just a function, something like
> 
> MaybeStartMemoryAblationExperiment() {
>   int size = GetFieldTrialParamByFeatureAsInt(....);
>   if (!size <= 0)
>    return;
>  std::unique_ptr mem(new uint8_t[size]);
>  for(i = 0 ....)  
>    touch memory
>   PostDelayedTask(FROM_HERE, Bind([] (std::unique_ptr<uint8_t>) {}, delay )
> }

I think class is more flexible, for example we might want to do periodic
touching, or allocate in several chunks.

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.cc:59: memory_[i]++;
On 2017/04/10 18:05:04, Primiano Tucci wrote:
> two things here:
> 1. touching uninitialized memory is undefined behavior. clang is very
aggressive
> on undefined behavior and I've seen it allowing to just insert deliberate
> crashes even on things that shouln't crash in any case.
> See crbug.com/681218 where wfh was doing something very similar to this and
> ended up with clang generating a crash instead.
> 
> 2. Nothing is doing anything with memory_, which allows the compiler to just
> omit this loop completely (if didn't emit a trap as above).
> 
> What I would do here instead is:
> 1. Just write without reading, i.e. memory[i] = static_cast<uint8_t>(i).
> if you want to be really paranoid, cast it to volatile uint8_t*, that should
> make the compiler even less conservative.
> 2. Add a call to base::debug::Alias(memory_), which will stop the compiler
form
> making escape analysis on this pointer

Yup, definitely UB. Fixed.

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
File chrome/browser/experiments/memory_ablation_experiment.h (right):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.h:14: #include
"base/sequenced_task_runner.h"
On 2017/04/10 18:05:04, Primiano Tucci wrote:
> can just forward declare this

Done.

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.h:40:
std::unique_ptr<uint8_t[], decltype(free)*> memory_;
On 2017/04/10 18:05:04, Primiano Tucci wrote:
> this one seems over-complicated for the need.
> Can you just have a std::unique_ptr<uint8_t[]> and just initialize that as
> 
> memory_(new uint8_t[size]) ?
> 
> using new vs delete shouldn't make any difference vs malloc. If you do to
avoid
> large zero-initializaiton, memory shouldn'be initialize unless you do new
> (uint8_t[size]().

Done.

[Primiano Tucci (use gerrit), 2017-04-10 19:40:11]

non-owner LGTM

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
File chrome/browser/experiments/memory_ablation_experiment.cc (right):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.cc:6: 
On 2017/04/10 19:31:00, DmitrySkiba wrote:
> On 2017/04/10 18:05:04, Primiano Tucci wrote:
> > do you need a class at all here?
> > It seems all this code could be just a function, something like
> > 
> > MaybeStartMemoryAblationExperiment() {
> >   int size = GetFieldTrialParamByFeatureAsInt(....);
> >   if (!size <= 0)
> >    return;
> >  std::unique_ptr mem(new uint8_t[size]);
> >  for(i = 0 ....)  
> >    touch memory
> >   PostDelayedTask(FROM_HERE, Bind([] (std::unique_ptr<uint8_t>) {}, delay )
> > }
> 
> I think class is more flexible, for example we might want to do periodic
> touching, or allocate in several chunks.

A good rule of thumb is don't introduce unnecessary complexity until it becomes
necessary ;-)
If everybody did this we'd end up with a .so which is 2x bigger

[DmitrySkiba, 2017-04-10 19:51:41]

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
File chrome/browser/experiments/memory_ablation_experiment.cc (right):

https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
chrome/browser/experiments/memory_ablation_experiment.cc:6: 
On 2017/04/10 19:40:11, Primiano Tucci wrote:
> On 2017/04/10 19:31:00, DmitrySkiba wrote:
> > On 2017/04/10 18:05:04, Primiano Tucci wrote:
> > > do you need a class at all here?
> > > It seems all this code could be just a function, something like
> > > 
> > > MaybeStartMemoryAblationExperiment() {
> > >   int size = GetFieldTrialParamByFeatureAsInt(....);
> > >   if (!size <= 0)
> > >    return;
> > >  std::unique_ptr mem(new uint8_t[size]);
> > >  for(i = 0 ....)  
> > >    touch memory
> > >   PostDelayedTask(FROM_HERE, Bind([] (std::unique_ptr<uint8_t>) {}, delay
)
> > > }
> > 
> > I think class is more flexible, for example we might want to do periodic
> > touching, or allocate in several chunks.
> 
> A good rule of thumb is don't introduce unnecessary complexity until it
becomes
> necessary ;-)
> If everybody did this we'd end up with a .so which is 2x bigger

I mean, we might end up doing those things in this CL.

[Primiano Tucci (use gerrit), 2017-04-10 19:53:00]

On 2017/04/10 19:51:41, DmitrySkiba wrote:
>
https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
> File chrome/browser/experiments/memory_ablation_experiment.cc (right):
> 
>
https://codereview.chromium.org/2810833002/diff/1/chrome/browser/experiments/...
> chrome/browser/experiments/memory_ablation_experiment.cc:6: 
> On 2017/04/10 19:40:11, Primiano Tucci wrote:
> > On 2017/04/10 19:31:00, DmitrySkiba wrote:
> > > On 2017/04/10 18:05:04, Primiano Tucci wrote:
> > > > do you need a class at all here?
> > > > It seems all this code could be just a function, something like
> > > > 
> > > > MaybeStartMemoryAblationExperiment() {
> > > >   int size = GetFieldTrialParamByFeatureAsInt(....);
> > > >   if (!size <= 0)
> > > >    return;
> > > >  std::unique_ptr mem(new uint8_t[size]);
> > > >  for(i = 0 ....)  
> > > >    touch memory
> > > >   PostDelayedTask(FROM_HERE, Bind([] (std::unique_ptr<uint8_t>) {},
delay
> )
> > > > }
> > > 
> > > I think class is more flexible, for example we might want to do periodic
> > > touching, or allocate in several chunks.
> > 
> > A good rule of thumb is don't introduce unnecessary complexity until it
> becomes
> > necessary ;-)
> > If everybody did this we'd end up with a .so which is 2x bigger
> 
> I mean, we might end up doing those things in this CL.

Ah if it's already planned it's different.

[Bryan McQuade, 2017-04-10 21:37:51]

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

[Bryan McQuade, 2017-04-10 21:37:52]

Thanks! This looks good for an experiment where we increase browser process
memory. Were we also planning to increase render process memory?

[Bryan McQuade, 2017-04-10 21:41:09]

On 2017/04/10 at 21:37:52, Bryan McQuade wrote:
> Thanks! This looks good for an experiment where we increase browser process
memory. Were we also planning to increase render process memory?

Ah, I see the experiment doc talks about browser process. From early
conversations I though I recalled we would do this in the render process but if
we've decided to move to browser process that seems fine to me, thanks!

[Maria, 2017-04-10 21:43:21]

On 2017/04/10 21:41:09, Bryan McQuade wrote:
> On 2017/04/10 at 21:37:52, Bryan McQuade wrote:
> > Thanks! This looks good for an experiment where we increase browser process
> memory. Were we also planning to increase render process memory?
> 
> Ah, I see the experiment doc talks about browser process. From early
> conversations I though I recalled we would do this in the render process but
if
> we've decided to move to browser process that seems fine to me, thanks!

Yep. We are increasing browser process memory, but watching the renderer OOM
rates effect. Doesn't make sense to run the renderer experiment right now
because we would be flying blind due to Renderer UMA being broken on M+ Android
devices.

[Ilya Sherman, 2017-04-10 21:46:14]

On 2017/04/10 21:43:21, Maria wrote:
> On 2017/04/10 21:41:09, Bryan McQuade wrote:
> > On 2017/04/10 at 21:37:52, Bryan McQuade wrote:
> > > Thanks! This looks good for an experiment where we increase browser
process
> > memory. Were we also planning to increase render process memory?
> > 
> > Ah, I see the experiment doc talks about browser process. From early
> > conversations I though I recalled we would do this in the render process but
> if
> > we've decided to move to browser process that seems fine to me, thanks!
> 
> Yep. We are increasing browser process memory, but watching the renderer OOM
> rates effect. Doesn't make sense to run the renderer experiment right now
> because we would be flying blind due to Renderer UMA being broken on M+
Android
> devices.

Renderer UMA metrics are broken on M+?  Why so?  Is there a bug tracking that? 
(Apologies for the tangent, obviously no need to block this CL on it.)

[Maria, 2017-04-10 21:48:57]

On 2017/04/10 21:46:14, Ilya Sherman wrote:
> On 2017/04/10 21:43:21, Maria wrote:
> > On 2017/04/10 21:41:09, Bryan McQuade wrote:
> > > On 2017/04/10 at 21:37:52, Bryan McQuade wrote:
> > > > Thanks! This looks good for an experiment where we increase browser
> process
> > > memory. Were we also planning to increase render process memory?
> > > 
> > > Ah, I see the experiment doc talks about browser process. From early
> > > conversations I though I recalled we would do this in the render process
but
> > if
> > > we've decided to move to browser process that seems fine to me, thanks!
> > 
> > Yep. We are increasing browser process memory, but watching the renderer OOM
> > rates effect. Doesn't make sense to run the renderer experiment right now
> > because we would be flying blind due to Renderer UMA being broken on M+
> Android
> > devices.
> 
> Renderer UMA metrics are broken on M+?  Why so?  Is there a bug tracking that?

> (Apologies for the tangent, obviously no need to block this CL on it.)

https://bugs.chromium.org/p/chromium/issues/detail?id=704606

[Ilya Sherman, 2017-04-10 21:51:28]

On 2017/04/10 21:48:57, Maria wrote:
> On 2017/04/10 21:46:14, Ilya Sherman wrote:
> > On 2017/04/10 21:43:21, Maria wrote:
> > > On 2017/04/10 21:41:09, Bryan McQuade wrote:
> > > > On 2017/04/10 at 21:37:52, Bryan McQuade wrote:
> > > > > Thanks! This looks good for an experiment where we increase browser
> > process
> > > > memory. Were we also planning to increase render process memory?
> > > > 
> > > > Ah, I see the experiment doc talks about browser process. From early
> > > > conversations I though I recalled we would do this in the render process
> but
> > > if
> > > > we've decided to move to browser process that seems fine to me, thanks!
> > > 
> > > Yep. We are increasing browser process memory, but watching the renderer
OOM
> > > rates effect. Doesn't make sense to run the renderer experiment right now
> > > because we would be flying blind due to Renderer UMA being broken on M+
> > Android
> > > devices.
> > 
> > Renderer UMA metrics are broken on M+?  Why so?  Is there a bug tracking
that?
> 
> > (Apologies for the tangent, obviously no need to block this CL on it.)
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=704606

Oh, okay!  Just the memory usage reporting is not working on M+ -- not all of
UMA.  Whew!  Thanks for the pointer =)

[brettw, 2017-04-11 16:54:29]

lgtm

[Maria, 2017-04-11 18:51:41]

lgtm

[Bryan McQuade, 2017-04-12 01:31:55]

lgtm

[DmitrySkiba, 2017-04-12 18:55:06]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2017-04-12 18:55:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, primiano@chromium.org, bmcquade@chromium.org,
brettw@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2810833002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-12 18:55:54]

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-12 20:36:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492023306578850,
"parent_rev": "a332bf72eded671758da727bf530313696128ad3", "commit_rev":
"950fbc86a6623aebe69b471dfa156504b8f4467b"}

[commit-bot: I haz the power, 2017-04-12 20:37:31]

Description was changed from

==========
Add memory ablation experiment.

BUG=710048
==========

to

==========
Add memory ablation experiment.

BUG=710048

Review-Url: https://codereview.chromium.org/2810833002
Cr-Commit-Position: refs/heads/master@{#464136}
Committed:
https://chromium.googlesource.com/chromium/src/+/950fbc86a6623aebe69b471dfa15...
==========

[commit-bot: I haz the power, 2017-04-12 20:37:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/950fbc86a6623aebe69b471dfa15...