Align ashmem region sizes to page size after ashmem creation failed.

Align ashmem region sizes to page size after ashmem creation failed.                                                                                                                                                                                                                                                           

When ashmem creation failed the new size (twice smaller) used to retry wasn't
page-aligned.

This also moves the page alignment function to the allocator's anonymous
namespace since it's now an implementation detail (since the allocator is
always used).

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

[Philippe, 2014-01-14 15:51:26]

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.cc (right):

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.cc:424: ashmem_region_size_ =
region_size;
FYI, I added this logic back here mainly for testability. I needed to call
ashmem_region_size() in the new unit test.

[willchan no longer on Chromium, 2014-01-15 00:37:48]

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.cc (right):

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.cc:424: ashmem_region_size_ =
region_size;
On 2014/01/14 15:51:26, Philippe wrote:
> FYI, I added this logic back here mainly for testability. I needed to call
> ashmem_region_size() in the new unit test.

But isn't this bad for performance? We don't want to always have
ashmem_region_size be small if it fails to create once, right? Why don't you
just use this to record the last ashmem region size?

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.h (right):

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.h:48: size_t
ashmem_region_size();
Should be const, and then you'd need to made the Lock mutable.

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.h:58: base::Lock lock_;
Shouldn't need base:: here.

[Philippe, 2014-01-15 09:18:11]

Thanks William!

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.cc (right):

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.cc:424: ashmem_region_size_ =
region_size;
On 2014/01/15 00:37:49, willchan wrote:
> On 2014/01/14 15:51:26, Philippe wrote:
> > FYI, I added this logic back here mainly for testability. I needed to call
> > ashmem_region_size() in the new unit test.
> 
> But isn't this bad for performance? We don't want to always have
> ashmem_region_size be small if it fails to create once, right? Why don't you
> just use this to record the last ashmem region size?

Yes, good idea. I don't have real proof that it would be really bad but let's be
safe :)

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.h (right):

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.h:48: size_t
ashmem_region_size();
On 2014/01/15 00:37:49, willchan wrote:
> Should be const, and then you'd need to made the Lock mutable.

Yeah, I thought it would have been slightly unfortunate to make the lock mutable
only for this testing method which doesn't need to be const in practice
(although having a getter like this not const looks really weird). But I'm also
fine with the 'mutable' :)

Completely off topic but I think we might need to reconsider your delegate idea
at some point to improve testability. It's still ok for now but at some point I
would like to consider adding some form of garbage collection of the unpinned
subranges which will depend on an ioctl() implemented by ashmem telling us
whether a subrange got evicted. We would need to mock that out for testing since
we would have no way otherwise to control the eviction of subranges.

The problem I would like to address with this is the fact that currently we can
end up with a few regions whose pages got completely evicted. Unfortunately we
have no way currently to reuse that space if the client keeps his (evicted)
DiscardableMemory instances alive. And he will keep them alive since image
decoding is moving to an "infinite" cache. This can lead to "bloating" the
address space since we are keeping the regions mapped (even though they have 0
private dirty bytes) which can in turn make large mmaps (e.g. by v8) fail. 
In fact the bug specified in the description is about that (although the problem
already existed with ashmem before we use the allocator). The website specified
in the bug looks like a pathological case though.

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.h:58: base::Lock lock_;
On 2014/01/15 00:37:49, willchan wrote:
> Shouldn't need base:: here.

Done.

[willchan no longer on Chromium, 2014-01-16 01:52:36]

LGTM

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.cc (right):

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.cc:424: ashmem_region_size_ =
region_size;
On 2014/01/15 09:18:12, Philippe wrote:
> On 2014/01/15 00:37:49, willchan wrote:
> > On 2014/01/14 15:51:26, Philippe wrote:
> > > FYI, I added this logic back here mainly for testability. I needed to call
> > > ashmem_region_size() in the new unit test.
> > 
> > But isn't this bad for performance? We don't want to always have
> > ashmem_region_size be small if it fails to create once, right? Why don't you
> > just use this to record the last ashmem region size?
> 
> Yes, good idea. I don't have real proof that it would be really bad but let's
be
> safe :) 

