[Not for review] Discardable memory emulation

Add discardable memory emulation for non-android non-mac platforms

Installs a very basic discardable memory emulator. It purges only when all
render widgets for the process are hidden, and when it purges, it purges every
unlocked discardable. No attempt at a MRU or max cache size is attempted.

BUG=237681

[Ian Vollick, 2013-05-28 14:59:38]

Nat, when you have a sec, can you please take a look at this first pass?

[Ian Vollick, 2013-05-29 13:49:29]

On 2013/05/28 14:59:38, vollick wrote:
> Nat, when you have a sec, can you please take a look at this first pass?

Nat, backer@ pointed out that we'll often have multiple render threads per
render process (whoops). I think he's got some more comments on the way, but in
any case, I'm going to have to rework this patch pretty significantly before
it's ready for you to look at.

[nduca, 2013-05-29 13:50:47]

Hmm, multiple render threads?

[nduca, 2013-05-29 13:57:00]

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
base/memory/discardable_memory.h:20: #if !defined(OS_ANDROID) &&
!defined(OS_MACOSX)
so, on windows 8, we have discardable memory but pre-8 we dont. How does the
provider fit in in that case?

maybe we can put everything behind a provider interface somehow?

[jonathan.backer, 2013-05-29 14:02:53]

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
base/memory/discardable_memory.cc:31: static DiscardableMemoryProvider*
s_provider = NULL;
Who owns the s_provider?  I see it being set, but not reset.

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
base/memory/discardable_memory.h:116: #if defined(OS_ANDROID) ||
defined(OS_MACOSX)
Maybe a #define DISCARDABLE_MEMORY_SUPPORTED defined(OS_ANDROID) ||
defined(OS_MACOSX)?

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_pr...
File content/renderer/render_process_discardable_memory_provider.cc (right):

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_pr...
content/renderer/render_process_discardable_memory_provider.cc:43: return;
DCHECK(!it->second.locked)?

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_pr...
content/renderer/render_process_discardable_memory_provider.cc:75: return NULL;
DCHECK(it->second.locked)?

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:416: discardable_provider_.get());
I don't see the advantage of a singleton here.  RenderProcessVisibilityManager
is only referenced from inside RenderThreadImpl. Maybe RenderThreadImpl can own
it.

Right now, there is a bit of a life-time issue with the Manager and Provider. I
think that you're leaking both (which is fine). But if Manager is no longer a
singleton, you'll want to tighten this up.

Maybe Manager owns Provider/is a Provider?

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:417:
base::DiscardableMemory::SetProvider(discardable_provider_.get());
I suspect that you may have many RenderThreadImpls per render process (multiple
tabs per process). In which case you'll have to rework SetProvider.

[Ian Vollick, 2013-06-06 00:21:46]

Nat, all platforms now use a provider. PTAL!

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
File base/memory/discardable_memory.cc (right):

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
base/memory/discardable_memory.cc:31: static DiscardableMemoryProvider*
s_provider = NULL;
On 2013/05/29 14:02:54, jonathan.backer wrote:
> Who owns the s_provider?  I see it being set, but not reset.
The render thread impl. It now resets it in its destructor.

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
base/memory/discardable_memory.h:20: #if !defined(OS_ANDROID) &&
!defined(OS_MACOSX)
On 2013/05/29 13:57:00, nduca wrote:
> so, on windows 8, we have discardable memory but pre-8 we dont. How does the
> provider fit in in that case?
> 
> maybe we can put everything behind a provider interface somehow?
I *think* this is done, but please let me know if my approach isn't what you
were hoping for.

https://codereview.chromium.org/15650016/diff/2001/base/memory/discardable_me...
base/memory/discardable_memory.h:116: #if defined(OS_ANDROID) ||
defined(OS_MACOSX)
On 2013/05/29 14:02:54, jonathan.backer wrote:
> Maybe a #define DISCARDABLE_MEMORY_SUPPORTED defined(OS_ANDROID) ||
> defined(OS_MACOSX)?

