Add a DiscardableMemoryShmemeAllocator.

Add a DiscardableMemoryAllocator to html_viewer.

Recent changes to skia now require this object, and crash if this
factory isn't supplied. We create the minimal implementation to get a
Google search working in html_viewer. This allocator will free
unlocked chunks once it has reached its memory threshold.

We're starting with a soft limit of 20 megabytes. We'll adjust this
based on observed performance.

Committed: https://crrev.com/5f32298dbf36a4c48ef02d18b6acb1f2be759195
Cr-Commit-Position: refs/heads/master@{#321461}

[Elliot Glaysher, 2015-03-16 20:26:00]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2015-03-16 20:26:01]

Without this, we immediately crash on text input to the google search box on tot
html_viewer.

[sky, 2015-03-16 21:25:31]

LGTM

[Elliot Glaysher, 2015-03-16 21:28:40]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-03-16 21:29:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016493002/1

[Elliot Glaysher, 2015-03-16 22:03:32]

erg@chromium.org changed reviewers:
+ jamesr@chromium.org

[Elliot Glaysher, 2015-03-16 22:03:32]

+jamesr: owners stomp

[commit-bot: I haz the power, 2015-03-16 22:05:18]

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

[commit-bot: I haz the power, 2015-03-16 22:05:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jamesr, 2015-03-16 22:34:48]

How is this code being reached? In the configuration we're using we shouldn't
require any shared memory

[Elliot Glaysher, 2015-03-16 22:49:48]

On 2015/03/16 22:34:48, jamesr wrote:
> How is this code being reached? In the configuration we're using we shouldn't
> require any shared memory

Through skia. First, here's a stack trace:

#0 0x7fbaf57c50fe base::debug::StackTrace::StackTrace()
#1 0x7fbaf56aa685 logging::LogMessage::~LogMessage()
#2 0x7fbaf57993e3 base::DiscardableMemoryShmemAllocator::GetInstance()
#3 0x7fbaf5798759
base::internal::DiscardableMemoryShmem::DiscardableMemoryShmem()
#4 0x7fbaf5798416 base::DiscardableMemory::CreateLockedMemory()
#5 0x7fbaf587adbc SkDiscardableMemory::Create()
#6 0x7fbaf59d1960 SkResourceCache::newCachedData()
#7 0x7fbaf59d2750 SkResourceCache::NewCachedData()
#8 0x7fbaf5a98602 copy_mask_to_cacheddata()
#9 0x7fbaf5a95de8 add_cached_rects()
#10 0x7fbaf5a959e4 SkBlurMaskFilterImpl::filterRectsToNine()
#11 0x7fbaf595649b SkMaskFilter::filterPath()
#12 0x7fbaf592b9e9 SkDraw::drawPath()
#13 0x7fbaf58b6b63 SkDraw::drawPath()
#14 0x7fbaf592a618 SkDraw::drawRect()
#15 0x7fbaf58b6b01 SkDraw::drawRect()
#16 0x7fbaf58b5d48 SkBitmapDevice::drawRect()
#17 0x7fbaf5906639 SkCanvas::onDrawRect()
#18 0x7fbaf5905865 SkCanvas::drawRect()
#19 0x7fbaf6899fad blink::GraphicsContext::drawRect()
#20 0x7fbaf6899707 blink::GraphicsContext::fillRect()
#21 0x7fbaf744400f blink::BoxPainter::paintBoxShadow()

So skia's makes discardable memory. (That seems fine.) And then it calls base's
discardable memory implementation. (Seems reasonable.) And in chrome, all
discardable memory is shared memory, so it asks the
base::internal::DiscardableMemoryShmem object which doesn't actually create a
shared memory segment, but calls into the
DiscardableMemoryShmemAllocator::GetInstance(), which will return a piece of
memory, but it may or may not be backed by shared memory.

The implementation that I put here doesn't actually use shared memory, and is
almost exactly a copy of the base::testing version of this object.

[jamesr, 2015-03-16 23:47:26]

https://codereview.chromium.org/1016493002/diff/1/mojo/services/html_viewer/h...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/1016493002/diff/1/mojo/services/html_viewer/h...
mojo/services/html_viewer/html_viewer.cc:241: // TODO(erg): Skia requires that
we have one of these. However, all the
what's the action item of the TODO here?

https://codereview.chromium.org/1016493002/diff/1/mojo/services/html_viewer/h...
File mojo/services/html_viewer/html_viewer_discardable_memory_shmem_allocator.cc
(right):

https://codereview.chromium.org/1016493002/diff/1/mojo/services/html_viewer/h...
mojo/services/html_viewer/html_viewer_discardable_memory_shmem_allocator.cc:17:
bool Lock() override { return false; }
returning false from Lock() means that the memory is unusable, so if you always
want to return false you can free the memory up immediately in Unlock(). but is
that really what you want? what this class does it provide a memory segment that
is only usable until it is first Unlock()ed (since it is initially locked).
that's not very useful for a cache

