Refactor DiscardableMemory for the upcoming DiscardableMemoryAllocator.

Refactor DiscardableMemory for the upcoming DiscardableMemoryAllocator.

DiscardableMemory has the current limitation on Android that it's file-based
thus uses file descriptor thus is a limited resource. This will be worked
around in a future CL by adding DiscardMemoryAllocator operating on large
pieces of discardable memory (thus using less file descriptors) and allowing
the clients to operate on specific chunks.

This CL prepares this addition by refactoring the existing DiscardableMemory
implementation.
It makes it a pure interface to allow the upcoming DiscardableMemoryAllocator
to create instances of its own implementation of DiscardableMemory. This
implementation won't be backed by files, only the allocator will.

This also fixes some issues with the previous interface/implementation:                                                                                                                                                                                                                                                        
- The fact that initialization happened through an Init() method which must be
  called only once. The only benefit of this design is to allow objects to be
  constructed on the stack. In practice all the current clients are allocating
  instances in the heap anyway (although not doing so would be a simple
  change). This also made the interface hard to use correctly but easy to use 
  incorrectly. Implementation also systematically had to check that the object
  was correctly initialized.
- The fact that unlock() did an munmap() while the class was providing a
  Memory() accessor. If the client stored the pointer returned by Memory(), did 
  an Unlock(), then a Lock() (doing an mmap()) and accessed the memory through
  the previously saved pointer, he could have ended up doing uses after free.
  unmap() is not needed as part of memory unpinning AFAICT.
- Some error code paths contained resource (e.g. fd) leaks.

BUG=299828
TBR=tomhudson@chromium.org, darin@chromium.org

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

[Philippe, 2013-09-27 17:28:05]

What do you guys think?

I intend to polish the CL and update the Mac implementation if the overall
approach looks good to you.

[Philippe, 2013-09-30 09:23:27]

On 2013/09/27 17:28:05, Philippe wrote:
> What do you guys think?
> 
> I intend to polish the CL and update the Mac implementation if the overall
> approach looks good to you.

CC'ing Ian who is currently working on
https://codereview.chromium.org/17106004/.

[willchan no longer on Chromium, 2013-09-30 16:16:35]

Have you guys spoken? Is this CL going to have to change? Do we have to
delay this one until the other one gets done? Ian, I'll get to that other
one today.


On Mon, Sep 30, 2013 at 2:23 AM, <pliard@chromium.org> wrote:

> On 2013/09/27 17:28:05, Philippe wrote:
>
>> What do you guys think?
>>
>
>  I intend to polish the CL and update the Mac implementation if the overall
>> approach looks good to you.
>>
>
> CC'ing Ian who is currently working on
>
https://codereview.chromium.**org/17106004/<https://codereview.chromium.org/1...
> .
>
>
https://chromiumcodereview.**appspot.com/24988003/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Philippe, 2013-09-30 16:29:44]

On 2013/09/30 16:16:35, willchan wrote:
> Have you guys spoken? Is this CL going to have to change? Do we have to
> delay this one until the other one gets done? Ian, I'll get to that other
> one today.
> 
> 
> On Mon, Sep 30, 2013 at 2:23 AM, <mailto:pliard@chromium.org> wrote:
> 
> > On 2013/09/27 17:28:05, Philippe wrote:
> >
> >> What do you guys think?
> >>
> >
> >  I intend to polish the CL and update the Mac implementation if the overall
> >> approach looks good to you.
> >>
> >
> > CC'ing Ian who is currently working on
> >
>
https://codereview.chromium.**org/17106004/%3Chttps://codereview.chromium.org...>
> > .
> >
> >
>
https://chromiumcodereview.**appspot.com/24988003/%3Chttps://chromiumcoderevi...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ian and I quickly chatted on Friday. The conclusion was that our efforts are not
conflicting (fortunately :)). There will only be a non-trivial rebase to do on
Ian's side. My CL is just abstracting DiscardableMemory in order to later
support DiscardableMemoryAllocator. This is independent of Ian's efforts I
believe since discardable memory emulation will only happen on platforms that
don't support it natively. Therefore DiscardableMemoryAllocator won't be used on
them anyway (or will just return DiscardableMemory instances that could be
emulated on those platforms).

I will let Ian look at this CL though to confirm that.

[willchan no longer on Chromium, 2013-10-02 14:20:52]

Sorry, I won't be able to look at this for the next two days most likely.
I'll be at the Chromium networking team offsite. I'll check in on Friday.


On Mon, Sep 30, 2013 at 9:29 AM, <pliard@chromium.org> wrote:

> On 2013/09/30 16:16:35, willchan wrote:
>
>> Have you guys spoken? Is this CL going to have to change? Do we have to
>> delay this one until the other one gets done? Ian, I'll get to that other
>> one today.
>>
>
>
>  On Mon, Sep 30, 2013 at 2:23 AM, <mailto:pliard@chromium.org> wrote:
>>
>
>  > On 2013/09/27 17:28:05, Philippe wrote:
>> >
>> >> What do you guys think?
>> >>
>> >
>> >  I intend to polish the CL and update the Mac implementation if the
>> overall
>> >> approach looks good to you.
>> >>
>> >
>> > CC'ing Ian who is currently working on
>> >
>>
>
> https://codereview.chromium.****org/17106004/%3Chttps://codere**
> view.chromium.org/17106004/ <http://codereview.chromium.org/17106004/>>
>
>> > .
>> >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/24988003/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/24988003/<http://appspot.com/24988003/%3Chttps://chromiumcodereview.appspot.com/24988003/>
> >
>
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to
mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org>
>> .
>>
>
> Ian and I quickly chatted on Friday. The conclusion was that our efforts
> are not
> conflicting (fortunately :)). There will only be a non-trivial rebase to
> do on
> Ian's side. My CL is just abstracting DiscardableMemory in order to later
> support DiscardableMemoryAllocator. This is independent of Ian's efforts I
> believe since discardable memory emulation will only happen on platforms
> that
> don't support it natively. Therefore DiscardableMemoryAllocator won't be
> used on
> them anyway (or will just return DiscardableMemory instances that could be
> emulated on those platforms).
>
> I will let Ian look at this CL though to confirm that.
>
>
https://chromiumcodereview.**appspot.com/24988003/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Philippe, 2013-10-02 14:23:21]