Done.

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_pr...
File content/renderer/render_process_discardable_memory_provider.cc (right):

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_pr...
content/renderer/render_process_discardable_memory_provider.cc:43: return;
On 2013/05/29 14:02:54, jonathan.backer wrote:
> DCHECK(!it->second.locked)?

Done.

https://codereview.chromium.org/15650016/diff/2001/content/renderer/render_pr...
content/renderer/render_process_discardable_memory_provider.cc:75: return NULL;
On 2013/05/29 14:02:54, jonathan.backer wrote:
> DCHECK(it->second.locked)?

Done.

[nduca, 2013-06-07 04:50:07]

+qinmin who added some of this. Min, who of the base/ owners should we get to
look at this?

[nduca, 2013-06-07 04:51:40]

This looks really solid. Before we add more folks, lets make this into a patch
just about no-new-features edits to base/discardable. Or make a patch that does
that, and get it reviewed and in. Then we can come back and add the new provider
interface, and as patch 3, do the renderer stuff.

[nduca, 2013-06-07 04:52:18]

https://codereview.chromium.org/15650016/diff/19001/webkit/glue/webkitplatfor...
File webkit/glue/webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/15650016/diff/19001/webkit/glue/webkitplatfor...
webkit/glue/webkitplatformsupport_impl.cc:993: if
(!base::DiscardableMemoryProvider::GetInstance())
lets seal this though behind base::DiscardableMemory::Supported. It'd be ideal
if the Provider part was hidden from the average user of the interface.

[qinmin, 2013-06-07 04:56:14]

+willchan

[Avi (use Gerrit), 2013-06-07 14:35:28]

