base: Discardable memory types.

base: Discardable memory types.

This adds a DiscardableMemoryType enum and a function that
can be used to select what type of discardable memory to
use. This is generally useful for debugging purposes and
when evaluating the performance of one implementation vs
another.

The unit test framework for discardable memory is improved
slightly by testing every supported type on each platform
rather then just the one specific type.

Furthermore, it allows us to expose ashmem based
discardable memory on ChromeOS in about:flags before we
make it default.

No change in behavior unless the new
--use-discardable-memory=type switch is used.

BUG=327516
TEST=base_unitttest --gtest_filter=DiscardableMemory*
TBR=tomhudson@google.com

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

[reveman, 2013-12-13 18:21:53]

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

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.h:19: DISCARDABLE_MEMORY_ANDROID,
Follow up patches will replace this with DISCARDABLE_MEMORY_ASHMEM, which will
be supported on both android and chromeos.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.h:20: DISCARDABLE_MEMORY_MAC,
We might want to rename this to DISCARDABLE_MEMORY_VM or
DISCARDABLE_MEMORY_VM_PURGEABLE in a follow up patch and move the implementation
out of _mac.cc if we need some implementation specific unit tests.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.h:97: static bool PurgeForTestingSupported();
This is now only true if all supported types can be purged.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.h:101: static void PurgeForTesting();
This is now purging all supported types and not just the current one.

