base: Refactor DiscardableMemoryManager for use with different type of allocations.

base: Refactor DiscdarableMemoryManager for use with different type of allocations.

This replaces the code in DiscardableMemoryManager that was
hard-coded to use heap allocations with code that handle
instances that implement the
DiscardableMemoryManagerAllocation interface instead.

This allows us to use the manager class not only for
emulated discardable memory but any other type that
might need some level of userspace control. This also
removes the circular dependency between
DiscardableMemoryManager and DiscardableMemory classes.

BUG=327516
TEST=base_unittests --gtest_filter=DiscardableMemory*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266174

[Philippe, 2014-03-19 17:33:10]

Thanks David! The interface changes (that will let us use the controller on
Android) look good to me modulo the tiny nit. I would need to look more into the
.cc and unit test changes though but I think we are on the same page :)

https://codereview.chromium.org/204733003/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_allocation.h (right):

https://codereview.chromium.org/204733003/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_allocation.h:18: virtual
linked_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
Nit: I would return a scoped_ptr<> here and have the controller convert it to a
linked_ptr<>.

[reveman, 2014-03-20 15:21:41]

https://codereview.chromium.org/204733003/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_allocation.h (right):

https://codereview.chromium.org/204733003/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_allocation.h:18: virtual
linked_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
On 2014/03/19 17:33:10, Philippe wrote:
> Nit: I would return a scoped_ptr<> here and have the controller convert it to
a
> linked_ptr<>.

Done.

https://codereview.chromium.org/204733003/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_allocation.h:27: virtual bool Lock() = 0;
Inverted the return value of this function and added comments to describe each
of these functions.

[Philippe, 2014-03-20 18:08:30]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
I think this part is slightly hairy :) Correct me if I'm wrong but I believe
that the vptr is not initialized at this point so if
DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call on
|this| (e.g. CreateLockedAllocation()) then the behavior is  undefined. The vptr
gets initialized between the call to the base class' constructor and the
construction of the class' members IIRC.

FWIW I think composition would help here, at least for readability :)

[reveman, 2014-03-20 19:08:21]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/20 18:08:31, Philippe wrote:
> I think this part is slightly hairy :) Correct me if I'm wrong but I believe
> that the vptr is not initialized at this point so if
> DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call on
> |this| (e.g. CreateLockedAllocation()) then the behavior is  undefined. The
vptr
> gets initialized between the call to the base class' constructor and the
> construction of the class' members IIRC.

Correct.

> 
> FWIW I think composition would help here, at least for readability :)

I think it's OK to rely on the base class not using this in ctor but can change
to a virtual DiscardableMemoryManager::CreateLockedMemory function to make this
more explicit or initialize the factory prior to the manager if you feel
strongly about this.

[Philippe, 2014-03-21 09:13:08]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/20 19:08:22, reveman wrote:
> On 2014/03/20 18:08:31, Philippe wrote:
> > I think this part is slightly hairy :) Correct me if I'm wrong but I believe
> > that the vptr is not initialized at this point so if
> > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call on
> > |this| (e.g. CreateLockedAllocation()) then the behavior is  undefined. The
> vptr
> > gets initialized between the call to the base class' constructor and the
> > construction of the class' members IIRC.
> 
> Correct.
> 
> > 
> > FWIW I think composition would help here, at least for readability :)
> 
> I think it's OK to rely on the base class not using this in ctor but can
change
> to a virtual DiscardableMemoryManager::CreateLockedMemory function to make
this
> more explicit or initialize the factory prior to the manager if you feel
> strongly about this.

I think we will have to initialize the factory before the manager if we want to
be consistent across platforms. On Android, the allocator is the factory and
there is no way I can make this class extend both DiscardableMemoryManager and
DiscardableMemoryAllocationFactoryAshmem :)

If you feel strongly about keeping (single) inheritance here vs using
composition you can also make DiscardableMemoryManager take ownership of the
factory. With the current situation (DiscardableMemoryManager taking a raw
pointer to the factory), I have no choice in the Android implementation other
than using composition (which I'm happy with FWIW) :)

[Philippe, 2014-03-21 09:49:19]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/21 09:13:09, Philippe wrote:
> On 2014/03/20 19:08:22, reveman wrote:
> > On 2014/03/20 18:08:31, Philippe wrote:
> > > I think this part is slightly hairy :) Correct me if I'm wrong but I
believe
> > > that the vptr is not initialized at this point so if
> > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call
on
> > > |this| (e.g. CreateLockedAllocation()) then the behavior is  undefined.
The
> > vptr
> > > gets initialized between the call to the base class' constructor and the
> > > construction of the class' members IIRC.
> > 
> > Correct.
> > 
> > > 
> > > FWIW I think composition would help here, at least for readability :)
> > 
> > I think it's OK to rely on the base class not using this in ctor but can
> change
> > to a virtual DiscardableMemoryManager::CreateLockedMemory function to make
> this
> > more explicit or initialize the factory prior to the manager if you feel
> > strongly about this.
> 
> I think we will have to initialize the factory before the manager if we want
to
> be consistent across platforms. On Android, the allocator is the factory and
> there is no way I can make this class extend both DiscardableMemoryManager and
> DiscardableMemoryAllocationFactoryAshmem :)
> 
> If you feel strongly about keeping (single) inheritance here vs using
> composition you can also make DiscardableMemoryManager take ownership of the
> factory. With the current situation (DiscardableMemoryManager taking a raw
> pointer to the factory), I have no choice in the Android implementation other
> than using composition (which I'm happy with FWIW) :)

I think this should be a reasonable compromise that both you and I could like if
you feel strongly about keeping (single) inheritance:
class DiscardableMemoryAllocationImpl : public DiscardableMemoryAllocation {
 public:
  class Factory : public DiscardableMemoryAllocation::Factory {
   public:
    virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedMemory(
        size_t bytes) OVERRIDE {
      return make_scoped_ptr<DiscardableMemoryAllocation>(
          new DiscardableMemoryAllocationImpl(bytes));
    }
  };

  // DiscardableMemoryAllocation:
  // Lock(), Unlock()...
};

class DiscardableMemoryManagerImpl : public DiscardableMemoryManager {
 public:
   DiscardableMemoryManagerImpl()
       : DiscardableMemoryManager(
           make_scoped_ptr<DiscardableMemoryAllocation::Factory>(
               new DiscardableMemoryAllocationImpl::Factory()) {
   }
};

I would just find it slightly unfortunate/unusual to have the manager take
ownership of the factory but I can live with it :)

[Philippe, 2014-03-21 10:15:31]

On 2014/03/21 09:49:19, Philippe wrote:
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> File base/memory/discardable_memory_emulated.cc (right):
> 
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> base/memory/discardable_memory_emulated.cc:46: :
> internal::DiscardableMemoryManager(this,
> On 2014/03/21 09:13:09, Philippe wrote:
> > On 2014/03/20 19:08:22, reveman wrote:
> > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > I think this part is slightly hairy :) Correct me if I'm wrong but I
> believe
> > > > that the vptr is not initialized at this point so if
> > > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call
> on
> > > > |this| (e.g. CreateLockedAllocation()) then the behavior is  undefined.
> The
> > > vptr
> > > > gets initialized between the call to the base class' constructor and the
> > > > construction of the class' members IIRC.
> > > 
> > > Correct.
> > > 
> > > > 
> > > > FWIW I think composition would help here, at least for readability :)
> > > 
> > > I think it's OK to rely on the base class not using this in ctor but can
> > change
> > > to a virtual DiscardableMemoryManager::CreateLockedMemory function to make
> > this
> > > more explicit or initialize the factory prior to the manager if you feel
> > > strongly about this.
> > 
> > I think we will have to initialize the factory before the manager if we want
> to
> > be consistent across platforms. On Android, the allocator is the factory and
> > there is no way I can make this class extend both DiscardableMemoryManager
and
> > DiscardableMemoryAllocationFactoryAshmem :)
> > 
> > If you feel strongly about keeping (single) inheritance here vs using
> > composition you can also make DiscardableMemoryManager take ownership of the
> > factory. With the current situation (DiscardableMemoryManager taking a raw
> > pointer to the factory), I have no choice in the Android implementation
other
> > than using composition (which I'm happy with FWIW) :)
> 
> I think this should be a reasonable compromise that both you and I could like
if
> you feel strongly about keeping (single) inheritance:
> class DiscardableMemoryAllocationImpl : public DiscardableMemoryAllocation {
>  public:
>   class Factory : public DiscardableMemoryAllocation::Factory {
>    public:
>     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedMemory(
>         size_t bytes) OVERRIDE {
>       return make_scoped_ptr<DiscardableMemoryAllocation>(
>           new DiscardableMemoryAllocationImpl(bytes));
>     }
>   };
> 
>   // DiscardableMemoryAllocation:
>   // Lock(), Unlock()...
> };
> 
> class DiscardableMemoryManagerImpl : public DiscardableMemoryManager {
>  public:
>    DiscardableMemoryManagerImpl()
>        : DiscardableMemoryManager(
>            make_scoped_ptr<DiscardableMemoryAllocation::Factory>(
>                new DiscardableMemoryAllocationImpl::Factory()) {
>    }
> };
> 
> I would just find it slightly unfortunate/unusual to have the manager take
> ownership of the factory but I can live with it :)

We might also want to reconsider using a Callback instead of the factory. I feel
like it would really simplify many things.

[reveman, 2014-03-21 12:32:16]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/21 09:13:09, Philippe wrote:
> On 2014/03/20 19:08:22, reveman wrote:
> > On 2014/03/20 18:08:31, Philippe wrote:
> > > I think this part is slightly hairy :) Correct me if I'm wrong but I
believe
> > > that the vptr is not initialized at this point so if
> > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call
on
> > > |this| (e.g. CreateLockedAllocation()) then the behavior is  undefined.
The
> > vptr
> > > gets initialized between the call to the base class' constructor and the
> > > construction of the class' members IIRC.
> > 
> > Correct.
> > 
> > > 
> > > FWIW I think composition would help here, at least for readability :)
> > 
> > I think it's OK to rely on the base class not using this in ctor but can
> change
> > to a virtual DiscardableMemoryManager::CreateLockedMemory function to make
> this
> > more explicit or initialize the factory prior to the manager if you feel
> > strongly about this.
> 
> I think we will have to initialize the factory before the manager if we want
to
> be consistent across platforms. On Android, the allocator is the factory and
> there is no way I can make this class extend both DiscardableMemoryManager and
> DiscardableMemoryAllocationFactoryAshmem :)

Why not?

> 
> If you feel strongly about keeping (single) inheritance here vs using
> composition you can also make DiscardableMemoryManager take ownership of the
> factory. With the current situation (DiscardableMemoryManager taking a raw
> pointer to the factory), I have no choice in the Android implementation other
> than using composition (which I'm happy with FWIW) :)

Sorry, I'm failing to see why. Can you explain?

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/21 09:49:19, Philippe wrote:
> On 2014/03/21 09:13:09, Philippe wrote:
> > On 2014/03/20 19:08:22, reveman wrote:
> > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > I think this part is slightly hairy :) Correct me if I'm wrong but I
> believe
> > > > that the vptr is not initialized at this point so if
> > > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call
> on
> > > > |this| (e.g. CreateLockedAllocation()) then the behavior is  undefined.
> The
> > > vptr
> > > > gets initialized between the call to the base class' constructor and the
> > > > construction of the class' members IIRC.
> > > 
> > > Correct.
> > > 
> > > > 
> > > > FWIW I think composition would help here, at least for readability :)
> > > 
> > > I think it's OK to rely on the base class not using this in ctor but can
> > change
> > > to a virtual DiscardableMemoryManager::CreateLockedMemory function to make
> > this
> > > more explicit or initialize the factory prior to the manager if you feel
> > > strongly about this.
> > 
> > I think we will have to initialize the factory before the manager if we want
> to
> > be consistent across platforms. On Android, the allocator is the factory and
> > there is no way I can make this class extend both DiscardableMemoryManager
and
> > DiscardableMemoryAllocationFactoryAshmem :)
> > 
> > If you feel strongly about keeping (single) inheritance here vs using
> > composition you can also make DiscardableMemoryManager take ownership of the
> > factory. With the current situation (DiscardableMemoryManager taking a raw
> > pointer to the factory), I have no choice in the Android implementation
other
> > than using composition (which I'm happy with FWIW) :)
> 
> I think this should be a reasonable compromise that both you and I could like
if
> you feel strongly about keeping (single) inheritance:
> class DiscardableMemoryAllocationImpl : public DiscardableMemoryAllocation {
>  public:
>   class Factory : public DiscardableMemoryAllocation::Factory {
>    public:
>     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedMemory(
>         size_t bytes) OVERRIDE {
>       return make_scoped_ptr<DiscardableMemoryAllocation>(
>           new DiscardableMemoryAllocationImpl(bytes));
>     }
>   };
> 
>   // DiscardableMemoryAllocation:
>   // Lock(), Unlock()...
> };
> 
> class DiscardableMemoryManagerImpl : public DiscardableMemoryManager {
>  public:
>    DiscardableMemoryManagerImpl()
>        : DiscardableMemoryManager(
>            make_scoped_ptr<DiscardableMemoryAllocation::Factory>(
>                new DiscardableMemoryAllocationImpl::Factory()) {
>    }
> };
> 
> I would just find it slightly unfortunate/unusual to have the manager take
> ownership of the factory but I can live with it :)

I don't like that either. Awkward for the manager to take ownership of an
instance that implements an interface when it's really just interested in the
interface. Let's not do that.

We can always use a different LazyInstance and instantiate the factory before
the manager impl if we have to. That's much better than passing the ownership
imo.

[Philippe, 2014-03-21 12:40:49]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/21 12:32:16, reveman wrote:
> On 2014/03/21 09:13:09, Philippe wrote:
> > On 2014/03/20 19:08:22, reveman wrote:
> > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > I think this part is slightly hairy :) Correct me if I'm wrong but I
> believe
> > > > that the vptr is not initialized at this point so if
> > > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual call
> on
> > > > |this| (e.g. CreateLockedAllocation()) then the behavior is  undefined.
> The
> > > vptr
> > > > gets initialized between the call to the base class' constructor and the
> > > > construction of the class' members IIRC.
> > > 
> > > Correct.
> > > 
> > > > 
> > > > FWIW I think composition would help here, at least for readability :)
> > > 
> > > I think it's OK to rely on the base class not using this in ctor but can
> > change
> > > to a virtual DiscardableMemoryManager::CreateLockedMemory function to make
> > this
> > > more explicit or initialize the factory prior to the manager if you feel
> > > strongly about this.
> > 
> > I think we will have to initialize the factory before the manager if we want
> to
> > be consistent across platforms. On Android, the allocator is the factory and
> > there is no way I can make this class extend both DiscardableMemoryManager
and
> > DiscardableMemoryAllocationFactoryAshmem :)
> 
> Why not?
> 
> > 
> > If you feel strongly about keeping (single) inheritance here vs using
> > composition you can also make DiscardableMemoryManager take ownership of the
> > factory. With the current situation (DiscardableMemoryManager taking a raw
> > pointer to the factory), I have no choice in the Android implementation
other
> > than using composition (which I'm happy with FWIW) :)
> 
> Sorry, I'm failing to see why. Can you explain?

Because this would be a direct violation of the Liskov substitution principle
just like subclassing the factory here already is. A
DiscardableMemoryManagerImpl is not a Factory (and it's even less an
AshmemFactory). I really think we should avoid such inheritance. I don't see why
you are opposed to composition here? That said having a separate LazyInstance
for the Factory is not ideal but it would already be much better than
subclassing the Factory IMO. What do you think?

[reveman, 2014-03-21 12:54:39]

On 2014/03/21 12:40:49, Philippe wrote:
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> File base/memory/discardable_memory_emulated.cc (right):
> 
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> base/memory/discardable_memory_emulated.cc:46: :
> internal::DiscardableMemoryManager(this,
> On 2014/03/21 12:32:16, reveman wrote:
> > On 2014/03/21 09:13:09, Philippe wrote:
> > > On 2014/03/20 19:08:22, reveman wrote:
> > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > I think this part is slightly hairy :) Correct me if I'm wrong but I
> > believe
> > > > > that the vptr is not initialized at this point so if
> > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual
call
> > on
> > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
undefined.
> > The
> > > > vptr
> > > > > gets initialized between the call to the base class' constructor and
the
> > > > > construction of the class' members IIRC.
> > > > 
> > > > Correct.
> > > > 
> > > > > 
> > > > > FWIW I think composition would help here, at least for readability :)
> > > > 
> > > > I think it's OK to rely on the base class not using this in ctor but can
> > > change
> > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function to
make
> > > this
> > > > more explicit or initialize the factory prior to the manager if you feel
> > > > strongly about this.
> > > 
> > > I think we will have to initialize the factory before the manager if we
want
> > to
> > > be consistent across platforms. On Android, the allocator is the factory
and
> > > there is no way I can make this class extend both DiscardableMemoryManager
> and
> > > DiscardableMemoryAllocationFactoryAshmem :)
> > 
> > Why not?
> > 
> > > 
> > > If you feel strongly about keeping (single) inheritance here vs using
> > > composition you can also make DiscardableMemoryManager take ownership of
the
> > > factory. With the current situation (DiscardableMemoryManager taking a raw
> > > pointer to the factory), I have no choice in the Android implementation
> other
> > > than using composition (which I'm happy with FWIW) :)
> > 
> > Sorry, I'm failing to see why. Can you explain?
> 
> Because this would be a direct violation of the Liskov substitution principle
> just like subclassing the factory here already is. A
> DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> AshmemFactory). I really think we should avoid such inheritance. I don't see
why
> you are opposed to composition here? That said having a separate LazyInstance
> for the Factory is not ideal but it would already be much better than
> subclassing the Factory IMO. What do you think?