Looking at the notes on the original implementation of DiscardableMemory
(https://code.google.com/p/chromium/issues/detail?id=169597), it was cpu@ who
made the decision to not have Win8-specific stuff.

Looping Carlos in.

[Avi (use Gerrit), 2013-06-07 14:46:53]

I'm OK with the interfacing of DiscardableMemory. I'm less ok with the
SetInstance deal. Have we done that before where we have a singleton but maybe,
and sometimes the caller will set it to something of their own implementation?

https://codereview.chromium.org/15650016/diff/19001/base/memory/discardable_m...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/15650016/diff/19001/base/memory/discardable_m...
base/memory/discardable_memory.h:29: // will attempt to create an instance via
CreateInstance(). It is possible
What is CreateInstance()?

https://codereview.chromium.org/15650016/diff/19001/base/memory/discardable_m...
base/memory/discardable_memory.h:30: // that this can return NULL.
"Can" return NULL is not helpful. In what scenario _will_ this return NULL,
exactly? Is this rewrite correct?

"If no instance of a DiscardableMemoryProvider was set via GetInstance, and
discardable memory isn't available natively, this call will return NULL."

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

https://codereview.chromium.org/15650016/diff/19001/base/memory/discardable_m...
base/memory/discardable_memory_unittest.cc:11: #if defined(OS_ANDROID) ||
defined(OS_MACOSX)
#if defined(DISCARDABLE_MEMORY_SUPPORTED_NATIVELY)

https://codereview.chromium.org/15650016/diff/19001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/15650016/diff/19001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:406: if
(!base::DiscardableMemoryProvider::GetInstance()) {
DISCARDABLE_MEMORY_SUPPORTED_NATIVELY?

But only if we want to say in the .h that iff
DISCARDABLE_MEMORY_SUPPORTED_NATIVELY then GetInstance is guaranteed to succeed.
Which might not be true if we decide to have a Win8 impl.

[Avi (use Gerrit), 2013-06-07 15:04:18]

On 2013/06/07 14:46:53, Avi wrote:
> I'm less ok with the
> SetInstance deal. Have we done that before where we have a singleton but
maybe,
> and sometimes the caller will set it to something of their own implementation?

Actually, that's bothering me a bit. If a RenderProcessDiscardableMemoryProvider
is set to be the instance of DiscardableMemoryProvider, then all users get it,
not just the render process. At that point, we're basically admitting that there
will never be another client.

I'm really disliking this approach now. If the render process wants to have its
own pseudo discardable memory, fine, but let it do it on its own, and not inject
it into base where everyone gets it.

[cpu_(ooo_6.6-7.5), 2013-06-07 17:57:41]

As is this emulation is like a weird cache. If you want a true emulation
somebody needs to be looking at the working set load for the machine and send a
signal to the discardable memory to discard when a threshold is reached.
Visibility of the widgets will of course lock/ unlock pages but freeing memory
at 0 count with no memory pressure just creates churn and fragmentation on the
VM subsystem.

Thus, not very enthusiastic about this approach, if I misunderstood please set
me straight.

[nduca, 2013-06-07 18:16:59]

Sorry Avi, I've got ti disagree with you rather strongly.

[nduca, 2013-06-07 18:23:13]

Ah shit, wrong button. So there is a ton of a discardable memory primitive. But,
as you may recall, there is a ton of non uniformity in support across the
different platforms --- and on some platforms, say early Windows, or even Linux,
you simply dont have it.

In those cases, I hear that you're saying "well then dont have it."

But then what do we do when we do have resources that are in fact discardable?

In my opinion, its NOT okay that we then roll a fresh system every time we have
that situation --- then there's no architectural benefit to base::Discardable
--- everyone who uses it needs to implement a fallback path. Thats no good.

Much better if, on platforms without kernel level handling, we use our (very
good) knowledge of the different processes and their memory requirements to
manage the discardable memory. We can do a good job on this (we know, as this is
how we manage gpu memory across tabs).

Now, if you're saying you want to move discardable somewhere, I'm open to
suggestions. If you want it in content, fine, though I think that doesn't quite
make sense --- then, say, cc can't use it.

@cpu, the intent here is to start with simple emulation and get better. With the
basic primitives in base/, we can then do more using global state a la the GPU
memory manager. But, we shouldn't shoot the basic scheme down because its not
perfect at the start... I believe render process visibility is a very good
approximation, and we can do better from there.

Just so you guys know, we keep 32MB **per process** around just in decoded
images. This is an absolutely silly waste of memory. Yes we could have a fancy
memory managment strategy, but on a heavy machine, thats 32M*numProcs-1 that we
can gain back *just* with this simple scheme. Thats no laughing matter, I think.

[Avi (use Gerrit), 2013-06-07 18:35:50]

On 2013/06/07 18:23:13, nduca wrote:
> Sorry Avi, I've got ti disagree with you rather strongly.

If this is your argument against my comment, then I must not have been clear
with my comments because I don't disagree with anything you say here.

I will try again... :)

> In those cases, I hear that you're saying "well then dont have it."

Oh, no. I completely agree with what you want to do. I disagree with how this CL
is attempting to accomplish that desirable goal.

> In my opinion, its NOT okay that we then roll a fresh system every time we
have
> that situation

Agreed.

> everyone who uses it needs to implement a fallback path. Thats no good.

Agreed that that would be bad. But the problem here is that on one hand, here
you're arguing that there will be multiple users (with the "everyone who uses
it" phrasing) yet in the code you build a system where there can be exactly one
client who installs a DiscardableMemoryProvider if functionality isn't available
natively.

DiscardableMemory is in base/. You argue that we need to design a system where
there is a common fallback path for everyone rather than have everyone roll
their own. I will not disagree, but only will point out that your CL doesn't
provide that. If that is indeed what you are arguing, then extend
DiscardableMemory in base. For platforms with real discardability, then the
current magic happens, otherwise you can have a tagging system with manual
purging.

But your current CL design where there is a single DiscardableMemoryProvider
allowed, feels like the wrong way to go for me.

[cpu_(ooo_6.6-7.5), 2013-06-07 21:53:38]

>> the intent here is to start with simple emulation

Like I said there is no emulation here, or that word has a completely different
meaning for you.

>> we keep 32MB **per process** around just in decoded
images.

That we never, never ever use? 

>> But, we shouldn't shoot the basic scheme down because its not
perfect at the start

The straw-man argument :)  Fear not, what I am proposing is far from perfect.
What I am proposing is an actual emulation. Here is in more detail:

Every time you allocate over N bytes delta, (say 8 MB) call GlobalMemoryStatusEx
 find the load, or come with your favorite trigger from it and when it does then
do take away the freed blocks. Like the OS would do.

[cpu_(ooo_6.6-7.5), 2013-06-07 21:55:45]

>> therwise you can have a tagging system with manual
purging.

I am proposing a system that requires no manual purging. Unless I am overlooking
something big (which happens often) this is a drop-in replacement,

[Avi (use Gerrit), 2013-06-07 22:33:07]

On 2013/06/07 21:55:45, cpu wrote:
> >> therwise you can have a tagging system with manual
> purging.
> 
> I am proposing a system that requires no manual purging. Unless I am
overlooking
> something big (which happens often) this is a drop-in replacement,

Your scheme sounds nice.

> Every time you allocate over N bytes delta

By any allocator? Through this mechanism?

BTW, are you still against MEM_RESET/MEM_RESET_UNDO?

[cpu_(ooo_6.6-7.5), 2013-06-08 02:14:14]

On 2013/06/07 22:33:07, Avi wrote:
> On 2013/06/07 21:55:45, cpu wrote:
> > >> therwise you can have a tagging system with manual
> > purging.
> > 
> > I am proposing a system that requires no manual purging. Unless I am
> overlooking
> > something big (which happens often) this is a drop-in replacement,
> 
> Your scheme sounds nice.
> 
> > Every time you allocate over N bytes delta
> 
> By any allocator? Through this mechanism?

I was just thinking on keeping a counter, the discard-able mem object will
either
be the middle man allocating or it has to be told what was allocated.

> 
> BTW, are you still against MEM_RESET/MEM_RESET_UNDO?

No. We can implement that for win8, the user base is growing. Not too long ago I
encouraged zturner@ to look into this.

[Avi (use Gerrit), 2013-06-08 02:15:37]

On 2013/06/08 02:14:14, cpu wrote:
> On 2013/06/07 22:33:07, Avi wrote:
> > On 2013/06/07 21:55:45, cpu wrote:
> > > >> therwise you can have a tagging system with manual
> > > purging.
> > > 
> > > I am proposing a system that requires no manual purging. Unless I am
> > overlooking
> > > something big (which happens often) this is a drop-in replacement,
> > 
> > Your scheme sounds nice.
> > 
> > > Every time you allocate over N bytes delta
> > 
> > By any allocator? Through this mechanism?
> 
> I was just thinking on keeping a counter, the discard-able mem object will
> either
> be the middle man allocating or it has to be told what was allocated.
> 
> > 
> > BTW, are you still against MEM_RESET/MEM_RESET_UNDO?
> 
> No. We can implement that for win8, the user base is growing. Not too long ago
I
> encouraged zturner@ to look into this.

Heck, while we argue about the future direction of this CL, _I_ could implement
that for today's base::DiscardableMemory. :)

[willchan no longer on Chromium, 2013-06-10 16:24:12]

I haven't read this yet. Has the dust settled? What's the key disagreement,
if any?

And specifically, I'd like to hear the arguments around the new interface
in base...this DiscardableMemoryProvider. Are there any disagreements here?
Or is it more about the actual usage?


On Fri, Jun 7, 2013 at 7:15 PM, <avi@chromium.org> wrote:

> On 2013/06/08 02:14:14, cpu wrote:
>
>> On 2013/06/07 22:33:07, Avi wrote:
>> > On 2013/06/07 21:55:45, cpu wrote:
>> > > >> therwise you can have a tagging system with manual
>> > > purging.
>> > >
>> > > I am proposing a system that requires no manual purging. Unless I am
>> > overlooking
>> > > something big (which happens often) this is a drop-in replacement,
>> >
>> > Your scheme sounds nice.
>> >
>> > > Every time you allocate over N bytes delta
>> >
>> > By any allocator? Through this mechanism?
>>
>
>  I was just thinking on keeping a counter, the discard-able mem object will
>> either
>> be the middle man allocating or it has to be told what was allocated.
>>
>
>  >
>> > BTW, are you still against MEM_RESET/MEM_RESET_UNDO?
>>
>
>  No. We can implement that for win8, the user base is growing. Not too
>> long ago
>>
> I
>
>> encouraged zturner@ to look into this.
>>
>
> Heck, while we argue about the future direction of this CL, _I_ could
> implement
> that for today's base::DiscardableMemory. :)
>
>
https://codereview.chromium.**org/15650016/<https://codereview.chromium.org/1...
>

[Ian Vollick, 2013-06-10 17:34:50]

On 2013/06/10 16:24:12, willchan wrote:
> I haven't read this yet. Has the dust settled? What's the key disagreement,
> if any?
> 
> And specifically, I'd like to hear the arguments around the new interface
> in base...this DiscardableMemoryProvider. Are there any disagreements here?
> Or is it more about the actual usage?

My apologies for the slow response, folks. I've been trying to write code to
accompany my reply, but it's taking a bit too long to write, so here goes. :)

I believe there are two points of contention. Reviewers, please let me know if
I've got any of this wrong.
 1. The ability to install a static discardable memory provider in base. This CL
made the implicit assumption that per-process, per-platform control over the
discardable memory implementation was good enough. If it's not (and I believe
that's Avi's contention -- please correct me if I'm wrong), then we'd need to
find a new home for the provider. This is certainly possible. An instance would
just need to be created and owned somewhere and access plumbed to the bits of
code that used it.
 2. That the trivial implementation of the discardable provider isn't really
emulation at all -- it's a weird caching scheme. It essentially doles out memory
willy-nilly until the tab gets backgrounded, at which point all memory is
purged. It was always my intention to write a real emulation scheme, I was just
worried that this bit of code would generate lots of.. opinions :) As such, I
wanted to stage it. In fact, this patch is too big already. As Nat suggested, I
need to break this one into pieces. The next version was going to do some real
emulation. I'm currently writing this patch now (I was actually hoping I could
post it along with this comment), but it's not done yet. I will post a follow up
as soon as it's ready.

> 
> 
> On Fri, Jun 7, 2013 at 7:15 PM, <mailto:avi@chromium.org> wrote:
> 
> > On 2013/06/08 02:14:14, cpu wrote:
> >
> >> On 2013/06/07 22:33:07, Avi wrote:
> >> > On 2013/06/07 21:55:45, cpu wrote:
> >> > > >> therwise you can have a tagging system with manual
> >> > > purging.
> >> > >
> >> > > I am proposing a system that requires no manual purging. Unless I am
> >> > overlooking
> >> > > something big (which happens often) this is a drop-in replacement,
> >> >
> >> > Your scheme sounds nice.
> >> >
> >> > > Every time you allocate over N bytes delta
> >> >
> >> > By any allocator? Through this mechanism?
> >>
> >
> >  I was just thinking on keeping a counter, the discard-able mem object will
> >> either
> >> be the middle man allocating or it has to be told what was allocated.
> >>
> >
> >  >
> >> > BTW, are you still against MEM_RESET/MEM_RESET_UNDO?
> >>
> >
> >  No. We can implement that for win8, the user base is growing. Not too
> >> long ago
> >>
> > I
> >
> >> encouraged zturner@ to look into this.
> >>
> >
> > Heck, while we argue about the future direction of this CL, _I_ could
> > implement
> > that for today's base::DiscardableMemory. :)
> >
> >
>
https://codereview.chromium.**org/15650016/%3Chttps://codereview.chromium.org...>
> >

[Avi (use Gerrit), 2013-06-11 15:47:14]