I think we should remove this from the general DiscardableMemory API unless it
can be supported by all types. Implementation specific test can be used for this
instead.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_unittest.cc:35: TEST_P(DiscardableMemoryTest,
TooLargeAllocationFails) {
Follow up patch will move this to _ashmem_unittest.cc.

[reveman, 2013-12-16 21:05:51]

More discardable memory refactor. This is particularly useful for upcoming use
of ashmem on ChromeOS, which will require a runtime check to determine if
supported. Please take a look.

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

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.h:24: enum LockDiscardableMemoryStatus {
How about we rename this to DiscardableMemoryLockStatus and use
DISCARDABLE_MEMORY_LOCK_STATUS_ prefix for all values? The prefix for the type
values above would then become DISCARDABLE_MEMORY_TYPE_.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_unittest.cc:127:
::testing::ValuesIn(DiscardableMemory::GetSupportedTypes()));
Runs all tests for all supported discardable memory types.

This file should only contain tests that apply to the general DiscardableMemory
API and not any implementation specific tests such as the current
TooLargeAllocationFails test.

All implementation specific tests should be moved into implementation specific
_unittest.cc files. I'm planning to do this in a follow up rather than include
it in this patch though.

[Philippe, 2013-12-17 14:28:21]

Thanks David! This looks mostly good to me. I only have one main comment on how
we could improve maintainability in my opinion by reducing the amount of
duplicated code/decreasing the complexity in every DiscardableMemory
implementation.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.cc:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
Nit: I believe there is no '(c)' anymore for new files.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.cc:17: DiscardableMemoryType v;
Nit: maybe call this 'value' or 'type'.

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

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.h:24: enum LockDiscardableMemoryStatus {
On 2013/12/16 21:05:51, David Reveman wrote:
> How about we rename this to DiscardableMemoryLockStatus and use
> DISCARDABLE_MEMORY_LOCK_STATUS_ prefix for all values? The prefix for the type
> values above would then become DISCARDABLE_MEMORY_TYPE_.

SGTM

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_android.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_android.cc:122: struct SupportedTypeVector {
This is just a suggestion but I see that we have now this struct duplicated in
many files with the LazyInstance and GetSupportedTypes() too and I see at the
same time that you now have discardable_memory.cc which looks like a good place
to me to have those details.

So I was thinking that maybe you could declare somewhere (e.g. at the bottom of
discardable_memory.h or maybe in discardable_memory_internal.h):
namespace internal {

void
InitializeSupportedDiscardableMemoryTypes(std::vector<DiscardableMemoryType>*
types);

const std::vector<DiscardableMemoryType>& GetSupportedDiscardableMemoryTypes();

}

DiscardableMemory::GetType() and DiscardableMemory::SetType() could possibly be
moved to that internal namespace too since I don't expect most of the clients to
mess up with that.

And then you would have the DiscardableMemory implementations define
InitializeSupportedTypes() like this for instance:
void InitializeSupportedTypes(std::vector<DiscardableMemoryType>* types) {
  types->push_back(DISCARDABLE_MEMORY_ANDROID);
  types->push_back(DISCARDABLE_MEMORY_EMULATED);
}

Then in discardable_memory.cc you would have:
namespace {

struct GlobalContext {
  GlobalContext() {
    internal::InitializeSupportedTypes(&supported_types);
    DCHECK(!supported_types.empty());
    current_type = supported_types.front();
  }

  std::vector<DiscardableMemoryType> supported_types;
  DiscardableMemoryType current_type;
};

base::LazyInstance<GlobalContext> g_context...

const std::vector<DiscardableMemoryType>& GetSupportedDiscardableMemoryTypes() {
  return g_context.Get().supported_types;
}

} 

What do you think?

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_android.cc:269: if (!memory->Initialize())
I have to say that I look forward to seeing DiscardableMemoryEmulated being
reused somewhere to justify this pattern being spread across the various
DiscardableMemory implementations :)

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_mac.cc:31: bool Initialize() {
Are you making this change for consistency with the other implementations? FWIW,
I preferred the previous version (which could have been improved with a static
factory method) but consistency is important too.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_unittest.cc:35: TEST_P(DiscardableMemoryTest,
TooLargeAllocationFails) {
On 2013/12/13 18:21:53, David Reveman wrote:
> Follow up patch will move this to _ashmem_unittest.cc.

SGTM

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_unittest.cc:56:
ASSERT_TRUE(std::find_if(DiscardableMemory::GetSupportedTypes().begin(),
JFYI, std::count_if() would be slightly more compact since you would not have to
compare the result with DiscardableMemory::GetSupportedTypes().end() but I'm
also fine with this.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_unittest.cc:127:
::testing::ValuesIn(DiscardableMemory::GetSupportedTypes()));
On 2013/12/16 21:05:51, David Reveman wrote:
> Runs all tests for all supported discardable memory types.
> 
> This file should only contain tests that apply to the general
DiscardableMemory
> API and not any implementation specific tests such as the current
> TooLargeAllocationFails test.
> 
> All implementation specific tests should be moved into implementation specific
> _unittest.cc files. I'm planning to do this in a follow up rather than include
> it in this patch though.

SGTM

[Philippe, 2013-12-17 15:26:22]

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_android.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_android.cc:269: if (!memory->Initialize())
On 2013/12/17 14:28:21, Philippe wrote:
> I have to say that I look forward to seeing DiscardableMemoryEmulated being
> reused somewhere to justify this pattern being spread across the various
> DiscardableMemory implementations :)

Just realized that this may sound harsh. Sorry if it was, I didn't mean it.

[reveman, 2013-12-18 08:12:38]

PTAL

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.cc:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
On 2013/12/17 14:28:21, Philippe wrote:
> Nit: I believe there is no '(c)' anymore for new files.

Done.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.cc:17: DiscardableMemoryType v;
On 2013/12/17 14:28:21, Philippe wrote:
> Nit: maybe call this 'value' or 'type'.

Done.

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

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory.h:24: enum LockDiscardableMemoryStatus {
On 2013/12/17 14:28:21, Philippe wrote:
> On 2013/12/16 21:05:51, David Reveman wrote:
> > How about we rename this to DiscardableMemoryLockStatus and use
> > DISCARDABLE_MEMORY_LOCK_STATUS_ prefix for all values? The prefix for the
type
> > values above would then become DISCARDABLE_MEMORY_TYPE_.
> 
> SGTM

Done.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_android.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_android.cc:122: struct SupportedTypeVector {
On 2013/12/17 14:28:21, Philippe wrote:
> This is just a suggestion but I see that we have now this struct duplicated in
> many files with the LazyInstance and GetSupportedTypes() too and I see at the
> same time that you now have discardable_memory.cc which looks like a good
place
> to me to have those details.
> 
> So I was thinking that maybe you could declare somewhere (e.g. at the bottom
of
> discardable_memory.h or maybe in discardable_memory_internal.h):
> namespace internal {
> 
> void
> InitializeSupportedDiscardableMemoryTypes(std::vector<DiscardableMemoryType>*
> types);
> 
> const std::vector<DiscardableMemoryType>&
GetSupportedDiscardableMemoryTypes();
> 
> }
> 
> DiscardableMemory::GetType() and DiscardableMemory::SetType() could possibly
be
> moved to that internal namespace too since I don't expect most of the clients
to
> mess up with that.
> 
> And then you would have the DiscardableMemory implementations define
> InitializeSupportedTypes() like this for instance:
> void InitializeSupportedTypes(std::vector<DiscardableMemoryType>* types) {
>   types->push_back(DISCARDABLE_MEMORY_ANDROID);
>   types->push_back(DISCARDABLE_MEMORY_EMULATED);
> }
> 
> Then in discardable_memory.cc you would have:
> namespace {
> 
> struct GlobalContext {
>   GlobalContext() {
>     internal::InitializeSupportedTypes(&supported_types);
>     DCHECK(!supported_types.empty());
>     current_type = supported_types.front();
>   }
> 
>   std::vector<DiscardableMemoryType> supported_types;
>   DiscardableMemoryType current_type;
> };
> 
> base::LazyInstance<GlobalContext> g_context...
> 
> const std::vector<DiscardableMemoryType>& GetSupportedDiscardableMemoryTypes()
{
>   return g_context.Get().supported_types;
> }
> 
> } 
> 
> What do you think?

Please have a look at the latest code. I simply made GetSupportedTypes() take a
pointer to a vector instead. The only code that prefer something else is the
unit test code and small utility function solved that.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_android.cc:269: if (!memory->Initialize())
On 2013/12/17 15:26:22, Philippe wrote:
> On 2013/12/17 14:28:21, Philippe wrote:
> > I have to say that I look forward to seeing DiscardableMemoryEmulated being
> > reused somewhere to justify this pattern being spread across the various
> > DiscardableMemory implementations :)
> 
> Just realized that this may sound harsh. Sorry if it was, I didn't mean it.

DiscardableMemoryEmulated is likely not going to be reused. This is for
consistency and make us write code that could be reused if needed.

When it comes to code duplication. I'm mostly concerned with duplication of code
that contains some logic that could potentially be broken. I think this is about
as simple as a one line statement and copying these few lines to a few places
rather than copying a one line statement that calls a static method makes little
difference to me.

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_mac.cc:31: bool Initialize() {
On 2013/12/17 14:28:21, Philippe wrote:
> Are you making this change for consistency with the other implementations?
FWIW,
> I preferred the previous version (which could have been improved with a static
> factory method) but consistency is important too. 

This is for consistency. I might also move this implementation to a separate
file and add specific unit tests in the future as part of getting rid of
PurgeForTesting().

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_unittest.cc:56:
ASSERT_TRUE(std::find_if(DiscardableMemory::GetSupportedTypes().begin(),
On 2013/12/17 14:28:21, Philippe wrote:
> JFYI, std::count_if() would be slightly more compact since you would not have
to
> compare the result with DiscardableMemory::GetSupportedTypes().end() but I'm
> also fine with this.

Good idea. Done.

[Philippe, 2013-12-18 09:07:46]

Thanks David! I like what you did with the vector of supported types. LGTM since
I only have tiny nits left (modulo the name of the command line flag maybe).

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_android.cc (right):

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_android.cc:122: struct SupportedTypeVector {
On 2013/12/18 08:12:38, David Reveman wrote:
> On 2013/12/17 14:28:21, Philippe wrote:
> > This is just a suggestion but I see that we have now this struct duplicated
in
> > many files with the LazyInstance and GetSupportedTypes() too and I see at
the
> > same time that you now have discardable_memory.cc which looks like a good
> place
> > to me to have those details.
> > 
> > So I was thinking that maybe you could declare somewhere (e.g. at the bottom
> of
> > discardable_memory.h or maybe in discardable_memory_internal.h):
> > namespace internal {
> > 
> > void
> >
InitializeSupportedDiscardableMemoryTypes(std::vector<DiscardableMemoryType>*
> > types);
> > 
> > const std::vector<DiscardableMemoryType>&
> GetSupportedDiscardableMemoryTypes();
> > 
> > }
> > 
> > DiscardableMemory::GetType() and DiscardableMemory::SetType() could possibly
> be
> > moved to that internal namespace too since I don't expect most of the
clients
> to
> > mess up with that.
> > 
> > And then you would have the DiscardableMemory implementations define
> > InitializeSupportedTypes() like this for instance:
> > void InitializeSupportedTypes(std::vector<DiscardableMemoryType>* types) {
> >   types->push_back(DISCARDABLE_MEMORY_ANDROID);
> >   types->push_back(DISCARDABLE_MEMORY_EMULATED);
> > }
> > 
> > Then in discardable_memory.cc you would have:
> > namespace {
> > 
> > struct GlobalContext {
> >   GlobalContext() {
> >     internal::InitializeSupportedTypes(&supported_types);
> >     DCHECK(!supported_types.empty());
> >     current_type = supported_types.front();
> >   }
> > 
> >   std::vector<DiscardableMemoryType> supported_types;
> >   DiscardableMemoryType current_type;
> > };
> > 
> > base::LazyInstance<GlobalContext> g_context...
> > 
> > const std::vector<DiscardableMemoryType>&
GetSupportedDiscardableMemoryTypes()
> {
> >   return g_context.Get().supported_types;
> > }
> > 
> > } 
> > 
> > What do you think?
> 
> Please have a look at the latest code. I simply made GetSupportedTypes() take
a
> pointer to a vector instead. The only code that prefer something else is the
> unit test code and small utility function solved that.

Yeah, even better!

https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_android.cc:269: if (!memory->Initialize())
On 2013/12/18 08:12:38, David Reveman wrote:
> On 2013/12/17 15:26:22, Philippe wrote:
> > On 2013/12/17 14:28:21, Philippe wrote:
> > > I have to say that I look forward to seeing DiscardableMemoryEmulated
being
> > > reused somewhere to justify this pattern being spread across the various
> > > DiscardableMemory implementations :)
> > 
> > Just realized that this may sound harsh. Sorry if it was, I didn't mean it.
> 
> DiscardableMemoryEmulated is likely not going to be reused. This is for
> consistency and make us write code that could be reused if needed.
> 
> When it comes to code duplication. I'm mostly concerned with duplication of
code
> that contains some logic that could potentially be broken. I think this is
about
> as simple as a one line statement and copying these few lines to a few places
> rather than copying a one line statement that calls a static method makes
little
> difference to me.

Those things are personal preferences obviously and not covered by the style
guide anyway so I'm also fine with it :) I'm just not a big fan of the
constructor + init method pattern when it's not strictly needed (e.g. for code
reuse or when we can't afford the extra heap allocation).
The main reasons in my opinion are the extra code needed on the call site, the
fact that it's easy to miss the Init() call and it's even possible to call
Init() twice.
In addition to that the fact that the object is constructed in two steps
prevents it from being immutable (which is not always possible anyway though).
In my opinion we should not prematurely optimize for code reuse when it implies
extra complexity in the interface. If there is need to reuse at some point then
we can refactor just like you are doing actually. But this is just my opinion
and mostly nitpicking :)

https://codereview.chromium.org/114923005/diff/20001/base/memory/discardable_...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/114923005/diff/20001/base/memory/discardable_...
base/memory/discardable_memory.h:65: // Get system supported discardable memory
types. Preferred type at the front
Tiny nit: I would s/Get/Gets and same below for consistency with the other
comments although PurgeForTesting* don't seem to follow this rule.

https://codereview.chromium.org/114923005/diff/20001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/114923005/diff/20001/content/public/common/co...
content/public/common/content_switches.cc:912: const char
kUseDiscardableMemory[] = "use-discardable-memory";
Nit: I would call it "discardable-memory-type" instead since the current flag
suggests IMO that it's a boolean switch.

https://codereview.chromium.org/114923005/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/114923005/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.cc:417: if (requested_type_name == "none") {
Nit: I would remove the braces for consistency with the rest of the file.

https://codereview.chromium.org/114923005/diff/20001/webkit/child/webkitplatf...
File webkit/child/webkitplatformsupport_child_impl.cc (right):

https://codereview.chromium.org/114923005/diff/20001/webkit/child/webkitplatf...
webkit/child/webkitplatformsupport_child_impl.cc:96: base::DiscardableMemoryType
type = base::DiscardableMemory::GetType();
Just realized that there was this SupportedNatively() check here :) Does this
mean that emulated DM isn't used (yet)?

[willchan no longer on Chromium, 2013-12-18 16:42:50]

I'm in classes today and tomorrow. I'll try to sneak some review time
in between.

On Wed, Dec 18, 2013 at 1:07 AM,  <pliard@chromium.org> wrote:
> Thanks David! I like what you did with the vector of supported types. LGTM
> since
> I only have tiny nits left (modulo the name of the command line flag maybe).
>
>
>
>
https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
> File base/memory/discardable_memory_android.cc (right):
>
>
https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
> base/memory/discardable_memory_android.cc:122: struct
> SupportedTypeVector {
> On 2013/12/18 08:12:38, David Reveman wrote:
>>
>> On 2013/12/17 14:28:21, Philippe wrote:
>> > This is just a suggestion but I see that we have now this struct
>
> duplicated in
>>
>> > many files with the LazyInstance and GetSupportedTypes() too and I
>
> see at the
>>
>> > same time that you now have discardable_memory.cc which looks like a
>
> good
>>
>> place
>> > to me to have those details.
>> >
>> > So I was thinking that maybe you could declare somewhere (e.g. at
>
> the bottom
>>
>> of
>> > discardable_memory.h or maybe in discardable_memory_internal.h):
>> > namespace internal {
>> >
>> > void
>> >
>
> InitializeSupportedDiscardableMemoryTypes(std::vector<DiscardableMemoryType>*
>>
>> > types);
>> >
>> > const std::vector<DiscardableMemoryType>&
>> GetSupportedDiscardableMemoryTypes();
>> >
>> > }
>> >
>> > DiscardableMemory::GetType() and DiscardableMemory::SetType() could
>
> possibly
>>
>> be
>> > moved to that internal namespace too since I don't expect most of
>
> the clients
>>
>> to
>> > mess up with that.
>> >
>> > And then you would have the DiscardableMemory implementations define
>> > InitializeSupportedTypes() like this for instance:
>> > void InitializeSupportedTypes(std::vector<DiscardableMemoryType>*
>
> types) {
>>
>> >   types->push_back(DISCARDABLE_MEMORY_ANDROID);
>> >   types->push_back(DISCARDABLE_MEMORY_EMULATED);
>> > }
>> >
>> > Then in discardable_memory.cc you would have:
>> > namespace {
>> >
>> > struct GlobalContext {
>> >   GlobalContext() {
>> >     internal::InitializeSupportedTypes(&supported_types);
>> >     DCHECK(!supported_types.empty());
>> >     current_type = supported_types.front();
>> >   }
>> >
>> >   std::vector<DiscardableMemoryType> supported_types;
>> >   DiscardableMemoryType current_type;
>> > };
>> >
>> > base::LazyInstance<GlobalContext> g_context...
>> >
>> > const std::vector<DiscardableMemoryType>&
>
> GetSupportedDiscardableMemoryTypes()
>>
>> {
>> >   return g_context.Get().supported_types;
>> > }
>> >
>> > }
>> >
>> > What do you think?
>
>
>> Please have a look at the latest code. I simply made
>
> GetSupportedTypes() take a
>>
>> pointer to a vector instead. The only code that prefer something else
>
> is the
>>
>> unit test code and small utility function solved that.
>
>
> Yeah, even better!
>
>
>
https://codereview.chromium.org/114923005/diff/1/base/memory/discardable_memo...
> base/memory/discardable_memory_android.cc:269: if
> (!memory->Initialize())
> On 2013/12/18 08:12:38, David Reveman wrote:
>>
>> On 2013/12/17 15:26:22, Philippe wrote:
>> > On 2013/12/17 14:28:21, Philippe wrote:
>> > > I have to say that I look forward to seeing
>
> DiscardableMemoryEmulated being
>>
>> > > reused somewhere to justify this pattern being spread across the
>
> various
>>
>> > > DiscardableMemory implementations :)
>> >
>> > Just realized that this may sound harsh. Sorry if it was, I didn't
>
> mean it.
>
>> DiscardableMemoryEmulated is likely not going to be reused. This is
>
> for
>>
>> consistency and make us write code that could be reused if needed.
>
>
>> When it comes to code duplication. I'm mostly concerned with
>
> duplication of code
>>
>> that contains some logic that could potentially be broken. I think
>
> this is about
>>
>> as simple as a one line statement and copying these few lines to a few
>
> places
>>
>> rather than copying a one line statement that calls a static method
>
> makes little
>>
>> difference to me.
>
>
> Those things are personal preferences obviously and not covered by the
> style guide anyway so I'm also fine with it :) I'm just not a big fan of
> the constructor + init method pattern when it's not strictly needed
> (e.g. for code reuse or when we can't afford the extra heap allocation).
> The main reasons in my opinion are the extra code needed on the call
> site, the fact that it's easy to miss the Init() call and it's even
> possible to call Init() twice.
> In addition to that the fact that the object is constructed in two steps
> prevents it from being immutable (which is not always possible anyway
> though).
> In my opinion we should not prematurely optimize for code reuse when it
> implies extra complexity in the interface. If there is need to reuse at
> some point then we can refactor just like you are doing actually. But
> this is just my opinion and mostly nitpicking :)
>
>
https://codereview.chromium.org/114923005/diff/20001/base/memory/discardable_...
> File base/memory/discardable_memory.h (right):
>
>
https://codereview.chromium.org/114923005/diff/20001/base/memory/discardable_...
> base/memory/discardable_memory.h:65: // Get system supported discardable
> memory types. Preferred type at the front
> Tiny nit: I would s/Get/Gets and same below for consistency with the
> other comments although PurgeForTesting* don't seem to follow this rule.
>
>
https://codereview.chromium.org/114923005/diff/20001/content/public/common/co...
> File content/public/common/content_switches.cc (right):
>
>
https://codereview.chromium.org/114923005/diff/20001/content/public/common/co...
> content/public/common/content_switches.cc:912: const char
> kUseDiscardableMemory[] = "use-discardable-memory";
> Nit: I would call it "discardable-memory-type" instead since the current
> flag suggests IMO that it's a boolean switch.
>
>
https://codereview.chromium.org/114923005/diff/20001/content/renderer/render_...
> File content/renderer/render_thread_impl.cc (right):
>
>
https://codereview.chromium.org/114923005/diff/20001/content/renderer/render_...
> content/renderer/render_thread_impl.cc:417: if (requested_type_name ==
> "none") {
> Nit: I would remove the braces for consistency with the rest of the
> file.
>
>
https://codereview.chromium.org/114923005/diff/20001/webkit/child/webkitplatf...
> File webkit/child/webkitplatformsupport_child_impl.cc (right):
>
>
https://codereview.chromium.org/114923005/diff/20001/webkit/child/webkitplatf...
> webkit/child/webkitplatformsupport_child_impl.cc:96:
> base::DiscardableMemoryType type = base::DiscardableMemory::GetType();
> Just realized that there was this SupportedNatively() check here :) Does
> this mean that emulated DM isn't used (yet)?
>
> https://codereview.chromium.org/114923005/

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

[reveman, 2013-12-18 19:44:25]

https://codereview.chromium.org/114923005/diff/20001/base/memory/discardable_...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/114923005/diff/20001/base/memory/discardable_...
base/memory/discardable_memory.h:65: // Get system supported discardable memory
types. Preferred type at the front
On 2013/12/18 09:07:47, Philippe wrote:
> Tiny nit: I would s/Get/Gets and same below for consistency with the other
> comments although PurgeForTesting* don't seem to follow this rule.

Done.

https://codereview.chromium.org/114923005/diff/20001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/114923005/diff/20001/content/public/common/co...
content/public/common/content_switches.cc:912: const char
kUseDiscardableMemory[] = "use-discardable-memory";
On 2013/12/18 09:07:47, Philippe wrote:
> Nit: I would call it "discardable-memory-type" instead since the current flag
> suggests IMO that it's a boolean switch.

I chose this to be consistent with --use-gl=impl but could be convinced to
change it.

https://codereview.chromium.org/114923005/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/114923005/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.cc:417: if (requested_type_name == "none") {
On 2013/12/18 09:07:47, Philippe wrote:
> Nit: I would remove the braces for consistency with the rest of the file.

I changed this a bit in latest patch by introducing type names to the
DiscardableMemory API. The reason I did this is that I think it's preferable to
keep those names close to the implementation and check in unit tests that all
types are named.

This code is now generating an error if the specified type is not supported.

PTAL.

https://codereview.chromium.org/114923005/diff/20001/webkit/child/webkitplatf...
File webkit/child/webkitplatformsupport_child_impl.cc (right):

https://codereview.chromium.org/114923005/diff/20001/webkit/child/webkitplatf...
webkit/child/webkitplatformsupport_child_impl.cc:96: base::DiscardableMemoryType
type = base::DiscardableMemory::GetType();
On 2013/12/18 09:07:47, Philippe wrote:
> Just realized that there was this SupportedNatively() check here :) Does this
> mean that emulated DM isn't used (yet)?

Existing code in blink that uses discardable memory doesn't get to use emulated
yet. Skia and the new image decode cache that is implemented in skia gets to use
it though.

The plan is to enable this later once we've transitioned to using the new image
cache everywhere.

[willchan no longer on Chromium, 2013-12-24 01:21:06]

Mostly just had nits.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.cc:38: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kTypeNamePairs); ++i) {
I don't think you need to use the unsafe version of arraysize here.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.cc:48: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kTypeNamePairs); ++i) {
ditto...i think you can switch to arraysize()

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.cc:58: g_discardable_memory_type.Get().value =
type;
This looks racy to me. If this is only allowed to be set once on startup, you
should change it so that value is only allowed to be set before the first
GetType() (and any attempt afterward is a bug and should fire a DCHECK()).
Otherwise, if it's allowed to be set at runtime, use a Lock. Or if you really
think performance is important (yada yada about premature optimization), use an
atomic. FWIW, I think we should only let this get set on startup.

Not sure if you've seen it before, but let me speculatively save a codereview
roundtrip by saying that there's no such thing as a benign data race:
https://www.usenix.org/legacy/event/hotpar11/tech/final_files/Boehm.pdf.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.h:20: DISCARDABLE_MEMORY_TYPE_ANDROID,
My inclination is to say we should guard these platform specific ones with the
platform specific defines. Any reason not to do this? Mainly, I don't feel like
we should let stuff like Mac code set the type to Android, and instead should
protect against this at compile-time. WDYT?

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_linux.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_linux.cc:4: 
The first thing to include here should be discardable_memory.h, since you define
some of those functions here.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_linux.cc:12: std::vector<DiscardableMemoryType>*
types) {
I think you should clear |types| everywhere first, so it's consistent behavior
across uses and hopefully would catch stupid places where the caller does not
pass an empty vector.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_mac.cc (left):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_mac.cc:5: #include
"base/memory/discardable_memory.h"
I'd keep this first.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:6: #if defined(OS_ANDROID)
Move platform specific includes to the end, as per
http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:10: #include
"base/memory/discardable_memory.h"
This should be first.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_win.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_win.cc:4: 
ditto here on including discardable_memory.h first

[reveman, 2013-12-26 11:46:08]

PTAL

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.cc:38: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kTypeNamePairs); ++i) {
On 2013/12/24 01:21:06, willchan wrote:
> I don't think you need to use the unsafe version of arraysize here.

Done. After after giving the array type a name.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.cc:48: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kTypeNamePairs); ++i) {
On 2013/12/24 01:21:06, willchan wrote:
> ditto...i think you can switch to arraysize()

Done.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.cc:58: g_discardable_memory_type.Get().value =
type;
On 2013/12/24 01:21:06, willchan wrote:
> This looks racy to me. If this is only allowed to be set once on startup, you
> should change it so that value is only allowed to be set before the first
> GetType() (and any attempt afterward is a bug and should fire a DCHECK()).
> Otherwise, if it's allowed to be set at runtime, use a Lock. Or if you really
> think performance is important (yada yada about premature optimization), use
an
> atomic. FWIW, I think we should only let this get set on startup.

Unit tests depend on being able to change this at runtime. Setting it once at
startup is fine elsewhere though.

I was planning to leave the thread safety responsibility up to the client so the
unit test code can do what it currently does. Latest patch does that in what I
think is a more appropriate way. It adds a type parameter to
DiscardableMemory::CreateLockedMemory that can be used by clients who want full
control over the type (ie. unit tests). DiscardableMemory::SetDefaultType() in
combination with CreateLockedMemory(DiscardableMemory::GetDefaultType()), can be
used by clients who want set-once-at-startup functionality. Default type is no
longer initialized to a preferred value and ::SetDefaultType() is instead
required to be called prior to ::GetDefaultType(). This allowed me to make the
code both thread-safe and enforce set-once-at-startup usage.

> 
> Not sure if you've seen it before, but let me speculatively save a codereview
> roundtrip by saying that there's no such thing as a benign data race:
> https://www.usenix.org/legacy/event/hotpar11/tech/final_files/Boehm.pdf.

Not seen that paper before but I've made the same claim in the past when
reviewing code some I'm glad to now have this paper to back it up. Thanks!

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.h:20: DISCARDABLE_MEMORY_TYPE_ANDROID,
On 2013/12/24 01:21:06, willchan wrote:
> My inclination is to say we should guard these platform specific ones with the
> platform specific defines. Any reason not to do this? Mainly, I don't feel
like
> we should let stuff like Mac code set the type to Android, and instead should
> protect against this at compile-time. WDYT?

My plan here is to turn TYPE_ANDROID into TYPE_ASHMEM as I move the
implementation into discardable_memory_ashmem.cc and allow it to be used on
linux. I'd also like to turn TYPE_MAC into something like TYPE_DARWIN so it's
possible to have an _ios.cc file.

Passing TYPE_ASHMEM to SetType on MacOSX without checking if it's supported is
the same as passing TYPE_NONE. Hypothetically, some implementation might also
build on a platform but not be supported. ie. maybe we decide that the current
mac implementation should not be used on iOS but we want to make sure it keeps
building in case we want to use it in the future. Or maybe we don't want to
support TYPE_EMULATED or some future experimental implementation on some
specific platform.

That said, we could still guard these types using the same conditions we use to
decide what implementations to build. I'm OK with that but I think the compile
time protection we get here is weak at its best and not worth polluting this and
the .cc file with platform ifdefs for.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_linux.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_linux.cc:4: 
On 2013/12/24 01:21:06, willchan wrote:
> The first thing to include here should be discardable_memory.h, since you
define
> some of those functions here.

Done.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_linux.cc:12: std::vector<DiscardableMemoryType>*
types) {
On 2013/12/24 01:21:06, willchan wrote:
> I think you should clear |types| everywhere first, so it's consistent behavior
> across uses and hopefully would catch stupid places where the caller does not
> pass an empty vector.

Done. By using assign() instead of push_back().

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_mac.cc (left):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_mac.cc:5: #include
"base/memory/discardable_memory.h"
On 2013/12/24 01:21:06, willchan wrote:
> I'd keep this first.

Done.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:10: #include
"base/memory/discardable_memory.h"
On 2013/12/24 01:21:06, willchan wrote:
> This should be first.

Done.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_win.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_win.cc:4: 
On 2013/12/24 01:21:06, willchan wrote:
> ditto here on including discardable_memory.h first

Done.

[willchan no longer on Chromium, 2013-12-26 18:25:53]

+jyasskin (Jeffrey, can you look at
https://codereview.chromium.org/114923005/diff/270001/base/memory/discardable...
when you get a chance?)

Let's not block on Jeffrey for that though, since any issue if present is minor,
and can be fixed later. I'm mostly fine with this change now, although I'd like
to simplify the API for users to create locked memory by ditching the parameter
if possible. Let me know what you think about that. And if you wanted, you could
loop in the other reviewers for the relevant other changes you made here now.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.cc:58: g_discardable_memory_type.Get().value =
type;
On 2013/12/26 11:46:08, David Reveman wrote:
> On 2013/12/24 01:21:06, willchan wrote:
> > This looks racy to me. If this is only allowed to be set once on startup,
you
> > should change it so that value is only allowed to be set before the first
> > GetType() (and any attempt afterward is a bug and should fire a DCHECK()).
> > Otherwise, if it's allowed to be set at runtime, use a Lock. Or if you
really
> > think performance is important (yada yada about premature optimization), use
> an
> > atomic. FWIW, I think we should only let this get set on startup.
> 
> Unit tests depend on being able to change this at runtime. Setting it once at
> startup is fine elsewhere though.
> 
> I was planning to leave the thread safety responsibility up to the client so
the
> unit test code can do what it currently does. Latest patch does that in what I
> think is a more appropriate way. It adds a type parameter to
> DiscardableMemory::CreateLockedMemory that can be used by clients who want
full
> control over the type (ie. unit tests). DiscardableMemory::SetDefaultType() in
> combination with CreateLockedMemory(DiscardableMemory::GetDefaultType()), can
be
> used by clients who want set-once-at-startup functionality. Default type is no

Are there any valid reasons for non-test clients to want full control over the
type? I think it'd be nice for most users not to have to pass in the default
type...WDYT about CreateLockedMemory() and
CreateLockedMemoryWithType[ForTests]()?

> longer initialized to a preferred value and ::SetDefaultType() is instead
> required to be called prior to ::GetDefaultType(). This allowed me to make the
> code both thread-safe and enforce set-once-at-startup usage.
> 
> > 
> > Not sure if you've seen it before, but let me speculatively save a
codereview
> > roundtrip by saying that there's no such thing as a benign data race:
> > https://www.usenix.org/legacy/event/hotpar11/tech/final_files/Boehm.pdf.
> 
> Not seen that paper before but I've made the same claim in the past when
> reviewing code some I'm glad to now have this paper to back it up. Thanks!

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.h:20: DISCARDABLE_MEMORY_TYPE_ANDROID,
On 2013/12/26 11:46:08, David Reveman wrote:
> On 2013/12/24 01:21:06, willchan wrote:
> > My inclination is to say we should guard these platform specific ones with
the
> > platform specific defines. Any reason not to do this? Mainly, I don't feel
> like
> > we should let stuff like Mac code set the type to Android, and instead
should
> > protect against this at compile-time. WDYT?
> 
> My plan here is to turn TYPE_ANDROID into TYPE_ASHMEM as I move the
> implementation into discardable_memory_ashmem.cc and allow it to be used on
> linux. I'd also like to turn TYPE_MAC into something like TYPE_DARWIN so it's
> possible to have an _ios.cc file.
> 
> Passing TYPE_ASHMEM to SetType on MacOSX without checking if it's supported is
> the same as passing TYPE_NONE. Hypothetically, some implementation might also
> build on a platform but not be supported. ie. maybe we decide that the current
> mac implementation should not be used on iOS but we want to make sure it keeps
> building in case we want to use it in the future. Or maybe we don't want to
> support TYPE_EMULATED or some future experimental implementation on some
> specific platform.
> 
> That said, we could still guard these types using the same conditions we use
to
> decide what implementations to build. I'm OK with that but I think the compile
> time protection we get here is weak at its best and not worth polluting this
and
> the .cc file with platform ifdefs for.

This isn't a big deal and is mostly stylistic preference. I'm cool with whatever
you want. Figured I'd just voice my minor pref just to make sure you considered
it.

https://codereview.chromium.org/114923005/diff/270001/base/memory/discardable...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/270001/base/memory/discardable...
base/memory/discardable_memory.cc:63: subtle::AtomicWord value =
subtle::Acquire_Load(&g_default_type);
OK, this is too subtle for me :) What's the acquire semantic needed for here?
Are there other memory operations that we need the memory barrier to prevent
reordering? I'm adding jyasskin@ because he knows this stuff way better than I
do to comment here.

[Jeffrey Yasskin, 2013-12-26 18:36:07]

https://codereview.chromium.org/114923005/diff/270001/base/memory/discardable...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/270001/base/memory/discardable...
base/memory/discardable_memory.cc:63: subtle::AtomicWord value =
subtle::Acquire_Load(&g_default_type);
On 2013/12/26 18:25:54, willchan wrote:
> OK, this is too subtle for me :) What's the acquire semantic needed for here?
> Are there other memory operations that we need the memory barrier to prevent
> reordering? I'm adding jyasskin@ because he knows this stuff way better than I
> do to comment here.