The DiscardableMemoryManager::Factory is not a type and neither does
DiscardableMemoryManagerFactoryAshmem have to be.

DiscardableMemoryManagerImpl is a DiscardableMemoryManager instance that
implements the factory interface. I think that pattern is OK and not too
uncommon within the chromium code base.

[Philippe, 2014-03-21 13:26:40]

On 2014/03/21 12:54:39, reveman wrote:
> On 2014/03/21 12:40:49, Philippe wrote:
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > File base/memory/discardable_memory_emulated.cc (right):
> > 
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > base/memory/discardable_memory_emulated.cc:46: :
> > internal::DiscardableMemoryManager(this,
> > On 2014/03/21 12:32:16, reveman wrote:
> > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > I think this part is slightly hairy :) Correct me if I'm wrong but I
> > > believe
> > > > > > that the vptr is not initialized at this point so if
> > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual
> call
> > > on
> > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> undefined.
> > > The
> > > > > vptr
> > > > > > gets initialized between the call to the base class' constructor and
> the
> > > > > > construction of the class' members IIRC.
> > > > > 
> > > > > Correct.
> > > > > 
> > > > > > 
> > > > > > FWIW I think composition would help here, at least for readability
:)
> > > > > 
> > > > > I think it's OK to rely on the base class not using this in ctor but
can
> > > > change
> > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function to
> make
> > > > this
> > > > > more explicit or initialize the factory prior to the manager if you
feel
> > > > > strongly about this.
> > > > 
> > > > I think we will have to initialize the factory before the manager if we
> want
> > > to
> > > > be consistent across platforms. On Android, the allocator is the factory
> and
> > > > there is no way I can make this class extend both
DiscardableMemoryManager
> > and
> > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > 
> > > Why not?
> > > 
> > > > 
> > > > If you feel strongly about keeping (single) inheritance here vs using
> > > > composition you can also make DiscardableMemoryManager take ownership of
> the
> > > > factory. With the current situation (DiscardableMemoryManager taking a
raw
> > > > pointer to the factory), I have no choice in the Android implementation
> > other
> > > > than using composition (which I'm happy with FWIW) :)
> > > 
> > > Sorry, I'm failing to see why. Can you explain?
> > 
> > Because this would be a direct violation of the Liskov substitution
principle
> > just like subclassing the factory here already is. A
> > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > AshmemFactory). I really think we should avoid such inheritance. I don't see
> why
> > you are opposed to composition here? That said having a separate
LazyInstance
> > for the Factory is not ideal but it would already be much better than
> > subclassing the Factory IMO. What do you think?
> 
> The DiscardableMemoryManager::Factory is not a type and neither does
> DiscardableMemoryManagerFactoryAshmem have to be.
> 
> DiscardableMemoryManagerImpl is a DiscardableMemoryManager instance that
> implements the factory interface. I think that pattern is OK and not too
> uncommon within the chromium code base.

Not sure what DiscardableMemoryManager::Factory and
DiscardableMemoryManagerFactoryAshmem would be but if they are classes then they
are type :) Inheritance should only be used to express a is-a relationship. A
DiscardableMemoryManager has/uses a Factory but it's not a Factory. Besides that
I already highlighted the current risk of ending up with undefined behavior and
I don't think we should keep unnecessarily subtle code in the codebase. If you
can inject the factory instance reasonably while keeping single inheritance I'm
fine with it but the less unreasonable approach would be to use a separate
LazyInstance, right?

I agree that naming is important but if your only opposition to composition is
about finding a good name for the class then perhaps we should just try to find
a good name, what do you think?

I don't think such code is unreasonable:

class DiscardableMemoryManagerAshmem {
 public:
  DiscardableMemoryManagerAshmem() : manager_(&factory_) {}
 
  DiscardableMemoryManager* instance() {
    return &manager_;
  }
 
 private:
  DiscardableMemoryAllocationFactoryAshmem factory_;
  DiscardableMemoryManager manager_;
};

vs:

class DiscardableMemoryManagerImpl
    : public DiscardableMemoryManager,
      public DiscardableMemoryAllocationFactoryAshmem {  // Violation of LSP.
 public:
  DiscardableMemoryManagerAshmem()
      : DiscardableMemoryManagerImpl(this) {  // Possibly leading to undefined
behavior.
  }
};

But again the best option in my opinion is just to remove the Factory interface
and only use inheritance to implement the internal DiscardableMemoryAllocation
and public DiscardableMemory interface. In such case we would have this:

class DiscardableMemoryManagerAshmem {
 public:
  DiscardableMemoryManagerAshmem()
      : manager_(base::Bind(&DiscardableMemoryAshmem::CreateAllocation,
                            base::Unretained(this)) {}
 
  DiscardableMemoryManager* instance() {
    return &manager_;
  }
 
 private:
  scoped_ptr<DiscardableMemoryAllocation> CreateAllocation(size_t bytes) {
    return ashmem_allocator_.Allocate(bytes);
  }

  DiscardableMemoryAllocatorAshmem ashmem_allocator_;
  DiscardableMemoryManager manager_;
};

[reveman, 2014-03-22 16:18:25]

On 2014/03/21 13:26:40, Philippe wrote:
> On 2014/03/21 12:54:39, reveman wrote:
> > On 2014/03/21 12:40:49, Philippe wrote:
> > >
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > > File base/memory/discardable_memory_emulated.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > > base/memory/discardable_memory_emulated.cc:46: :
> > > internal::DiscardableMemoryManager(this,
> > > On 2014/03/21 12:32:16, reveman wrote:
> > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > I think this part is slightly hairy :) Correct me if I'm wrong but
I
> > > > believe
> > > > > > > that the vptr is not initialized at this point so if
> > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
virtual
> > call
> > > > on
> > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > undefined.
> > > > The
> > > > > > vptr
> > > > > > > gets initialized between the call to the base class' constructor
and
> > the
> > > > > > > construction of the class' members IIRC.
> > > > > > 
> > > > > > Correct.
> > > > > > 
> > > > > > > 
> > > > > > > FWIW I think composition would help here, at least for readability
> :)
> > > > > > 
> > > > > > I think it's OK to rely on the base class not using this in ctor but
> can
> > > > > change
> > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function
to
> > make
> > > > > this
> > > > > > more explicit or initialize the factory prior to the manager if you
> feel
> > > > > > strongly about this.
> > > > > 
> > > > > I think we will have to initialize the factory before the manager if
we
> > want
> > > > to
> > > > > be consistent across platforms. On Android, the allocator is the
factory
> > and
> > > > > there is no way I can make this class extend both
> DiscardableMemoryManager
> > > and
> > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > 
> > > > Why not?
> > > > 
> > > > > 
> > > > > If you feel strongly about keeping (single) inheritance here vs using
> > > > > composition you can also make DiscardableMemoryManager take ownership
of
> > the
> > > > > factory. With the current situation (DiscardableMemoryManager taking a
> raw
> > > > > pointer to the factory), I have no choice in the Android
implementation
> > > other
> > > > > than using composition (which I'm happy with FWIW) :)
> > > > 
> > > > Sorry, I'm failing to see why. Can you explain?
> > > 
> > > Because this would be a direct violation of the Liskov substitution
> principle
> > > just like subclassing the factory here already is. A
> > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > AshmemFactory). I really think we should avoid such inheritance. I don't
see
> > why
> > > you are opposed to composition here? That said having a separate
> LazyInstance
> > > for the Factory is not ideal but it would already be much better than
> > > subclassing the Factory IMO. What do you think?
> > 
> > The DiscardableMemoryManager::Factory is not a type and neither does
> > DiscardableMemoryManagerFactoryAshmem have to be.
> > 
> > DiscardableMemoryManagerImpl is a DiscardableMemoryManager instance that
> > implements the factory interface. I think that pattern is OK and not too
> > uncommon within the chromium code base.
> 
> Not sure what DiscardableMemoryManager::Factory and
> DiscardableMemoryManagerFactoryAshmem would be but if they are classes then
they
> are type :) Inheritance should only be used to express a is-a relationship. A
> DiscardableMemoryManager has/uses a Factory but it's not a Factory. Besides
that
> I already highlighted the current risk of ending up with undefined behavior
and
> I don't think we should keep unnecessarily subtle code in the codebase. If you
> can inject the factory instance reasonably while keeping single inheritance
I'm
> fine with it but the less unreasonable approach would be to use a separate
> LazyInstance, right?

I agree that it's maybe not a great idea to inherit an interface implementation.
There's nothing wrong with having DiscardableMemoryManagerImpl implement the
DiscardableMemoryManager::Factory interface though.

> 
> I agree that naming is important but if your only opposition to composition is
> about finding a good name for the class then perhaps we should just try to
find
> a good name, what do you think?

I'm not against composition. It's probably the right thing to do in this case.
I'm just not sure that your current patch is the best approach.

> 
> I don't think such code is unreasonable:
> 
> class DiscardableMemoryManagerAshmem {
>  public:
>   DiscardableMemoryManagerAshmem() : manager_(&factory_) {}
>  
>   DiscardableMemoryManager* instance() {
>     return &manager_;
>   }
>  
>  private:
>   DiscardableMemoryAllocationFactoryAshmem factory_;
>   DiscardableMemoryManager manager_;
> };

This code is confusing to me. There's a DiscardableMemoryManagerAshmem class
that is not a DiscardableMemoryManager. And there's a non static instance()
function that returns a different type. I've never seen this pattern in the
chromium codebase before.

> 
> vs:
> 
> class DiscardableMemoryManagerImpl
>     : public DiscardableMemoryManager,
>       public DiscardableMemoryAllocationFactoryAshmem {  // Violation of LSP.
>  public:
>   DiscardableMemoryManagerAshmem()
>       : DiscardableMemoryManagerImpl(this) {  // Possibly leading to undefined
> behavior.
>   }
> };

I agree that inheriting the implementation of that interface is not great.
Passing "this" to the ctor and not expecting it to be used in ctor is common on
the chromium codebase so I don't think we need to worry about that unless we
intend to remove that type of usage elsewhere.

