base: Add soft memory limit to DiscardableMemoryManager.

base: Add soft memory limit to DiscardableMemoryManager.   
                                                                                 
This adds a soft memory limit to the discardable memory manager. Only            
allocations that haven't been used in specific amount of time are                
affected by this limit. This allows a client to use a much higher hard           
limit so temporary spikes in required usage can be handled efficiently           
without causing an increase in usage while idle.                                 
                                                                                 
A "reduce memory usage" function is added to the discardable memory                 
interface to allow the client to effectively trigger purging of memory           
that is affected by the soft limit when system reaches what would be             
considered an idle state.                                                        
                                                                                 
BUG=383361, 372158
TEST=base_unittests --gtest_filter=DiscardableMemoryManagerTest.ReduceMemoryUsage

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

[reveman, 2014-06-20 03:36:27]

This seems to be working well. Allocations not used for some time are effected
by a soft memory limit that can be set relatively low. And we attempt to reduce
memory to within this soft limit in a similar way to how we handle v8 garbage
collection, when we're "idle".

I'm not changing any of the limits in this patch. That's done separately by
https://codereview.chromium.org/342943008/ instead.

Please take a look.

[Stephen White, 2014-06-20 14:06:01]

This seems reasonable. One concern I have (possibly stemming from ignorance):
can a spinning CPU prevent the idle notification from being sent? If so, could
this allow us to exceed the hard memory limit? I'm thinking of long-running JS,
or fast scrolling.

Also, looks like you'll need jamesr's approval on the content/renderer change,
if I'm reading the OWNERS file correctly.

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

https://codereview.chromium.org/336273003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_android.cc:61: return
internal::DiscardableMemoryEmulated::IdleNotification();
Not new to this patch, but it seems odd to have the discardable memory
implementions fall back to the emulated implementation for these methods. Maybe
they should be refactored to a common base class instead
(DiscardableMemoryBase?).

[Justin Novosad, 2014-06-20 14:43:47]

I think this is a great improvement but I am still concerned that if the working
set of images that are used at every frame takes more than
hard_memory_limit_expiration_time_ to decode, we may end-up discarding and
redecoding the entire set at every frame.  

This is one of the classic pitfalls of LRU eviction with cyclic access patterns:
once the working set exceeds cache capacity, we fall from 100% cache hits to 0%.
 It would be nice to find a way to degrade performance more gracefully in this
kind of situation, but that does not need to be resolved here.  Just something
to keep in mind as a future improvement.

[reveman, 2014-06-20 18:06:29]

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

https://codereview.chromium.org/336273003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_android.cc:61: return
internal::DiscardableMemoryEmulated::IdleNotification();
On 2014/06/20 14:06:01, Stephen White wrote:
> Not new to this patch, but it seems odd to have the discardable memory
> implementions fall back to the emulated implementation for these methods.
Maybe
> they should be refactored to a common base class instead
> (DiscardableMemoryBase?).

It's not really a fallback. IdleNotification will notify all implementations
that care about this signal. Just happens that "emulated" is the only
implementation that care right now. If ashmem implementation starts taking
advantage of this notification in the future, we'd call
internal::DiscardableMemoryAshmem::IdleNotification() too and only return true
if both functions return true.

FYI, we typically don't use more than one type discardable memory implementation
except for unit tests but there's nothing stopping us from using multiple if
that would ever be useful.

[reveman, 2014-06-20 18:11:37]

On 2014/06/20 14:06:01, Stephen White wrote:
> This seems reasonable. One concern I have (possibly stemming from ignorance):
> can a spinning CPU prevent the idle notification from being sent? If so, could
> this allow us to exceed the hard memory limit? I'm thinking of long-running
JS,
> or fast scrolling.

The hard limit is controlled just like before and the "idle" notification
shouldn't affect that. It's possible that we never reduce our memory down to the
soft limit with a spinning CPU though but that's sort of the intended behavior.

[Stephen White, 2014-06-20 18:12:58]

On 2014/06/20 18:11:37, reveman wrote:
> On 2014/06/20 14:06:01, Stephen White wrote:
> > This seems reasonable. One concern I have (possibly stemming from
ignorance):
> > can a spinning CPU prevent the idle notification from being sent? If so,
could
> > this allow us to exceed the hard memory limit? I'm thinking of long-running
> JS,
> > or fast scrolling.
> 
> The hard limit is controlled just like before and the "idle" notification
> shouldn't affect that. It's possible that we never reduce our memory down to
the
> soft limit with a spinning CPU though but that's sort of the intended
behavior.

SGTM and LGTM.

[reveman, 2014-06-20 18:36:37]