Yep, I don't see any release operation in this file, so the acquire has no
effect. Either change some other operation to Release or change this one to
NoBarrier.

Or, if the caller is supposed to guarantee that the two uses have an external
happens-before edge, change them to non-atomic so that TSan can catch mistakes.

[reveman, 2013-12-26 21:53:34]

PTAL

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.cc:58: g_discardable_memory_type.Get().value =
type;
On 2013/12/26 18:25:54, willchan wrote:
> On 2013/12/26 11:46:08, David Reveman wrote:
> > On 2013/12/24 01:21:06, willchan wrote:
> > > This looks racy to me. If this is only allowed to be set once on startup,
> you
> > > should change it so that value is only allowed to be set before the first
> > > GetType() (and any attempt afterward is a bug and should fire a DCHECK()).
> > > Otherwise, if it's allowed to be set at runtime, use a Lock. Or if you
> really
> > > think performance is important (yada yada about premature optimization),
use
> > an
> > > atomic. FWIW, I think we should only let this get set on startup.
> > 
> > Unit tests depend on being able to change this at runtime. Setting it once
at
> > startup is fine elsewhere though.
> > 
> > I was planning to leave the thread safety responsibility up to the client so
> the
> > unit test code can do what it currently does. Latest patch does that in what
I
> > think is a more appropriate way. It adds a type parameter to
> > DiscardableMemory::CreateLockedMemory that can be used by clients who want
> full
> > control over the type (ie. unit tests). DiscardableMemory::SetDefaultType()
in
> > combination with CreateLockedMemory(DiscardableMemory::GetDefaultType()),
can
> be
> > used by clients who want set-once-at-startup functionality. Default type is
no
> 
> Are there any valid reasons for non-test clients to want full control over the
> type?