> 
> But again the best option in my opinion is just to remove the Factory
interface
> and only use inheritance to implement the internal DiscardableMemoryAllocation
> and public DiscardableMemory interface. In such case we would have this:
> 
> class DiscardableMemoryManagerAshmem {
>  public:
>   DiscardableMemoryManagerAshmem()
>       : manager_(base::Bind(&DiscardableMemoryAshmem::CreateAllocation,
>                             base::Unretained(this)) {}
>  
>   DiscardableMemoryManager* instance() {
>     return &manager_;
>   }
>  
>  private:
>   scoped_ptr<DiscardableMemoryAllocation> CreateAllocation(size_t bytes) {
>     return ashmem_allocator_.Allocate(bytes);
>   }
> 
>   DiscardableMemoryAllocatorAshmem ashmem_allocator_;
>   DiscardableMemoryManager manager_;
> };

I'm not a fan of using anonymous functions for these kind of things. I think a
real interface is more descriptive and easier to understand. Also allows me to
use use cs.chromium.org and easily get a list of each implementation of the
interface.

Here's an alternative to how you can do composition:

class DiscardableMemoryManagerImpl : public DiscardableMemoryManager,
                                     public DiscardableMemoryAllocation::Factory
{
 public:
  DiscardableMemoryManagerAshmem() : DiscardableMemoryManagerImpl(this) {}

  // Overridden from DiscardableMemoryAllocation::Factory:             
  virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(size_t
bytes) OVERRIDE {                                
    return ashmem_allocator_.CreateLocked(bytes);
  }

 private:
  DiscardableMemoryAllocatorAshmem ashmem_allocator_;
};

and if we feel like making CreateLockedAllocation an abstract
DiscardableMemoryManager function instead that would be trivial.

It might be a good idea to take a step back and forget about the existing
DiscardableMemoryAllocatorAndroid implementation and think about how we would
implement this if the current ashmem code didn't already exist. What you need
for ashmem is a DiscardableMemoryAllocationAshmem implementation and an
implementation of the DiscardableMemoryAllocation::Factory interface. You
wouldn't necessarily need a DiscardableMemoryAllocatorAshmem class. Maybe we can
move all allocator logic into DiscardableMemoryAllocationAshmem (some of it as
static functions) and keep some minimal state in DiscardableMemoryManagerImpl.
That would probably be follow-up work though but might still be good to think
about how we want this to work long term even if we're not going to change this
right now.

[reveman, 2014-03-22 16:49:33]

Here's another option for how to implement the factory interface and the manager
type for ashmem:

Add a DiscarableMemoryManagerAshmem type that implements most of the current
DiscarableMemoryAllocatorAndroid logic. The DiscarableMemoryManagerImpl for
android would look something like this:

class DiscardableMemoryManagerImpl
    : public DiscardableMemoryManagerAshmem,
      public DiscardableMemoryAllocation::Factory {
 public:
  DiscardableMemoryManagerImpl()
      : DiscardableMemoryManagerAshmem(this) {}

  // Overridden from DiscardableMemoryAllocation::Factory:
  virtual scoped_ptr<DiscardableMemoryAllocation>
  CreateLockedAllocation(size_t bytes) OVERRIDE {
      return CreatedLockedAshmemAllocation(bytes);
  }
};

CreatedLockedAshmemAllocation is implemented by the base class.

Could even have DiscardableMemoryManagerAshmem implement the
DiscardableMemoryAllocation::Factory interface directly.

I think this is my preferred approach. Easy to understand and makes code reuse
easy at every level.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/21 12:40:50, Philippe wrote:
> On 2014/03/21 12:32:16, reveman wrote:
> > On 2014/03/21 09:13:09, Philippe wrote:
> > > On 2014/03/20 19:08:22, reveman wrote:
> > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > I think this part is slightly hairy :) Correct me if I'm wrong but I
> > believe
> > > > > that the vptr is not initialized at this point so if
> > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual
call
> > on
> > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
undefined.
> > The
> > > > vptr
> > > > > gets initialized between the call to the base class' constructor and
the
> > > > > construction of the class' members IIRC.
> > > > 
> > > > Correct.
> > > > 
> > > > > 
> > > > > FWIW I think composition would help here, at least for readability :)
> > > > 
> > > > I think it's OK to rely on the base class not using this in ctor but can
> > > change
> > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function to
make
> > > this
> > > > more explicit or initialize the factory prior to the manager if you feel
> > > > strongly about this.
> > > 
> > > I think we will have to initialize the factory before the manager if we
want
> > to
> > > be consistent across platforms. On Android, the allocator is the factory
and
> > > there is no way I can make this class extend both DiscardableMemoryManager
> and
> > > DiscardableMemoryAllocationFactoryAshmem :)
> > 
> > Why not?
> > 
> > > 
> > > If you feel strongly about keeping (single) inheritance here vs using
> > > composition you can also make DiscardableMemoryManager take ownership of
the
> > > factory. With the current situation (DiscardableMemoryManager taking a raw
> > > pointer to the factory), I have no choice in the Android implementation
> other
> > > than using composition (which I'm happy with FWIW) :)
> > 
> > Sorry, I'm failing to see why. Can you explain?
> 
> Because this would be a direct violation of the Liskov substitution principle
> just like subclassing the factory here already is. A
> DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> AshmemFactory). I really think we should avoid such inheritance. I don't see
why
> you are opposed to composition here? That said having a separate LazyInstance
> for the Factory is not ideal but it would already be much better than
> subclassing the Factory IMO. What do you think?

Correct, DiscardableMemoryManagerImpl is not a Factory as there's no such thing
as "a Factory". There's only the Factory interface that
DiscardableMemoryManagerImpl implements. The use of interfaces throughout the
chromium code is far from rare and this is not doing anything that is not
commonly found in existing chromium code.

To summarize my current thinking; this patch is well aligned with the rest of
the chromium codebase. Passing "this" to the ctor and expecting it not to be
called is not uncommon. Inheriting an interface implementation might be
confusing. Probably better to use composition if DiscardableMemoryManagerImpl
can't implement the interface by itself. Nothing in this patch that prevents
that though.

[reveman, 2014-03-22 17:00:04]

+willchan

Please take a look.

[Philippe, 2014-03-24 09:35:30]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/22 16:49:34, reveman wrote:
> On 2014/03/21 12:40:50, Philippe wrote:
> > On 2014/03/21 12:32:16, reveman wrote:
> > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > I think this part is slightly hairy :) Correct me if I'm wrong but I
> > > believe
> > > > > > that the vptr is not initialized at this point so if
> > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a virtual
> call
> > > on
> > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> undefined.
> > > The
> > > > > vptr
> > > > > > gets initialized between the call to the base class' constructor and
> the
> > > > > > construction of the class' members IIRC.
> > > > > 
> > > > > Correct.
> > > > > 
> > > > > > 
> > > > > > FWIW I think composition would help here, at least for readability
:)
> > > > > 
> > > > > I think it's OK to rely on the base class not using this in ctor but
can
> > > > change
> > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function to
> make
> > > > this
> > > > > more explicit or initialize the factory prior to the manager if you
feel
> > > > > strongly about this.
> > > > 
> > > > I think we will have to initialize the factory before the manager if we
> want
> > > to
> > > > be consistent across platforms. On Android, the allocator is the factory
> and
> > > > there is no way I can make this class extend both
DiscardableMemoryManager
> > and
> > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > 
> > > Why not?
> > > 
> > > > 
> > > > If you feel strongly about keeping (single) inheritance here vs using
> > > > composition you can also make DiscardableMemoryManager take ownership of
> the
> > > > factory. With the current situation (DiscardableMemoryManager taking a
raw
> > > > pointer to the factory), I have no choice in the Android implementation
> > other
> > > > than using composition (which I'm happy with FWIW) :)
> > > 
> > > Sorry, I'm failing to see why. Can you explain?
> > 
> > Because this would be a direct violation of the Liskov substitution
principle
> > just like subclassing the factory here already is. A
> > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > AshmemFactory). I really think we should avoid such inheritance. I don't see
> why
> > you are opposed to composition here? That said having a separate
LazyInstance
> > for the Factory is not ideal but it would already be much better than
> > subclassing the Factory IMO. What do you think?
> 
> Correct, DiscardableMemoryManagerImpl is not a Factory as there's no such
thing
> as "a Factory". There's only the Factory interface that
> DiscardableMemoryManagerImpl implements. The use of interfaces throughout the
> chromium code is far from rare and this is not doing anything that is not
> commonly found in existing chromium code.
> 
> To summarize my current thinking; this patch is well aligned with the rest of
> the chromium codebase. Passing "this" to the ctor and expecting it not to be
> called is not uncommon. Inheriting an interface implementation might be
> confusing. Probably better to use composition if DiscardableMemoryManagerImpl
> can't implement the interface by itself. Nothing in this patch that prevents
> that though.

Just to be clear and so that the information is present here with the full
context, my main concern is not the existence of the Factory interface (although
I would still prefer to replace it with the use of a Callback) but rather the
unnecessary/unreasonable (IMO) use of inheritance here (for the reasons I
mentioned before).

Moreover the Google style guide is also pretty clear:
"Do not overuse implementation inheritance. Composition is often more
appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
subclasses Foo if it can reasonably be said that Bar "is a kind of" Foo."

I don't know what data you are using to say that this is common in the Chromium
codebase. It might be but I don't think we have ever encouraged/should encourage
such idioms over composition.

If you are concerned about the instance() method in the version of my patch
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
FTR), then we can also use composition with delegate methods, as it should
really be (although it's slightly more verbose). The code below should be safe,
correct and simple IMO.

class DiscardableMemoryManagerEmulated {
 public:
  typedef DiscardableMemoryManager::AllocationId AllocationId;

  DiscardableMemoryManagerEmulated() : manager_(&factory_) {}

  AllocationId Register(size_t size) { return manager_.Register(size); }

  void Unregister(AllocationId id) { manager_.Unregister(id); }  

  void* Lock(AllocationId id, bool* purged) {
    return manager_.Lock(id, purged);
  }

  void Unlock(AllocationId id) { manager_.Unlock(id); }

 private:
  class Factory : public DiscardableMemoryAllocation::Factory {
   public:
    virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
        size_t size) OVERRIDE {
      return scoped_ptr<DiscardableMemoryAllocation>(
          new DiscardableMemoryAllocationImpl(size));
    }
  };

  Factory factory_;
  DiscardableMemoryManager manager_;
};

base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
    LAZY_INSTANCE_INITIALIZER;

[reveman, 2014-03-24 16:29:57]

On 2014/03/24 09:35:30, Philippe wrote:
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> File base/memory/discardable_memory_emulated.cc (right):
> 
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> base/memory/discardable_memory_emulated.cc:46: :
> internal::DiscardableMemoryManager(this,
> On 2014/03/22 16:49:34, reveman wrote:
> > On 2014/03/21 12:40:50, Philippe wrote:
> > > On 2014/03/21 12:32:16, reveman wrote:
> > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > I think this part is slightly hairy :) Correct me if I'm wrong but
I
> > > > believe
> > > > > > > that the vptr is not initialized at this point so if
> > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
virtual
> > call
> > > > on
> > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > undefined.
> > > > The
> > > > > > vptr
> > > > > > > gets initialized between the call to the base class' constructor
and
> > the
> > > > > > > construction of the class' members IIRC.
> > > > > > 
> > > > > > Correct.
> > > > > > 
> > > > > > > 
> > > > > > > FWIW I think composition would help here, at least for readability
> :)
> > > > > > 
> > > > > > I think it's OK to rely on the base class not using this in ctor but
> can
> > > > > change
> > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function
to
> > make
> > > > > this
> > > > > > more explicit or initialize the factory prior to the manager if you
> feel
> > > > > > strongly about this.
> > > > > 
> > > > > I think we will have to initialize the factory before the manager if
we
> > want
> > > > to
> > > > > be consistent across platforms. On Android, the allocator is the
factory
> > and
> > > > > there is no way I can make this class extend both
> DiscardableMemoryManager
> > > and
> > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > 
> > > > Why not?
> > > > 
> > > > > 
> > > > > If you feel strongly about keeping (single) inheritance here vs using
> > > > > composition you can also make DiscardableMemoryManager take ownership
of
> > the
> > > > > factory. With the current situation (DiscardableMemoryManager taking a
> raw
> > > > > pointer to the factory), I have no choice in the Android
implementation
> > > other
> > > > > than using composition (which I'm happy with FWIW) :)
> > > > 
> > > > Sorry, I'm failing to see why. Can you explain?
> > > 
> > > Because this would be a direct violation of the Liskov substitution
> principle
> > > just like subclassing the factory here already is. A
> > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > AshmemFactory). I really think we should avoid such inheritance. I don't
see
> > why
> > > you are opposed to composition here? That said having a separate
> LazyInstance
> > > for the Factory is not ideal but it would already be much better than
> > > subclassing the Factory IMO. What do you think?
> > 
> > Correct, DiscardableMemoryManagerImpl is not a Factory as there's no such
> thing
> > as "a Factory". There's only the Factory interface that
> > DiscardableMemoryManagerImpl implements. The use of interfaces throughout
the
> > chromium code is far from rare and this is not doing anything that is not
> > commonly found in existing chromium code.
> > 
> > To summarize my current thinking; this patch is well aligned with the rest
of
> > the chromium codebase. Passing "this" to the ctor and expecting it not to be
> > called is not uncommon. Inheriting an interface implementation might be
> > confusing. Probably better to use composition if
DiscardableMemoryManagerImpl
> > can't implement the interface by itself. Nothing in this patch that prevents
> > that though.
> 
> Just to be clear and so that the information is present here with the full
> context, my main concern is not the existence of the Factory interface
(although
> I would still prefer to replace it with the use of a Callback) but rather the
> unnecessary/unreasonable (IMO) use of inheritance here (for the reasons I
> mentioned before).
> 
> Moreover the Google style guide is also pretty clear:
> "Do not overuse implementation inheritance. Composition is often more
> appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
> subclasses Foo if it can reasonably be said that Bar "is a kind of" Foo."

I don't think this is a case of overuse.

> 
> I don't know what data you are using to say that this is common in the
Chromium
> codebase. It might be but I don't think we have ever encouraged/should
encourage
> such idioms over composition.

RenderThreadImpl is one example (content/renderer/render_thread_impl.h). It's a
RenderThread implementation that also implements the GpuChannelHostFactory
interface.

> 
> If you are concerned about the instance() method in the version of my patch
>
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
> FTR), then we can also use composition with delegate methods, as it should
> really be (although it's slightly more verbose). The code below should be
safe,
> correct and simple IMO.
> 
> class DiscardableMemoryManagerEmulated {
>  public:
>   typedef DiscardableMemoryManager::AllocationId AllocationId;
> 
>   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> 
>   AllocationId Register(size_t size) { return manager_.Register(size); }
> 
>   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> 
>   void* Lock(AllocationId id, bool* purged) {
>     return manager_.Lock(id, purged);
>   }
> 
>   void Unlock(AllocationId id) { manager_.Unlock(id); }
> 
>  private:
>   class Factory : public DiscardableMemoryAllocation::Factory {
>    public:
>     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
>         size_t size) OVERRIDE {
>       return scoped_ptr<DiscardableMemoryAllocation>(
>           new DiscardableMemoryAllocationImpl(size));
>     }
>   };
> 
>   Factory factory_;
>   DiscardableMemoryManager manager_;
> };
> 
> base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
>     LAZY_INSTANCE_INITIALIZER;