https://codereview.chromium.org/1016493002/diff/1/mojo/services/html_viewer/h...
File mojo/services/html_viewer/html_viewer_discardable_memory_shmem_allocator.h
(right):

https://codereview.chromium.org/1016493002/diff/1/mojo/services/html_viewer/h...
mojo/services/html_viewer/html_viewer_discardable_memory_shmem_allocator.h:12:
// This is a copy of the testing allocator, which doesn't actually do any
statements about history are not very useful in comments and likely to go stale
immediately. they're far more useful in the patch description. please describe
what this class does in the header, not where it's from

https://codereview.chromium.org/1016493002/diff/1/mojo/services/html_viewer/h...
mojo/services/html_viewer/html_viewer_discardable_memory_shmem_allocator.h:15:
class HtmlViewerDiscardableMemoryShmemAllocator
you're in the directory html_viewer and in the namespace html_viewer so
repeating "HtmlViewer" in the class name only makes this class name really long
without adding any information. please shorten it up to describe what it does

[jamesr, 2015-03-16 23:51:15]

If you want something that never discards then return true from Lock() always. 
I'm not sure how large the memory use would grow with such a scheme, but we
could find out.  If that's not workable you probably need to implement an LRU
cache to evict things.

[Elliot Glaysher, 2015-03-17 20:40:48]

ptal

I've rewritten the entire patch, and have added tests. (But I'm not sure I have
enough tests. If you have any suggestions...)

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
mojo/services/html_viewer/html_viewer.cc:64: size_t kMaxMemory = 20 * 1024 *
1024;
I have no justification for this constant. It can go up or down based on
performance.

[jamesr, 2015-03-17 23:26:20]

Sorry - I still owe you a review here, but I just noticed
https://codereview.chromium.org/1001873002/ landed.  Does that impact this?

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
File mojo/services/html_viewer/discardable_memory_allocator.cc (right):

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
mojo/services/html_viewer/discardable_memory_allocator.cc:17:
OwnedMemoryChunk(size_t size)
explicit

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
mojo/services/html_viewer/discardable_memory_allocator.cc:40: void* Memory()
const { return memory_.get(); }
DCHECK(is_locked_) here

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
File mojo/services/html_viewer/discardable_memory_allocator.h (right):

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
mojo/services/html_viewer/discardable_memory_allocator.h:19: class
OwnedMemoryChunk;
does this forward declaration need to be public?

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
mojo/services/html_viewer/discardable_memory_allocator.h:21:
DiscardableMemoryAllocator(size_t max_memory);
explicit

[Elliot Glaysher, 2015-03-18 20:36:22]

ptal. I've rebased to tot and did your other comments.

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
File mojo/services/html_viewer/discardable_memory_allocator.h (right):

https://codereview.chromium.org/1016493002/diff/60001/mojo/services/html_view...
mojo/services/html_viewer/discardable_memory_allocator.h:19: class
OwnedMemoryChunk;
On 2015/03/17 23:26:19, jamesr wrote:
> does this forward declaration need to be public?

It does if I want to hide the implementation of DiscardableMemoryChunkImpl from
the linker. I'm fine moving that to an inner class of DiscardableMemoryAllocator
instead if you want, but I think this is slightly better.