Not at this point.  

> I think it'd be nice for most users not to have to pass in the default
> type...WDYT about CreateLockedMemory() and
> CreateLockedMemoryWithType[ForTests]()?

Done. I also brought back the default preferred type as I think it would be
weird to require the type to be set before CreateLockedMemory() is called.

The function that needs a platform specific implementation is now
CreateLockedMemoryWithType. CreateLockedMemory is just a convenience function
for "CreateLockedMemoryWithType(GetPreferredType(), size)"

> 
> > longer initialized to a preferred value and ::SetDefaultType() is instead
> > required to be called prior to ::GetDefaultType(). This allowed me to make
the
> > code both thread-safe and enforce set-once-at-startup usage.
> > 
> > > 
> > > Not sure if you've seen it before, but let me speculatively save a
> codereview
> > > roundtrip by saying that there's no such thing as a benign data race:
> > > https://www.usenix.org/legacy/event/hotpar11/tech/final_files/Boehm.pdf.
> > 
> > Not seen that paper before but I've made the same claim in the past when
> > reviewing code some I'm glad to now have this paper to back it up. Thanks!
>

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.h:20: DISCARDABLE_MEMORY_TYPE_ANDROID,
On 2013/12/26 18:25:54, willchan wrote:
> On 2013/12/26 11:46:08, David Reveman wrote:
> > On 2013/12/24 01:21:06, willchan wrote:
> > > My inclination is to say we should guard these platform specific ones with
> the
> > > platform specific defines. Any reason not to do this? Mainly, I don't feel
> > like
> > > we should let stuff like Mac code set the type to Android, and instead
> should
> > > protect against this at compile-time. WDYT?
> > 
> > My plan here is to turn TYPE_ANDROID into TYPE_ASHMEM as I move the
> > implementation into discardable_memory_ashmem.cc and allow it to be used on
> > linux. I'd also like to turn TYPE_MAC into something like TYPE_DARWIN so
it's
> > possible to have an _ios.cc file.
> > 
> > Passing TYPE_ASHMEM to SetType on MacOSX without checking if it's supported
is
> > the same as passing TYPE_NONE. Hypothetically, some implementation might
also
> > build on a platform but not be supported. ie. maybe we decide that the
current
> > mac implementation should not be used on iOS but we want to make sure it
keeps
> > building in case we want to use it in the future. Or maybe we don't want to
> > support TYPE_EMULATED or some future experimental implementation on some
> > specific platform.
> > 
> > That said, we could still guard these types using the same conditions we use
> to
> > decide what implementations to build. I'm OK with that but I think the
compile
> > time protection we get here is weak at its best and not worth polluting this
> and
> > the .cc file with platform ifdefs for.
> 
> This isn't a big deal and is mostly stylistic preference. I'm cool with
whatever
> you want. Figured I'd just voice my minor pref just to make sure you
considered
> it.