[Philippe, 2014-03-24 16:54:15]

On 2014/03/24 16:29:57, reveman wrote:
> On 2014/03/24 09:35:30, Philippe wrote:
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > File base/memory/discardable_memory_emulated.cc (right):
> > 
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > base/memory/discardable_memory_emulated.cc:46: :
> > internal::DiscardableMemoryManager(this,
> > On 2014/03/22 16:49:34, reveman wrote:
> > > On 2014/03/21 12:40:50, Philippe wrote:
> > > > On 2014/03/21 12:32:16, reveman wrote:
> > > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > > I think this part is slightly hairy :) Correct me if I'm wrong
but
> I
> > > > > believe
> > > > > > > > that the vptr is not initialized at this point so if
> > > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
> virtual
> > > call
> > > > > on
> > > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > > undefined.
> > > > > The
> > > > > > > vptr
> > > > > > > > gets initialized between the call to the base class' constructor
> and
> > > the
> > > > > > > > construction of the class' members IIRC.
> > > > > > > 
> > > > > > > Correct.
> > > > > > > 
> > > > > > > > 
> > > > > > > > FWIW I think composition would help here, at least for
readability
> > :)
> > > > > > > 
> > > > > > > I think it's OK to rely on the base class not using this in ctor
but
> > can
> > > > > > change
> > > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function
> to
> > > make
> > > > > > this
> > > > > > > more explicit or initialize the factory prior to the manager if
you
> > feel
> > > > > > > strongly about this.
> > > > > > 
> > > > > > I think we will have to initialize the factory before the manager if
> we
> > > want
> > > > > to
> > > > > > be consistent across platforms. On Android, the allocator is the
> factory
> > > and
> > > > > > there is no way I can make this class extend both
> > DiscardableMemoryManager
> > > > and
> > > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > > 
> > > > > Why not?
> > > > > 
> > > > > > 
> > > > > > If you feel strongly about keeping (single) inheritance here vs
using
> > > > > > composition you can also make DiscardableMemoryManager take
ownership
> of
> > > the
> > > > > > factory. With the current situation (DiscardableMemoryManager taking
a
> > raw
> > > > > > pointer to the factory), I have no choice in the Android
> implementation
> > > > other
> > > > > > than using composition (which I'm happy with FWIW) :)
> > > > > 
> > > > > Sorry, I'm failing to see why. Can you explain?
> > > > 
> > > > Because this would be a direct violation of the Liskov substitution
> > principle
> > > > just like subclassing the factory here already is. A
> > > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > > AshmemFactory). I really think we should avoid such inheritance. I don't
> see
> > > why
> > > > you are opposed to composition here? That said having a separate
> > LazyInstance
> > > > for the Factory is not ideal but it would already be much better than
> > > > subclassing the Factory IMO. What do you think?
> > > 
> > > Correct, DiscardableMemoryManagerImpl is not a Factory as there's no such
> > thing
> > > as "a Factory". There's only the Factory interface that
> > > DiscardableMemoryManagerImpl implements. The use of interfaces throughout
> the
> > > chromium code is far from rare and this is not doing anything that is not
> > > commonly found in existing chromium code.
> > > 
> > > To summarize my current thinking; this patch is well aligned with the rest
> of
> > > the chromium codebase. Passing "this" to the ctor and expecting it not to
be
> > > called is not uncommon. Inheriting an interface implementation might be
> > > confusing. Probably better to use composition if
> DiscardableMemoryManagerImpl
> > > can't implement the interface by itself. Nothing in this patch that
prevents
> > > that though.
> > 
> > Just to be clear and so that the information is present here with the full
> > context, my main concern is not the existence of the Factory interface
> (although
> > I would still prefer to replace it with the use of a Callback) but rather
the
> > unnecessary/unreasonable (IMO) use of inheritance here (for the reasons I
> > mentioned before).
> > 
> > Moreover the Google style guide is also pretty clear:
> > "Do not overuse implementation inheritance. Composition is often more
> > appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
> > subclasses Foo if it can reasonably be said that Bar "is a kind of" Foo."
> 
> I don't think this is a case of overuse.
> 
> > 
> > I don't know what data you are using to say that this is common in the
> Chromium
> > codebase. It might be but I don't think we have ever encouraged/should
> encourage
> > such idioms over composition.
> 
> RenderThreadImpl is one example (content/renderer/render_thread_impl.h). It's
a
> RenderThread implementation that also implements the GpuChannelHostFactory
> interface.
> 
> > 
> > If you are concerned about the instance() method in the version of my patch
> >
>
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
> > FTR), then we can also use composition with delegate methods, as it should
> > really be (although it's slightly more verbose). The code below should be
> safe,
> > correct and simple IMO.
> > 
> > class DiscardableMemoryManagerEmulated {
> >  public:
> >   typedef DiscardableMemoryManager::AllocationId AllocationId;
> > 
> >   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> > 
> >   AllocationId Register(size_t size) { return manager_.Register(size); }
> > 
> >   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> > 
> >   void* Lock(AllocationId id, bool* purged) {
> >     return manager_.Lock(id, purged);
> >   }
> > 
> >   void Unlock(AllocationId id) { manager_.Unlock(id); }
> > 
> >  private:
> >   class Factory : public DiscardableMemoryAllocation::Factory {
> >    public:
> >     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
> >         size_t size) OVERRIDE {
> >       return scoped_ptr<DiscardableMemoryAllocation>(
> >           new DiscardableMemoryAllocationImpl(size));
> >     }
> >   };
> > 
> >   Factory factory_;
> >   DiscardableMemoryManager manager_;
> > };
> > 
> > base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
> >     LAZY_INSTANCE_INITIALIZER;

I do think this is overuse of inheritance. It's up to William to make the final
decision but this does not lgtm to me as it is currently.

[reveman, 2014-03-24 17:06:04]