On 2014/06/20 14:43:47, junov wrote:
> I think this is a great improvement but I am still concerned that if the
working
> set of images that are used at every frame takes more than
> hard_memory_limit_expiration_time_ to decode, we may end-up discarding and
> redecoding the entire set at every frame.  

The usage of the idle notification is supposed to solve that and it seems to be
working afaict. We're only purging memory based on
hard_memory_limit_expiration_time_ when we become idle so if we're repeatedly
drawing a frame that takes longer than hard_memory_limit_expiration_time_ all
decoded images should stay in the cache unless we become idle in between frames.

> 
> This is one of the classic pitfalls of LRU eviction with cyclic access
patterns:
> once the working set exceeds cache capacity, we fall from 100% cache hits to
0%.
>  It would be nice to find a way to degrade performance more gracefully in this
> kind of situation, but that does not need to be resolved here.  Just something
> to keep in mind as a future improvement.

Yea, switching to an adaptive algorithm might be a worthwhile follow up. That
would make sense after this patch as native and emulated implementations can now
use the same hard limit. An adaptive algorithm would improve our behavior when
hitting the hard limit in both cases.

[reveman, 2014-06-23 14:59:41]

jamesr can you take a look at content/renderer/ changes?

willchan for owner review of base/memory

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

https://codereview.chromium.org/336273003/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:884: void RenderThreadImpl::IdleHandler()
{
Using this idle handler seem to work Ok but there might be corner cases where
this is not doing exactly what we want as it seems to be somewhat specifically
designed for v8 garbage collection. I think the intent is at least clear and we
can improve it using follow up patches if necessary.

[willchan no longer on Chromium, 2014-06-24 16:33:00]

Note that I'm still traveling, so expect high latency.

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

https://codereview.chromium.org/336273003/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:169: void
PurgeIfNotUsedSinceTimestampUntilUsageIsWithinLimitWithLockAcquired(
Nice function name.

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

https://codereview.chromium.org/336273003/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:884: void RenderThreadImpl::IdleHandler()
{
On 2014/06/23 14:59:40, reveman wrote:
> Using this idle handler seem to work Ok but there might be corner cases where
> this is not doing exactly what we want as it seems to be somewhat specifically
> designed for v8 garbage collection. I think the intent is at least clear and
we
> can improve it using follow up patches if necessary.

I'm not going to bother with the review until James OKs this. We've historically
been very cautious with idle handlers since detecting idleness (predicting the
future) is a hard problem.

[jamesr, 2014-06-24 17:48:10]

When do you hope this function will be called?

[reveman, 2014-06-24 20:03:24]

On 2014/06/24 17:48:10, jamesr wrote:
> When do you hope this function will be called?

I'm hoping that it's not called too frequently while the renderer is busy doing
work that will greatly benefit from cached data. Rasterization happens to be one
of those things but it's not necessarily restricted to that as it's really about
anything that uses discardable memory. It doesn't have to be perfect. If it's a
bit better than just calling this function every 10s, that's a start.

Called too frequently -> more cache misses
Never called -> a renderer with visible widgets doing nothing will use more
memory but will still stay within the hard limit

[jamesr, 2014-06-24 20:22:54]

I think this is pretty reasonable.  It's using the idle handler to potentially
toss memory, much like the V8 GCs.  So long as it's OK for this to not happen
for potentially very long periods of time for foreground tabs this should work
out.

lgtm for content/renderer/ but please keep a close eye on it.  It might get hard
to tune both this and the V8 GC needs if they stay on the same timer, we'll just
have to see.

[reveman, 2014-06-24 21:00:21]

On 2014/06/24 20:22:54, jamesr wrote:
> I think this is pretty reasonable.  It's using the idle handler to potentially
> toss memory, much like the V8 GCs.  So long as it's OK for this to not happen
> for potentially very long periods of time for foreground tabs this should work
> out.
> 
> lgtm for content/renderer/ but please keep a close eye on it.  It might get
hard
> to tune both this and the V8 GC needs if they stay on the same timer, we'll
just
> have to see.

I'll keep a close eye on it. The discardable memory crash key should be useful
when diagnosing issues related to this.

Will, I know you're traveling, so no rush, ptal you have a chance.

[willchan no longer on Chromium, 2014-06-27 14:12:57]

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

https://codereview.chromium.org/336273003/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.h:100: // Call this when idle. Discardable memory
implementations might use this
I'd give more guidance on "idle". Specifically, discuss the tradeoffs. Calling
this too often will hurt performance, whereas calling it too infrequently will
bloat memory usage. At least, that's how I understand it :)

Actually, perhaps we should rename this. This function name assumes too much
about how the caller might want to use it. Yes, most likely the sane way to use
this is in an idle handler, but functionally what it does is try to discard
memory up to a threshold.

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

https://codereview.chromium.org/336273003/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:138: protected:
I don't think this needs to be protected. Private functions can still be
overridden, just not called by derived classes. The test subclass does not
appear to call Now().

https://codereview.chromium.org/336273003/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:174: void BytesAllocatedChanged(size_t
new_bytes_allocated) const;
Why is it necessary to pass this? If it's better for some reason (I actually
like passing things explicitly, so it seems good to me), then maybe we should
make this a static function so it's clear this is the only state exchanged here?

[reveman, 2014-06-27 20:38:02]

PTAL

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

https://codereview.chromium.org/336273003/diff/60001/base/memory/discardable_...
base/memory/discardable_memory.h:100: // Call this when idle. Discardable memory
implementations might use this
On 2014/06/27 14:12:56, willchan (traveling 5.23-6.27) wrote:
> I'd give more guidance on "idle". Specifically, discuss the tradeoffs. Calling
> this too often will hurt performance, whereas calling it too infrequently will
> bloat memory usage. At least, that's how I understand it :)

Correct. I've updated the comment.

> 
> Actually, perhaps we should rename this. This function name assumes too much
> about how the caller might want to use it. Yes, most likely the sane way to
use
> this is in an idle handler, but functionally what it does is try to discard
> memory up to a threshold.

Good idea. I changed the name to ReduceMemoryUsage and made it clear in the
comment what this function might do.

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

https://codereview.chromium.org/336273003/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:138: protected:
On 2014/06/27 14:12:57, willchan (traveling 5.23-6.27) wrote:
> I don't think this needs to be protected. Private functions can still be
> overridden, just not called by derived classes. The test subclass does not
> appear to call Now().

Done.

https://codereview.chromium.org/336273003/diff/60001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:174: void BytesAllocatedChanged(size_t
new_bytes_allocated) const;
On 2014/06/27 14:12:57, willchan (traveling 5.23-6.27) wrote:
> Why is it necessary to pass this? If it's better for some reason (I actually
> like passing things explicitly, so it seems good to me), then maybe we should
> make this a static function so it's clear this is the only state exchanged
here?