[jamesr, 2015-03-18 22:40:15]

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
File mojo/services/html_viewer/discardable_memory_allocator.cc (right):

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator.cc:22:
allocator_(nullptr),
does it every make sense for allocator_ to be null or to change? seems like it
should be a constructor param if not (and i can't see how it would)

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator.cc:90: if (memory_chunk_)
seems like this should be a DCHECK since it's not valid to call Unlock() unless
the memory is locked and it's not valid for memory_chunk_ to go null while the
memory is locked

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator.cc:110:
total_live_memory_(0) {
0u

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator.cc:124: // Purge unlocked
chunks from the cache until |total_live_memory_| is under
what if the size of all currently locked chunks exceeds the desired maximum
size?

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator.cc:127: while
(total_live_memory_ > max_memory_ && it != live_chunks_.end()) {
the second clause here looks like we'll break even if we're over the limit if
everything is locked, but the comment says otherwise. could this instead be a
for loop that iterates through the link_chunks_ at most once evicting as it
goes, if possible?

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
File mojo/services/html_viewer/discardable_memory_allocator_unittest.cc (right):

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator_unittest.cc:67:
EXPECT_NE(nullptr, chunk_three->Memory());
please also unlock one of these and see what happens. i think that unlocking one
of these should immediately discard it since we're at almost 3x budget here,
right?

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/html_viewer.cc:63: // Only have 20 megabytes of
discardable memory to start out with.
what do you mean "to start out with"? it doesn't seem that this value can change
at runtime. if this is intended as a statement about the history of the code,
put it in the patch description

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/html_viewer.cc:64: size_t kMaxMemory = 20 * 1024 *
1024;
if i'm reading the allocator correctly the first allocation of any amount of
discardable memory will succeed (i.e. allocating 50MB would work) so this isn't
really a max value, it's a value that the system will try to evict down towards
on future allocations. maybe call this a target or something if that's the
intended behavior?

[Elliot Glaysher, 2015-03-18 23:54:44]

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
File mojo/services/html_viewer/discardable_memory_allocator.cc (right):

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator.cc:127: while
(total_live_memory_ > max_memory_ && it != live_chunks_.end()) {
On 2015/03/18 22:40:14, jamesr wrote:
> the second clause here looks like we'll break even if we're over the limit if
> everything is locked, but the comment says otherwise. could this instead be a
> for loop that iterates through the link_chunks_ at most once evicting as it
> goes, if possible?

I'm not sure what you're asking for here. I get that the comment is
ambiguous--I've fixed that up. But in the case where all chunks are locked,
there's nothing really that we can do. I've renamed the variable to
desired_max_memory_ to emphasize that this is more of a soft guideline.

If you're asking to do a for loop across the entire linked list, unallocating
all unlocked chunks, we don't want to do that because now we aren't an effective
cache--we want to hold unlocked chunks and only free them on memory pressure.

(However, in the case where we blow through our desired memory limit, and have
our hands tied because all memory is locked, I'd argue that our caller is not
obeying the interface; they need to unlock memory once they're done with it.)

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
File mojo/services/html_viewer/html_viewer.cc (right):

https://codereview.chromium.org/1016493002/diff/100001/mojo/services/html_vie...
mojo/services/html_viewer/html_viewer.cc:64: size_t kMaxMemory = 20 * 1024 *
1024;
On 2015/03/18 22:40:14, jamesr wrote:
> if i'm reading the allocator correctly the first allocation of any amount of
> discardable memory will succeed (i.e. allocating 50MB would work) so this
isn't
> really a max value, it's a value that the system will try to evict down
towards
> on future allocations. maybe call this a target or something if that's the
> intended behavior?

Renamed to 'kDesiredMaxMemory', and moved the above comment into the commit
message.

[Elliot Glaysher, 2015-03-19 20:40:24]

Redid with keeping track of just the unlocked chunks.

Added debugging statements to make sure we aren't holding on to locked chunks.
At least in limited usage of the html_viewer, these dchecks didn't fire.

[jamesr, 2015-03-19 22:09:56]

lgtm

https://codereview.chromium.org/1016493002/diff/140001/mojo/services/html_vie...
File mojo/services/html_viewer/discardable_memory_allocator.cc (right):

https://codereview.chromium.org/1016493002/diff/140001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator.cc:18: explicit
OwnedMemoryChunk(size_t size, DiscardableMemoryAllocator* allocator)
nit: no explicit for 2-arg ctors

(yes it has a meaning with c++11 uniform initialization syntax, but we don't
allow that in chromium and i think we probably never will)

https://codereview.chromium.org/1016493002/diff/140001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator.cc:38: bool locked()
const { return is_locked_; }
call it is_locked() to match the member (and i think it's a nicer name)

https://codereview.chromium.org/1016493002/diff/140001/mojo/services/html_vie...
File mojo/services/html_viewer/discardable_memory_allocator_unittest.cc (right):

https://codereview.chromium.org/1016493002/diff/140001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator_unittest.cc:18:
TEST(DiscardableMemoryAllocator, Basic) {
put this in an anonymous namespace. i've had ODR violations and other weirdness
in unit test targets before and it sucks

https://codereview.chromium.org/1016493002/diff/140001/mojo/services/html_vie...
mojo/services/html_viewer/discardable_memory_allocator_unittest.cc:22:
html_viewer::DiscardableMemoryAllocator allocator(kOneMegabyte);
if you wrap the whole file in

namespace html_viewer {
namespace {
...
}  // namespace
}  // namespace html_viewer

then you won't have to spell out this namespace here and still get the safety of
an anonymous namespace

[Elliot Glaysher, 2015-03-19 22:36:42]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-03-19 22:36:43]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
jamesr@chromium.org
Link to the patchset: https://codereview.chromium.org/1016493002/#ps160001
(title: "jamesr comments + rebase to ToT")

[commit-bot: I haz the power, 2015-03-19 22:37:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016493002/160001

[commit-bot: I haz the power, 2015-03-19 22:59:39]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-19 23:00:15]

Patchset 9 (id:??) landed as
https://crrev.com/5f32298dbf36a4c48ef02d18b6acb1f2be759195
Cr-Commit-Position: refs/heads/master@{#321461}