I'll keep it in mind and re-evaluate as I land more refactor patches.

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
File base/memory/discardable_memory_unittest.cc (right):

https://codereview.chromium.org/114923005/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:6: #if defined(OS_ANDROID)
On 2013/12/24 01:21:06, willchan wrote:
> Move platform specific includes to the end, as per
> http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code

Done.

https://codereview.chromium.org/114923005/diff/270001/base/memory/discardable...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/114923005/diff/270001/base/memory/discardable...
base/memory/discardable_memory.cc:63: subtle::AtomicWord value =
subtle::Acquire_Load(&g_default_type);
On 2013/12/26 18:36:07, JeffreyYasskin OOO until Jan 3 wrote:
> On 2013/12/26 18:25:54, willchan wrote:
> > OK, this is too subtle for me :) What's the acquire semantic needed for
here?
> > Are there other memory operations that we need the memory barrier to prevent
> > reordering? I'm adding jyasskin@ because he knows this stuff way better than
I
> > do to comment here.
> 
> Yep, I don't see any release operation in this file, so the acquire has no
> effect. Either change some other operation to Release or change this one to
> NoBarrier.
> 
> Or, if the caller is supposed to guarantee that the two uses have an external
> happens-before edge, change them to non-atomic so that TSan can catch
mistakes.