This function previously assumed that it was always called while it's safe to
read bytes_allocated_ without causing a race. I could have fixed that by adding
WithLockAcquired to the name but that adds some confusion to the ctor where we
can call this without acquiring the lock.

Passing the value as a function argument solves this but I think it's still
cleaner to keep it a normal member function as we're using the instance address
as unique id for the trace counter.

[willchan no longer on Chromium, 2014-07-07 22:28:08]

lgtm

https://codereview.chromium.org/336273003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager.h (right):

https://codereview.chromium.org/336273003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:68: // When notified that idle, the
manager purges memory not used since the hard
Fix comment not to mention idle, but requested to reduce memory? Or just delete
this comment since the ReduceMemoryUsage() function already describes it well.

https://codereview.chromium.org/336273003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager_unittest.cc (right):

https://codereview.chromium.org/336273003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager_unittest.cc:64: virtual TimeTicks Now()
const OVERRIDE { return now_; }
Likewise, this doesn't have to be protected. It's called only in the base class.

[reveman, 2014-07-07 23:49:03]

https://codereview.chromium.org/336273003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager.h (right):

https://codereview.chromium.org/336273003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager.h:68: // When notified that idle, the
manager purges memory not used since the hard
On 2014/07/07 22:28:08, willchan wrote:
> Fix comment not to mention idle, but requested to reduce memory? Or just
delete
> this comment since the ReduceMemoryUsage() function already describes it well.

Done.

https://codereview.chromium.org/336273003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_manager_unittest.cc (right):

https://codereview.chromium.org/336273003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_manager_unittest.cc:64: virtual TimeTicks Now()
const OVERRIDE { return now_; }
On 2014/07/07 22:28:08, willchan wrote:
> Likewise, this doesn't have to be protected. It's called only in the base
class.

Done.

[reveman, 2014-07-07 23:49:47]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-07-07 23:52:09]

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

[commit-bot: I haz the power, 2014-07-08 03:22:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25697)

[commit-bot: I haz the power, 2014-07-08 03:24:27]

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

[commit-bot: I haz the power, 2014-07-08 03:24:28]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[reveman, 2014-07-08 15:54:39]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-07-08 15:55:25]

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

[commit-bot: I haz the power, 2014-07-08 19:31:31]

Change committed as 281795