No worries, thanks for the update :) By the way, will we need also some
other reviewers (who could potentially start the review soon)?


On Wed, Oct 2, 2013 at 4:20 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Sorry, I won't be able to look at this for the next two days most likely.
> I'll be at the Chromium networking team offsite. I'll check in on Friday.
>
>
> On Mon, Sep 30, 2013 at 9:29 AM, <pliard@chromium.org> wrote:
>
>> On 2013/09/30 16:16:35, willchan wrote:
>>
>>> Have you guys spoken? Is this CL going to have to change? Do we have to
>>> delay this one until the other one gets done? Ian, I'll get to that other
>>> one today.
>>>
>>
>>
>>  On Mon, Sep 30, 2013 at 2:23 AM, <mailto:pliard@chromium.org> wrote:
>>>
>>
>>  > On 2013/09/27 17:28:05, Philippe wrote:
>>> >
>>> >> What do you guys think?
>>> >>
>>> >
>>> >  I intend to polish the CL and update the Mac implementation if the
>>> overall
>>> >> approach looks good to you.
>>> >>
>>> >
>>> > CC'ing Ian who is currently working on
>>> >
>>>
>>
>> https://codereview.chromium.****org/17106004/%3Chttps://codere**
>> view.chromium.org/17106004/ <http://codereview.chromium.org/17106004/>>
>>
>>> > .
>>> >
>>> >
>>>
>>
>> https://chromiumcodereview.**a**ppspot.com/24988003/%3Chttps:/**
>>
/chromiumcodereview.appspot.**com/24988003/<http://appspot.com/24988003/%3Chttps://chromiumcodereview.appspot.com/24988003/>
>> >
>>
>>> >
>>>
>>
>>  To unsubscribe from this group and stop receiving emails from it, send an
>>>
>> email
>>
>>> to
mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org>
>>> .
>>>
>>
>> Ian and I quickly chatted on Friday. The conclusion was that our efforts
>> are not
>> conflicting (fortunately :)). There will only be a non-trivial rebase to
>> do on
>> Ian's side. My CL is just abstracting DiscardableMemory in order to later
>> support DiscardableMemoryAllocator. This is independent of Ian's efforts I
>> believe since discardable memory emulation will only happen on platforms
>> that
>> don't support it natively. Therefore DiscardableMemoryAllocator won't be
>> used on
>> them anyway (or will just return DiscardableMemory instances that could be
>> emulated on those platforms).
>>
>> I will let Ian look at this CL though to confirm that.
>>
>>
https://chromiumcodereview.**appspot.com/24988003/<https://chromiumcodereview...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2013-10-02 14:26:39]

I haven't looked, but I'm not in OWNERS for skia and webkit. So you'll need
someone else to approve those. I assume they're just minor updates though
and will be easy.


On Wed, Oct 2, 2013 at 7:23 AM, Philippe Liard <pliard@chromium.org> wrote:

> No worries, thanks for the update :) By the way, will we need also some
> other reviewers (who could potentially start the review soon)?
>
>
> On Wed, Oct 2, 2013 at 4:20 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
>
>> Sorry, I won't be able to look at this for the next two days most likely.
>> I'll be at the Chromium networking team offsite. I'll check in on Friday.
>>
>>
>> On Mon, Sep 30, 2013 at 9:29 AM, <pliard@chromium.org> wrote:
>>
>>> On 2013/09/30 16:16:35, willchan wrote:
>>>
>>>> Have you guys spoken? Is this CL going to have to change? Do we have to
>>>> delay this one until the other one gets done? Ian, I'll get to that
>>>> other
>>>> one today.
>>>>
>>>
>>>
>>>  On Mon, Sep 30, 2013 at 2:23 AM, <mailto:pliard@chromium.org> wrote:
>>>>
>>>
>>>  > On 2013/09/27 17:28:05, Philippe wrote:
>>>> >
>>>> >> What do you guys think?
>>>> >>
>>>> >
>>>> >  I intend to polish the CL and update the Mac implementation if the
>>>> overall
>>>> >> approach looks good to you.
>>>> >>
>>>> >
>>>> > CC'ing Ian who is currently working on
>>>> >
>>>>
>>>
>>> https://codereview.chromium.****org/17106004/%3Chttps://codere**
>>> view.chromium.org/17106004/ <http://codereview.chromium.org/17106004/>>
>>>
>>>> > .
>>>> >
>>>> >
>>>>
>>>
>>> https://chromiumcodereview.**a**ppspot.com/24988003/%3Chttps:/**
>>>
/chromiumcodereview.appspot.**com/24988003/<http://appspot.com/24988003/%3Chttps://chromiumcodereview.appspot.com/24988003/>
>>> >
>>>
>>>> >
>>>>
>>>
>>>  To unsubscribe from this group and stop receiving emails from it, send
>>>> an
>>>>
>>> email
>>>
>>>> to
mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org>
>>>> .
>>>>
>>>
>>> Ian and I quickly chatted on Friday. The conclusion was that our efforts
>>> are not
>>> conflicting (fortunately :)). There will only be a non-trivial rebase to
>>> do on
>>> Ian's side. My CL is just abstracting DiscardableMemory in order to later
>>> support DiscardableMemoryAllocator. This is independent of Ian's efforts
>>> I
>>> believe since discardable memory emulation will only happen on platforms
>>> that
>>> don't support it natively. Therefore DiscardableMemoryAllocator won't be
>>> used on
>>> them anyway (or will just return DiscardableMemory instances that could
>>> be
>>> emulated on those platforms).
>>>
>>> I will let Ian look at this CL though to confirm that.
>>>
>>>
https://chromiumcodereview.**appspot.com/24988003/<https://chromiumcodereview...
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Philippe, 2013-10-02 14:32:31]