Ah yes, I have no proof either :)

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.h (right):

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.h:48: size_t
ashmem_region_size();
On 2014/01/15 09:18:12, Philippe wrote:
> On 2014/01/15 00:37:49, willchan wrote:
> > Should be const, and then you'd need to made the Lock mutable.
> 
> Yeah, I thought it would have been slightly unfortunate to make the lock
mutable
> only for this testing method which doesn't need to be const in practice
> (although having a getter like this not const looks really weird). But I'm
also
> fine with the 'mutable' :)
> 
> Completely off topic but I think we might need to reconsider your delegate
idea
> at some point to improve testability. It's still ok for now but at some point
I
> would like to consider adding some form of garbage collection of the unpinned
> subranges which will depend on an ioctl() implemented by ashmem telling us
> whether a subrange got evicted. We would need to mock that out for testing
since
> we would have no way otherwise to control the eviction of subranges.
> 
> The problem I would like to address with this is the fact that currently we
can
> end up with a few regions whose pages got completely evicted. Unfortunately we
> have no way currently to reuse that space if the client keeps his (evicted)
> DiscardableMemory instances alive. And he will keep them alive since image
> decoding is moving to an "infinite" cache. This can lead to "bloating" the
> address space since we are keeping the regions mapped (even though they have 0
> private dirty bytes) which can in turn make large mmaps (e.g. by v8) fail. 
> In fact the bug specified in the description is about that (although the
problem
> already existed with ashmem before we use the allocator). The website
specified
> in the bug looks like a pathological case though.

If there is any context on this "infinite" case thing, I'd love to read.
Bugs/docs?

[Philippe, 2014-01-16 09:24:21]

Thanks William!

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.h (right):

https://codereview.chromium.org/136723005/diff/30001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.h:48: size_t
ashmem_region_size();
On 2014/01/16 01:52:37, willchan wrote:
> On 2014/01/15 09:18:12, Philippe wrote:
> > On 2014/01/15 00:37:49, willchan wrote:
> > > Should be const, and then you'd need to made the Lock mutable.
> > 
> > Yeah, I thought it would have been slightly unfortunate to make the lock
> mutable
> > only for this testing method which doesn't need to be const in practice
> > (although having a getter like this not const looks really weird). But I'm
> also
> > fine with the 'mutable' :)
> > 
> > Completely off topic but I think we might need to reconsider your delegate
> idea
> > at some point to improve testability. It's still ok for now but at some
point
> I
> > would like to consider adding some form of garbage collection of the
unpinned
> > subranges which will depend on an ioctl() implemented by ashmem telling us
> > whether a subrange got evicted. We would need to mock that out for testing
> since
> > we would have no way otherwise to control the eviction of subranges.
> > 
> > The problem I would like to address with this is the fact that currently we
> can
> > end up with a few regions whose pages got completely evicted. Unfortunately
we
> > have no way currently to reuse that space if the client keeps his (evicted)
> > DiscardableMemory instances alive. And he will keep them alive since image
> > decoding is moving to an "infinite" cache. This can lead to "bloating" the
> > address space since we are keeping the regions mapped (even though they have
0
> > private dirty bytes) which can in turn make large mmaps (e.g. by v8) fail. 
> > In fact the bug specified in the description is about that (although the
> problem
> > already existed with ashmem before we use the allocator). The website
> specified
> > in the bug looks like a pathological case though.
> 
> If there is any context on this "infinite" case thing, I'd love to read.
> Bugs/docs? 

I'm not aware of bugs/docs for this new infinite cache but hclam@ is. I've just
created this bug and hclam@ is also CC'ed on it :)
https://code.google.com/p/chromium/issues/detail?id=334996

[commit-bot: I haz the power, 2014-01-16 09:24:37]

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

[commit-bot: I haz the power, 2014-01-16 15:54:45]

Change committed as 245201