Done. The caller is supposed to guarantee correct ordering.

[willchan no longer on Chromium, 2014-01-06 23:45:16]

lgtm

[commit-bot: I haz the power, 2014-01-06 23:55:27]

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

[commit-bot: I haz the power, 2014-01-06 23:55:33]

Failed to apply patch for base/memory/discardable_memory_android.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file base/memory/discardable_memory_android.cc
  Hunk #1 FAILED at 17.
  Hunk #2 FAILED at 97.
  Hunk #3 succeeded at 100 (offset -87 lines).
  Hunk #4 succeeded at 122 with fuzz 1 (offset -87 lines).
  Hunk #5 FAILED at 234.
  3 out of 5 hunks FAILED -- saving rejects to file
base/memory/discardable_memory_android.cc.rej

Patch:       base/memory/discardable_memory_android.cc
Index: base/memory/discardable_memory_android.cc
diff --git a/base/memory/discardable_memory_android.cc
b/base/memory/discardable_memory_android.cc
index
519aa8d8d24a2d12198dd577f917e639c56f101c..a2ea070616027dab55bae0fff04480f29d9a92e1
100644
--- a/base/memory/discardable_memory_android.cc
+++ b/base/memory/discardable_memory_android.cc
@@ -17,8 +17,8 @@
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
-#include "base/memory/discardable_memory.h"
 #include "base/memory/discardable_memory_allocator_android.h"