Yes indeed, thanks.


On Wed, Oct 2, 2013 at 4:26 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> I haven't looked, but I'm not in OWNERS for skia and webkit. So you'll
> need someone else to approve those. I assume they're just minor updates
> though and will be easy.
>
>
> On Wed, Oct 2, 2013 at 7:23 AM, Philippe Liard <pliard@chromium.org>wrote:
>
>> No worries, thanks for the update :) By the way, will we need also some
>> other reviewers (who could potentially start the review soon)?
>>
>>
>> On Wed, Oct 2, 2013 at 4:20 PM, William Chan (陈智昌) <willchan@chromium.org
>> > wrote:
>>
>>> Sorry, I won't be able to look at this for the next two days most
>>> likely. I'll be at the Chromium networking team offsite. I'll check in on
>>> Friday.
>>>
>>>
>>> On Mon, Sep 30, 2013 at 9:29 AM, <pliard@chromium.org> wrote:
>>>
>>>> On 2013/09/30 16:16:35, willchan wrote:
>>>>
>>>>> Have you guys spoken? Is this CL going to have to change? Do we have to
>>>>> delay this one until the other one gets done? Ian, I'll get to that
>>>>> other
>>>>> one today.
>>>>>
>>>>
>>>>
>>>>  On Mon, Sep 30, 2013 at 2:23 AM, <mailto:pliard@chromium.org> wrote:
>>>>>
>>>>
>>>>  > On 2013/09/27 17:28:05, Philippe wrote:
>>>>> >
>>>>> >> What do you guys think?
>>>>> >>
>>>>> >
>>>>> >  I intend to polish the CL and update the Mac implementation if the
>>>>> overall
>>>>> >> approach looks good to you.
>>>>> >>
>>>>> >
>>>>> > CC'ing Ian who is currently working on
>>>>> >
>>>>>
>>>>
>>>> https://codereview.chromium.****org/17106004/%3Chttps://codere**
>>>> view.chromium.org/17106004/ <http://codereview.chromium.org/17106004/>>
>>>>
>>>>> > .
>>>>> >
>>>>> >
>>>>>
>>>>
>>>> https://chromiumcodereview.**a**ppspot.com/24988003/%3Chttps:/**
>>>>
/chromiumcodereview.appspot.**com/24988003/<http://appspot.com/24988003/%3Chttps://chromiumcodereview.appspot.com/24988003/>
>>>> >
>>>>
>>>>> >
>>>>>
>>>>
>>>>  To unsubscribe from this group and stop receiving emails from it, send
>>>>> an
>>>>>
>>>> email
>>>>
>>>>> to
mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org>
>>>>> .
>>>>>
>>>>
>>>> Ian and I quickly chatted on Friday. The conclusion was that our
>>>> efforts are not
>>>> conflicting (fortunately :)). There will only be a non-trivial rebase
>>>> to do on
>>>> Ian's side. My CL is just abstracting DiscardableMemory in order to
>>>> later
>>>> support DiscardableMemoryAllocator. This is independent of Ian's
>>>> efforts I
>>>> believe since discardable memory emulation will only happen on
>>>> platforms that
>>>> don't support it natively. Therefore DiscardableMemoryAllocator won't
>>>> be used on
>>>> them anyway (or will just return DiscardableMemory instances that could
>>>> be
>>>> emulated on those platforms).
>>>>
>>>> I will let Ian look at this CL though to confirm that.
>>>>
>>>>
https://chromiumcodereview.**appspot.com/24988003/<https://chromiumcodereview...
>>>>
>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2013-10-09 22:47:37]

Can you explain the design choice in using a DiscardableMemory static factory
method to construct an abstract DiscardableMemory? Why do we want polymorphism
here? Do we have plans to ever use different implementations within the same
application?

My understanding is you just want to enable shimming in a
DiscardableMemoryAllocator. If that's the case, why can't that all be done in
the implementation using a global (since the
DiscardableMemory::CreateLockedMemory() is a global anyway).

[Philippe, 2013-10-14 09:22:56]

On 2013/10/09 22:47:37, willchan wrote:
> Can you explain the design choice in using a DiscardableMemory static factory
> method to construct an abstract DiscardableMemory? Why do we want polymorphism
> here? Do we have plans to ever use different implementations within the same
> application?
> 
> My understanding is you just want to enable shimming in a
> DiscardableMemoryAllocator. If that's the case, why can't that all be done in
> the implementation using a global (since the
> DiscardableMemory::CreateLockedMemory() is a global anyway).

I would like to have two different DiscardableMemory (trivial) implementations
on Android. The motivation behind that is not to force clients to use the
allocator when they only need a single instance of DiscardableMemory. That would
be unnecessarily cumbersome/complicated for them since they would have to make
sure for instance that the allocator outlives the returned discardable memory
instances.
As for the static factory method, it has a few benefits IMO as I tried to
capture in the CL description.

See
https://codereview.chromium.org/25293002/diff/38001/base/memory/discardable_m...
for the bigger picture.

Does that make any sense?

Thanks

[willchan no longer on Chromium, 2013-10-14 18:03:44]

On 2013/10/14 09:22:56, Philippe wrote:
> On 2013/10/09 22:47:37, willchan wrote:
> > Can you explain the design choice in using a DiscardableMemory static
factory
> > method to construct an abstract DiscardableMemory? Why do we want
polymorphism
> > here? Do we have plans to ever use different implementations within the same
> > application?
> > 
> > My understanding is you just want to enable shimming in a
> > DiscardableMemoryAllocator. If that's the case, why can't that all be done
in
> > the implementation using a global (since the
> > DiscardableMemory::CreateLockedMemory() is a global anyway).
> 
> I would like to have two different DiscardableMemory (trivial) implementations
> on Android. The motivation behind that is not to force clients to use the
> allocator when they only need a single instance of DiscardableMemory. That
would
> be unnecessarily cumbersome/complicated for them since they would have to make
> sure for instance that the allocator outlives the returned discardable memory
> instances.
> As for the static factory method, it has a few benefits IMO as I tried to
> capture in the CL description.
> 
> See
>
https://codereview.chromium.org/25293002/diff/38001/base/memory/discardable_m...
> for the bigger picture.
> 
> Does that make any sense?
> 
> Thanks