On 2014/03/24 16:54:15, Philippe wrote:
> On 2014/03/24 16:29:57, reveman wrote:
> > On 2014/03/24 09:35:30, Philippe wrote:
> > >
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > > File base/memory/discardable_memory_emulated.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > > base/memory/discardable_memory_emulated.cc:46: :
> > > internal::DiscardableMemoryManager(this,
> > > On 2014/03/22 16:49:34, reveman wrote:
> > > > On 2014/03/21 12:40:50, Philippe wrote:
> > > > > On 2014/03/21 12:32:16, reveman wrote:
> > > > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > > > I think this part is slightly hairy :) Correct me if I'm wrong
> but
> > I
> > > > > > believe
> > > > > > > > > that the vptr is not initialized at this point so if
> > > > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
> > virtual
> > > > call
> > > > > > on
> > > > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > > > undefined.
> > > > > > The
> > > > > > > > vptr
> > > > > > > > > gets initialized between the call to the base class'
constructor
> > and
> > > > the
> > > > > > > > > construction of the class' members IIRC.
> > > > > > > > 
> > > > > > > > Correct.
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > FWIW I think composition would help here, at least for
> readability
> > > :)
> > > > > > > > 
> > > > > > > > I think it's OK to rely on the base class not using this in ctor
> but
> > > can
> > > > > > > change
> > > > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory
function
> > to
> > > > make
> > > > > > > this
> > > > > > > > more explicit or initialize the factory prior to the manager if
> you
> > > feel
> > > > > > > > strongly about this.
> > > > > > > 
> > > > > > > I think we will have to initialize the factory before the manager
if
> > we
> > > > want
> > > > > > to
> > > > > > > be consistent across platforms. On Android, the allocator is the
> > factory
> > > > and
> > > > > > > there is no way I can make this class extend both
> > > DiscardableMemoryManager
> > > > > and
> > > > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > > > 
> > > > > > Why not?
> > > > > > 
> > > > > > > 
> > > > > > > If you feel strongly about keeping (single) inheritance here vs
> using
> > > > > > > composition you can also make DiscardableMemoryManager take
> ownership
> > of
> > > > the
> > > > > > > factory. With the current situation (DiscardableMemoryManager
taking
> a
> > > raw
> > > > > > > pointer to the factory), I have no choice in the Android
> > implementation
> > > > > other
> > > > > > > than using composition (which I'm happy with FWIW) :)
> > > > > > 
> > > > > > Sorry, I'm failing to see why. Can you explain?
> > > > > 
> > > > > Because this would be a direct violation of the Liskov substitution
> > > principle
> > > > > just like subclassing the factory here already is. A
> > > > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > > > AshmemFactory). I really think we should avoid such inheritance. I
don't
> > see
> > > > why
> > > > > you are opposed to composition here? That said having a separate
> > > LazyInstance
> > > > > for the Factory is not ideal but it would already be much better than
> > > > > subclassing the Factory IMO. What do you think?
> > > > 
> > > > Correct, DiscardableMemoryManagerImpl is not a Factory as there's no
such
> > > thing
> > > > as "a Factory". There's only the Factory interface that
> > > > DiscardableMemoryManagerImpl implements. The use of interfaces
throughout
> > the
> > > > chromium code is far from rare and this is not doing anything that is
not
> > > > commonly found in existing chromium code.
> > > > 
> > > > To summarize my current thinking; this patch is well aligned with the
rest
> > of
> > > > the chromium codebase. Passing "this" to the ctor and expecting it not
to
> be
> > > > called is not uncommon. Inheriting an interface implementation might be
> > > > confusing. Probably better to use composition if
> > DiscardableMemoryManagerImpl
> > > > can't implement the interface by itself. Nothing in this patch that
> prevents
> > > > that though.
> > > 
> > > Just to be clear and so that the information is present here with the full
> > > context, my main concern is not the existence of the Factory interface
> > (although
> > > I would still prefer to replace it with the use of a Callback) but rather
> the
> > > unnecessary/unreasonable (IMO) use of inheritance here (for the reasons I
> > > mentioned before).
> > > 
> > > Moreover the Google style guide is also pretty clear:
> > > "Do not overuse implementation inheritance. Composition is often more
> > > appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
> > > subclasses Foo if it can reasonably be said that Bar "is a kind of" Foo."
> > 
> > I don't think this is a case of overuse.
> > 
> > > 
> > > I don't know what data you are using to say that this is common in the
> > Chromium
> > > codebase. It might be but I don't think we have ever encouraged/should
> > encourage
> > > such idioms over composition.
> > 
> > RenderThreadImpl is one example (content/renderer/render_thread_impl.h).
It's
> a
> > RenderThread implementation that also implements the GpuChannelHostFactory
> > interface.
> > 
> > > 
> > > If you are concerned about the instance() method in the version of my
patch
> > >
> >
>
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
> > > FTR), then we can also use composition with delegate methods, as it should
> > > really be (although it's slightly more verbose). The code below should be
> > safe,
> > > correct and simple IMO.
> > > 
> > > class DiscardableMemoryManagerEmulated {
> > >  public:
> > >   typedef DiscardableMemoryManager::AllocationId AllocationId;
> > > 
> > >   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> > > 
> > >   AllocationId Register(size_t size) { return manager_.Register(size); }
> > > 
> > >   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> > > 
> > >   void* Lock(AllocationId id, bool* purged) {
> > >     return manager_.Lock(id, purged);
> > >   }
> > > 
> > >   void Unlock(AllocationId id) { manager_.Unlock(id); }
> > > 
> > >  private:
> > >   class Factory : public DiscardableMemoryAllocation::Factory {
> > >    public:
> > >     virtual scoped_ptr<DiscardableMemoryAllocation>
CreateLockedAllocation(
> > >         size_t size) OVERRIDE {
> > >       return scoped_ptr<DiscardableMemoryAllocation>(
> > >           new DiscardableMemoryAllocationImpl(size));
> > >     }
> > >   };
> > > 
> > >   Factory factory_;
> > >   DiscardableMemoryManager manager_;
> > > };
> > > 
> > > base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
> > >     LAZY_INSTANCE_INITIALIZER;
> 
> I do think this is overuse of inheritance. It's up to William to make the
final
> decision but this does not lgtm to me as it is currently.

Just to be clear. You're not OK with the DiscardableMemoryManagerImpl in
discardable_memory_emulated.cc implementing the Factory interface, correct?

[Philippe, 2014-03-24 17:14:25]

On 2014/03/24 17:06:04, reveman wrote:
> On 2014/03/24 16:54:15, Philippe wrote:
> > On 2014/03/24 16:29:57, reveman wrote:
> > > On 2014/03/24 09:35:30, Philippe wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > > > File base/memory/discardable_memory_emulated.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
> > > > base/memory/discardable_memory_emulated.cc:46: :
> > > > internal::DiscardableMemoryManager(this,
> > > > On 2014/03/22 16:49:34, reveman wrote:
> > > > > On 2014/03/21 12:40:50, Philippe wrote:
> > > > > > On 2014/03/21 12:32:16, reveman wrote:
> > > > > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > > > > I think this part is slightly hairy :) Correct me if I'm
wrong
> > but
> > > I
> > > > > > > believe
> > > > > > > > > > that the vptr is not initialized at this point so if
> > > > > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
> > > virtual
> > > > > call
> > > > > > > on
> > > > > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > > > > undefined.
> > > > > > > The
> > > > > > > > > vptr
> > > > > > > > > > gets initialized between the call to the base class'
> constructor
> > > and
> > > > > the
> > > > > > > > > > construction of the class' members IIRC.
> > > > > > > > > 
> > > > > > > > > Correct.
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > FWIW I think composition would help here, at least for
> > readability
> > > > :)
> > > > > > > > > 
> > > > > > > > > I think it's OK to rely on the base class not using this in
ctor
> > but
> > > > can
> > > > > > > > change
> > > > > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory
> function
> > > to
> > > > > make
> > > > > > > > this
> > > > > > > > > more explicit or initialize the factory prior to the manager
if
> > you
> > > > feel
> > > > > > > > > strongly about this.
> > > > > > > > 
> > > > > > > > I think we will have to initialize the factory before the
manager
> if
> > > we
> > > > > want
> > > > > > > to
> > > > > > > > be consistent across platforms. On Android, the allocator is the
> > > factory
> > > > > and
> > > > > > > > there is no way I can make this class extend both
> > > > DiscardableMemoryManager
> > > > > > and
> > > > > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > > > > 
> > > > > > > Why not?
> > > > > > > 
> > > > > > > > 
> > > > > > > > If you feel strongly about keeping (single) inheritance here vs
> > using
> > > > > > > > composition you can also make DiscardableMemoryManager take
> > ownership
> > > of
> > > > > the
> > > > > > > > factory. With the current situation (DiscardableMemoryManager
> taking
> > a
> > > > raw
> > > > > > > > pointer to the factory), I have no choice in the Android
> > > implementation
> > > > > > other
> > > > > > > > than using composition (which I'm happy with FWIW) :)
> > > > > > > 
> > > > > > > Sorry, I'm failing to see why. Can you explain?
> > > > > > 
> > > > > > Because this would be a direct violation of the Liskov substitution
> > > > principle
> > > > > > just like subclassing the factory here already is. A
> > > > > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > > > > AshmemFactory). I really think we should avoid such inheritance. I
> don't
> > > see
> > > > > why
> > > > > > you are opposed to composition here? That said having a separate
> > > > LazyInstance
> > > > > > for the Factory is not ideal but it would already be much better
than
> > > > > > subclassing the Factory IMO. What do you think?
> > > > > 
> > > > > Correct, DiscardableMemoryManagerImpl is not a Factory as there's no
> such
> > > > thing
> > > > > as "a Factory". There's only the Factory interface that
> > > > > DiscardableMemoryManagerImpl implements. The use of interfaces
> throughout
> > > the
> > > > > chromium code is far from rare and this is not doing anything that is
> not
> > > > > commonly found in existing chromium code.
> > > > > 
> > > > > To summarize my current thinking; this patch is well aligned with the
> rest
> > > of
> > > > > the chromium codebase. Passing "this" to the ctor and expecting it not
> to
> > be
> > > > > called is not uncommon. Inheriting an interface implementation might
be
> > > > > confusing. Probably better to use composition if
> > > DiscardableMemoryManagerImpl
> > > > > can't implement the interface by itself. Nothing in this patch that
> > prevents
> > > > > that though.
> > > > 
> > > > Just to be clear and so that the information is present here with the
full
> > > > context, my main concern is not the existence of the Factory interface
> > > (although
> > > > I would still prefer to replace it with the use of a Callback) but
rather
> > the
> > > > unnecessary/unreasonable (IMO) use of inheritance here (for the reasons
I
> > > > mentioned before).
> > > > 
> > > > Moreover the Google style guide is also pretty clear:
> > > > "Do not overuse implementation inheritance. Composition is often more
> > > > appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
> > > > subclasses Foo if it can reasonably be said that Bar "is a kind of"
Foo."
> > > 
> > > I don't think this is a case of overuse.
> > > 
> > > > 
> > > > I don't know what data you are using to say that this is common in the
> > > Chromium
> > > > codebase. It might be but I don't think we have ever encouraged/should
> > > encourage
> > > > such idioms over composition.
> > > 
> > > RenderThreadImpl is one example (content/renderer/render_thread_impl.h).
> It's
> > a
> > > RenderThread implementation that also implements the GpuChannelHostFactory
> > > interface.
> > > 
> > > > 
> > > > If you are concerned about the instance() method in the version of my
> patch
> > > >
> > >
> >
>
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
> > > > FTR), then we can also use composition with delegate methods, as it
should
> > > > really be (although it's slightly more verbose). The code below should
be
> > > safe,
> > > > correct and simple IMO.
> > > > 
> > > > class DiscardableMemoryManagerEmulated {
> > > >  public:
> > > >   typedef DiscardableMemoryManager::AllocationId AllocationId;
> > > > 
> > > >   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> > > > 
> > > >   AllocationId Register(size_t size) { return manager_.Register(size); }
> > > > 
> > > >   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> > > > 
> > > >   void* Lock(AllocationId id, bool* purged) {
> > > >     return manager_.Lock(id, purged);
> > > >   }
> > > > 
> > > >   void Unlock(AllocationId id) { manager_.Unlock(id); }
> > > > 
> > > >  private:
> > > >   class Factory : public DiscardableMemoryAllocation::Factory {
> > > >    public:
> > > >     virtual scoped_ptr<DiscardableMemoryAllocation>
> CreateLockedAllocation(
> > > >         size_t size) OVERRIDE {
> > > >       return scoped_ptr<DiscardableMemoryAllocation>(
> > > >           new DiscardableMemoryAllocationImpl(size));
> > > >     }
> > > >   };
> > > > 
> > > >   Factory factory_;
> > > >   DiscardableMemoryManager manager_;
> > > > };
> > > > 
> > > > base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
> > > >     LAZY_INSTANCE_INITIALIZER;
> > 
> > I do think this is overuse of inheritance. It's up to William to make the
> final
> > decision but this does not lgtm to me as it is currently.
> 
> Just to be clear. You're not OK with the DiscardableMemoryManagerImpl in
> discardable_memory_emulated.cc implementing the Factory interface, correct?

Right, I don't think DiscardableMemoryManagerImpl should subclass the Factory
interface and I don't think we need to subclass DiscardableMemoryManager at all
as I wrote in my previous comment with a code snippet showing how we could use
composition. My main concern is about what I will have to do in
discardable_memory_android.cc but you correctly pointed out that we should be
consistent across platforms which makes me worry about what we do here in
emulated DM as well.

[willchan no longer on Chromium, 2014-04-01 01:11:07]

I haven't reviewed the composition vs inheritance discussion in detail, but my
initial reaction is to prefer composition too. And that one concrete class
shouldn't inherit from another. We violate that rule in some cases in Chromium
code and I consider that a bug (unfortunately, it's more common than I would
like, due to historical reasons) and definitely not the majority case.

I also didn't review the test in detail. It looked substantial and I'm leaving
the office, so I wanted to send off these few first comments.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_allocation.h (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_allocation.h:14: class BASE_EXPORT_PRIVATE
DiscardableMemoryAllocation {
Can you comment this class so it's obvious how this relates to the other classes
here? Or having a link to another file with some central comment about the
overall architecture here would be helpful. There are enough layers now that
they keep getting paged out of my head and I have to swap them back in.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:17: class
DiscardableMemoryAllocationImpl
Maybe we should name this DiscardableMemoryHeapAllocation to emphasize what type
of allocation it is? Impl is a bit generic.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/03/24 09:35:31, Philippe wrote:
> On 2014/03/22 16:49:34, reveman wrote:
> > On 2014/03/21 12:40:50, Philippe wrote:
> > > On 2014/03/21 12:32:16, reveman wrote:
> > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > I think this part is slightly hairy :) Correct me if I'm wrong but
I
> > > > believe
> > > > > > > that the vptr is not initialized at this point so if
> > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
virtual
> > call
> > > > on
> > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > undefined.
> > > > The
> > > > > > vptr
> > > > > > > gets initialized between the call to the base class' constructor
and
> > the
> > > > > > > construction of the class' members IIRC.
> > > > > > 
> > > > > > Correct.
> > > > > > 
> > > > > > > 
> > > > > > > FWIW I think composition would help here, at least for readability
> :)
> > > > > > 
> > > > > > I think it's OK to rely on the base class not using this in ctor but
> can
> > > > > change
> > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function
to
> > make
> > > > > this
> > > > > > more explicit or initialize the factory prior to the manager if you
> feel
> > > > > > strongly about this.
> > > > > 
> > > > > I think we will have to initialize the factory before the manager if
we
> > want
> > > > to
> > > > > be consistent across platforms. On Android, the allocator is the
factory
> > and
> > > > > there is no way I can make this class extend both
> DiscardableMemoryManager
> > > and
> > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > 
> > > > Why not?
> > > > 
> > > > > 
> > > > > If you feel strongly about keeping (single) inheritance here vs using
> > > > > composition you can also make DiscardableMemoryManager take ownership
of
> > the
> > > > > factory. With the current situation (DiscardableMemoryManager taking a
> raw
> > > > > pointer to the factory), I have no choice in the Android
implementation
> > > other
> > > > > than using composition (which I'm happy with FWIW) :)
> > > > 
> > > > Sorry, I'm failing to see why. Can you explain?
> > > 
> > > Because this would be a direct violation of the Liskov substitution
> principle
> > > just like subclassing the factory here already is. A
> > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > AshmemFactory). I really think we should avoid such inheritance. I don't
see
> > why
> > > you are opposed to composition here? That said having a separate
> LazyInstance
> > > for the Factory is not ideal but it would already be much better than
> > > subclassing the Factory IMO. What do you think?
> > 
> > Correct, DiscardableMemoryManagerImpl is not a Factory as there's no such
> thing
> > as "a Factory". There's only the Factory interface that
> > DiscardableMemoryManagerImpl implements. The use of interfaces throughout
the
> > chromium code is far from rare and this is not doing anything that is not
> > commonly found in existing chromium code.
> > 
> > To summarize my current thinking; this patch is well aligned with the rest
of
> > the chromium codebase. Passing "this" to the ctor and expecting it not to be
> > called is not uncommon. Inheriting an interface implementation might be
> > confusing. Probably better to use composition if
DiscardableMemoryManagerImpl
> > can't implement the interface by itself. Nothing in this patch that prevents
> > that though.
> 
> Just to be clear and so that the information is present here with the full
> context, my main concern is not the existence of the Factory interface
(although
> I would still prefer to replace it with the use of a Callback) but rather the
> unnecessary/unreasonable (IMO) use of inheritance here (for the reasons I
> mentioned before).
> 
> Moreover the Google style guide is also pretty clear:
> "Do not overuse implementation inheritance. Composition is often more
> appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
> subclasses Foo if it can reasonably be said that Bar "is a kind of" Foo."
> 
> I don't know what data you are using to say that this is common in the
Chromium
> codebase. It might be but I don't think we have ever encouraged/should
encourage
> such idioms over composition.
> 
> If you are concerned about the instance() method in the version of my patch
>
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
> FTR), then we can also use composition with delegate methods, as it should
> really be (although it's slightly more verbose). The code below should be
safe,
> correct and simple IMO.
> 
> class DiscardableMemoryManagerEmulated {
>  public:
>   typedef DiscardableMemoryManager::AllocationId AllocationId;
> 
>   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> 
>   AllocationId Register(size_t size) { return manager_.Register(size); }
> 
>   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> 
>   void* Lock(AllocationId id, bool* purged) {
>     return manager_.Lock(id, purged);
>   }
> 
>   void Unlock(AllocationId id) { manager_.Unlock(id); }
> 
>  private:
>   class Factory : public DiscardableMemoryAllocation::Factory {
>    public:
>     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
>         size_t size) OVERRIDE {
>       return scoped_ptr<DiscardableMemoryAllocation>(
>           new DiscardableMemoryAllocationImpl(size));
>     }
>   };
> 
>   Factory factory_;
>   DiscardableMemoryManager manager_;
> };
> 
> base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
>     LAZY_INSTANCE_INITIALIZER;

