OSX allows one to allocate memory that gets thrown out under memory pressure instead of getting paged to disk. This is useful for storing bulky recomputable things like image cache, memory network cache, etc in an OS-friendly way. In theory this lets the OS virtual memory eviction policy take care of managing memory more efficiently than our heuristics can. Chrome uses this, see http://code.google.com/p/chromium/source/search?q=VM_FLAGS_PURGABLE&origq=VM_FLAGS_PURGABLE&btnG=Search+Trunk See also https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/NSCache_Class/Reference/Reference.html and http://fxr.watson.org/fxr/source/osfmk/vm/vm_object.c?v=xnu-1228;im=bigexcerpts#L5265 In similar vein android has ashmem, this has now been upstreamed in linux.

> bool PurgeableBuffer::wasPurged() const This is a pretty obvious footgun; you can't write code like if (!wasPurged()) { // use the buffer }. I wonder how you atomically * check that the memory is not purged, and, if so * mark it as not purgeable.

(In reply to Justin Lebar [:jlebar] from comment #1) > > bool PurgeableBuffer::wasPurged() const > > This is a pretty obvious footgun; you can't write code like > > if (!wasPurged()) { > // use the buffer > }. > > I wonder how you atomically > > * check that the memory is not purged, and, if so > * mark it as not purgeable. It uses locking. You set the item to be non-volatile, if is still in memory, you can access it, if not..recreate

Oh, I see; you set it to non-volatile and then you do: m_state = state & VM_PURGABLE_EMPTY ? Purged : NonVolatile; Got it.

Mike, what memory are you planning to cache? Decoded images? On Android, I expect those are stored in X, not in process -- at least, that's how it works on desktop Linux.

(In reply to Justin Lebar [:jlebar] from comment #5) > Mike, what memory are you planning to cache? Decoded images? On Android, I > expect those are stored in X, not in process -- at least, that's how it > works on desktop Linux. There is no X on Android. My first step will be to have a cross-platform (mac-windows-android) class for a purgeable buffer. From there, we'll see how it can be used.

For reference, for a Windows implementation: http://code.google.com/p/chromium/issues/detail?id=21926#c10

Network memory cache is also a candidate.

I've never seen the network memory cache grow above 5mb. Have you?

(In reply to Justin Lebar [:jlebar] from comment #9) > I've never seen the network memory cache grow above 5mb. Have you? a) 5mb is a lot on fennec b) point of this stuff is so we can start allocating things more aggressively

Mike, can you upload your wip patch here?

Not a patch per se. It's the tentative API I originally wrote but doesn't fit anywhere in the img loader or the network cache. Both would need an API that allows to lock the buffer, similarly, but have the caller request whether the buffer was purged by the kernel before filling it itself.

Comment on attachment 648655 [details] Tentative API The patch will prematurely release buffers if two calls to Get cause ScopedPtrs to have overlapping lifetimes. Do you have to use void* for the returned buffer? Some minimal type-safety for callers would be nice. It is confusing that ThrowableBuffer has both a "Get" method (returning a ScoredPtr) and a "get" method (intended to be used by ScoredPtr). ThrowableBuffer::get should be marked as 'protected' to ensure callers use ScoredPtr. ScopedPtr and ThrowableBuffer should be non-copyable. Testing (*(char *)buf == 0) seems sketchy. What if the actual data (placed into the buf by the 'fill' callback) starts with a zero byte? The #else case should use an "init" bool (like the Mac case) instead of pointlessly clearing the buffer every time. (Or is the hope that the OS might see that the pages are all zero and remove their backing?? That doesn't seem worth the performance surprise.)

How will we test: * That operating systems purge buffers when needed? * That operating systems do not purge buffers when inappropriate? (e.g. when not actually low on memory, or while we think we're in the "non-purgeable" state) * That callers deal appropriately? (Even if we make Gecko's memory-pressure notifications free these buffers, that will only let us test the case where the event loop intervenes.)

Joe says this might be a win for image stuff. I'll have someone on my team complete this once Joe or networking is ready to benefit from it. Jesse, we'll do some telemetry to study if OS behavior is good.

We really want this for B2G.

Michal, something we may need to think of soon at the new cache implementation.

This is a PoC that works for me on a Nexus 5 running B2G. I created a simple VolatileBuffer which uses a Lock/Unlock api rather than callback. I suspect that in most cases that aren't dynamic linkers, we can't take advantage of per-page granularity in discarding memory. If we do find a such cases in the future, I think they can exist alongside a simpler API. Additionally, this VolatileBuffer only uses ashmem when the amount allocated is greater than 8192 bytes. This is an entirely arbitrary number that felt good to me. I have a simple program that purges all purgable ashmem buffers which I used to test the program. I can confirm that we end up losing the memory but the RasterImage is able recover and decode the images again.

This is the test program I used. The patch applies to gonk-misc in a standard B2G build environment.

This fixes a scenario where images weren't getting decoded again.

Started implementing VolatileBuffers for other OS's. This patch builds on everything but Windows. (Previous patches only built on B2G.) OSX might work with a little bit more hacking - I think image optimization ends up converting things into quartz surfaces, which then throws away the VolatileBuffer. Going to split the VolatileBuffer backed imgFrames part of the patch into another bug once I get VolatileBuffers building on Windows. Curious if this helps us avoid getting killed on Fennec, though I'm not sure how to test that.

Note that this patch depends on having the patch from bug 958008 applied, if your tree doesn't have it already.

This one works on OSX. Turns out OSX doesn't really have any sort of special offscreen surfaces for images, so it was easy to make work.

I compiled this with clang++ without any particular options. Seems to just work.

This seems to work, though getting the test to link involved a bit of a hack. I don't know if this really belongs in xpcom/ds - suggestions welcome. I have a windows implementation which is going into a follow up bug.

One liner build fix for OSX.

In case anyone wants to try all these patches without finding all the bugs and applying them individually.

Some thoughts on this API - 1. wasPurged() is not implemented because at least on ashmem, one can't check the status of the pages without also making the kernel think we're actually using the pages. Might be worth implementing on platforms where that isn't a problem though, so we can dispose of any objects dedicated to managing these buffers. 2. Due to the explicit Locking/Unlocking in the API, we can do userspace LRU purging on platforms that don't have a working VolatileBuffer implementation.

This adds a SizeOf function. Otherwise, largely the same. The cppunittest fails on the pandas on try, but I think that's because it needs to run as root. Seems to run fine on my (rooted) nexus 5.

Comment on attachment 8365589 [details] [diff] [review] Implement VolatileBuffer for OSX and Ashmem, v3 Review of attachment 8365589 [details] [diff] [review]: ----------------------------------------------------------------- If you move this to memory/, you won't have to include the cpp sources from the test. I'm not particularly convinced by the API as it is, see inline comments. ::: xpcom/ds/VolatileBufferAshmem.cpp @@ +14,5 @@ > +#include <unistd.h> > + > +#define MIN_VOLATILE_ALLOC_SIZE 8192 > + > +using namespace mozilla; Not usually great to add in unified sources. @@ +16,5 @@ > +#define MIN_VOLATILE_ALLOC_SIZE 8192 > + > +using namespace mozilla; > + > +VolatileBuffer::VolatileBuffer(size_t aSize, size_t aAlignment) Not sure what you want to do with alignment, especially if you're not enforcing it in the malloc case. @@ +23,5 @@ > + , mCount(0) > + , mFd(-1) > +{ > + if (aSize < MIN_VOLATILE_ALLOC_SIZE) > + goto heap_alloc; It would be better to have a e.g. InitFromHeap private method and call it. @@ +68,5 @@ > + return mFd >= 0 || mBuf; > +} > + > +bool > +VolatileBuffer::Lock(void **aBuf) I think it would be better to have a RAII way of doing this (and no manual locking/unlocking) @@ +78,5 @@ > + if (!mBuf) { > + return false; > + } > + > + mCount++; The uses of mCount are thread-unsafe. At the very least, you want it with atomic updates. Not sure this would need full blown locking, haven't thought too much about it. @@ +84,5 @@ > + if (mFd < 0 || mCount > 1) { > + return true; > + } > + > + struct ashmem_pin pin = { 0, 0 }; Add a comment that this means the entire ashmem buffer (offset=0, len=0, with len=0 meaning size - offset (modulo alignment)) ::: xpcom/ds/VolatileBufferOSX.cpp @@ +26,5 @@ > + > + ret = vm_allocate(mach_task_self(), > + (vm_address_t *)&mBuf, > + mSize, > + VM_FLAGS_PURGABLE | VM_FLAGS_ANYWHERE | VM_MAKE_TAG(69)); Remove VM_MAKE_TAG(69).

Comment on attachment 8365589 [details] [diff] [review] Implement VolatileBuffer for OSX and Ashmem, v3 Review of attachment 8365589 [details] [diff] [review]: ----------------------------------------------------------------- I originally had this in memory/ but there were issues with refcounted objects not being exported properly on Windows. Not planning to support Windows in this bug though, so I guess we'll deal with it in another bug. ::: xpcom/ds/VolatileBufferAshmem.cpp @@ +16,5 @@ > +#define MIN_VOLATILE_ALLOC_SIZE 8192 > + > +using namespace mozilla; > + > +VolatileBuffer::VolatileBuffer(size_t aSize, size_t aAlignment) What do you mean? The fallback uses the specified alignment if moz_posix_memalign is available. @@ +68,5 @@ > + return mFd >= 0 || mBuf; > +} > + > +bool > +VolatileBuffer::Lock(void **aBuf) Agreed. For some reason I just never thought of it. @@ +78,5 @@ > + if (!mBuf) { > + return false; > + } > + > + mCount++; I'll have to check, but I think we currently only need to use this interface on a single thread. Guess there's no reason not to support multiple threads though. ::: xpcom/ds/VolatileBufferOSX.cpp @@ +26,5 @@ > + > + ret = vm_allocate(mach_task_self(), > + (vm_address_t *)&mBuf, > + mSize, > + VM_FLAGS_PURGABLE | VM_FLAGS_ANYWHERE | VM_MAKE_TAG(69)); Both blink and webkit implementations specify a tag - I'll try to see if this has any value.

(In reply to Michael Wu [:mwu] from comment #32) > ::: xpcom/ds/VolatileBufferAshmem.cpp > @@ +16,5 @@ > > +#define MIN_VOLATILE_ALLOC_SIZE 8192 > > + > > +using namespace mozilla; > > + > > +VolatileBuffer::VolatileBuffer(size_t aSize, size_t aAlignment) > > What do you mean? The fallback uses the specified alignment if > moz_posix_memalign is available. if moz_posix_memalign is available. But that doesn't answer the question.

(In reply to Mike Hommey [:glandium] from comment #33) > (In reply to Michael Wu [:mwu] from comment #32) > > ::: xpcom/ds/VolatileBufferAshmem.cpp > > @@ +16,5 @@ > > > +#define MIN_VOLATILE_ALLOC_SIZE 8192 > > > + > > > +using namespace mozilla; > > > + > > > +VolatileBuffer::VolatileBuffer(size_t aSize, size_t aAlignment) > > > > What do you mean? The fallback uses the specified alignment if > > moz_posix_memalign is available. > > if moz_posix_memalign is available. But that doesn't answer the question. I didn't answer it because I wasn't sure what it is. But let me explain what's going on here and maybe that'll work. When we're allocating for image surfaces, gfxImageSurface usually requests 16 byte alignment (gfxAlphaRecovery::GoodAlignmentLog2()), and this allows us to request 16 byte alignment in the case that we fall back to heap allocation.

This addresses most review comments except for using InitFromHeap - it doesn't seem to do enough to be worth splitting off and complicating things.

(In reply to Michael Wu [:mwu] from comment #34) > I didn't answer it because I wasn't sure what it is. But let me explain > what's going on here and maybe that'll work. When we're allocating for image > surfaces, gfxImageSurface usually requests 16 byte alignment > (gfxAlphaRecovery::GoodAlignmentLog2()), and this allows us to request 16 > byte alignment in the case that we fall back to heap allocation. Which you don't enforce on the malloc path.

(In reply to Mike Hommey [:glandium] from comment #36) > (In reply to Michael Wu [:mwu] from comment #34) > > I didn't answer it because I wasn't sure what it is. But let me explain > > what's going on here and maybe that'll work. When we're allocating for image > > surfaces, gfxImageSurface usually requests 16 byte alignment > > (gfxAlphaRecovery::GoodAlignmentLog2()), and this allows us to request 16 > > byte alignment in the case that we fall back to heap allocation. > > Which you don't enforce on the malloc path. We try our best. :) gfxImageSurface does the same. Alignment is more of a hint rather than requirement.

I think ideally, we would support alignment a lot more than we do now. Right now, support for alignment depends on whether the system allocator supports posix_memalign, which shouldn't matter when we have jemalloc. As a result, Windows and Android don't have guaranteed aligned image bufs. Might not matter as much for Windows since a lot of surfaces are allocated by the OS, but Fennec allocates its own surfaces.

Figured out my Windows build problem and added the Windows implementation back in.

Comment on attachment 8369291 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v5 Review of attachment 8369291 [details] [diff] [review]: ----------------------------------------------------------------- Haven't looked deep enough to toggle the flag, but i figured you could use the comments I already have about the patch. ::: memory/moz.build @@ +15,4 @@ > if CONFIG['MOZ_REPLACE_MALLOC_LINKAGE'] == 'dummy library': > DIRS += ['replace/dummy'] > > +TEST_TOOL_DIRS += ['tests'] That's going to be traversed only if jemalloc is enabled. ::: memory/mozalloc/VolatileBufferAshmem.cpp @@ +44,5 @@ > +heap_alloc: > +#if defined(HAVE_POSIX_MEMALIGN) > + moz_posix_memalign(&mBuf, aAlignment, aSize); > +#else > + mBuf = moz_malloc(aSize); You're still not enforcing alignment in this case. Also this would be better shared with the implementation in VolatileBufferFallback (btw, I'm not convinced there's much value separating everything in different files, especially with all the redundant parts). ::: memory/mozalloc/VolatileBufferWindows.cpp @@ +100,5 @@ > + > + void *addr = VirtualAllocEx(GetCurrentProcess(), > + mBuf, > + mSize, > + MEM_RESET_UNDO, I bet MEM_RESET_UNDO is not defined in the windows 7 SDK, and AFAIK, we still support building against it (as long as you disable metro)

Comment on attachment 8369291 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v5 Review of attachment 8369291 [details] [diff] [review]: ----------------------------------------------------------------- Also, I'd like another pair of eyes to look at it. Maybe njn?

Comment on attachment 8369291 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v5 Review of attachment 8369291 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/moz.build @@ +15,4 @@ > if CONFIG['MOZ_REPLACE_MALLOC_LINKAGE'] == 'dummy library': > DIRS += ['replace/dummy'] > > +TEST_TOOL_DIRS += ['tests'] I'll move the test to memory/mozalloc/tests ::: memory/mozalloc/VolatileBufferAshmem.cpp @@ +44,5 @@ > +heap_alloc: > +#if defined(HAVE_POSIX_MEMALIGN) > + moz_posix_memalign(&mBuf, aAlignment, aSize); > +#else > + mBuf = moz_malloc(aSize); The first user of this code doesn't care if we can't get the requested alignment though. We can either fail without the request alignment, or attempt to ensure alignment manually, but this is the simplest thing that works. I do want to improve this later though - we should be able to do an aligned allocation on all platforms. Combining into a single file would allow some sharing of code, but would also involve a pile of ifdefs.. I'll think about it.

Comment on attachment 8369291 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v5 Review of attachment 8369291 [details] [diff] [review]: ----------------------------------------------------------------- Having read the whole patch I almost understand how this is supposed to be used, but I'd like another look after you've added explanatory comments in VolatileBuffer.h. I agree with glandium that having a single file with #ifdefs within it would be better, because then it's much easier to see which are the OS-specific bits and which aren't. This is under memory/mozalloc/, but would MFBT be a better place for it? You've used 4-space indents throughout, but standard Mozilla style is 2-space. ::: memory/mozalloc/VolatileBuffer.h @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozalloc_volatile_h > +#define mozalloc_volatile_h This file has zero comments. Please add some -- entering this review I only had a rough idea of what ashmem is and how it works, and the lack of comments here gave me little to work with. A module-level "what this is all about, and when you should use it" comment would be good, as would individual comments on class methods. @@ +15,5 @@ > +class VolatileBufferPtr_base; > + > +class MOZALLOC_EXPORT VolatileBuffer : public RefCounted<VolatileBuffer> > +{ > +friend class VolatileBufferPtr_base; Nit: indentation. @@ +17,5 @@ > +class MOZALLOC_EXPORT VolatileBuffer : public RefCounted<VolatileBuffer> > +{ > +friend class VolatileBufferPtr_base; > +public: > + VolatileBuffer(size_t aSize, size_t aAlignment = sizeof(void *)); Nit: '*' to the left, here, and in numerous other places in the patch. @@ +63,5 @@ > + void *mMapping; > + > +private: > + RefPtr<VolatileBuffer> mVBuf; > + bool mPurged; At this point in the review I've read VolatileBuffer.h and VolatileBufferAshmem.cpp and I have only a vague idea what this purged stuff is all about. ::: memory/mozalloc/VolatileBufferAshmem.cpp @@ +29,5 @@ > + mFd = open("/" ASHMEM_NAME_DEF, O_RDWR); > + if (mFd < 0) > + goto heap_alloc; > + > + mSize = (aSize + (PAGE_SIZE - 1)) & PAGE_MASK; In this file you round up |mSize| in the constructor. In the OS X and Windows versions you don't, and then you have to round up in SizeOf(). Rounding up here is better, because then |mSize| is accurate. @@ +57,5 @@ > + return; > + } > + > + munmap(mBuf, mSize); > + close(mFd); Nit: when there are two fairly equal cases like this, I'd remove the early |return| and put the munmap/close into an |else| branch. @@ +61,5 @@ > + close(mFd); > +} > + > +bool > +VolatileBuffer::Initialized() const I'm more used to classes like this having a boolean Init() function which is called after the class is constructed, like so: Foo* foo = new Foo(); if (!foo || !foo->Init()) // handle error The virtue of the Init() function is that if you forget to call it, it's very likely things will break badly. Whereas if you forget to call Initialized(), things will work until it fails. @@ +101,5 @@ > + return mFd < 0; > +} > + > +size_t > +VolatileBuffer::SizeOf(MallocSizeOf aMallocSizeOf) const This should be called SizeOfExcludingThis(). Also, we need to distinguish between the heap and non-heap cases, because the two kinds of memory are reported separately. Perhaps change this to a void function with two outparams: one that's the size, and one that's set to either nsIMemoryReporter::KIND_HEAP or KIND_NONHEAP. Alternatively, you could have two functions: HeapSizeOfExcludingThis() and NonHeapSizeOfExcludingThis(), and one of them would return zero. (I guess you could use OnHeap(), but it would be easy to fail to do that, and always report the memory as heap or always non-heap.) @@ +106,5 @@ > +{ > + if (mFd >= 0 || !aMallocSizeOf) > + return mSize; > + > + return aMallocSizeOf(mBuf); You could do this in a single line with ?: if you liked. ::: memory/mozalloc/VolatileBufferFallback.cpp @@ +58,5 @@ > + > +size_t > +VolatileBuffer::SizeOf(MallocSizeOf aMallocSizeOf) const > +{ > + return aMallocSizeOf ? aMallocSizeOf(mBuf) : mSize; Don't bother with the null-check; no other functions of this kind do that. ::: memory/tests/TestVolatileBuffer.cpp @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "TestHarness.h" Not testing Windows in this file? @@ +34,5 @@ > + > + { > + VolatileBufferPtr<char> map(buf); > + if (map.WasPurged()) { > + fail("Failed to Lock VolatileBuffer after initialization"); Leaky abstraction alert: the error message doesn't make sense unless you know has WasPurged() is implemented. Seems like this could be made clearer.

Comment on attachment 8369291 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v5 Review of attachment 8369291 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/mozalloc/VolatileBufferAshmem.cpp @@ +61,5 @@ > + close(mFd); > +} > + > +bool > +VolatileBuffer::Initialized() const I thought about it for a bit and I agree. Things should fail sooner rather than later. ::: memory/tests/TestVolatileBuffer.cpp @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "TestHarness.h" Every platform is tested, but testing that we handle purged buffers correctly requires extra support. OSX and Ashmem provide ways to tell the OS to drop all volatile pages directly. Windows, as far as I can tell, doesn't have anything more sophisticated than allocating a bunch of memory to force pages out of cache. I checked google and https://wiki.mozilla.org/Firefox/Projects/Startup_Time_Improvements_Notes . Would be great if someone knows a better way. @@ +34,5 @@ > + > + { > + VolatileBufferPtr<char> map(buf); > + if (map.WasPurged()) { > + fail("Failed to Lock VolatileBuffer after initialization"); This actually made a lot more sense before I switched to using the VolatileBufferPtr API. I'll fix the message.

(In reply to Nicholas Nethercote [:njn] from comment #43) > I agree with glandium that having a single file with #ifdefs within it would > be better, because then it's much easier to see which are the OS-specific > bits and which aren't. > I disagree since there's not a whole lot of common code. The most common code is the mSize setting. > This is under memory/mozalloc/, but would MFBT be a better place for it? > I don't think so, though I'm not sure how mfbt is linked. It's been tricky to get this code to link right on all platforms. > You've used 4-space indents throughout, but standard Mozilla style is > 2-space. > The style in this directory is 4 space.

Comment on attachment 8369291 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v5 Review of attachment 8369291 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/mozalloc/VolatileBufferAshmem.cpp @@ +29,5 @@ > + mFd = open("/" ASHMEM_NAME_DEF, O_RDWR); > + if (mFd < 0) > + goto heap_alloc; > + > + mSize = (aSize + (PAGE_SIZE - 1)) & PAGE_MASK; BTW, in the new patch, I switched everything over to storing the requested size in mSize. Things are only rounded up to the closest page when we attempt to calculate the space used. I had originally thought the ashmem api required page size values, but it turns out that it rounds things up to the nearest page as needed. I prefer passing the actual size to the OS so we don't try to outsmart it.

> The style in this directory is 4 space. I know we typically follow style within an existing file, but following style within a directory is an idea that's new to me, and I don't think it's a good one. We're heading towards a world with fewer styles (see recent dev-plaform discussions). Please use two-space indents so this doesn't have to be re-indented in the future.

Comment on attachment 8371597 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v6 Review of attachment 8371597 [details] [diff] [review]: ----------------------------------------------------------------- I think the API could be improved. Let me write down how I think it's supposed to work. - You create a VolatileBuffer, |buf|. - You then create a VolatileBufferPtr, |ptr|, into |buf|. The comment in VolatileBuffer.h says "Buffers are not purgable after a VolatileBuffer is initialized" so supposedly this is safe to use immediately, though I don't see what actually enforces that in the code. - You create another VolatileBufferPtr, |ptr2|, into |buf|. Before using it, you should check if |ptr2.wasPurged()| is true, and if so, you shouldn't use |ptr2|. However, there's nothing to save you if you forget this check or ignore its result. (Or perhaps you'd want to use it to refill the buffer?) I find the whole locking/purging interaction really hard to keep straight in my head, and it wasn't until I wrote that down that I felt like I understood it, which isn't good. So the problems I have: - WasPurged() is a bad name. It confused me for a long time, possibly because it describes the _buffer_, not the _pointer_. Something like IsValid() might be better. - I don't like the asymmetry: the first VolatileBufferPtr doesn't need to be checked/inited, but any subsequent ones do. Would it be simpler to just always require the check/initialization? - I don't like that you can forget the check, or ignore it. VolatileBufferPtr should assert if you access the buffer through it unsafely. Or maybe have a Reset() function you can call that lets you explicitly override that? With these things fixed, I think it'll be a lot easier to understand. I have more nits below, though I haven't done a full re-review yet, because I figured the above was enough to go on with. It's still very much heading in the right direction, though :) ::: memory/mozalloc/VolatileBuffer.h @@ +22,5 @@ > + * VolatileBuffers may not always be volatile - if the allocation is too small, > + * or if the OS doesn't support the feature, or if the OS doesn't want to, > + * the buffer will be allocated on heap. > + * > + * VolatileBuffer allocations are not infallible. They are intended for uses Nit: s/not infallible/fallible/. @@ +24,5 @@ > + * the buffer will be allocated on heap. > + * > + * VolatileBuffer allocations are not infallible. They are intended for uses > + * where one may allocate large buffers for caching data. They can only be > + * initialized once. This last sentence is potentially confusing; "initialize" refers to calling Init(), but at first I thought you meant "assign values to". I'd change it to something like "Init() must be called exactly once before use." @@ +28,5 @@ > + * initialized once. > + * > + * After getting a reference to VolatileBuffer using VolatileBufferPtr, > + * WasPurged() can be used to check if the OS purged any pages in the buffer. > + * Buffers are not purgable after a VolatileBuffer is initialized. At least Nit: s/purgable/purgeable/ @@ +48,5 @@ > + ~VolatileBuffer(); > + > + bool Init(size_t aSize, size_t aAlignment = sizeof(void*)); > + > + bool OnHeap() const; Nit: IsOnHeap()? I like "is" and "has" prefixes on boolean functions. @@ +60,5 @@ > + void* mBuf; > + size_t mSize; > + Atomic<int> mCount; > +#ifdef ANDROID > + int mFd; Some comments about these fields would be good, particularly what values will be used to represent exceptional states, such as mFd<0 representing a heap buffer. @@ +62,5 @@ > + Atomic<int> mCount; > +#ifdef ANDROID > + int mFd; > +#endif > +#if defined(XP_WIN) || defined(XP_DARWIN) Use #elif here and below. ::: memory/mozalloc/VolatileBufferAshmem.cpp @@ +71,5 @@ > +} > + > +VolatileBuffer::~VolatileBuffer() > +{ > + if (mFd < 0) { Use OnHeap() here instead. @@ +89,5 @@ > + *aBuf = mBuf; > + if (++mCount > 1 || mFd < 0) > + return true; > + > + // Zero offset and zero length means we want to pin/unpin the entire thing Period at end of sentence. @@ +98,5 @@ > +void > +VolatileBuffer::Unlock() > +{ > + MOZ_ASSERT(mCount > 0, "VolatileBuffer unlocked too many times!"); > + if (--mCount || mFd < 0) Use OnHeap() here instead of |mFd < 0|. @@ +114,5 @@ > + > +size_t > +VolatileBuffer::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const > +{ > + if (mFd >= 0) Use !OnHeap() here. ::: memory/mozalloc/VolatileBufferOSX.cpp @@ +101,5 @@ > +size_t > +VolatileBuffer::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const > +{ > + if (mHeap) > + return aMallocSizeOf ? aMallocSizeOf(mBuf) : mSize; As I said before: you don't need to null-check aMallocSizeOf. ::: memory/mozalloc/VolatileBufferWindows.cpp @@ +33,5 @@ > +{ > +} > + > +static bool sCheckedVersion; > +static bool sUndoSupported; These two are only used inside Init(), so they should be moved inside that function. ::: memory/mozalloc/tests/TestVolatileBuffer.cpp @@ +40,5 @@ > + fail("Buffer should not be purged immediately after initialization"); > + return 1; > + } > + > + if (!map) { It's weird to see |!map| just after seeing |map.wasPurged()|. I think the use of |operator T*()| is a little too clever, and suggest just calling it get() (or something else, so long as it's explicit) instead. Also, |map| is a weird name for this variable. How about |ptr|? The others below should be changed too. @@ +48,5 @@ > + > + { > + VolatileBufferPtr<char> map2(buf); > + if (map2.WasPurged()) { > + fail("Failed to Lock VolatileBuffer again"); This error message is talking about locks after WasPurged() failed. Maybe "buffer shouldn't have been purged while |ptr| exists", or something. @@ +64,5 @@ > + > + { > + VolatileBufferPtr<char> map(buf); > + if (map.WasPurged()) { > + fail("Buffer was immediately purged after unlock"); Can't this case happen legitimately? @@ +76,5 @@ > + } > + > + // Test purging if we can > +#if defined(MOZ_WIDGET_GONK) > + // This also works on Android, but we need root Period at end of sentence. @@ +101,5 @@ > + > + { > + VolatileBufferPtr<char> map(buf); > + if (!map.WasPurged()) { > + fail("Lock succeeded despite purge"); Again, the comment talks about locks. @@ +114,5 @@ > + > + { > + VolatileBufferPtr<char> map(buf); > + if (map.WasPurged()) { > + fail("Lock failed after lock"); Ditto.

(In reply to Nicholas Nethercote [:njn] from comment #49) > Comment on attachment 8371597 [details] [diff] [review] > Implement VolatileBuffer for OSX, Windows and Ashmem, v6 > > Review of attachment 8371597 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think the API could be improved. Let me write down how I think it's > supposed to work. > > - You create a VolatileBuffer, |buf|. > > - You then create a VolatileBufferPtr, |ptr|, into |buf|. The comment in > VolatileBuffer.h says "Buffers are not purgable after a VolatileBuffer is > initialized" so supposedly this is safe to use immediately, though I don't > see what actually enforces that in the code. > This is an artifact of how volatile buffers are implemented in every operating system. > - You create another VolatileBufferPtr, |ptr2|, into |buf|. Before using it, > you should check if |ptr2.wasPurged()| is true, and if so, you shouldn't use > |ptr2|. However, there's nothing to save you if you forget this check or > ignore its result. (Or perhaps you'd want to use it to refill the buffer?) > Exactly - the buffer is always usable. WasPurged only tells you if one or more pages in the buffer was set to zero. So reading from it may not be a good idea, but you could certainly write to it. > I find the whole locking/purging interaction really hard to keep straight in > my head, and it wasn't until I wrote that down that I felt like I understood > it, which isn't good. > > So the problems I have: > > - WasPurged() is a bad name. It confused me for a long time, possibly > because it describes the _buffer_, not the _pointer_. Something like > IsValid() might be better. > Ok. Maybe WasBufferPurged()? IsValid() makes it sound like it's invalid to use the pointer if it's purged, which is false. > - I don't like the asymmetry: the first VolatileBufferPtr doesn't need to be > checked/inited, but any subsequent ones do. Would it be simpler to just > always require the check/initialization? > This exception actually simplifies the implementation and matches what users actually do. The first time one creates a VolatileBuffer, it is almost certain to be used immediately. Also, the way volatile memory is implemented on every platform means one has to explicitly unlock memory immediately after allocation to require this. > - I don't like that you can forget the check, or ignore it. > VolatileBufferPtr should assert if you access the buffer through it > unsafely. Or maybe have a Reset() function you can call that lets you > explicitly override that? > Whether something is unsafe or not depends entirely on what the buffer is being used for. In the case of decoded images, there's actually a new image surface type (gfxLockedImageSurface) which asserts if we end up accidentally using a purged buffer. I suspect that users will often wrap the VolatileBufferPtr in some other class that can also do the appropriate checks based on context. It's possible to tell VolatileBufferPtr whether we intend to use it for read only purposes or not and have some sanity check in there. I'll see if anything can be done - not sure if we can improve much here.

Comment on attachment 8371597 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v6 Review of attachment 8371597 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/mozalloc/tests/TestVolatileBuffer.cpp @@ +40,5 @@ > + fail("Buffer should not be purged immediately after initialization"); > + return 1; > + } > + > + if (!map) { I.. don't think operator T*() is being clever? It's just a smart pointer. I'll change the variable name. @@ +48,5 @@ > + > + { > + VolatileBufferPtr<char> map2(buf); > + if (map2.WasPurged()) { > + fail("Failed to Lock VolatileBuffer again"); This test is mostly meant to help people implementing VolatileBuffer, so I think talking about locking here and elsewhere makes sense since it points directly to a bug in the implementation of Lock(). Though, I can improve this particular message a bit still. @@ +64,5 @@ > + > + { > + VolatileBufferPtr<char> map(buf); > + if (map.WasPurged()) { > + fail("Buffer was immediately purged after unlock"); Technically yes. In practice, it won't unless some other program on the test system is trying to exhaust all memory on the system or the OS's volatile memory implementation is buggy. This a sanity check that helps catch cases where the VolatileBuffer implementation or the OS is flushing buffers immediately after they're marked volatile.

(In reply to Michael Wu [:mwu] from comment #50) > > So the problems I have: > > > > - WasPurged() is a bad name. It confused me for a long time, possibly > > because it describes the _buffer_, not the _pointer_. Something like > > IsValid() might be better. > > > > Ok. Maybe WasBufferPurged()? IsValid() makes it sound like it's invalid to > use the pointer if it's purged, which is false. > BTW, I prefer to use WasBufferPurged() over IsBufferPurged() because the buffer is no longer purged once the lock is taken. If multiple users try to use a VolatileBufferPtr to lock a VolatileBuffer that was purged, only the first user gets to see that the buffer was purged.

Comment on attachment 8371597 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v6 Review of attachment 8371597 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/mozalloc/VolatileBuffer.h @@ +48,5 @@ > + ~VolatileBuffer(); > + > + bool Init(size_t aSize, size_t aAlignment = sizeof(void*)); > + > + bool OnHeap() const; Hmm, there's a lot of cases where we start a boolean function with On, though. Seems common enough. @@ +60,5 @@ > + void* mBuf; > + size_t mSize; > + Atomic<int> mCount; > +#ifdef ANDROID > + int mFd; I switched everything to OnHeap() where it makes sense. I think reading the OnHeap() implementation should be enough. @@ +62,5 @@ > + Atomic<int> mCount; > +#ifdef ANDROID > + int mFd; > +#endif > +#if defined(XP_WIN) || defined(XP_DARWIN) #elif works here, but not below.

BTW, I think what would make sense is an API that lets you mark memory as immutable. If the memory gets purged, the VolatileBuffer would immediately free everything and VolatileBufferPtr would return null. That also avoids the issue of accidentally clearing the purged status on a buffer and then using the buffer afterwards as if it's still valid. This is a good match for what a lot of caches actually need, though I don't think it would necessary serve every purpose. I would implement this API on or alongside what we have now as a followup.

Comment on attachment 8372651 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v7 Review of attachment 8372651 [details] [diff] [review]: ----------------------------------------------------------------- You're still using 4-space indents! Please fix this. ::: memory/mozalloc/VolatileBuffer.h @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozalloc_volatile_h > +#define mozalloc_volatile_h mozalloc_VolatileBuffer_h? @@ +29,5 @@ > + * > + * After getting a reference to VolatileBuffer using VolatileBufferPtr, > + * WasPurged() can be used to check if the OS purged any pages in the buffer. > + * Buffers are not purgeable after a VolatileBuffer is initialized. At least > + * one VolatileBufferPtr must created before the buffer can be purged. The implication is obscure. How about "The OS cannot purge the buffer until at least one VolatileBufferPtr has been created. This means you don't need to check if a purge has occurred unless you create a second or subsequent VolatileBufferPtr into the buffer." Or something like that. A brief example of a typical sequence of calls into the API would be very helpful. @@ +49,5 @@ > + > + bool Init(size_t aSize, size_t aAlignment = sizeof(void*)); > + > + bool OnHeap() const; > + size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const; As I said in my first set of comments, I'd like to have HeapSizeOfExcludingThis() and NonHeapSizeOfExcludingThis(). For any particular buffer, one would always be zero. And then OnHeap() could probably be private. @@ +65,5 @@ > +#elif defined(XP_WIN) || defined(XP_DARWIN) > + bool mHeap; > +#endif > +#ifdef XP_WIN > + bool mFirstLock; I'd probably have an XP_WIN section and a XP_DARWIN section, and just duplicate mHeap in each of them. It violates DRY but it clearly overall. ::: memory/mozalloc/VolatileBufferAshmem.cpp @@ +39,5 @@ > +{ > + MOZ_ASSERT(!mSize && !mBuf, "Init called twice"); > + > + mSize = aSize; > + if (aSize < MIN_VOLATILE_ALLOC_SIZE) Standard moz style is to brace even one-line blocks. There are lots of other violations of this rule elsewhere; I trust you to find and fix them. @@ +72,5 @@ > + > +VolatileBuffer::~VolatileBuffer() > +{ > + if (OnHeap()) { > + moz_free(mBuf); Why not use free() instead of moz_free()? @@ +73,5 @@ > +VolatileBuffer::~VolatileBuffer() > +{ > + if (OnHeap()) { > + moz_free(mBuf); > + return; I like how in the OS X and Windows version of this function you don't have the early return. ::: memory/mozalloc/VolatileBufferFallback.cpp @@ +14,5 @@ > + > +VolatileBuffer::VolatileBuffer() > + : mBuf(nullptr) > + , mSize(0) > + , mCount(0) Would mRefCount be a better name? The meaning would be more immediately obvious... |mCount| could be anything. ::: memory/mozalloc/VolatileBufferOSX.cpp @@ +49,5 @@ > + > +VolatileBuffer::~VolatileBuffer() > +{ > + if (!mBuf) > + return; You don't need to check this, since moz_free(nullptr) does the right thing. @@ +103,5 @@ > +{ > + if (mHeap) > + return aMallocSizeOf(mBuf); > + > + unsigned int pagemask = getpagesize() - 1; On Linux, PAGE_MASK == ~(PAGE_SIZE - 1). This code inverts that. Perhaps this would be better: unsigned int pagesize = getpagesize(); unsigned int pagemask = ~(pagesize - 1); return (mSize + pagesize - 1) & pagemask; ::: memory/mozalloc/VolatileBufferWindows.cpp @@ +83,5 @@ > + > +VolatileBuffer::~VolatileBuffer() > +{ > + if (!mBuf) > + return; Again, I don't think you need this test. ::: memory/mozalloc/tests/TestVolatileBuffer.cpp @@ +29,5 @@ > + RefPtr<VolatileBuffer> buf = new VolatileBuffer(); > + if (!buf || !buf->Init(16384)) { > + fail("Failed to initialize VolatileBuffer"); > + return 1; > + } Can you test the |size < 8192| case? Nothing complicated, just to make sure it doesn't crash on basic use.

Review comments addressed except for indentation and bracing style. I'll handle those in a later update - still need to run this by try and make sure everything still works.

BTW, I just realized this still isn't thread safe. The atomic counter isn't enough to ensure the OS specific pin/unpin operations are done in the right order. We don't need thread safety at the moment, though it may be required later for other users.

(In reply to Michael Wu [:mwu] from comment #58) > BTW, I just realized this still isn't thread safe. Please add a big warning in the header, then.

Comment on attachment 8374521 [details] [diff] [review] Implement VolatileBuffer for OSX, Windows and Ashmem, v8 Review of attachment 8374521 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +1822,5 @@ > if test "$HAVE_64BIT_OS"; then > MOZ_MEMORY=1 > fi > + # Bug 967556 > + AC_DEFINE(HAVE_POSIX_MEMALIGN) This shouldn't sneak in this patch. ::: memory/mozalloc/VolatileBuffer.h @@ +29,5 @@ > + * > + * After getting a reference to VolatileBuffer using VolatileBufferPtr, > + * WasPurged() can be used to check if the OS purged any pages in the buffer. > + * The OS cannot purge a buffer immediately after a VolatileBuffer is > + * initialized. At least one VolatileBufferPtr must created before the must be @@ +34,5 @@ > + * buffer can be purged, so the first use of VolatileBufferPtr does not need > + * to check WasPurged(). > + * > + * When a buffer is purged, some or all of the buffer is zeroed out. This API > + * cannot tell you which parts of the buffer were lost. s/you// @@ +39,5 @@ > + */ > + > +namespace mozilla { > + > +class VolatileBufferPtr_base; this forward declaration should not be necessary, because the friend class definition below should be enough. ::: memory/mozalloc/VolatileBufferAshmem.cpp @@ +19,5 @@ > +extern "C" int __wrap_posix_memalign(void** memptr, size_t alignment, size_t size); > +#else > +extern "C" int posix_memalign(void** memptr, size_t alignment, size_t size); > +#endif > +#endif You shouldn't need this if you #include "mozmemory.h". But I won't block you on making this work. @@ +64,5 @@ > +#else > + posix_memalign(&mBuf, aAlignment, aSize); > +#endif > +#else > + mBuf = memalign(aAlignment, aSize); memalign and posix_memalign have a slightly different requirement on alignment, please assert that aAlignment matches the most restrictive one.

Review comments addressed. Thanks for reviews!

memory/mozalloc/VolatileBufferFallback.cpp:34:2: error: "No memalign implementation found" Poor OpenBSD :( Fwiw we have posix_memalign() in stdlib.h, so i dont know why it's not detected.. will have to dig.

mwu, you should blog this. We should encourage more parts of the codebase to use this.

I landed a trivial cross compilation fixup: https://hg.mozilla.org/integration/mozilla-inbound/rev/997127a13fd5

Can you please prep a patch for 1.3T?

hi Michael, do we know how much memory saving we can get from this one? thanks

(In reply to Joe Cheng [:jcheng] from comment #70) > hi Michael, do we know how much memory saving we can get from this one? > thanks This patch doesn't directly save memory. This combined with things like bug 962670 allow us to behave better under low memory conditions by automatically throwing away buffers. These are buffers that should be thrown away rather than compressed and stored in zram.

Hi Ying Xu, heard that you will be doing uplifts to 1.3T branch. After you completed the uplift, can you please set status-b2g-v1.3T to fixed? please let us know if you have problems with it. thanks

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/a32c32b24a34