base: Use LazyInstanceTraits instead of SharedState class for discardable memory.

base: Use LazyInstanceTraits instead of SharedState class for discardable memory.

Switch to using LazyInstanceTraits instead of classes named SharedState
for the purpose of initializing a DiscardableMemoryManager instance
with a certain set of parameters.

BUG=422953
Committed: https://crrev.com/31f0530105bfc993e7d949a4a33bc70bb023acb1
Cr-Commit-Position: refs/heads/master@{#299756}

[reveman, 2014-10-13 16:50:42]

reveman@chromium.org changed reviewers:
+ avi@chromium.org, danakj@chromium.org

[Avi (use Gerrit), 2014-10-13 17:08:10]

lgtm

[reveman, 2014-10-15 17:56:14]

ping for base/ owner review

[danakj, 2014-10-15 18:04:08]

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:22: static const bool kRegisterOnExit
= false;
why false?

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:24: static const bool
kAllowedToAccessOnNonjoinableThread = true;
why true?

Sounds like you're making a LeakyLazyInstanceTraits not a
DefaultLazyInstanceTraits as the comment suggests? And not what this code used
to do?

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:29: kEmulatedMemoryLimit,
What about adding an Initialize(pass, constants, here) method instead that the
DiscardableMemoryEmulated can call after construction, instead of having to make
our own traits here?

[reveman, 2014-10-15 18:38:57]

PTAL

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:22: static const bool kRegisterOnExit
= false;
On 2014/10/15 18:04:08, danakj wrote:
> why false?

There's no guarantee that this is not be used after OnExit is called. For
example, discardable memory clients might store DM instances in LazyInstances.
Added comments to make this clear.

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:24: static const bool
kAllowedToAccessOnNonjoinableThread = true;
On 2014/10/15 18:04:08, danakj wrote:
> why true?
> 
> Sounds like you're making a LeakyLazyInstanceTraits not a
> DefaultLazyInstanceTraits as the comment suggests? And not what this code used
> to do?

This is what the code used to do. It used LazyInstance<SharedState>::Leaky.

I've updated the comments to instead mention LeakyLazyInstanceTraits instead of
DefaultLazyInstanceTraits.

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:29: kEmulatedMemoryLimit,
On 2014/10/15 18:04:08, danakj wrote:
> What about adding an Initialize(pass, constants, here) method instead that the
> DiscardableMemoryEmulated can call after construction, instead of having to
make
> our own traits here?

That would not be thread safe unless I misunderstood your suggestion. There
would be a race when two threads would use this for the first time..

[danakj, 2014-10-15 18:48:47]

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:29: kEmulatedMemoryLimit,
On 2014/10/15 18:38:57, reveman wrote:
> On 2014/10/15 18:04:08, danakj wrote:
> > What about adding an Initialize(pass, constants, here) method instead that
the
> > DiscardableMemoryEmulated can call after construction, instead of having to
> make
> > our own traits here?
> 
> That would not be thread safe unless I misunderstood your suggestion. There
> would be a race when two threads would use this for the first time..

Oh, Register holds a mutex I guess? I see.

[danakj, 2014-10-15 18:52:23]

Er, Pointer() not Register(). Anyway, thanks for the comment changes and
explanation. Looks like it's the same as before, hopefully this is easier code
to read and understand. LGTM

[reveman, 2014-10-15 18:54:36]

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:29: kEmulatedMemoryLimit,
On 2014/10/15 18:48:47, danakj wrote:
> On 2014/10/15 18:38:57, reveman wrote:
> > On 2014/10/15 18:04:08, danakj wrote:
> > > What about adding an Initialize(pass, constants, here) method instead that
> the
> > > DiscardableMemoryEmulated can call after construction, instead of having
to
> > make
> > > our own traits here?
> > 
> > That would not be thread safe unless I misunderstood your suggestion. There
> > would be a race when two threads would use this for the first time..
> 
> Oh, Register holds a mutex I guess? I see.

hm, I probably misunderstood your suggestion. When would this Initialize call
that pass parameters to the manager be made?

[danakj, 2014-10-15 18:59:33]

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:49:
g_manager.Pointer()->Register(this, bytes);
before/after this register call, in this method.

[reveman, 2014-10-15 18:59:36]

On 2014/10/15 18:52:23, danakj wrote:
> Er, Pointer() not Register(). Anyway, thanks for the comment changes and
> explanation. Looks like it's the same as before, hopefully this is easier code
> to read and understand. LGTM

Ok, no need to reply to my last comment :)

[reveman, 2014-10-15 18:59:41]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-15 19:01:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650233002/180001

[reveman, 2014-10-15 19:02:04]

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:49:
g_manager.Pointer()->Register(this, bytes);
On 2014/10/15 18:59:33, danakj wrote:
> before/after this register call, in this method.

Two threads can call this ctor at the same time. Both might end up calling
Initialize.

[danakj, 2014-10-15 19:11:53]

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/650233002/diff/160001/base/memory/discardable...
base/memory/discardable_memory_emulated.cc:49:
g_manager.Pointer()->Register(this, bytes);
On 2014/10/15 19:02:04, reveman wrote:
> On 2014/10/15 18:59:33, danakj wrote:
> > before/after this register call, in this method.
> 
> Two threads can call this ctor at the same time. Both might end up calling
> Initialize.

Right, sketchy. Carry on then :)

[commit-bot: I haz the power, 2014-10-15 19:12:09]

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

[commit-bot: I haz the power, 2014-10-15 19:12:10]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[reveman, 2014-10-15 19:23:34]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-15 19:24:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650233002/200001

[commit-bot: I haz the power, 2014-10-15 20:29:05]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2014-10-15 20:30:22]

Patchset 4 (id:??) landed as
https://crrev.com/31f0530105bfc993e7d949a4a33bc70bb023acb1
Cr-Commit-Position: refs/heads/master@{#299756}