I am not fully up to date with Philippe's proposals, but I agree that
implementation inheritance is not desirable. The thing that sticks out to me is
that DiscardableMemoryManagerImpl should not inherit from
DiscardableMemoryManager if DiscardableMemoryManager has a concrete
implementation. Composition is indeed better here IMO.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_manager.h (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:79:
linked_ptr<DiscardableMemoryAllocation> allocation;
So sad that we don't have unique_ptr yet :( linked_ptr is major ugh. Oh well.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:94: mutable base::Lock lock_;
Not really necessary, but who cares I guess.

[reveman, 2014-04-03 17:02:12]

PTAL

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_allocation.h (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_allocation.h:14: class BASE_EXPORT_PRIVATE
DiscardableMemoryAllocation {
On 2014/04/01 01:11:08, willchan wrote:
> Can you comment this class so it's obvious how this relates to the other
classes
> here? Or having a link to another file with some central comment about the
> overall architecture here would be helpful. There are enough layers now that
> they keep getting paged out of my head and I have to swap them back in.

Added a comment here and improved the comments in _manager.h so they better
explain how all this fits together.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:17: class
DiscardableMemoryAllocationImpl
On 2014/04/01 01:11:08, willchan wrote:
> Maybe we should name this DiscardableMemoryHeapAllocation to emphasize what
type
> of allocation it is? Impl is a bit generic.

I added a comment here to emphasize the use of heap memory. I like using the
"Impl" suffix in the anonymous namespace of .cc files. In the case of
discardable_memory_emulated.cc, Type/InterfaceImpl can only be interpreted as
the implementation of Type/Interface for emulated discardable memory. In this
case, it's the implementation of the DiscardableMemoryAllocation type for
emulated discardable memory.

I can change this if you feel strongly about it though.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/04/01 01:11:08, willchan wrote:
> On 2014/03/24 09:35:31, Philippe wrote:
> > On 2014/03/22 16:49:34, reveman wrote:
> > > On 2014/03/21 12:40:50, Philippe wrote:
> > > > On 2014/03/21 12:32:16, reveman wrote:
> > > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > > I think this part is slightly hairy :) Correct me if I'm wrong
but
> I
> > > > > believe
> > > > > > > > that the vptr is not initialized at this point so if
> > > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
> virtual
> > > call
> > > > > on
> > > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > > undefined.
> > > > > The
> > > > > > > vptr
> > > > > > > > gets initialized between the call to the base class' constructor
> and
> > > the
> > > > > > > > construction of the class' members IIRC.
> > > > > > > 
> > > > > > > Correct.
> > > > > > > 
> > > > > > > > 
> > > > > > > > FWIW I think composition would help here, at least for
readability
> > :)
> > > > > > > 
> > > > > > > I think it's OK to rely on the base class not using this in ctor
but
> > can
> > > > > > change
> > > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory function
> to
> > > make
> > > > > > this
> > > > > > > more explicit or initialize the factory prior to the manager if
you
> > feel
> > > > > > > strongly about this.
> > > > > > 
> > > > > > I think we will have to initialize the factory before the manager if
> we
> > > want
> > > > > to
> > > > > > be consistent across platforms. On Android, the allocator is the
> factory
> > > and
> > > > > > there is no way I can make this class extend both
> > DiscardableMemoryManager
> > > > and
> > > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > > 
> > > > > Why not?
> > > > > 
> > > > > > 
> > > > > > If you feel strongly about keeping (single) inheritance here vs
using
> > > > > > composition you can also make DiscardableMemoryManager take
ownership
> of
> > > the
> > > > > > factory. With the current situation (DiscardableMemoryManager taking
a
> > raw
> > > > > > pointer to the factory), I have no choice in the Android
> implementation
> > > > other
> > > > > > than using composition (which I'm happy with FWIW) :)
> > > > > 
> > > > > Sorry, I'm failing to see why. Can you explain?
> > > > 
> > > > Because this would be a direct violation of the Liskov substitution
> > principle
> > > > just like subclassing the factory here already is. A
> > > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > > AshmemFactory). I really think we should avoid such inheritance. I don't
> see
> > > why
> > > > you are opposed to composition here? That said having a separate
> > LazyInstance
> > > > for the Factory is not ideal but it would already be much better than
> > > > subclassing the Factory IMO. What do you think?
> > > 
> > > Correct, DiscardableMemoryManagerImpl is not a Factory as there's no such
> > thing
> > > as "a Factory". There's only the Factory interface that
> > > DiscardableMemoryManagerImpl implements. The use of interfaces throughout
> the
> > > chromium code is far from rare and this is not doing anything that is not
> > > commonly found in existing chromium code.
> > > 
> > > To summarize my current thinking; this patch is well aligned with the rest
> of
> > > the chromium codebase. Passing "this" to the ctor and expecting it not to
be
> > > called is not uncommon. Inheriting an interface implementation might be
> > > confusing. Probably better to use composition if
> DiscardableMemoryManagerImpl
> > > can't implement the interface by itself. Nothing in this patch that
prevents
> > > that though.
> > 
> > Just to be clear and so that the information is present here with the full
> > context, my main concern is not the existence of the Factory interface
> (although
> > I would still prefer to replace it with the use of a Callback) but rather
the
> > unnecessary/unreasonable (IMO) use of inheritance here (for the reasons I
> > mentioned before).
> > 
> > Moreover the Google style guide is also pretty clear:
> > "Do not overuse implementation inheritance. Composition is often more
> > appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
> > subclasses Foo if it can reasonably be said that Bar "is a kind of" Foo."
> > 
> > I don't know what data you are using to say that this is common in the
> Chromium
> > codebase. It might be but I don't think we have ever encouraged/should
> encourage
> > such idioms over composition.
> > 
> > If you are concerned about the instance() method in the version of my patch
> >
>
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
> > FTR), then we can also use composition with delegate methods, as it should
> > really be (although it's slightly more verbose). The code below should be
> safe,
> > correct and simple IMO.
> > 
> > class DiscardableMemoryManagerEmulated {
> >  public:
> >   typedef DiscardableMemoryManager::AllocationId AllocationId;
> > 
> >   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> > 
> >   AllocationId Register(size_t size) { return manager_.Register(size); }
> > 
> >   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> > 
> >   void* Lock(AllocationId id, bool* purged) {
> >     return manager_.Lock(id, purged);
> >   }
> > 
> >   void Unlock(AllocationId id) { manager_.Unlock(id); }
> > 
> >  private:
> >   class Factory : public DiscardableMemoryAllocation::Factory {
> >    public:
> >     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
> >         size_t size) OVERRIDE {
> >       return scoped_ptr<DiscardableMemoryAllocation>(
> >           new DiscardableMemoryAllocationImpl(size));
> >     }
> >   };
> > 
> >   Factory factory_;
> >   DiscardableMemoryManager manager_;
> > };
> > 
> > base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
> >     LAZY_INSTANCE_INITIALIZER;
> 
> I am not fully up to date with Philippe's proposals, but I agree that
> implementation inheritance is not desirable. The thing that sticks out to me
is
> that DiscardableMemoryManagerImpl should not inherit from
> DiscardableMemoryManager if DiscardableMemoryManager has a concrete
> implementation. Composition is indeed better here IMO.

Do we really need to avoid all kinds of concrete class inheritance? Right now
the base class DiscardableMemoryManager, implements the essential parts of the
manager that will be shared across all platforms while this class here extends
it with the memory signal handling we want for emulated discardable memory. I
find this simple and easy to understand and not sure why we'd need to do it
differently unless there's a strict "no concrete class inheritance" policy in
place.

That said, I can change to two different types if that's preferred but we then
need two different names that describe the different roles of these two types
properly. Poorly named classes are worse for readability than some minimal
concrete class inheritance IMO.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_manager.h (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:94: mutable base::Lock lock_;
On 2014/04/01 01:11:08, willchan wrote:
> Not really necessary, but who cares I guess.

Necessary as this class now has a function named "Lock".

[reveman, 2014-04-14 15:27:26]

Let's move this forward. If current patch is not OK, please provide some
specific suggestion for how to improve it or I'd like to move forward with the
current patch.

[Philippe, 2014-04-14 15:35:46]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/04/03 17:02:12, reveman wrote:
> On 2014/04/01 01:11:08, willchan wrote:
> > On 2014/03/24 09:35:31, Philippe wrote:
> > > On 2014/03/22 16:49:34, reveman wrote:
> > > > On 2014/03/21 12:40:50, Philippe wrote:
> > > > > On 2014/03/21 12:32:16, reveman wrote:
> > > > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > > > I think this part is slightly hairy :) Correct me if I'm wrong
> but
> > I
> > > > > > believe
> > > > > > > > > that the vptr is not initialized at this point so if
> > > > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
> > virtual
> > > > call
> > > > > > on
> > > > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > > > undefined.
> > > > > > The
> > > > > > > > vptr
> > > > > > > > > gets initialized between the call to the base class'
constructor
> > and
> > > > the
> > > > > > > > > construction of the class' members IIRC.
> > > > > > > > 
> > > > > > > > Correct.
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > FWIW I think composition would help here, at least for
> readability
> > > :)
> > > > > > > > 
> > > > > > > > I think it's OK to rely on the base class not using this in ctor
> but
> > > can
> > > > > > > change
> > > > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory
function
> > to
> > > > make
> > > > > > > this
> > > > > > > > more explicit or initialize the factory prior to the manager if
> you
> > > feel
> > > > > > > > strongly about this.
> > > > > > > 
> > > > > > > I think we will have to initialize the factory before the manager
if
> > we
> > > > want
> > > > > > to
> > > > > > > be consistent across platforms. On Android, the allocator is the
> > factory
> > > > and
> > > > > > > there is no way I can make this class extend both
> > > DiscardableMemoryManager
> > > > > and
> > > > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > > > 
> > > > > > Why not?
> > > > > > 
> > > > > > > 
> > > > > > > If you feel strongly about keeping (single) inheritance here vs
> using
> > > > > > > composition you can also make DiscardableMemoryManager take
> ownership
> > of
> > > > the
> > > > > > > factory. With the current situation (DiscardableMemoryManager
taking
> a
> > > raw
> > > > > > > pointer to the factory), I have no choice in the Android
> > implementation
> > > > > other
> > > > > > > than using composition (which I'm happy with FWIW) :)
> > > > > > 
> > > > > > Sorry, I'm failing to see why. Can you explain?
> > > > > 
> > > > > Because this would be a direct violation of the Liskov substitution
> > > principle
> > > > > just like subclassing the factory here already is. A
> > > > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > > > AshmemFactory). I really think we should avoid such inheritance. I
don't
> > see
> > > > why
> > > > > you are opposed to composition here? That said having a separate
> > > LazyInstance
> > > > > for the Factory is not ideal but it would already be much better than
> > > > > subclassing the Factory IMO. What do you think?
> > > > 
> > > > Correct, DiscardableMemoryManagerImpl is not a Factory as there's no
such
> > > thing
> > > > as "a Factory". There's only the Factory interface that
> > > > DiscardableMemoryManagerImpl implements. The use of interfaces
throughout
> > the
> > > > chromium code is far from rare and this is not doing anything that is
not
> > > > commonly found in existing chromium code.
> > > > 
> > > > To summarize my current thinking; this patch is well aligned with the
rest
> > of
> > > > the chromium codebase. Passing "this" to the ctor and expecting it not
to
> be
> > > > called is not uncommon. Inheriting an interface implementation might be
> > > > confusing. Probably better to use composition if
> > DiscardableMemoryManagerImpl
> > > > can't implement the interface by itself. Nothing in this patch that
> prevents
> > > > that though.
> > > 
> > > Just to be clear and so that the information is present here with the full
> > > context, my main concern is not the existence of the Factory interface
> > (although
> > > I would still prefer to replace it with the use of a Callback) but rather
> the
> > > unnecessary/unreasonable (IMO) use of inheritance here (for the reasons I
> > > mentioned before).
> > > 
> > > Moreover the Google style guide is also pretty clear:
> > > "Do not overuse implementation inheritance. Composition is often more
> > > appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
> > > subclasses Foo if it can reasonably be said that Bar "is a kind of" Foo."
> > > 
> > > I don't know what data you are using to say that this is common in the
> > Chromium
> > > codebase. It might be but I don't think we have ever encouraged/should
> > encourage
> > > such idioms over composition.
> > > 
> > > If you are concerned about the instance() method in the version of my
patch
> > >
> >
>
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
> > > FTR), then we can also use composition with delegate methods, as it should
> > > really be (although it's slightly more verbose). The code below should be
> > safe,
> > > correct and simple IMO.
> > > 
> > > class DiscardableMemoryManagerEmulated {
> > >  public:
> > >   typedef DiscardableMemoryManager::AllocationId AllocationId;
> > > 
> > >   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> > > 
> > >   AllocationId Register(size_t size) { return manager_.Register(size); }
> > > 
> > >   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> > > 
> > >   void* Lock(AllocationId id, bool* purged) {
> > >     return manager_.Lock(id, purged);
> > >   }
> > > 
> > >   void Unlock(AllocationId id) { manager_.Unlock(id); }
> > > 
> > >  private:
> > >   class Factory : public DiscardableMemoryAllocation::Factory {
> > >    public:
> > >     virtual scoped_ptr<DiscardableMemoryAllocation>
CreateLockedAllocation(
> > >         size_t size) OVERRIDE {
> > >       return scoped_ptr<DiscardableMemoryAllocation>(
> > >           new DiscardableMemoryAllocationImpl(size));
> > >     }
> > >   };
> > > 
> > >   Factory factory_;
> > >   DiscardableMemoryManager manager_;
> > > };
> > > 
> > > base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
> > >     LAZY_INSTANCE_INITIALIZER;
> > 
> > I am not fully up to date with Philippe's proposals, but I agree that
> > implementation inheritance is not desirable. The thing that sticks out to me
> is
> > that DiscardableMemoryManagerImpl should not inherit from
> > DiscardableMemoryManager if DiscardableMemoryManager has a concrete
> > implementation. Composition is indeed better here IMO.
> 
> Do we really need to avoid all kinds of concrete class inheritance? Right now
> the base class DiscardableMemoryManager, implements the essential parts of the
> manager that will be shared across all platforms while this class here extends
> it with the memory signal handling we want for emulated discardable memory. I
> find this simple and easy to understand and not sure why we'd need to do it
> differently unless there's a strict "no concrete class inheritance" policy in
> place.
> 
> That said, I can change to two different types if that's preferred but we then
> need two different names that describe the different roles of these two types
> properly. Poorly named classes are worse for readability than some minimal
> concrete class inheritance IMO.

I agree, let's move this forward :) I think I have already shared my opinion
here but my favorite alternative FWIW would be the one in my previous comment
which I'm pasting here:

class DiscardableMemoryManagerEmulated {
 public:
  typedef DiscardableMemoryManager::AllocationId AllocationId;

  DiscardableMemoryManagerEmulated() : manager_(&factory_) {}

  AllocationId Register(size_t size) { return manager_.Register(size); }

  void Unregister(AllocationId id) { manager_.Unregister(id); }  

  void* Lock(AllocationId id, bool* purged) {
    return manager_.Lock(id, purged);
  }

  void Unlock(AllocationId id) { manager_.Unlock(id); }

 private:
  class Factory : public DiscardableMemoryAllocation::Factory {
   public:
    virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
        size_t size) OVERRIDE {
      return scoped_ptr<DiscardableMemoryAllocation>(
          new DiscardableMemoryAllocationImpl(size));
    }
  };

  Factory factory_;
  DiscardableMemoryManager manager_;
};

base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
    LAZY_INSTANCE_INITIALIZER;

Replacing the factory with a callback would be even better IMO but I would
already be OK with the factory if you still feel strongly about it.

[reveman, 2014-04-14 16:24:22]

On 2014/04/14 15:35:46, Philippe wrote:
> 
> I agree, let's move this forward :) I think I have already shared my opinion
> here but my favorite alternative FWIW would be the one in my previous comment
> which I'm pasting here:
> 
> class DiscardableMemoryManagerEmulated {

I'm ok with this if you can come up with a good name that describes the role and
purpose of this class. "Manager" is already taken by DiscardableMemoryManager
and having two classes that are not related but have the same naming is not OK
IMO.

>  public:
>   typedef DiscardableMemoryManager::AllocationId AllocationId;
> 
>   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> 
>   AllocationId Register(size_t size) { return manager_.Register(size); }
> 
>   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> 
>   void* Lock(AllocationId id, bool* purged) {
>     return manager_.Lock(id, purged);
>   }
> 
>   void Unlock(AllocationId id) { manager_.Unlock(id); }
> 
>  private:
>   class Factory : public DiscardableMemoryAllocation::Factory {
>    public:
>     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
>         size_t size) OVERRIDE {
>       return scoped_ptr<DiscardableMemoryAllocation>(
>           new DiscardableMemoryAllocationImpl(size));
>     }
>   };
> 
>   Factory factory_;
>   DiscardableMemoryManager manager_;
> };

We can also settle in the middle if you're OK with:

class DiscardableMemoryManagerImpl : public DiscardableMemoryManager {
 public:
  DiscardableMemoryManagerImpl()
      : DiscardableMemoryManager(&allocation_factory_) {}

 private:
  DiscardableMemoryAllocationImpl::Factory allocation_factory_;
};

The manager is still a manager but it doesn't implement the factory interface
anymore.

[Philippe, 2014-04-15 13:31:23]