I looked at it and I see what you're doing now. But it's still not obvious to me
why you need run-time polymorphism rather than compile-time implementation
selection. Can you explain why you need to swap in different implementations
during run-time?

[Philippe, 2013-10-15 09:05:12]

On 2013/10/14 18:03:44, willchan wrote:
> On 2013/10/14 09:22:56, Philippe wrote:
> > On 2013/10/09 22:47:37, willchan wrote:
> > > Can you explain the design choice in using a DiscardableMemory static
> factory
> > > method to construct an abstract DiscardableMemory? Why do we want
> polymorphism
> > > here? Do we have plans to ever use different implementations within the
same
> > > application?
> > > 
> > > My understanding is you just want to enable shimming in a
> > > DiscardableMemoryAllocator. If that's the case, why can't that all be done
> in
> > > the implementation using a global (since the
> > > DiscardableMemory::CreateLockedMemory() is a global anyway).
> > 
> > I would like to have two different DiscardableMemory (trivial)
implementations
> > on Android. The motivation behind that is not to force clients to use the
> > allocator when they only need a single instance of DiscardableMemory. That
> would
> > be unnecessarily cumbersome/complicated for them since they would have to
make
> > sure for instance that the allocator outlives the returned discardable
memory
> > instances.
> > As for the static factory method, it has a few benefits IMO as I tried to
> > capture in the CL description.
> > 
> > See
> >
>
https://codereview.chromium.org/25293002/diff/38001/base/memory/discardable_m...
> > for the bigger picture.
> > 
> > Does that make any sense?
> > 
> > Thanks
> 
> I looked at it and I see what you're doing now. But it's still not obvious to
me
> why you need run-time polymorphism rather than compile-time implementation
> selection. Can you explain why you need to swap in different implementations
> during run-time?

To be honest I won't need run-time polymorphism. My biggest concern with having
a single non-abstract DiscardableMemory class (I assume that you excluded
duplicating the interface :)) is complexity and coupling with the allocator's
internals (e.g. the AshmemRegion class that needs to get notified when
DiscardableMemory instances get deleted).

What are your concerns with the current approach?
- extra functionality (run-time polymorphism) that isn't really being used?
- overhead of virtual method calls (followed for most of them by a syscall BTW)?
- possible inconsistency with the rest of base?

Having a single concrete DiscardableMemory class would imply:
- having at least this extra state (in the header):
  DeletionObserver* const deletion_observer_;
  void* const previous_chunk_address_;
- which also means exposing the DeletionObserver interface (which BTW I used in
place of a Callback to save the extra heap allocation). This interface is very
Android and allocator-specific (since it deals with a fd and a pointer to the
previous chunk).
Putting everything together in this discardable_memory.h header would be hard to
maintain in a multi-platform context IMO.

FWIW I'm not a huge fan of the previous use of platform-specific ifdefs. It
doesn't scale nicely either as I mentioned with the extra state.

As for Init() vs static factory method I understand that this is a separate
question that is independent of compile-time vs run-time polymorphism (although
I'm still quite strongly in favor of the static factory method given that
clients had to heap-allocate the object anyway).

What do you think? Am I missing something?

Thanks

[willchan no longer on Chromium, 2013-10-15 16:21:41]

To be clear, I'm just asking about your design choices. If one of your
goals is to eliminate the platform specific ifdefs since they're not
scalable, then that's a fine goal. I just wanted to get an explanation.

Some alternatives to accomplishing this same goal would be pimpl and
composition. pimpl would let you define everything in the .cc file as you
have, but adds some boilerplate which inheritance saves you. Composition
(without the pointer) would require defining the class in the header, but
you could put it in another header file and stuff it in the base::internal
namespace.

The advantage of these approaches over the abstract interface approach is a
tighter coupling (which has its obvious downsides). It reduces cognitive
overhead, since the immediate question when you have an abstract interface
is...what's the run-time implementation? Now, it's true that you should
code to the interface and not the implementation, but sometimes you gotta
know what's actually happening.

So, generally I take the stance that, if you have a good reason to make it
polymorphic, then make it polymorphic. But default to not doing so. What's
your thought on the alternatives? If you feel strongly that it should be
polymorphic, I'm willing to go with that.


On Tue, Oct 15, 2013 at 2:05 AM, <pliard@chromium.org> wrote:

> On 2013/10/14 18:03:44, willchan wrote:
>
>> On 2013/10/14 09:22:56, Philippe wrote:
>> > On 2013/10/09 22:47:37, willchan wrote:
>> > > Can you explain the design choice in using a DiscardableMemory static
>> factory
>> > > method to construct an abstract DiscardableMemory? Why do we want
>> polymorphism
>> > > here? Do we have plans to ever use different implementations within
>> the
>>
> same
>
>> > > application?
>> > >
>> > > My understanding is you just want to enable shimming in a
>> > > DiscardableMemoryAllocator. If that's the case, why can't that all be
>> done
>> in
>> > > the implementation using a global (since the
>> > > DiscardableMemory::**CreateLockedMemory() is a global anyway).
>> >
>> > I would like to have two different DiscardableMemory (trivial)
>>
> implementations
>
>> > on Android. The motivation behind that is not to force clients to use
>> the
>> > allocator when they only need a single instance of DiscardableMemory.
>> That
>> would
>> > be unnecessarily cumbersome/complicated for them since they would have
>> to
>>
> make
>
>> > sure for instance that the allocator outlives the returned discardable
>>
> memory
>
>> > instances.
>> > As for the static factory method, it has a few benefits IMO as I tried
>> to
>> > capture in the CL description.
>> >
>> > See
>> >
>>
>
> https://codereview.chromium.**org/25293002/diff/38001/base/**
>
memory/discardable_memory_**allocator_android.cc<https://codereview.chromium.org/25293002/diff/38001/base/memory/discardable_memory_allocator_android.cc>
>
>> > for the bigger picture.
>> >
>> > Does that make any sense?
>> >
>> > Thanks
>>
>
>  I looked at it and I see what you're doing now. But it's still not
>> obvious to
>>
> me
>
>> why you need run-time polymorphism rather than compile-time implementation
>> selection. Can you explain why you need to swap in different
>> implementations
>> during run-time?
>>
>
> To be honest I won't need run-time polymorphism. My biggest concern with
> having
> a single non-abstract DiscardableMemory class (I assume that you excluded
> duplicating the interface :)) is complexity and coupling with the
> allocator's
> internals (e.g. the AshmemRegion class that needs to get notified when
> DiscardableMemory instances get deleted).
>
> What are your concerns with the current approach?
> - extra functionality (run-time polymorphism) that isn't really being used?
> - overhead of virtual method calls (followed for most of them by a syscall
> BTW)?
> - possible inconsistency with the rest of base?
>
> Having a single concrete DiscardableMemory class would imply:
> - having at least this extra state (in the header):
>   DeletionObserver* const deletion_observer_;
>   void* const previous_chunk_address_;
> - which also means exposing the DeletionObserver interface (which BTW I
> used in
> place of a Callback to save the extra heap allocation). This interface is
> very
> Android and allocator-specific (since it deals with a fd and a pointer to
> the
> previous chunk).
> Putting everything together in this discardable_memory.h header would be
> hard to
> maintain in a multi-platform context IMO.
>
> FWIW I'm not a huge fan of the previous use of platform-specific ifdefs. It
> doesn't scale nicely either as I mentioned with the extra state.
>
> As for Init() vs static factory method I understand that this is a separate
> question that is independent of compile-time vs run-time polymorphism
> (although
> I'm still quite strongly in favor of the static factory method given that
> clients had to heap-allocate the object anyway).
>
> What do you think? Am I missing something?
>
> Thanks
>
>
https://chromiumcodereview.**appspot.com/24988003/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Philippe, 2013-10-15 17:02:18]

On 2013/10/15 16:21:41, willchan wrote:
> To be clear, I'm just asking about your design choices. If one of your
> goals is to eliminate the platform specific ifdefs since they're not
> scalable, then that's a fine goal. I just wanted to get an explanation.
> 
> Some alternatives to accomplishing this same goal would be pimpl and
> composition. pimpl would let you define everything in the .cc file as you
> have, but adds some boilerplate which inheritance saves you. Composition
> (without the pointer) would require defining the class in the header, but
> you could put it in another header file and stuff it in the base::internal
> namespace.
> 
> The advantage of these approaches over the abstract interface approach is a
> tighter coupling (which has its obvious downsides). It reduces cognitive
> overhead, since the immediate question when you have an abstract interface
> is...what's the run-time implementation? Now, it's true that you should
> code to the interface and not the implementation, but sometimes you gotta
> know what's actually happening.
> 
> So, generally I take the stance that, if you have a good reason to make it
> polymorphic, then make it polymorphic. But default to not doing so. What's
> your thought on the alternatives? If you feel strongly that it should be
> polymorphic, I'm willing to go with that.
> 
> 
> On Tue, Oct 15, 2013 at 2:05 AM, <mailto:pliard@chromium.org> wrote:
> 
> > On 2013/10/14 18:03:44, willchan wrote:
> >
> >> On 2013/10/14 09:22:56, Philippe wrote:
> >> > On 2013/10/09 22:47:37, willchan wrote:
> >> > > Can you explain the design choice in using a DiscardableMemory static
> >> factory
> >> > > method to construct an abstract DiscardableMemory? Why do we want
> >> polymorphism
> >> > > here? Do we have plans to ever use different implementations within
> >> the
> >>
> > same
> >
> >> > > application?
> >> > >
> >> > > My understanding is you just want to enable shimming in a
> >> > > DiscardableMemoryAllocator. If that's the case, why can't that all be
> >> done
> >> in
> >> > > the implementation using a global (since the
> >> > > DiscardableMemory::**CreateLockedMemory() is a global anyway).
> >> >
> >> > I would like to have two different DiscardableMemory (trivial)
> >>
> > implementations
> >
> >> > on Android. The motivation behind that is not to force clients to use
> >> the
> >> > allocator when they only need a single instance of DiscardableMemory.
> >> That
> >> would
> >> > be unnecessarily cumbersome/complicated for them since they would have
> >> to
> >>
> > make
> >
> >> > sure for instance that the allocator outlives the returned discardable
> >>
> > memory
> >
> >> > instances.
> >> > As for the static factory method, it has a few benefits IMO as I tried
> >> to
> >> > capture in the CL description.
> >> >
> >> > See
> >> >
> >>
> >
> > https://codereview.chromium.**org/25293002/diff/38001/base/**
> >
>
memory/discardable_memory_**allocator_android.cc<https://codereview.chromium.org/25293002/diff/38001/base/memory/discardable_memory_allocator_android.cc>
> >
> >> > for the bigger picture.
> >> >
> >> > Does that make any sense?
> >> >
> >> > Thanks
> >>
> >
> >  I looked at it and I see what you're doing now. But it's still not
> >> obvious to
> >>
> > me
> >
> >> why you need run-time polymorphism rather than compile-time implementation
> >> selection. Can you explain why you need to swap in different
> >> implementations
> >> during run-time?
> >>
> >
> > To be honest I won't need run-time polymorphism. My biggest concern with
> > having
> > a single non-abstract DiscardableMemory class (I assume that you excluded
> > duplicating the interface :)) is complexity and coupling with the
> > allocator's
> > internals (e.g. the AshmemRegion class that needs to get notified when
> > DiscardableMemory instances get deleted).
> >
> > What are your concerns with the current approach?
> > - extra functionality (run-time polymorphism) that isn't really being used?
> > - overhead of virtual method calls (followed for most of them by a syscall
> > BTW)?
> > - possible inconsistency with the rest of base?
> >
> > Having a single concrete DiscardableMemory class would imply:
> > - having at least this extra state (in the header):
> >   DeletionObserver* const deletion_observer_;
> >   void* const previous_chunk_address_;
> > - which also means exposing the DeletionObserver interface (which BTW I
> > used in
> > place of a Callback to save the extra heap allocation). This interface is
> > very
> > Android and allocator-specific (since it deals with a fd and a pointer to
> > the
> > previous chunk).
> > Putting everything together in this discardable_memory.h header would be
> > hard to
> > maintain in a multi-platform context IMO.
> >
> > FWIW I'm not a huge fan of the previous use of platform-specific ifdefs. It
> > doesn't scale nicely either as I mentioned with the extra state.
> >
> > As for Init() vs static factory method I understand that this is a separate
> > question that is independent of compile-time vs run-time polymorphism
> > (although
> > I'm still quite strongly in favor of the static factory method given that
> > clients had to heap-allocate the object anyway).
> >
> > What do you think? Am I missing something?
> >
> > Thanks
> >
> >
>
https://chromiumcodereview.**appspot.com/24988003/%3Chttps://chromiumcoderevi...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks William!