On 2013/06/10 17:34:50, vollick wrote:
>  1. The ability to install a static discardable memory provider in base. This
CL
> made the implicit assumption that per-process, per-platform control over the
> discardable memory implementation was good enough.

Good re-statement.

It's not that it's not good enough, but that I disagree with the design. We
don't delegate responsibility like that today, and it's not obvious at all to
future code that calls into base/ that it's actually executing code that is in
renderer/ and that plays by those rules.

>  2. That the trivial implementation of the discardable provider isn't really
> emulation at all

That is how I read Carlos's objection, though I will leave it to him to reply.

[ajuma, 2013-06-11 21:24:47]

On 2013/06/11 15:47:14, Avi wrote:
> On 2013/06/10 17:34:50, vollick wrote:
> >  1. The ability to install a static discardable memory provider in base.
This
> CL
> > made the implicit assumption that per-process, per-platform control over the
> > discardable memory implementation was good enough.
> 
> Good re-statement.
> 
> It's not that it's not good enough, but that I disagree with the design. We
> don't delegate responsibility like that today, and it's not obvious at all to
> future code that calls into base/ that it's actually executing code that is in
> renderer/ and that plays by those rules.

How about moving the implementation into base/ (minus the parts that depend on
renderer visibility), and providing a hook for requesting all memory to be
purged? Then renderers would use this hook when they become invisible, and other
processes could make their own decisions about when to use this (or could choose
to ignore this hook altogether).

[Avi (use Gerrit), 2013-06-11 21:27:19]

On 2013/06/11 21:24:47, ajuma wrote:
> On 2013/06/11 15:47:14, Avi wrote:
> > On 2013/06/10 17:34:50, vollick wrote:
> > >  1. The ability to install a static discardable memory provider in base.
> This
> > CL
> > > made the implicit assumption that per-process, per-platform control over
the
> > > discardable memory implementation was good enough.
> > 
> > Good re-statement.
> > 
> > It's not that it's not good enough, but that I disagree with the design. We
> > don't delegate responsibility like that today, and it's not obvious at all
to
> > future code that calls into base/ that it's actually executing code that is
in
> > renderer/ and that plays by those rules.
> 
> How about moving the implementation into base/ (minus the parts that depend on
> renderer visibility), and providing a hook for requesting all memory to be
> purged? Then renderers would use this hook when they become invisible, and
other
> processes could make their own decisions about when to use this (or could
choose
> to ignore this hook altogether).

For platforms that don't have native support? Sure, that sounds like a
reasonable first pass. Would you be planning on making it smarter? Seems like
Carlos would like it if you were to.

[Ian Vollick, 2013-06-11 23:36:33]

>  How about moving the implementation into base/ (minus the parts that
>> depend on
>> renderer visibility), and providing a hook for requesting all memory to be
>> purged? Then renderers would use this hook when they become invisible, and
>>
> other
>
>> processes could make their own decisions about when to use this (or could
>>
> choose
>
>> to ignore this hook altogether).
>>
> Sounds good to me! I'll post a patch with this approach shortly.

>
> Would you be planning on making it smarter? Seems like
> Carlos would like it if you were to.
>
Already have :) If you take a look at the latest patch set, I'm doing some
basic emulation now.

I'll move the guts into base as Ali suggested and then ping you folks for
another round of reviews.


>
>
https://codereview.chromium.**org/15650016/<https://codereview.chromium.org/1...
>

[Ian Vollick, 2013-06-12 12:21:10]

On 2013/06/11 23:36:33, vollick wrote:
> >  How about moving the implementation into base/ (minus the parts that
> >> depend on
> >> renderer visibility), and providing a hook for requesting all memory to be
> >> purged? Then renderers would use this hook when they become invisible, and
> >>
> > other
> >
> >> processes could make their own decisions about when to use this (or could
> >>
> > choose
> >
> >> to ignore this hook altogether).
> >>
> > Sounds good to me! I'll post a patch with this approach shortly.
> 
> >
> > Would you be planning on making it smarter? Seems like
> > Carlos would like it if you were to.
> >
> Already have :) If you take a look at the latest patch set, I'm doing some
> basic emulation now.
> 
> I'll move the guts into base as Ali suggested and then ping you folks for
> another round of reviews.
> 
> 
> >
> >
>
https://codereview.chromium.**org/15650016/%3Chttps://codereview.chromium.org...>
> >

