Use DiscardableMemoryManager on Android.

Use DiscardableMemoryManager on Android.

This allows userspace (DiscardableMemoryManager) to control eviction of
unlocked DiscardableMemory instances to prevent the process from running out of
address space in cases of heavy use of unlocked DiscardableMemory.

This also removes all the occurences of 'Android' from the ashmem allocator to
allow it to be later used on ChromeOS.

BUG=327516, 334996
R=reveman@chromium.org, willchan@chromium.org

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

[Philippe, 2014-03-20 17:10:14]

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

https://codereview.chromium.org/195863005/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_manager.cc:123: it->second.allocation->Unlock();
FYI: I need to do a rebase here.

[reveman, 2014-03-20 19:33:43]

I think the file names should be discardable_memory_allocation_ashmem.h/cc to be
consistent with existing code. For the file names to be
discardable_memory_allocation_factor_ashmem.h/cc we should first rename
discardable_memory_allocation.h/cc to
discardable_memory_allocation_factory.h/cc.

https://codereview.chromium.org/195863005/diff/20001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/195863005/diff/20001/base/base.gypi#newcode798
base/base.gypi:798: ['OS == "android" and _toolset == "target"', {
what is _toolset == "target" for?

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
File base/memory/discardable_memory_allocation_ashmem_factory.h (right):

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
base/memory/discardable_memory_allocation_ashmem_factory.h:6: #define
BASE_MEMORY_DISCARDABLE_MEMORY_ALLOCATION_ASHMEM_FACTORY_H_
Let's make sure the type is always the last suffix of the file (and class name):
discardable_memory_allocation_factory_ashmem.h

Maybe it was a mistake to have the factory interface be a subclass of
allocation.

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
base/memory/discardable_memory_allocation_ashmem_factory.h:19: // On
Android/ChromeOS ashmem is used to implement discardable memory. It is
I don't think the Android, ChromeOS or any other specific platform should be
mentioned here.

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
base/memory/discardable_memory_allocation_ashmem_factory.h:28: class
BASE_EXPORT_PRIVATE DiscardableMemoryAllocationAshmemFactory :
I much prefer DiscardableMemoryAllocationFactoryAshmem

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
File base/memory/discardable_memory_android.cc (right):

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
base/memory/discardable_memory_android.cc:27: class DiscardableMemoryManagerImpl
{
This is not a DiscardableMemoryManagerImpl. This should be consistent with
whatever we're doing in discardable_memory_emulated.cc. Currently that means
DiscardableMemoryManagerImpl implementing both the manager and factory
interfaces. If you don't like that for some reason, we need to first adjust how
we do things in discardable_memory_emulated.cc. Consistency is more important to
me than what specific pattern we use.

[Philippe, 2014-03-21 10:17:44]

https://codereview.chromium.org/195863005/diff/20001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/195863005/diff/20001/base/base.gypi#newcode798
base/base.gypi:798: ['OS == "android" and _toolset == "target"', {
On 2014/03/20 19:33:43, reveman wrote:
> what is _toolset == "target" for?

On platforms where cross-compilation is used, we distinguish the target (Android
here) and the host (Linux). In an Android build we build libbase not only for
the target but also for the host since we have some standalone binaries
(depending on base) that we use as part of the build itself and for testing.
Does that make sense?

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
File base/memory/discardable_memory_allocation_ashmem_factory.h (right):

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
base/memory/discardable_memory_allocation_ashmem_factory.h:6: #define
BASE_MEMORY_DISCARDABLE_MEMORY_ALLOCATION_ASHMEM_FACTORY_H_
On 2014/03/20 19:33:44, reveman wrote:
> Let's make sure the type is always the last suffix of the file (and class
name):
> discardable_memory_allocation_factory_ashmem.h
> 
> Maybe it was a mistake to have the factory interface be a subclass of
> allocation.

Yeah. I think it's still ok though to have the Factory as a nested class.

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
base/memory/discardable_memory_allocation_ashmem_factory.h:19: // On
Android/ChromeOS ashmem is used to implement discardable memory. It is
On 2014/03/20 19:33:44, reveman wrote:
> I don't think the Android, ChromeOS or any other specific platform should be
> mentioned here.

Done.

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
base/memory/discardable_memory_allocation_ashmem_factory.h:28: class
BASE_EXPORT_PRIVATE DiscardableMemoryAllocationAshmemFactory :
On 2014/03/20 19:33:44, reveman wrote:
> I much prefer DiscardableMemoryAllocationFactoryAshmem

Yeah, done.

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
File base/memory/discardable_memory_android.cc (right):

https://codereview.chromium.org/195863005/diff/100001/base/memory/discardable...
base/memory/discardable_memory_android.cc:27: class DiscardableMemoryManagerImpl
{
On 2014/03/20 19:33:44, reveman wrote:
> This is not a DiscardableMemoryManagerImpl. This should be consistent with
> whatever we're doing in discardable_memory_emulated.cc. Currently that means
> DiscardableMemoryManagerImpl implementing both the manager and factory
> interfaces. If you don't like that for some reason, we need to first adjust
how
> we do things in discardable_memory_emulated.cc. Consistency is more important
to
> me than what specific pattern we use.

Agreed for consistency. Let's discuss this in your CL. Subclassing the ashmem
factory is not suitable/desirable here.

[Philippe, 2014-04-25 15:51:58]

PTAL :) I will update the CL's description once I'm sure the approach looks
reasonable to you. I also removed the allocator renaming part from this CL (and
will do it instead in a follow-up CL).

[reveman, 2014-04-26 00:13:37]

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_allocator_android.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_allocator_android.h:58: // On Android ashmem is
used to implement discardable memory. It is backed by a
nit: Please remove "Android" from here if this code is not longer supposed to be
android specific.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_allocator_android.h:64: class BASE_EXPORT_PRIVATE
DiscardableMemoryAllocator {
What's the correct name for this class and these files? Is this still Android
specific (DiscardableMemoryAllocatorAndroid as the file name suggests)? Or
should it be DiscardableMemoryAshmemAllocator? DiscardableMemoryAllocatorAshmem?
I'd like if these files/classes were renamed properly as part of this patch
where you introduce disacrdable_memory_ashmem.h.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_android.cc (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_android.cc:26: ((sizeof(void*) == 4) ? 512 : 16 *
1024) * 1024 * 1024;
I would prefer if used the same constant on 64bit to start with. 512MB is fine
if that's the highest you think we should use on 32bit.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_android.cc:66: DISCARDABLE_MEMORY_TYPE_ANDROID,
Please rename this to DISCARDABLE_MEMORY_TYPE_ASHMEM as part of this patch.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_ashmem.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_ashmem.h:44: DiscardableMemoryManager* const
manager_;
nit: I don't think we typically use "const" for pointers in chromium.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_manager.cc (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_manager.cc:90: allocation->ReleaseLock();
It's currently up to the client to call DiscardableMemoryManager::ReleaseLock
before Unregister. Accounting will be incorrect if Unregister is called for an
allocation which a lock is currently acquired.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_manager.cc:95: allocation->Purge();
I don't think we should do this here. I prefer to have Unregister simply stop
tracking the allocation and leave it up to the client to decide what to do with
it.

Maybe add a DiscardableMemoryManager::Purge(Allocation* allocation) function
instead that you can call prior to Unregister()?

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_manager.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_manager.h:23: // AcuireLock(), ReleaseLock()...).
It's really the usage by the manager rather than the interface itself that
provides this guarantee. How about we move this comment to the Manager below?
Maybe next to the Register() function. ie. "allocations registered with the
manager will never be used concurrently.."

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_manager_unittest.cc:27: DCHECK(!bytes_ || bytes_
== bytes);
how about we remove the |bytes| parameter from AllocateAndAcquireLock instead?

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_manager_unittest.cc:32: DCHECK_EQ(is_locked_,
is_allocated_);
hm, this seems unnecessary. What sequence of events does this cover that was not
covered before?

[Philippe, 2014-04-28 12:23:14]

Thanks David for the review, PTAL.

After reconsideration I really think that it's both simpler and safer to keep
the lock in the allocator. The two locks are both acquired by the same thread
only when allocation happens. When this happens they are always
deterministically acquired in the same order: the manager's lock first then the
allocator's lock.
This also means that there is absolutely no change required in the manager.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_allocator_android.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_allocator_android.h:58: // On Android ashmem is
used to implement discardable memory. It is backed by a
On 2014/04/26 00:13:37, reveman wrote:
> nit: Please remove "Android" from here if this code is not longer supposed to
be
> android specific.

Done.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_allocator_android.h:64: class BASE_EXPORT_PRIVATE
DiscardableMemoryAllocator {
On 2014/04/26 00:13:37, reveman wrote:
> What's the correct name for this class and these files? Is this still Android
> specific (DiscardableMemoryAllocatorAndroid as the file name suggests)? Or
> should it be DiscardableMemoryAshmemAllocator?
DiscardableMemoryAllocatorAshmem?
> I'd like if these files/classes were renamed properly as part of this patch
> where you introduce disacrdable_memory_ashmem.h.

I had suggested in my previous comment to keep the renaming part for a follow up
CL but you may have not seen it :) That said I don't mind doing the renaming
here. DiscardableMemoryAshmemAllocator SGTM.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_android.cc (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_android.cc:26: ((sizeof(void*) == 4) ? 512 : 16 *
1024) * 1024 * 1024;
On 2014/04/26 00:13:37, reveman wrote:
> I would prefer if used the same constant on 64bit to start with. 512MB is fine
> if that's the highest you think we should use on 32bit.

Done.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_android.cc:66: DISCARDABLE_MEMORY_TYPE_ANDROID,
On 2014/04/26 00:13:37, reveman wrote:
> Please rename this to DISCARDABLE_MEMORY_TYPE_ASHMEM as part of this patch.

Sure.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_ashmem.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_ashmem.h:44: DiscardableMemoryManager* const
manager_;
On 2014/04/26 00:13:37, reveman wrote:
> nit: I don't think we typically use "const" for pointers in chromium.

Unfortunately, yes. I saw that const is underused in Chrome whereas this usage
is very typical in Google3. I have been quite successful though so far at adding
them in Chromium (it's actually the first time a reviewer comments on this). In
addition to what the style guide says I see one clear benefit here with this
usage of const: a compilation error is generated in case the author forgets to
initialize the field. You don't get this without the const.

And of course the style guide still applies here:
"Pros: Easier for people to understand how variables are being used. Allows the
compiler to do better type checking, and, conceivably, generate better code.
Helps people convince themselves of program correctness because they know the
functions they call are limited in how they can modify your variables. Helps
people know what functions are safe to use without locks in multi-threaded
programs."

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_manager_unittest.cc:27: DCHECK(!bytes_ || bytes_
== bytes);
On 2014/04/26 00:13:37, reveman wrote:
> how about we remove the |bytes| parameter from AllocateAndAcquireLock instead?

Good idea, I was tempted to make the change too.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_manager_unittest.cc:32: DCHECK_EQ(is_locked_,
is_allocated_);
On 2014/04/26 00:13:37, reveman wrote:
> hm, this seems unnecessary. What sequence of events does this cover that was
not
> covered before?

I think this was needed for the zero allocation (with my changes in Unregister()
though). I have now reverted my changes in the manager though.

[Philippe, 2014-04-28 14:49:21]

On 2014/04/28 12:23:14, Philippe wrote:
> Thanks David for the review, PTAL.
> 
> After reconsideration I really think that it's both simpler and safer to keep
> the lock in the allocator. The two locks are both acquired by the same thread
> only when allocation happens. When this happens they are always
> deterministically acquired in the same order: the manager's lock first then
the
> allocator's lock.
> This also means that there is absolutely no change required in the manager.
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> File base/memory/discardable_memory_allocator_android.h (right):
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> base/memory/discardable_memory_allocator_android.h:58: // On Android ashmem is
> used to implement discardable memory. It is backed by a
> On 2014/04/26 00:13:37, reveman wrote:
> > nit: Please remove "Android" from here if this code is not longer supposed
to
> be
> > android specific.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> base/memory/discardable_memory_allocator_android.h:64: class
BASE_EXPORT_PRIVATE
> DiscardableMemoryAllocator {
> On 2014/04/26 00:13:37, reveman wrote:
> > What's the correct name for this class and these files? Is this still
Android
> > specific (DiscardableMemoryAllocatorAndroid as the file name suggests)? Or
> > should it be DiscardableMemoryAshmemAllocator?
> DiscardableMemoryAllocatorAshmem?
> > I'd like if these files/classes were renamed properly as part of this patch
> > where you introduce disacrdable_memory_ashmem.h.
> 
> I had suggested in my previous comment to keep the renaming part for a follow
up
> CL but you may have not seen it :) That said I don't mind doing the renaming
> here. DiscardableMemoryAshmemAllocator SGTM.
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> File base/memory/discardable_memory_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> base/memory/discardable_memory_android.cc:26: ((sizeof(void*) == 4) ? 512 : 16
*
> 1024) * 1024 * 1024;
> On 2014/04/26 00:13:37, reveman wrote:
> > I would prefer if used the same constant on 64bit to start with. 512MB is
fine
> > if that's the highest you think we should use on 32bit.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> base/memory/discardable_memory_android.cc:66: DISCARDABLE_MEMORY_TYPE_ANDROID,
> On 2014/04/26 00:13:37, reveman wrote:
> > Please rename this to DISCARDABLE_MEMORY_TYPE_ASHMEM as part of this patch.
> 
> Sure.
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> File base/memory/discardable_memory_ashmem.h (right):
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> base/memory/discardable_memory_ashmem.h:44: DiscardableMemoryManager* const
> manager_;
> On 2014/04/26 00:13:37, reveman wrote:
> > nit: I don't think we typically use "const" for pointers in chromium.
> 
> Unfortunately, yes. I saw that const is underused in Chrome whereas this usage
> is very typical in Google3. I have been quite successful though so far at
adding
> them in Chromium (it's actually the first time a reviewer comments on this).
In
> addition to what the style guide says I see one clear benefit here with this
> usage of const: a compilation error is generated in case the author forgets to
> initialize the field. You don't get this without the const.
> 
> And of course the style guide still applies here:
> "Pros: Easier for people to understand how variables are being used. Allows
the
> compiler to do better type checking, and, conceivably, generate better code.
> Helps people convince themselves of program correctness because they know the
> functions they call are limited in how they can modify your variables. Helps
> people know what functions are safe to use without locks in multi-threaded
> programs."
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> File base/memory/discardable_memory_manager_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> base/memory/discardable_memory_manager_unittest.cc:27: DCHECK(!bytes_ ||
bytes_
> == bytes);
> On 2014/04/26 00:13:37, reveman wrote:
> > how about we remove the |bytes| parameter from AllocateAndAcquireLock
instead?
> 
> Good idea, I was tempted to make the change too.
> 
>
https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
> base/memory/discardable_memory_manager_unittest.cc:32: DCHECK_EQ(is_locked_,
> is_allocated_);
> On 2014/04/26 00:13:37, reveman wrote:
> > hm, this seems unnecessary. What sequence of events does this cover that was
> not
> > covered before?
> 
> I think this was needed for the zero allocation (with my changes in
Unregister()
> though). I have now reverted my changes in the manager though.

Please hold on for a few minutes/hours :) I'm fixing a bug and adding a unit
test.

[Philippe, 2014-04-28 15:26:05]

PTAL :)

[reveman, 2014-04-28 15:39:16]

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_allocator_android.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_allocator_android.h:64: class BASE_EXPORT_PRIVATE
DiscardableMemoryAllocator {
On 2014/04/28 12:23:14, Philippe wrote:
> On 2014/04/26 00:13:37, reveman wrote:
> > What's the correct name for this class and these files? Is this still
Android
> > specific (DiscardableMemoryAllocatorAndroid as the file name suggests)? Or
> > should it be DiscardableMemoryAshmemAllocator?
> DiscardableMemoryAllocatorAshmem?
> > I'd like if these files/classes were renamed properly as part of this patch
> > where you introduce disacrdable_memory_ashmem.h.
> 
> I had suggested in my previous comment to keep the renaming part for a follow
up
> CL but you may have not seen it :) That said I don't mind doing the renaming
> here. DiscardableMemoryAshmemAllocator SGTM.

Sorry I missed that. I think it makes sense as part of this patch or in a
prerequisite patch if you want to keep it separate.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_ashmem.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_ashmem.h:44: DiscardableMemoryManager* const
manager_;
On 2014/04/28 12:23:14, Philippe wrote:
> On 2014/04/26 00:13:37, reveman wrote:
> > nit: I don't think we typically use "const" for pointers in chromium.
> 
> Unfortunately, yes. I saw that const is underused in Chrome whereas this usage
> is very typical in Google3. I have been quite successful though so far at
adding
> them in Chromium (it's actually the first time a reviewer comments on this).
In
> addition to what the style guide says I see one clear benefit here with this
> usage of const: a compilation error is generated in case the author forgets to
> initialize the field. You don't get this without the const.
> 
> And of course the style guide still applies here:
> "Pros: Easier for people to understand how variables are being used. Allows
the
> compiler to do better type checking, and, conceivably, generate better code.
> Helps people convince themselves of program correctness because they know the
> functions they call are limited in how they can modify your variables. Helps
> people know what functions are safe to use without locks in multi-threaded
> programs."

I agree that it's valuable and would prefer that it was used consistently
throughout chromium. I've been told to remove it from my patches in the past and
at the end of the day I prefer consistency. I'm perfectly fine with you keeping
this 'const' if you like. If you do, we should make sure it's consistent within
the discardable memory code at least.

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
File base/memory/discardable_memory.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory.h:120: static bool
PurgeForTestingSupported(DiscardableMemoryType type);
How about we just remove PurgeForTestingSupported now that it's supported by all
types?

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory.h:124: static void
PurgeForTesting(DiscardableMemoryType type);
Can you explain why we need to pass a type here instead of just purging memory
for all supported types?

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
File base/memory/discardable_memory_manager.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory_manager.h:24: virtual bool
AllocateAndAcquireLock() = 0;
Thanks for changing this. I think this is better.

[Philippe, 2014-04-28 15:53:45]

Thanks David! I will upload a new patch set shortly to address the compilation
issue on non-Android platforms.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_allocator_android.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_allocator_android.h:64: class BASE_EXPORT_PRIVATE
DiscardableMemoryAllocator {
On 2014/04/28 15:39:16, reveman wrote:
> On 2014/04/28 12:23:14, Philippe wrote:
> > On 2014/04/26 00:13:37, reveman wrote:
> > > What's the correct name for this class and these files? Is this still
> Android
> > > specific (DiscardableMemoryAllocatorAndroid as the file name suggests)? Or
> > > should it be DiscardableMemoryAshmemAllocator?
> > DiscardableMemoryAllocatorAshmem?
> > > I'd like if these files/classes were renamed properly as part of this
patch
> > > where you introduce disacrdable_memory_ashmem.h.
> > 
> > I had suggested in my previous comment to keep the renaming part for a
follow
> up
> > CL but you may have not seen it :) That said I don't mind doing the renaming
> > here. DiscardableMemoryAshmemAllocator SGTM.
> 
> Sorry I missed that. I think it makes sense as part of this patch or in a
> prerequisite patch if you want to keep it separate.

No problem, I was worried about increasing the diff but it doesn't look so bad
actually with a proper similarity percentage so I'm more than fine with doing
the rename now.

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
File base/memory/discardable_memory_ashmem.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/180001/base/memory/disc...
base/memory/discardable_memory_ashmem.h:44: DiscardableMemoryManager* const
manager_;
On 2014/04/28 15:39:16, reveman wrote:
> On 2014/04/28 12:23:14, Philippe wrote:
> > On 2014/04/26 00:13:37, reveman wrote:
> > > nit: I don't think we typically use "const" for pointers in chromium.
> > 
> > Unfortunately, yes. I saw that const is underused in Chrome whereas this
usage
> > is very typical in Google3. I have been quite successful though so far at
> adding
> > them in Chromium (it's actually the first time a reviewer comments on this).
> In
> > addition to what the style guide says I see one clear benefit here with this
> > usage of const: a compilation error is generated in case the author forgets
to
> > initialize the field. You don't get this without the const.
> > 
> > And of course the style guide still applies here:
> > "Pros: Easier for people to understand how variables are being used. Allows
> the
> > compiler to do better type checking, and, conceivably, generate better code.
> > Helps people convince themselves of program correctness because they know
the
> > functions they call are limited in how they can modify your variables. Helps
> > people know what functions are safe to use without locks in multi-threaded
> > programs."
> 
> I agree that it's valuable and would prefer that it was used consistently
> throughout chromium. I've been told to remove it from my patches in the past
and
> at the end of the day I prefer consistency. I'm perfectly fine with you
keeping
> this 'const' if you like. If you do, we should make sure it's consistent
within
> the discardable memory code at least.
> 

I see, thanks. I went over all our discardable memory files and AFAICS there is
no other const opportunity. The reason for that I guess is that we rarely pass
raw pointers in constructors in the discardable memory codebase.

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
File base/memory/discardable_memory.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory.h:120: static bool
PurgeForTestingSupported(DiscardableMemoryType type);
On 2014/04/28 15:39:16, reveman wrote:
> How about we just remove PurgeForTestingSupported now that it's supported by
all
> types?

I would be happy to do that but unfortunately the malloc type doesn't support
purging. I wasn't very comfortable with adding some extra global state keeping
track of the allocations just for testing but I can reconsider that.

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory.h:124: static void
PurgeForTesting(DiscardableMemoryType type);
On 2014/04/28 15:39:16, reveman wrote:
> Can you explain why we need to pass a type here instead of just purging memory
> for all supported types?

Just to clarify, I was initially overriding the preferred type in the test
fixture and having PurgeForTesting() rely on the current global preferred type
but I was hitting a DCHECK() (which is quite valuable IMO) asserting that we are
setting the preferred type only once. So I thought it would be easier to avoid
messing up with the global state.

Does that make sense?

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
File base/memory/discardable_memory_manager.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory_manager.h:24: virtual bool
AllocateAndAcquireLock() = 0;
On 2014/04/28 15:39:16, reveman wrote:
> Thanks for changing this. I think this is better.

Agreed :)

[reveman, 2014-04-28 16:02:24]

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
File base/memory/discardable_memory.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory.h:120: static bool
PurgeForTestingSupported(DiscardableMemoryType type);
On 2014/04/28 15:53:45, Philippe wrote:
> On 2014/04/28 15:39:16, reveman wrote:
> > How about we just remove PurgeForTestingSupported now that it's supported by
> all
> > types?
> 
> I would be happy to do that but unfortunately the malloc type doesn't support
> purging. I wasn't very comfortable with adding some extra global state keeping
> track of the allocations just for testing but I can reconsider that. 

I think purging in the case of the malloc type is a NoOp. Everything that can be
purged is already purged.

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory.h:124: static void
PurgeForTesting(DiscardableMemoryType type);
On 2014/04/28 15:53:45, Philippe wrote:
> On 2014/04/28 15:39:16, reveman wrote:
> > Can you explain why we need to pass a type here instead of just purging
memory
> > for all supported types?
> 
> Just to clarify, I was initially overriding the preferred type in the test
> fixture and having PurgeForTesting() rely on the current global preferred type
> but I was hitting a DCHECK() (which is quite valuable IMO) asserting that we
are
> setting the preferred type only once. So I thought it would be easier to avoid
> messing up with the global state.
> 
> Does that make sense?

I agree that we shouldn't mess with preferred type here. It's just not clear to
me why PurgeForTesting can't look like this on Android:

PurgeForTesting {
  g_context.Pointer()->manager.PurgeAll();
  internal::DiscardableMemoryEmulated::PurgeForTesting();
}

https://chromiumcodereview.appspot.com/195863005/diff/270001/base/memory/disc...
File base/memory/discardable_memory_mac.cc (right):

https://chromiumcodereview.appspot.com/195863005/diff/270001/base/memory/disc...
base/memory/discardable_memory_mac.cc:169:
internal::DiscardableMemoryEmulated::PurgeForTesting();
if we add a type parameter then we shouldn't purge emulated type here.

[Philippe, 2014-04-28 16:31:21]

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
File base/memory/discardable_memory.h (right):

https://chromiumcodereview.appspot.com/195863005/diff/250001/base/memory/disc...
base/memory/discardable_memory.h:124: static void
PurgeForTesting(DiscardableMemoryType type);
On 2014/04/28 16:02:24, reveman wrote:
> On 2014/04/28 15:53:45, Philippe wrote:
> > On 2014/04/28 15:39:16, reveman wrote:
> > > Can you explain why we need to pass a type here instead of just purging
> memory
> > > for all supported types?
> > 
> > Just to clarify, I was initially overriding the preferred type in the test
> > fixture and having PurgeForTesting() rely on the current global preferred
type
> > but I was hitting a DCHECK() (which is quite valuable IMO) asserting that we
> are
> > setting the preferred type only once. So I thought it would be easier to
avoid
> > messing up with the global state.
> > 
> > Does that make sense?
> 
> I agree that we shouldn't mess with preferred type here. It's just not clear
to
> me why PurgeForTesting can't look like this on Android:
> 
> PurgeForTesting {
>   g_context.Pointer()->manager.PurgeAll();
>   internal::DiscardableMemoryEmulated::PurgeForTesting();
> }

Yeah, good idea. I got rid of PurgeForTestingSupported().

https://chromiumcodereview.appspot.com/195863005/diff/270001/base/memory/disc...
File base/memory/discardable_memory_mac.cc (right):

https://chromiumcodereview.appspot.com/195863005/diff/270001/base/memory/disc...
base/memory/discardable_memory_mac.cc:169:
internal::DiscardableMemoryEmulated::PurgeForTesting();
On 2014/04/28 16:02:24, reveman wrote:
> if we add a type parameter then we shouldn't purge emulated type here.

Yes, that was a mistake. I got rid of the type though.

https://chromiumcodereview.appspot.com/195863005/diff/300001/base/memory/disc...
File base/memory/discardable_memory_unittest.cc (right):

https://chromiumcodereview.appspot.com/195863005/diff/300001/base/memory/disc...
base/memory/discardable_memory_unittest.cc:9: #include "base/logging.h"
FYI, I will remove this stale change in the next patch set.

[reveman, 2014-04-28 17:13:33]

lgtm

[Philippe, 2014-04-28 17:34:21]

Thanks David! I'm adding William for the approval.

[pliard, 2014-04-29 19:02:27]

On 2014/04/28 17:34:21, Philippe wrote:
> Thanks David! I'm adding William for the approval.

Ping William in case you wouldn't have seen this :)

[willchan no longer on Chromium, 2014-04-30 06:38:20]

Reviewing this made me feel pretty happy about the refactoring y'all are doing
here. It looks fairly elegant how it all comes together.

I have one concern that there are some invariants that need to be followed
across all the different layers here in the ashmem allocator case. The emulated
case is thankfully simple, but the ashmem case involves some re-entrancy
situations that worry me a bit, and I also had to make sure that we didn't
re-enter the manager from within the manager allocation implementation (e.g.
Allocation::Purge() and Allocation::ReleaseLock()). In the emulated case, this
was obvious, but in the ashmem case, I had to read through to make sure we
didn't have any re-entrancy.

While analyzing that, I noted that the ashmem allocation's Unlock()
implementation called into the manager which then called back into the
allocation. It made me realize that this re-entrancy happens because of
implementing both the DiscardableMemory and DiscardableMemoryManagerAllocation
interfaces in the same object. Overall, I think this is still the right
decision, because increasing the number of classes imposes more mental burden,
but if we ever have an especially complicated implementation, it may be
advisable to use a class to implement each interface separately, which would
obviously change the ownership model.

I mostly wanted to write this down so you folks were aware of the _weak_
concerns I have in this area, and can keep it in mind as the code evolves in the
future. In any case, I don't see it as an issue.

I also had a dumb question about the _toolset stuff as I'm not familiar with it.
I trust that you're doing the right thing, but would like it explained.

LGTM.

Oh, and thanks to the both of you for the clean CL. I had no substantive
comments which is always a great thing :)

https://codereview.chromium.org/195863005/diff/180001/base/memory/discardable...
File base/memory/discardable_memory_ashmem.h (right):

https://codereview.chromium.org/195863005/diff/180001/base/memory/discardable...
base/memory/discardable_memory_ashmem.h:44: DiscardableMemoryManager* const
manager_;
On 2014/04/28 15:53:45, Philippe wrote:
> On 2014/04/28 15:39:16, reveman wrote:
> > On 2014/04/28 12:23:14, Philippe wrote:
> > > On 2014/04/26 00:13:37, reveman wrote:
> > > > nit: I don't think we typically use "const" for pointers in chromium.
> > > 
> > > Unfortunately, yes. I saw that const is underused in Chrome whereas this
> usage
> > > is very typical in Google3. I have been quite successful though so far at
> > adding
> > > them in Chromium (it's actually the first time a reviewer comments on
this).
> > In
> > > addition to what the style guide says I see one clear benefit here with
this
> > > usage of const: a compilation error is generated in case the author
forgets
> to
> > > initialize the field. You don't get this without the const.
> > > 
> > > And of course the style guide still applies here:
> > > "Pros: Easier for people to understand how variables are being used.
Allows
> > the
> > > compiler to do better type checking, and, conceivably, generate better
code.
> > > Helps people convince themselves of program correctness because they know
> the
> > > functions they call are limited in how they can modify your variables.
Helps
> > > people know what functions are safe to use without locks in multi-threaded
> > > programs."
> > 
> > I agree that it's valuable and would prefer that it was used consistently
> > throughout chromium. I've been told to remove it from my patches in the past
> and
> > at the end of the day I prefer consistency. I'm perfectly fine with you
> keeping
> > this 'const' if you like. If you do, we should make sure it's consistent
> within
> > the discardable memory code at least.
> > 
> 
> I see, thanks. I went over all our discardable memory files and AFAICS there
is
> no other const opportunity. The reason for that I guess is that we rarely pass
> raw pointers in constructors in the discardable memory codebase.  

FWIW, I am positive on const pointers as well, but I don't comment on it in
usage. And yes, consistency is most important.

https://codereview.chromium.org/195863005/diff/340001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/195863005/diff/340001/base/base.gyp#newcode758
base/base.gyp:758: ['OS == "android" and _toolset == "target"', {
Can you explain why this is restricted to "target"? I am not familiar with this
conditional.

[Philippe, 2014-04-30 08:38:09]

Thanks William!

I agree that the re-entrancy happening in DiscardableMemoryAshmem and
DiscardableMemoryEmulated through the manager may not be ideal in the long term
but it's at least reasonably safe in the current state.

https://codereview.chromium.org/195863005/diff/340001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/195863005/diff/340001/base/base.gyp#newcode758
base/base.gyp:758: ['OS == "android" and _toolset == "target"', {
On 2014/04/30 06:38:21, willchan wrote:
> Can you explain why this is restricted to "target"? I am not familiar with
this
> conditional.

David also asked about it in the first patch set. Let me paste my reply below:
On platforms where cross-compilation is used, we distinguish the target (Android
here) and the host (Linux). In an Android build we build libbase not only for
the target but also for the host since we have some standalone binaries
(depending on base) that we use as part of the build itself and for testing.

[Philippe, 2014-04-30 08:39:45]

The CQ bit was checked by pliard@chromium.org

[commit-bot: I haz the power, 2014-04-30 08:40:07]

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

[commit-bot: I haz the power, 2014-04-30 08:43:56]

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

[commit-bot: I haz the power, 2014-04-30 08:43:56]

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

[Philippe, 2014-04-30 08:47:40]

The CQ bit was checked by pliard@chromium.org

[commit-bot: I haz the power, 2014-04-30 08:48:18]

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

[commit-bot: I haz the power, 2014-04-30 08:53:32]

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

[commit-bot: I haz the power, 2014-04-30 08:53:32]

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

[Philippe, 2014-04-30 10:54:47]

Committed patchset #15 manually as r267170 (presubmit successful).

[henrika (OOO until Aug 14), 2014-04-30 11:06:26]

A revert of this CL has been created in
https://codereview.chromium.org/264633002/ by henrika@chromium.org.

The reason for reverting is: Speculative revert since it seems to break Android
GN. See
http://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/593.

[Nikita (slow), 2014-04-30 11:08:30]

A revert of this CL has been created in
https://codereview.chromium.org/260673003/ by nkostylev@chromium.org.

The reason for reverting is: Fails compile

http://goo.gl/qePAC4.