I hadn't thought about pimpl which is indeed a good alternative in general. Let
me see what the whole thing would look like in practice with pimpl and
composition and I will get back to you (and possibly upload the corresponding
patch set(s)). One immediate downside that I can see with pimpl though is the
double heap allocation if we want to keep the static factory method (which I was
personally in favor of).

Cheers,
Philippe.

[Philippe, 2013-10-16 11:47:40]

On 2013/10/15 17:02:18, Philippe wrote:
> On 2013/10/15 16:21:41, willchan wrote:
> > To be clear, I'm just asking about your design choices. If one of your
> > goals is to eliminate the platform specific ifdefs since they're not
> > scalable, then that's a fine goal. I just wanted to get an explanation.
> > 
> > Some alternatives to accomplishing this same goal would be pimpl and
> > composition. pimpl would let you define everything in the .cc file as you
> > have, but adds some boilerplate which inheritance saves you. Composition
> > (without the pointer) would require defining the class in the header, but
> > you could put it in another header file and stuff it in the base::internal
> > namespace.
> > 
> > The advantage of these approaches over the abstract interface approach is a
> > tighter coupling (which has its obvious downsides). It reduces cognitive
> > overhead, since the immediate question when you have an abstract interface
> > is...what's the run-time implementation? Now, it's true that you should
> > code to the interface and not the implementation, but sometimes you gotta
> > know what's actually happening.
> > 
> > So, generally I take the stance that, if you have a good reason to make it
> > polymorphic, then make it polymorphic. But default to not doing so. What's
> > your thought on the alternatives? If you feel strongly that it should be
> > polymorphic, I'm willing to go with that.
> > 
> > 
> > On Tue, Oct 15, 2013 at 2:05 AM, <mailto:pliard@chromium.org> wrote:
> > 
> > > On 2013/10/14 18:03:44, willchan wrote:
> > >
> > >> On 2013/10/14 09:22:56, Philippe wrote:
> > >> > On 2013/10/09 22:47:37, willchan wrote:
> > >> > > Can you explain the design choice in using a DiscardableMemory static
> > >> factory
> > >> > > method to construct an abstract DiscardableMemory? Why do we want
> > >> polymorphism
> > >> > > here? Do we have plans to ever use different implementations within
> > >> the
> > >>
> > > same
> > >
> > >> > > application?
> > >> > >
> > >> > > My understanding is you just want to enable shimming in a
> > >> > > DiscardableMemoryAllocator. If that's the case, why can't that all be
> > >> done
> > >> in
> > >> > > the implementation using a global (since the
> > >> > > DiscardableMemory::**CreateLockedMemory() is a global anyway).
> > >> >
> > >> > I would like to have two different DiscardableMemory (trivial)
> > >>
> > > implementations
> > >
> > >> > on Android. The motivation behind that is not to force clients to use
> > >> the
> > >> > allocator when they only need a single instance of DiscardableMemory.
> > >> That
> > >> would
> > >> > be unnecessarily cumbersome/complicated for them since they would have
> > >> to
> > >>
> > > make
> > >
> > >> > sure for instance that the allocator outlives the returned discardable
> > >>
> > > memory
> > >
> > >> > instances.
> > >> > As for the static factory method, it has a few benefits IMO as I tried
> > >> to
> > >> > capture in the CL description.
> > >> >
> > >> > See
> > >> >
> > >>
> > >
> > > https://codereview.chromium.**org/25293002/diff/38001/base/**
> > >
> >
>
memory/discardable_memory_**allocator_android.cc<https://codereview.chromium.org/25293002/diff/38001/base/memory/discardable_memory_allocator_android.cc>
> > >
> > >> > for the bigger picture.
> > >> >
> > >> > Does that make any sense?
> > >> >
> > >> > Thanks
> > >>
> > >
> > >  I looked at it and I see what you're doing now. But it's still not
> > >> obvious to
> > >>
> > > me
> > >
> > >> why you need run-time polymorphism rather than compile-time
implementation
> > >> selection. Can you explain why you need to swap in different
> > >> implementations
> > >> during run-time?
> > >>
> > >
> > > To be honest I won't need run-time polymorphism. My biggest concern with
> > > having
> > > a single non-abstract DiscardableMemory class (I assume that you excluded
> > > duplicating the interface :)) is complexity and coupling with the
> > > allocator's
> > > internals (e.g. the AshmemRegion class that needs to get notified when
> > > DiscardableMemory instances get deleted).
> > >
> > > What are your concerns with the current approach?
> > > - extra functionality (run-time polymorphism) that isn't really being
used?
> > > - overhead of virtual method calls (followed for most of them by a syscall
> > > BTW)?
> > > - possible inconsistency with the rest of base?
> > >
> > > Having a single concrete DiscardableMemory class would imply:
> > > - having at least this extra state (in the header):
> > >   DeletionObserver* const deletion_observer_;
> > >   void* const previous_chunk_address_;
> > > - which also means exposing the DeletionObserver interface (which BTW I
> > > used in
> > > place of a Callback to save the extra heap allocation). This interface is
> > > very
> > > Android and allocator-specific (since it deals with a fd and a pointer to
> > > the
> > > previous chunk).
> > > Putting everything together in this discardable_memory.h header would be
> > > hard to
> > > maintain in a multi-platform context IMO.
> > >
> > > FWIW I'm not a huge fan of the previous use of platform-specific ifdefs.
It
> > > doesn't scale nicely either as I mentioned with the extra state.
> > >
> > > As for Init() vs static factory method I understand that this is a
separate
> > > question that is independent of compile-time vs run-time polymorphism
> > > (although
> > > I'm still quite strongly in favor of the static factory method given that
> > > clients had to heap-allocate the object anyway).
> > >
> > > What do you think? Am I missing something?
> > >
> > > Thanks
> > >
> > >
> >
>
https://chromiumcodereview.**appspot.com/24988003/%253Chttps://chromiumcodere...>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Thanks William!
> 
> I hadn't thought about pimpl which is indeed a good alternative in general.
Let
> me see what the whole thing would look like in practice with pimpl and
> composition and I will get back to you (and possibly upload the corresponding
> patch set(s)). One immediate downside that I can see with pimpl though is the
> double heap allocation if we want to keep the static factory method (which I
was
> personally in favor of).
> 
> Cheers,
> Philippe.