+#include "base/memory/discardable_memory_emulated.h"
 #include "base/synchronization/lock.h"
 #include "third_party/ashmem/ashmem.h"
 
@@ -97,7 +97,7 @@ class DiscardableMemoryAndroidSimple : public
DiscardableMemory {
   }
 
   // DiscardableMemory:
-  virtual LockDiscardableMemoryStatus Lock() OVERRIDE {
+  virtual DiscardableMemoryLockStatus Lock() OVERRIDE {
     return internal::LockAshmemRegion(fd_, 0, size_, memory_);
   }
 
@@ -187,14 +187,14 @@ bool CloseAshmemRegion(int fd, size_t size, void* address)
{
   return close(fd) == 0;
 }
 
-LockDiscardableMemoryStatus LockAshmemRegion(int fd,
+DiscardableMemoryLockStatus LockAshmemRegion(int fd,
                                              size_t off,
                                              size_t size,
                                              const void* address) {
   const int result = ashmem_pin_region(fd, off, size);
   DCHECK_EQ(0, mprotect(address, size, PROT_READ | PROT_WRITE));
-  return result == ASHMEM_WAS_PURGED ?
-      DISCARDABLE_MEMORY_PURGED : DISCARDABLE_MEMORY_SUCCESS;
+  return result == ASHMEM_WAS_PURGED ? DISCARDABLE_MEMORY_LOCK_STATUS_PURGED
+                                     : DISCARDABLE_MEMORY_LOCK_STATUS_SUCCESS;
 }
 
 bool UnlockAshmemRegion(int fd, size_t off, size_t size, const void* address) {
@@ -209,8 +209,13 @@ bool UnlockAshmemRegion(int fd, size_t off, size_t size,
const void* address) {
 }  // namespace internal
 
 // static
-bool DiscardableMemory::SupportedNatively() {
-  return true;
+void DiscardableMemory::GetSupportedTypes(
+    std::vector<DiscardableMemoryType>* types) {
+  const DiscardableMemoryType supported_types[] = {
+    DISCARDABLE_MEMORY_TYPE_ANDROID,
+    DISCARDABLE_MEMORY_TYPE_EMULATED
+  };
+  types->assign(supported_types, supported_types + arraysize(supported_types));
 }
 
 // Allocation can happen in two ways:
@@ -229,28 +234,47 @@ bool DiscardableMemory::SupportedNatively() {
 // allocations systematically go through individual ashmem regions.
 //
 // static
-scoped_ptr<DiscardableMemory> DiscardableMemory::CreateLockedMemory(
-    size_t size) {
-  if (!CheckSizeCanBeAlignedToNextPage(size))
-    return scoped_ptr<DiscardableMemory>();
-  // Pinning & unpinning works with page granularity therefore align the size
-  // upfront.
-  const size_t aligned_size = internal::AlignToNextPage(size);
-  // Note that the following code is slightly racy. The worst that can happen
in
-  // practice though is taking the wrong decision (e.g. using the allocator
-  // rather than DiscardableMemoryAndroidSimple). Moreover keeping the lock
-  // acquired for the whole allocation would cause a deadlock when the
allocator
-  // tries to create an ashmem region.
-  GlobalContext* const global_context = g_context.Pointer();
-  if (GetCurrentNumberOfAshmemFDs() < 0.9 * global_context->ashmem_fd_limit) {
-    int fd;
-    void* address;
-    if (internal::CreateAshmemRegion("", aligned_size, &fd, &address)) {
-      return scoped_ptr<DiscardableMemory>(
-          new DiscardableMemoryAndroidSimple(fd, address, aligned_size));
+scoped_ptr<DiscardableMemory> DiscardableMemory::CreateLockedMemoryWithType(
+    DiscardableMemoryType type, size_t size) {
+  switch (type) {
+    case DISCARDABLE_MEMORY_TYPE_NONE:
+    case DISCARDABLE_MEMORY_TYPE_MAC:
+      return scoped_ptr<DiscardableMemory>();
+    case DISCARDABLE_MEMORY_TYPE_ANDROID: {
+      if (!CheckSizeCanBeAlignedToNextPage(size))
+        return scoped_ptr<DiscardableMemory>();
+      // Pinning & unpinning works with page granularity therefore align the
+      // size upfront.
+      const size_t aligned_size = internal::AlignToNextPage(size);
+      // Note that the following code is slightly racy. The worst that can
+      // happen in practice though is taking the wrong decision (e.g. using
+      // the allocator rather than DiscardableMemoryAndroidSimple). Moreover
+      // keeping the lock acquired for the whole allocation would cause a
+      // deadlock when the allocator tries to create an ashmem region.
+      GlobalContext* const global_context = g_context.Pointer();
+      if (GetCurrentNumberOfAshmemFDs() <
+          0.9 * global_context->ashmem_fd_limit) {
+        int fd;
+        void* address;
+        if (internal::CreateAshmemRegion("", aligned_size, &fd, &address)) {
+          return scoped_ptr<DiscardableMemory>(
+              new DiscardableMemoryAndroidSimple(fd, address, aligned_size));
+        }
+      }
+      return global_context->allocator.Allocate(size);
+    }
+    case DISCARDABLE_MEMORY_TYPE_EMULATED: {
+      scoped_ptr<internal::DiscardableMemoryEmulated> memory(
+          new internal::DiscardableMemoryEmulated(size));
+      if (!memory->Initialize())
+        return scoped_ptr<DiscardableMemory>();
+
+      return memory.PassAs<DiscardableMemory>();
     }
   }
-  return global_context->allocator.Allocate(size);
+
+  NOTREACHED();
+  return scoped_ptr<DiscardableMemory>();
 }
 
 // static

[reveman, 2014-01-07 04:19:14]

need some owner reviews,

+piman for content/ changes
+tomhudson for skia/ rubberstamp
+jamesr for webkit/ rubberstamp

[Tom Hudson, 2014-01-07 12:29:26]

Skia rubberstamp LGTM. Didn't know you'd be visiting this week!

[jamesr, 2014-01-07 18:09:56]

lgtm

[piman, 2014-01-08 01:42:22]

content/ LGTM + one thing.

https://codereview.chromium.org/114923005/diff/440002/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/114923005/diff/440002/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1072:
switches::kUseDiscardableMemory,
Can you add this to kForwardSwitches in
chrome/browser/chromeos/login/chrome_restart_request.cc for Guest mode?

[reveman, 2014-01-08 03:05:25]

https://codereview.chromium.org/114923005/diff/440002/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/114923005/diff/440002/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1072:
switches::kUseDiscardableMemory,
On 2014/01/08 01:42:23, piman (OOO back 2014-1-7) wrote:
> Can you add this to kForwardSwitches in
> chrome/browser/chromeos/login/chrome_restart_request.cc for Guest mode?

Done.

[commit-bot: I haz the power, 2014-01-08 03:05:52]

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

[commit-bot: I haz the power, 2014-01-08 03:23:05]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-08 05:33:39]

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

[commit-bot: I haz the power, 2014-01-08 12:27:40]

Change committed as 243532