On 2014/04/14 16:24:22, reveman wrote:
> On 2014/04/14 15:35:46, Philippe wrote:
> > 
> > I agree, let's move this forward :) I think I have already shared my opinion
> > here but my favorite alternative FWIW would be the one in my previous
comment
> > which I'm pasting here:
> > 
> > class DiscardableMemoryManagerEmulated {
> 
> I'm ok with this if you can come up with a good name that describes the role
and
> purpose of this class. "Manager" is already taken by DiscardableMemoryManager
> and having two classes that are not related but have the same naming is not OK
> IMO.

DiscardableMemoryManagerEmulated is not a bad name IMO. The thing is that they
are precisely related. Relationship is not restricted to "is-a" and "has-a" is
in fact a perfectly valid (and preferable) one as well :) A class' interface is
not limited to its parent class(es). The methods it exposes (through inheritance
or not) are also a significant part of its interface. Structural typing (that
can be more or less achieved with templates) actually completely relies on the
operations that are exposed by classes as opposed to some "tags".

> >  public:
> >   typedef DiscardableMemoryManager::AllocationId AllocationId;
> > 
> >   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> > 
> >   AllocationId Register(size_t size) { return manager_.Register(size); }
> > 
> >   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> > 
> >   void* Lock(AllocationId id, bool* purged) {
> >     return manager_.Lock(id, purged);
> >   }
> > 
> >   void Unlock(AllocationId id) { manager_.Unlock(id); }
> > 
> >  private:
> >   class Factory : public DiscardableMemoryAllocation::Factory {
> >    public:
> >     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
> >         size_t size) OVERRIDE {
> >       return scoped_ptr<DiscardableMemoryAllocation>(
> >           new DiscardableMemoryAllocationImpl(size));
> >     }
> >   };
> > 
> >   Factory factory_;
> >   DiscardableMemoryManager manager_;
> > };
> 
> We can also settle in the middle if you're OK with:
> 
> class DiscardableMemoryManagerImpl : public DiscardableMemoryManager {
>  public:
>   DiscardableMemoryManagerImpl()
>       : DiscardableMemoryManager(&allocation_factory_) {}
> 
>  private:
>   DiscardableMemoryAllocationImpl::Factory allocation_factory_;
> };
> 
> The manager is still a manager but it doesn't implement the factory interface
> anymore.

Removing the factory inheritance certainly helps but I still don't think you
need inheritance at all because:
- You are not overriding any method in DiscardableMemoryManager
- You are never passing a DiscardableMemoryManagerImpl instance as a
DiscardableMemoryManager (i.e. not leveraging polymorphism).

[willchan no longer on Chromium, 2014-04-15 20:51:31]

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:17: class
DiscardableMemoryAllocationImpl
On 2014/04/03 17:02:12, reveman wrote:
> On 2014/04/01 01:11:08, willchan wrote:
> > Maybe we should name this DiscardableMemoryHeapAllocation to emphasize what
> type
> > of allocation it is? Impl is a bit generic.
> 
> I added a comment here to emphasize the use of heap memory. I like using the
> "Impl" suffix in the anonymous namespace of .cc files. In the case of
> discardable_memory_emulated.cc, Type/InterfaceImpl can only be interpreted as
> the implementation of Type/Interface for emulated discardable memory. In this
> case, it's the implementation of the DiscardableMemoryAllocation type for
> emulated discardable memory.
> 
> I can change this if you feel strongly about it though.

I will defer to you here. Please take my comment as purely advisory. I suspect
that it's more obvious to you since you understand the code better that
"emulated" implies emulation using the heap. As an outside reader, it's less
obvious that this is the case, although when I think about it more, it does seem
like the logical conclusion. But I didn't want to have to think about it.