I've moved the provider/emulation into base, and it's really simplified the
patch (thanks again for the suggestion, Ali!), but it's not entirely ready for
review.

Avi, feel free to let me know what you think of the new design, but Carlos,
please hold off reviewing the provider in discardable_memory_emulated.cc -- the
emulation is going to need another update. Marcus is currently landing a memory
pressure listener that I'd like to use
(https://codereview.chromium.org/15995014/). Once it's landed I'll post an
updated patch.

[Avi (use Gerrit), 2013-06-12 14:56:48]

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory.h:26: class DiscardableMemoryProvider;
Why is this class forward declared here? It's only used below if
!DISCARDABLE_MEMORY_SUPPORTED_NATIVELY, so this declaration seems useless.

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory.h:101: #endif
I would re-order this up two functions, so it doesn't appear to be covered by
the "testing utility calls" header. Or give it a header of its own.

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:35: kSystemMemoryLowerBound;
Way too magical. Does this function belong in SysInfo? I'm borderline here, but
would be OK if you pushed hard.

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:38: typedef
HashingMRUCache<DiscardableMemory*, bool> AllocationMap;
bool? If that's just a placeholder, say so.

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:49: class DiscardableMemoryProvider {
Give the member functions explanatory comments. Member functions love
explanatory comments.

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:49: class DiscardableMemoryProvider {
DiscardableMemoryProvider desperately needs a unit test. The current unittest
file tests basic functionality of a system out of our control, but if we control
DiscardableMemoryProvider, we should be able to put it through a much more
thorough evaluation.

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:69: Unregister(discardable);
Why the Unregister? Register is only called from
DiscardableMemory::InitializeAndLock below, which is an error to call more than
once, so it shouldn't ever be in the allocation cache. Definitely DCHECK if you
want, but Unregistering seems useless.

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:83: discardable->Deallocate();
Eh...

Unregister is only called by ~DiscardableMemory, and it's weird that the
destructor of the class calls into a different class, which then calls back into
the class to dealloc. Can we move this call to DiscardableMemory::Deallocate()
into ~DiscardableMemory?

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:148: Lock lock_;
Member variables need comment love too. In particular, what is lock_ locking?

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:160: return Lock() !=
DISCARDABLE_MEMORY_FAILED;
LockDiscardableMemoryStatus status = Lock();
DCHECK_NE(DISCARDABLE_MEMORY_SUCCESS, status);  // InitializeAndLock should only
be called once; why is there already memory?
return status == DISCARDABLE_MEMORY_PURGED;

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:185: memory_ = malloc(size_ *
sizeof(char));
sizeof(char) is defined by the standard to be 1 (C++98 §5.3.3¶1, expr.sizeof),
so just do |malloc(size_)|.

[cpu_(ooo_6.6-7.5), 2013-06-13 20:16:06]

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
File base/memory/discardable_memory_emulated.cc (right):

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:132: if (bytes_allocated_ <
kCacheLimitPurgeBound)
I was expecting freeing N megs, not freeing until N megs total

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory_emulated.cc:139: PurgeAll();
again, surprised on the full purge.

[avi_google.com, 2013-06-14 11:09:33]

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
File base/memory/discardable_memory.h (right):

https://codereview.chromium.org/15650016/diff/53001/base/memory/discardable_m...
base/memory/discardable_memory.h:25: #define
DISCARDABLE_MEMORY_SUPPORTED_NATIVELY
BTW it's likely that we will need to turn this into a runtime function rather
than a #define. The reason I had DiscardableMemory::Supported was that the
concept showed up in Win8, so if we go native on Win32 it won't be compile time.