It turns out that I will actually need/use run-time polymorphism. Sorry for not
spotting that earlier :/ The allocator is conservative in the sense that it only
starts operating on big ashmem regions (as opposed to individual ones for each
allocation request) once we exceeded a certain amount of file descriptors
(currently 90% of the ashmem fd limit which will itself be ~25% (might require
some tuning) of the overall process fd limit (= 1024 on most of the devices). So
the allocator will return "regular" instances (i.e.
DiscardableMemory::CreateLockedMemory()) below this threshold and will start
returning its own instances past this threshold. The main motivation behind that
is to avoid fragmentation as much as possible.

Does that make sense to you?

[willchan no longer on Chromium, 2013-10-17 01:21:37]

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
File base/memory/discardable_memory_android.cc (right):

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
base/memory/discardable_memory_android.cc:140: HANDLE_EINTR(close(fd));
Please drop this HANDLE_EINTR:
https://code.google.com/p/chromium/issues/detail?id=269623.

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
base/memory/discardable_memory_android.cc:143: return HANDLE_EINTR(close(fd)) ==
0;
Ditto

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
base/memory/discardable_memory_android.cc:151: #if !defined(NDEBUG)
I don't get this. Why is this NDEBUG stuff needed? In opt mode, DCHECK_EQ will
get compiled out anyway.

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
File base/memory/discardable_memory_android.h (right):

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
base/memory/discardable_memory_android.h:17: bool CreateAshmemRegion(const char*
name, size_t size, int* fd, void** address);
BASE_EXPORT_PRIVATE all of these? Why are these even in the header file in the
first place? I don't see where they get used except in the .cc file, but I must
be missing something.

[Philippe, 2013-10-17 08:56:24]

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
File base/memory/discardable_memory_android.cc (right):

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
base/memory/discardable_memory_android.cc:140: HANDLE_EINTR(close(fd));
On 2013/10/17 01:21:37, willchan wrote:
> Please drop this HANDLE_EINTR:
> https://code.google.com/p/chromium/issues/detail?id=269623.

Thanks, I didn't know about this issue.

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
base/memory/discardable_memory_android.cc:143: return HANDLE_EINTR(close(fd)) ==
0;
On 2013/10/17 01:21:37, willchan wrote:
> Ditto

Done.

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
base/memory/discardable_memory_android.cc:151: #if !defined(NDEBUG)
On 2013/10/17 01:21:37, willchan wrote:
> I don't get this. Why is this NDEBUG stuff needed? In opt mode, DCHECK_EQ will
> get compiled out anyway.

Yeah, good point. I guess I added the DCHECK after the mprotect without thinking
about this.

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
File base/memory/discardable_memory_android.h (right):

https://codereview.chromium.org/24988003/diff/12001/base/memory/discardable_m...
base/memory/discardable_memory_android.h:17: bool CreateAshmemRegion(const char*
name, size_t size, int* fd, void** address);
On 2013/10/17 01:21:37, willchan wrote:
> BASE_EXPORT_PRIVATE all of these? Why are these even in the header file in the
> first place? I don't see where they get used except in the .cc file, but I
must
> be missing something.

This will be used by the allocator. I agree that this should not be exposed
before it's being used so I deleted this header file, moved these functions in
the .cc file to the anonymous namespace and will add the header back/move the
functions back to the internal namespace in the allocator CL.

[Philippe, 2013-10-17 11:05:24]

Adding Avi for the Mac side at least.

[Avi (use Gerrit), 2013-10-17 15:09:37]

I have issues with the ordering.

https://codereview.chromium.org/24988003/diff/48001/base/memory/discardable_m...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/24988003/diff/48001/base/memory/discardable_m...
base/memory/discardable_memory_mac.cc:38: DCHECK_EQ(0, mprotect(memory_, size_,
PROT_READ | PROT_WRITE));
If you're going to mprotect, do it _after_ you get the memory back with the
vm_purgable_control.

https://codereview.chromium.org/24988003/diff/48001/base/memory/discardable_m...
base/memory/discardable_memory_mac.cc:59: DCHECK_EQ(0, mprotect(memory_, size_,
PROT_NONE));
Can we mprotect before we mark it as purgeable? Purgeable memory is
underdocumented enough that I want to not play with it when it's in that state.

[Avi (use Gerrit), 2013-10-17 15:10:56]

Also, the mprotect looks like a useful debug feature. We should have a death
test in debug mode to make sure it works.

[willchan no longer on Chromium, 2013-10-17 15:46:19]

FYI, I'm getting an old chunk mismatch on the current patchset, so I can't view
the diff. Maybe re-upload?

[Philippe, 2013-10-17 15:57:49]

Yeah, I will upload a patch set very shortly that will address Avi's
comments (that turn out to have some unexpected implications).


On Thu, Oct 17, 2013 at 5:46 PM, <willchan@chromium.org> wrote:

> FYI, I'm getting an old chunk mismatch on the current patchset, so I can't
> view
> the diff. Maybe re-upload?
>
>
https://codereview.chromium.**org/24988003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Philippe, 2013-10-17 16:20:35]

Thanks Avi!

Please ignore patch set 9. You can diff against patch set 8 or the base.

Adding a death unit test was a great idea since it made me catch a small bug
(that made the mprotect a no-op) in the Android implementation! Unfortunately
death tests are not supported with Android APKs (I did check the mprotect
behavior by making sure that it crashes in a regular test though).

https://codereview.chromium.org/24988003/diff/48001/base/memory/discardable_m...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/24988003/diff/48001/base/memory/discardable_m...
base/memory/discardable_memory_mac.cc:38: DCHECK_EQ(0, mprotect(memory_, size_,
PROT_READ | PROT_WRITE));
On 2013/10/17 15:09:38, Avi wrote:
> If you're going to mprotect, do it _after_ you get the memory back with the
> vm_purgable_control.

The memory must be touchable to let vm_purgeable_control() work. See my attempt
to address your comment in the next patch set (patch set 9) that failed (just
like the initial mistake that I had made in Unlock() below in patch set 7) :/

https://codereview.chromium.org/24988003/diff/48001/base/memory/discardable_m...
base/memory/discardable_memory_mac.cc:59: DCHECK_EQ(0, mprotect(memory_, size_,
PROT_NONE));
On 2013/10/17 15:09:38, Avi wrote:
> Can we mprotect before we mark it as purgeable? Purgeable memory is
> underdocumented enough that I want to not play with it when it's in that
state.

See the previous patch set (patch set 7) and my comment above.

[Avi (use Gerrit), 2013-10-17 17:58:54]

lgtm

I didn't know that about the mprotect deal. Gotcha.

Nice new test.

https://codereview.chromium.org/24988003/diff/80001/base/memory/discardable_m...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/24988003/diff/80001/base/memory/discardable_m...
base/memory/discardable_memory_mac.cc:47: return state & VM_PURGABLE_EMPTY ?
DISCARDABLE_MEMORY_PURGED
I liked the extra line before this one.

[Avi (use Gerrit), 2013-10-17 18:00:26]

https://codereview.chromium.org/24988003/diff/80001/base/memory/discardable_m...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/24988003/diff/80001/base/memory/discardable_m...
base/memory/discardable_memory_unittest.cc:59: TEST(DiscardableMemoryTest,
UnlockedMemoryAccessCrashesInDebugMode) {
Actually, can we ifdef this whole test out if we're not running in debug mode?

[Philippe, 2013-10-18 08:25:21]

Thanks Avi!

https://codereview.chromium.org/24988003/diff/80001/base/memory/discardable_m...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/24988003/diff/80001/base/memory/discardable_m...
base/memory/discardable_memory_mac.cc:47: return state & VM_PURGABLE_EMPTY ?
DISCARDABLE_MEMORY_PURGED
On 2013/10/17 17:58:55, Avi wrote:
> I liked the extra line before this one.

Yeah, sorry. I tend to be quite aggressive at removing blank lines. I was very
used to code with a lot of blank lines before joining Google but got used to the
very packed style in Google3 (at least in my previous team). IMO blank lines
create inconsistencies since people have very different preferences with regards
to their position so I prefer to split functions when it's possible and when it
improves readability but this is only my opinion :)

https://codereview.chromium.org/24988003/diff/80001/base/memory/discardable_m...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/24988003/diff/80001/base/memory/discardable_m...
base/memory/discardable_memory_unittest.cc:59: TEST(DiscardableMemoryTest,
UnlockedMemoryAccessCrashesInDebugMode) {
On 2013/10/17 18:00:27, Avi wrote:
> Actually, can we ifdef this whole test out if we're not running in debug mode?

Yeah definitely. I used to have this but lost it after I messed up wit the
defines here.

[willchan no longer on Chromium, 2013-10-19 00:45:48]

LGTM, but just a comment on your assertion in the CL description. You've claimed
the only point to the previous API was to allow stack allocations, but that's
not true. If you look at
https://codereview.chromium.org/24988003/diff/91001/skia/ext/SkDiscardableMem...
and
https://codereview.chromium.org/24988003/diff/91001/webkit/child/web_discarda...,
you'll see that even though the authors did not choose to do so, they could have
directly embedded the DiscardableMemory object into the containing object,
rather than requiring a separate heap allocation. Just a minor nit on your
description.

[Philippe, 2013-10-21 13:19:55]

On 2013/10/19 00:45:48, willchan wrote:
> LGTM, but just a comment on your assertion in the CL description. You've
claimed
> the only point to the previous API was to allow stack allocations, but that's
> not true. If you look at
>
https://codereview.chromium.org/24988003/diff/91001/skia/ext/SkDiscardableMem...
> and
>
https://codereview.chromium.org/24988003/diff/91001/webkit/child/web_discarda...,
> you'll see that even though the authors did not choose to do so, they could
have
> directly embedded the DiscardableMemory object into the containing object,
> rather than requiring a separate heap allocation. Just a minor nit on your
> description.

Thanks William! I updated the CL description to reflect your note.

David, sorry for the upcoming rebase :)

[commit-bot: I haz the power, 2013-10-21 13:20:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/24988003/381001

[commit-bot: I haz the power, 2013-10-21 14:03:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/24988003/381001

[commit-bot: I haz the power, 2013-10-21 16:54:10]

Change committed as 229838