https://codereview.chromium.org/204733003/diff/40001/base/memory/discardable_...
base/memory/discardable_memory_emulated.cc:46: :
internal::DiscardableMemoryManager(this,
On 2014/04/14 15:35:47, Philippe wrote:
> On 2014/04/03 17:02:12, reveman wrote:
> > On 2014/04/01 01:11:08, willchan wrote:
> > > On 2014/03/24 09:35:31, Philippe wrote:
> > > > On 2014/03/22 16:49:34, reveman wrote:
> > > > > On 2014/03/21 12:40:50, Philippe wrote:
> > > > > > On 2014/03/21 12:32:16, reveman wrote:
> > > > > > > On 2014/03/21 09:13:09, Philippe wrote:
> > > > > > > > On 2014/03/20 19:08:22, reveman wrote:
> > > > > > > > > On 2014/03/20 18:08:31, Philippe wrote:
> > > > > > > > > > I think this part is slightly hairy :) Correct me if I'm
wrong
> > but
> > > I
> > > > > > > believe
> > > > > > > > > > that the vptr is not initialized at this point so if
> > > > > > > > > > DiscardableMemoryManager::DiscardableMemoryManager() does a
> > > virtual
> > > > > call
> > > > > > > on
> > > > > > > > > > |this| (e.g. CreateLockedAllocation()) then the behavior is 
> > > > > undefined.
> > > > > > > The
> > > > > > > > > vptr
> > > > > > > > > > gets initialized between the call to the base class'
> constructor
> > > and
> > > > > the
> > > > > > > > > > construction of the class' members IIRC.
> > > > > > > > > 
> > > > > > > > > Correct.
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > FWIW I think composition would help here, at least for
> > readability
> > > > :)
> > > > > > > > > 
> > > > > > > > > I think it's OK to rely on the base class not using this in
ctor
> > but
> > > > can
> > > > > > > > change
> > > > > > > > > to a virtual DiscardableMemoryManager::CreateLockedMemory
> function
> > > to
> > > > > make
> > > > > > > > this
> > > > > > > > > more explicit or initialize the factory prior to the manager
if
> > you
> > > > feel
> > > > > > > > > strongly about this.
> > > > > > > > 
> > > > > > > > I think we will have to initialize the factory before the
manager
> if
> > > we
> > > > > want
> > > > > > > to
> > > > > > > > be consistent across platforms. On Android, the allocator is the
> > > factory
> > > > > and
> > > > > > > > there is no way I can make this class extend both
> > > > DiscardableMemoryManager
> > > > > > and
> > > > > > > > DiscardableMemoryAllocationFactoryAshmem :)
> > > > > > > 
> > > > > > > Why not?
> > > > > > > 
> > > > > > > > 
> > > > > > > > If you feel strongly about keeping (single) inheritance here vs
> > using
> > > > > > > > composition you can also make DiscardableMemoryManager take
> > ownership
> > > of
> > > > > the
> > > > > > > > factory. With the current situation (DiscardableMemoryManager
> taking
> > a
> > > > raw
> > > > > > > > pointer to the factory), I have no choice in the Android
> > > implementation
> > > > > > other
> > > > > > > > than using composition (which I'm happy with FWIW) :)
> > > > > > > 
> > > > > > > Sorry, I'm failing to see why. Can you explain?
> > > > > > 
> > > > > > Because this would be a direct violation of the Liskov substitution
> > > > principle
> > > > > > just like subclassing the factory here already is. A
> > > > > > DiscardableMemoryManagerImpl is not a Factory (and it's even less an
> > > > > > AshmemFactory). I really think we should avoid such inheritance. I
> don't
> > > see
> > > > > why
> > > > > > you are opposed to composition here? That said having a separate
> > > > LazyInstance
> > > > > > for the Factory is not ideal but it would already be much better
than
> > > > > > subclassing the Factory IMO. What do you think?
> > > > > 
> > > > > Correct, DiscardableMemoryManagerImpl is not a Factory as there's no
> such
> > > > thing
> > > > > as "a Factory". There's only the Factory interface that
> > > > > DiscardableMemoryManagerImpl implements. The use of interfaces
> throughout
> > > the
> > > > > chromium code is far from rare and this is not doing anything that is
> not
> > > > > commonly found in existing chromium code.
> > > > > 
> > > > > To summarize my current thinking; this patch is well aligned with the
> rest
> > > of
> > > > > the chromium codebase. Passing "this" to the ctor and expecting it not
> to
> > be
> > > > > called is not uncommon. Inheriting an interface implementation might
be
> > > > > confusing. Probably better to use composition if
> > > DiscardableMemoryManagerImpl
> > > > > can't implement the interface by itself. Nothing in this patch that
> > prevents
> > > > > that though.
> > > > 
> > > > Just to be clear and so that the information is present here with the
full
> > > > context, my main concern is not the existence of the Factory interface
> > > (although
> > > > I would still prefer to replace it with the use of a Callback) but
rather
> > the
> > > > unnecessary/unreasonable (IMO) use of inheritance here (for the reasons
I
> > > > mentioned before).
> > > > 
> > > > Moreover the Google style guide is also pretty clear:
> > > > "Do not overuse implementation inheritance. Composition is often more
> > > > appropriate. Try to restrict use of inheritance to the "is-a" case: Bar
> > > > subclasses Foo if it can reasonably be said that Bar "is a kind of"
Foo."
> > > > 
> > > > I don't know what data you are using to say that this is common in the
> > > Chromium
> > > > codebase. It might be but I don't think we have ever encouraged/should
> > > encourage
> > > > such idioms over composition.
> > > > 
> > > > If you are concerned about the instance() method in the version of my
> patch
> > > >
> > >
> >
>
(https://codereview.chromium.org/195863005/diff/120001/base/memory/discardable...
> > > > FTR), then we can also use composition with delegate methods, as it
should
> > > > really be (although it's slightly more verbose). The code below should
be
> > > safe,
> > > > correct and simple IMO.
> > > > 
> > > > class DiscardableMemoryManagerEmulated {
> > > >  public:
> > > >   typedef DiscardableMemoryManager::AllocationId AllocationId;
> > > > 
> > > >   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> > > > 
> > > >   AllocationId Register(size_t size) { return manager_.Register(size); }
> > > > 
> > > >   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> > > > 
> > > >   void* Lock(AllocationId id, bool* purged) {
> > > >     return manager_.Lock(id, purged);
> > > >   }
> > > > 
> > > >   void Unlock(AllocationId id) { manager_.Unlock(id); }
> > > > 
> > > >  private:
> > > >   class Factory : public DiscardableMemoryAllocation::Factory {
> > > >    public:
> > > >     virtual scoped_ptr<DiscardableMemoryAllocation>
> CreateLockedAllocation(
> > > >         size_t size) OVERRIDE {
> > > >       return scoped_ptr<DiscardableMemoryAllocation>(
> > > >           new DiscardableMemoryAllocationImpl(size));
> > > >     }
> > > >   };
> > > > 
> > > >   Factory factory_;
> > > >   DiscardableMemoryManager manager_;
> > > > };
> > > > 
> > > > base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
> > > >     LAZY_INSTANCE_INITIALIZER;
> > > 
> > > I am not fully up to date with Philippe's proposals, but I agree that
> > > implementation inheritance is not desirable. The thing that sticks out to
me
> > is
> > > that DiscardableMemoryManagerImpl should not inherit from
> > > DiscardableMemoryManager if DiscardableMemoryManager has a concrete
> > > implementation. Composition is indeed better here IMO.
> > 
> > Do we really need to avoid all kinds of concrete class inheritance? Right
now
> > the base class DiscardableMemoryManager, implements the essential parts of
the
> > manager that will be shared across all platforms while this class here
extends
> > it with the memory signal handling we want for emulated discardable memory.
I
> > find this simple and easy to understand and not sure why we'd need to do it
> > differently unless there's a strict "no concrete class inheritance" policy
in
> > place.

It's not strictly disallowed, but it's strongly discouraged in the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inheritance ("For
implementation inheritance, because the code implementing a sub-class is spread
between the base and the sub-class, it can be more difficult to understand an
implementation. The sub-class cannot override functions that are not virtual, so
the sub-class cannot change implementation. The base class may also define some
data members, so that specifies physical layout of the base class.") It also
suffers from the Fragile Base Class problem
(http://en.wikipedia.org/wiki/Fragile_base_class).

> > 
> > That said, I can change to two different types if that's preferred but we
then
> > need two different names that describe the different roles of these two
types
> > properly. Poorly named classes are worse for readability than some minimal
> > concrete class inheritance IMO.
> 
> I agree, let's move this forward :) I think I have already shared my opinion
> here but my favorite alternative FWIW would be the one in my previous comment
> which I'm pasting here:

I am in favor of composition of some form. Philippe's suggestion here seems
reasonable to me. I prefer his solution of defining a private type that
implements the appropriate interface, although sometimes others will choose not
to do this since it does admittedly add some boilerplate. I am more of a
stickler for doing this if the type will be publicly exposed in a header file,
but within a .cc file, I'd be OK avoiding this if that's what you prefer in
order to reduce the need for an extra type. That said, I am more hardline about
not doing implementation inheritance. Composing the DiscardableMemoryManager
instead does feel like the right solution to me.

> 
> class DiscardableMemoryManagerEmulated {
>  public:
>   typedef DiscardableMemoryManager::AllocationId AllocationId;
> 
>   DiscardableMemoryManagerEmulated() : manager_(&factory_) {}
> 
>   AllocationId Register(size_t size) { return manager_.Register(size); }
> 
>   void Unregister(AllocationId id) { manager_.Unregister(id); }  
> 
>   void* Lock(AllocationId id, bool* purged) {
>     return manager_.Lock(id, purged);
>   }
> 
>   void Unlock(AllocationId id) { manager_.Unlock(id); }
> 
>  private:
>   class Factory : public DiscardableMemoryAllocation::Factory {
>    public:
>     virtual scoped_ptr<DiscardableMemoryAllocation> CreateLockedAllocation(
>         size_t size) OVERRIDE {
>       return scoped_ptr<DiscardableMemoryAllocation>(
>           new DiscardableMemoryAllocationImpl(size));
>     }
>   };
> 
>   Factory factory_;
>   DiscardableMemoryManager manager_;
> };
> 
> base::LazyInstance<DiscardableMemoryManagerEmulated> g_manager =
>     LAZY_INSTANCE_INITIALIZER;
> 
> Replacing the factory with a callback would be even better IMO but I would
> already be OK with the factory if you still feel strongly about it.

[reveman, 2014-04-21 16:46:40]

Limiting this to only what we need to allow different type of allocations. We
can do platform specific adjustments to the manager class as a follow up instead
if necessary.

The previous design seemed more complicated than necessary and after taking a
step back and looking at what's needed I think I've managed to re-design this in
a way that's significantly less complicated.

Only one new interface is introduced in latest patch,
DiscardableMemoryManagerAllocation. This is all that is needed by
DiscardableMemoryManager class to manage different type of allocations. No
factory required and less constraints on the client.

PTAL.

[willchan no longer on Chromium, 2014-04-21 19:56:03]

I skimmed this and the design seems reasonable to me. Philippe, mind doing the
detailed review?

[Philippe, 2014-04-22 09:48:11]

With this new patch set you are adding some complexity to the way the manager
interacts with the ashmem allocator. I'm OK with this though if implementing
DiscardableMemoryManagerAllocation in DiscardableMemoryAshmem (as opposed to the
allocator) sounds good to you as well (see my comment).

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager.h (right):

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:32: virtual void Purge() = 0;
This does add some complexity to the clients that implement the interface. An
allocation instance now has three states: unlocked, locked and purged which
prevents the underlying implementation from being immutable in particular.
Working around this issue on Android would imply adding an extra layer.
Fortunately we are already planning to have this extra layer as
DiscardableMemoryAshmem. 

If you want to keep this interface as opposed to the original design we agreed
on then I think it should be implemented on Android at the
DiscardableMemoryAshmem layer as opposed to the allocator, i.e. the allocator
would keep returning instances of DiscardableMemory (or an internal type, see
below) and it would be up to DiscardableMemoryAshmem to implement the
DiscardableMemoryManagerAllocation interface by using the allocator-provided
DiscardableMemory instance. DiscardableMemoryAshmem is going to be the layer
that deals with the manager anyway so it makes more sense to have it directly
implement the DiscardableMemoryManagerAllocation. 
The allocator could also return instances of an internal class that would mimic
the DiscardableMemory interface as opposed to directly returning
DiscardableMemory instances.

[reveman, 2014-04-22 16:33:49]

Philippe, thanks for having a look. If you're OK with current patch let's move
forward and we can always make adjustments in a follow up patch if needed by
ashmem.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager.h (right):

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:32: virtual void Purge() = 0;
On 2014/04/22 09:48:11, Philippe wrote:
> This does add some complexity to the clients that implement the interface. An
> allocation instance now has three states: unlocked, locked and purged which
> prevents the underlying implementation from being immutable in particular.

We basically had this in the previous design too but in a more obscure way as
the usage looked like this:

id = Register();  // create an allocation in purged state
Lock(id);  // enter locked state (create a real "Allocation" instance)
Unlock(id);  // enter unlocked state
delete allocation_instances[id];  // enter purged state (delete "Allocation"
instance, id is still a valid allocation id)

so no more states are introduced imo. It just becomes more explicit what the
states are and the client is not forced to be able to create heap allocated
"Allocation" instances to use the API.

> Working around this issue on Android would imply adding an extra layer.
> Fortunately we are already planning to have this extra layer as
> DiscardableMemoryAshmem. 
> 
> If you want to keep this interface as opposed to the original design we agreed
> on then I think it should be implemented on Android at the
> DiscardableMemoryAshmem layer as opposed to the allocator, i.e. the allocator
> would keep returning instances of DiscardableMemory (or an internal type, see
> below) and it would be up to DiscardableMemoryAshmem to implement the
> DiscardableMemoryManagerAllocation interface by using the allocator-provided
> DiscardableMemory instance. DiscardableMemoryAshmem is going to be the layer
> that deals with the manager anyway so it makes more sense to have it directly
> implement the DiscardableMemoryManagerAllocation. 
> The allocator could also return instances of an internal class that would
mimic
> the DiscardableMemory interface as opposed to directly returning
> DiscardableMemory instances.

I'd like to focus on what makes sense from the manager's point of view first and
make sure that's as simple and as easy to understand as possible. The current
patch gives a lot of flexibility to the client. DiscardableMemoryAshmem could
implement the Allocation interface or we can use a separate instance to
implement it if that's more appropriate. I think that discussion is better left
to the follow up patch that makes our ashmem implementation use the manager
though.

[Philippe, 2014-04-22 16:50:08]

On 2014/04/22 16:33:49, reveman wrote:
> Philippe, thanks for having a look. If you're OK with current patch let's move
> forward and we can always make adjustments in a follow up patch if needed by
> ashmem.
> 
>
https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
> File base/memory/discardable_memory_manager.h (right):
> 
>
https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
> base/memory/discardable_memory_manager.h:32: virtual void Purge() = 0;
> On 2014/04/22 09:48:11, Philippe wrote:
> > This does add some complexity to the clients that implement the interface.
An
> > allocation instance now has three states: unlocked, locked and purged which
> > prevents the underlying implementation from being immutable in particular.
> 
> We basically had this in the previous design too but in a more obscure way as
> the usage looked like this:
> 
> id = Register();  // create an allocation in purged state
> Lock(id);  // enter locked state (create a real "Allocation" instance)
> Unlock(id);  // enter unlocked state
> delete allocation_instances[id];  // enter purged state (delete "Allocation"
> instance, id is still a valid allocation id)
> 
> so no more states are introduced imo. It just becomes more explicit what the
> states are and the client is not forced to be able to create heap allocated
> "Allocation" instances to use the API.
> 
> > Working around this issue on Android would imply adding an extra layer.
> > Fortunately we are already planning to have this extra layer as
> > DiscardableMemoryAshmem. 
> > 
> > If you want to keep this interface as opposed to the original design we
agreed
> > on then I think it should be implemented on Android at the
> > DiscardableMemoryAshmem layer as opposed to the allocator, i.e. the
allocator
> > would keep returning instances of DiscardableMemory (or an internal type,
see
> > below) and it would be up to DiscardableMemoryAshmem to implement the
> > DiscardableMemoryManagerAllocation interface by using the allocator-provided
> > DiscardableMemory instance. DiscardableMemoryAshmem is going to be the layer
> > that deals with the manager anyway so it makes more sense to have it
directly
> > implement the DiscardableMemoryManagerAllocation. 
> > The allocator could also return instances of an internal class that would
> mimic
> > the DiscardableMemory interface as opposed to directly returning
> > DiscardableMemory instances.
> 
> I'd like to focus on what makes sense from the manager's point of view first
and
> make sure that's as simple and as easy to understand as possible. The current
> patch gives a lot of flexibility to the client. DiscardableMemoryAshmem could
> implement the Allocation interface or we can use a separate instance to
> implement it if that's more appropriate. I think that discussion is better
left
> to the follow up patch that makes our ashmem implementation use the manager
> though.

I just want to make sure that the current CL is compatible with the long term
plans since its purpose really is to prepare the ground for the ashmem
allocator/manager integration. If implementing the
DiscardableMemoryManagerAllocation interface in DiscardableMemoryAshmem (again,
as opposed to the allocator) makes sense to you then I'm fine with this CL. I
will now proceed with a more detailed review.

[reveman, 2014-04-22 18:27:08]

On 2014/04/22 16:50:08, Philippe wrote:
> On 2014/04/22 16:33:49, reveman wrote:
> > Philippe, thanks for having a look. If you're OK with current patch let's
move
> > forward and we can always make adjustments in a follow up patch if needed by
> > ashmem.
> > 
> >
>
https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
> > File base/memory/discardable_memory_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
> > base/memory/discardable_memory_manager.h:32: virtual void Purge() = 0;
> > On 2014/04/22 09:48:11, Philippe wrote:
> > > This does add some complexity to the clients that implement the interface.
> An
> > > allocation instance now has three states: unlocked, locked and purged
which
> > > prevents the underlying implementation from being immutable in particular.
> > 
> > We basically had this in the previous design too but in a more obscure way
as
> > the usage looked like this:
> > 
> > id = Register();  // create an allocation in purged state
> > Lock(id);  // enter locked state (create a real "Allocation" instance)
> > Unlock(id);  // enter unlocked state
> > delete allocation_instances[id];  // enter purged state (delete "Allocation"
> > instance, id is still a valid allocation id)
> > 
> > so no more states are introduced imo. It just becomes more explicit what the
> > states are and the client is not forced to be able to create heap allocated
> > "Allocation" instances to use the API.
> > 
> > > Working around this issue on Android would imply adding an extra layer.
> > > Fortunately we are already planning to have this extra layer as
> > > DiscardableMemoryAshmem. 
> > > 
> > > If you want to keep this interface as opposed to the original design we
> agreed
> > > on then I think it should be implemented on Android at the
> > > DiscardableMemoryAshmem layer as opposed to the allocator, i.e. the
> allocator
> > > would keep returning instances of DiscardableMemory (or an internal type,
> see
> > > below) and it would be up to DiscardableMemoryAshmem to implement the
> > > DiscardableMemoryManagerAllocation interface by using the
allocator-provided
> > > DiscardableMemory instance. DiscardableMemoryAshmem is going to be the
layer
> > > that deals with the manager anyway so it makes more sense to have it
> directly
> > > implement the DiscardableMemoryManagerAllocation. 
> > > The allocator could also return instances of an internal class that would
> > mimic
> > > the DiscardableMemory interface as opposed to directly returning
> > > DiscardableMemory instances.
> > 
> > I'd like to focus on what makes sense from the manager's point of view first
> and
> > make sure that's as simple and as easy to understand as possible. The
current
> > patch gives a lot of flexibility to the client. DiscardableMemoryAshmem
could
> > implement the Allocation interface or we can use a separate instance to
> > implement it if that's more appropriate. I think that discussion is better
> left
> > to the follow up patch that makes our ashmem implementation use the manager
> > though.
> 
> I just want to make sure that the current CL is compatible with the long term
> plans since its purpose really is to prepare the ground for the ashmem
> allocator/manager integration. If implementing the
> DiscardableMemoryManagerAllocation interface in DiscardableMemoryAshmem
(again,
> as opposed to the allocator) makes sense to you then I'm fine with this CL. I
> will now proceed with a more detailed review.

DiscardableMemoryAshmem implementing the Allocation interface sgtm.

[Philippe, 2014-04-23 11:34:05]

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager.cc (right):

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:80: AllocationInfo& info = it->second;
Nit: it seems that this could be a const reference.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:98: AllocationInfo& info = it->second;
Nit: I would also make this a const reference and would rewrite line 119 as:
AllocationInfo* mutable_info = &it->second;
mutable_info->bytes_purgeable = 0U;

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:133: AllocationInfo& info =
it->second;
Nit: I would use a pointer here to make the state mutation on line 136 more
explicit (as encouraged by the style guide).

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:200: AllocationInfo& info =
it->second;
Nit: same here as line 133.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager.h (right):

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:122: size_t bytes;
This is/should be marked as const, right?

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:123: size_t bytes_purgable;
|bytes_purgeable| is either equal to 0 or |bytes|, right? A bool or enum state
would be clearer IMO.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager_unittest.cc (right):

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager_unittest.cc:48: : manager_(new
internal::DiscardableMemoryManager) {
Nit: I know that this is pre-existing but it seems that |manager_| doesn't need
to be heap-allocated since it's never reset AFAICT during the lifetime of the
TestBase instance.

You are touching most of the lines referencing |manager_| here so I thought it
could be a good opportunity to make this change :)

[reveman, 2014-04-23 18:51:26]

Thanks for the review. PTAL.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager.cc (right):

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:80: AllocationInfo& info = it->second;
On 2014/04/23 11:34:05, Philippe wrote:
> Nit: it seems that this could be a const reference.

Done.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:98: AllocationInfo& info = it->second;
On 2014/04/23 11:34:05, Philippe wrote:
> Nit: I would also make this a const reference and would rewrite line 119 as:
> AllocationInfo* mutable_info = &it->second;
> mutable_info->bytes_purgeable = 0U;

Made this "AllocationInfo* info" instead of adding a mutable_info variable.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:133: AllocationInfo& info =
it->second;
On 2014/04/23 11:34:05, Philippe wrote:
> Nit: I would use a pointer here to make the state mutation on line 136 more
> explicit (as encouraged by the style guide).

Good idea. I made this "AllocationInfo* info = &it->second;" everywhere it can't
be const.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:200: AllocationInfo& info =
it->second;
On 2014/04/23 11:34:05, Philippe wrote:
> Nit: same here as line 133.

Done.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager.h (right):

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:122: size_t bytes;
On 2014/04/23 11:34:05, Philippe wrote:
> This is/should be marked as const, right?

Done.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:123: size_t bytes_purgable;
On 2014/04/23 11:34:05, Philippe wrote:
> |bytes_purgeable| is either equal to 0 or |bytes|, right? A bool or enum state
> would be clearer IMO.

Done.

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager_unittest.cc (right):

https://codereview.chromium.org/204733003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager_unittest.cc:48: : manager_(new
internal::DiscardableMemoryManager) {
On 2014/04/23 11:34:05, Philippe wrote:
> Nit: I know that this is pre-existing but it seems that |manager_| doesn't
need
> to be heap-allocated since it's never reset AFAICT during the lifetime of the
> TestBase instance.
> 
> You are touching most of the lines referencing |manager_| here so I thought it
> could be a good opportunity to make this change :)

Good idea, done.

[Philippe, 2014-04-24 08:05:40]

LGTM, thanks David! I will update my CL now.

[willchan no longer on Chromium, 2014-04-24 20:02:18]

lgtm

I unfortunately didn't have time to more than skim the tests, but I'm trusting
that Philippe thoroughly reviewed that. I did read all the .cc files and it
looks great.

[reveman, 2014-04-24 20:19:48]

On 2014/04/24 20:02:18, willchan wrote:
> lgtm
> 
> I unfortunately didn't have time to more than skim the tests, but I'm trusting
> that Philippe thoroughly reviewed that. I did read all the .cc files and it
> looks great.

Thanks! FYI, tests should be the same except
UnlockedMemoryAccessCrashesInDebugMode which I had remove as it relied on
allocations being backed by heap allocated memory. Our higher level discardable
memory unit tests should cover that case though.

[reveman, 2014-04-24 20:20:04]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/204733003/100001

[commit-bot: I haz the power, 2014-04-24 22:39:50]

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

[commit-bot: I haz the power, 2014-04-24 22:39:51]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[reveman, 2014-04-25 01:23:36]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-25 01:35:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/204733003/100001

[commit-bot: I haz the power, 2014-04-25 09:26:34]

Change committed as 266174