Last Comment Bug 748598 - Store cache in purgeable memory on mac, ashmem on android
: Store cache in purgeable memory on mac, ashmem on android
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All Mac OS X
: -- normal with 1 vote (vote)
: mozilla30
Assigned To: Michael Wu [:mwu]
:
:
Mentors:
: 686804 878367 (view as bug list)
Depends on: 967556 972693 973192
Blocks: 256meg 962670
  Show dependency treegraph
 
Reported: 2012-04-24 16:13 PDT by (dormant account)
Modified: 2014-03-11 02:37 PDT (History)
32 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
1.3T+
fixed


Attachments
Tentative API (4.25 KB, text/plain)
2012-08-03 03:13 PDT, Mike Hommey [:glandium]
no flags Details
PoC - ashmem backed decoded images (23.45 KB, patch)
2014-01-14 01:29 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Test program for purging ashmem (1.71 KB, patch)
2014-01-14 01:31 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
PoC - ashmem backed decoded images, v2 (23.47 KB, patch)
2014-01-14 03:06 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
WIP: VolatileBuffer backed decoded images, v3 (40.60 KB, patch)
2014-01-15 08:27 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
WIP: VolatileBuffer backed decoded images, v4 (29.59 KB, patch)
2014-01-21 20:09 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Test program for purging purgable memory on OSX (139 bytes, text/x-c++src)
2014-01-21 20:11 PST, Michael Wu [:mwu]
no flags Details
Implement VolatileBuffer for OSX and Ashmem (11.14 KB, patch)
2014-01-22 09:50 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Implement VolatileBuffer for OSX and Ashmem, v2 (11.16 KB, patch)
2014-01-22 10:45 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Roll up patch with VolatileBuffer backed images (30.56 KB, patch)
2014-01-22 10:49 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Implement VolatileBuffer for OSX and Ashmem, v3 (11.82 KB, patch)
2014-01-25 12:35 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Implement VolatileBuffer for OSX and Ashmem, v4 (12.94 KB, patch)
2014-01-28 14:40 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Implement VolatileBuffer for OSX, Windows and Ashmem, v5 (16.74 KB, patch)
2014-02-02 19:12 PST, Michael Wu [:mwu]
n.nethercote: feedback+
Details | Diff | Splinter Review
Implement VolatileBuffer for OSX, Windows and Ashmem, v6 (19.77 KB, patch)
2014-02-06 09:27 PST, Michael Wu [:mwu]
n.nethercote: feedback+
Details | Diff | Splinter Review
Implement VolatileBuffer for OSX, Windows and Ashmem, v7 (19.88 KB, patch)
2014-02-07 15:55 PST, Michael Wu [:mwu]
n.nethercote: review+
Details | Diff | Splinter Review
Implement VolatileBuffer for OSX, Windows and Ashmem, v8 (21.65 KB, patch)
2014-02-11 16:31 PST, Michael Wu [:mwu]
mh+mozilla: review+
Details | Diff | Splinter Review
Implement VolatileBuffer for OSX, Windows and Ashmem, v9 (20.61 KB, patch)
2014-02-13 14:26 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Implement VolatileBuffer for OSX, Windows and Ashmem, v9 (1.3T) (20.65 KB, patch)
2014-03-07 16:07 PST, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review

Description (dormant account) 2012-04-24 16:13:47 PDT
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.
Comment 1 Justin Lebar (not reading bugmail) 2012-04-24 16:29:12 PDT
> 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.
Comment 2 (dormant account) 2012-04-24 16:58:04 PDT
(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
Comment 3 Justin Lebar (not reading bugmail) 2012-04-24 17:36:40 PDT
Oh, I see; you set it to non-volatile and then you do:

    m_state = state & VM_PURGABLE_EMPTY ? Purged : NonVolatile;

Got it.
Comment 4 (dormant account) 2012-05-02 09:44:18 PDT
*** Bug 686804 has been marked as a duplicate of this bug. ***
Comment 5 Justin Lebar (not reading bugmail) 2012-05-23 10:21:22 PDT
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.
Comment 6 Mike Hommey [:glandium] 2012-05-23 10:24:14 PDT
(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.
Comment 7 Mike Hommey [:glandium] 2012-05-23 10:25:07 PDT
For reference, for a Windows implementation:
http://code.google.com/p/chromium/issues/detail?id=21926#c10
Comment 8 (dormant account) 2012-05-23 10:25:28 PDT
Network memory cache is also a candidate.
Comment 9 Justin Lebar (not reading bugmail) 2012-05-23 10:27:54 PDT
I've never seen the network memory cache grow above 5mb.  Have you?
Comment 10 (dormant account) 2012-05-23 10:30:32 PDT
(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
Comment 11 (dormant account) 2012-08-02 16:53:55 PDT
Mike, can you upload your wip patch here?
Comment 12 Mike Hommey [:glandium] 2012-08-03 03:13:44 PDT
Created attachment 648655 [details]
Tentative API

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 13 Jesse Ruderman 2012-11-28 21:23:11 PST
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.)
Comment 14 Jesse Ruderman 2012-11-28 21:23:53 PST
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.)
Comment 15 (dormant account) 2013-03-07 14:54:54 PST
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.
Comment 16 Andreas Gal :gal 2013-04-20 01:26:57 PDT
We really want this for B2G.
Comment 17 Mike Hommey [:glandium] 2013-05-31 23:32:01 PDT
*** Bug 878367 has been marked as a duplicate of this bug. ***
Comment 18 Honza Bambas (:mayhemer) 2013-06-01 06:39:58 PDT
Michal, something we may need to think of soon at the new cache implementation.
Comment 19 Michael Wu [:mwu] 2014-01-14 01:29:16 PST
Created attachment 8359668 [details] [diff] [review]
PoC - ashmem backed decoded images

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.
Comment 20 Michael Wu [:mwu] 2014-01-14 01:31:21 PST
Created attachment 8359670 [details] [diff] [review]
Test program for purging ashmem

This is the test program I used. The patch applies to gonk-misc in a standard B2G build environment.
Comment 21 Michael Wu [:mwu] 2014-01-14 03:06:42 PST
Created attachment 8359719 [details] [diff] [review]
PoC - ashmem backed decoded images, v2

This fixes a scenario where images weren't getting decoded again.
Comment 22 Michael Wu [:mwu] 2014-01-15 08:27:08 PST
Created attachment 8360461 [details] [diff] [review]
WIP: VolatileBuffer backed decoded images, v3

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.
Comment 23 Michael Wu [:mwu] 2014-01-15 09:07:53 PST
Note that this patch depends on having the patch from bug 958008 applied, if your tree doesn't have it already.
Comment 24 Michael Wu [:mwu] 2014-01-21 20:09:32 PST
Created attachment 8363436 [details] [diff] [review]
WIP: VolatileBuffer backed decoded images, v4

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.
Comment 25 Michael Wu [:mwu] 2014-01-21 20:11:53 PST
Created attachment 8363437 [details]
Test program for purging purgable memory on OSX

I compiled this with clang++ without any particular options. Seems to just work.
Comment 26 Michael Wu [:mwu] 2014-01-22 09:50:14 PST
Created attachment 8363768 [details] [diff] [review]
Implement VolatileBuffer for OSX and Ashmem

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.
Comment 27 Michael Wu [:mwu] 2014-01-22 10:45:47 PST
Created attachment 8363817 [details] [diff] [review]
Implement VolatileBuffer for OSX and Ashmem, v2

One liner build fix for OSX.
Comment 28 Michael Wu [:mwu] 2014-01-22 10:49:01 PST
Created attachment 8363823 [details] [diff] [review]
Roll up patch with VolatileBuffer backed images

In case anyone wants to try all these patches without finding all the bugs and applying them individually.
Comment 29 Michael Wu [:mwu] 2014-01-23 14:03:08 PST
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.
Comment 30 Michael Wu [:mwu] 2014-01-25 12:35:53 PST
Created attachment 8365589 [details] [diff] [review]
Implement VolatileBuffer for OSX and Ashmem, v3

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 31 Mike Hommey [:glandium] 2014-01-27 18:35:40 PST
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 32 Michael Wu [:mwu] 2014-01-28 08:54:27 PST
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.
Comment 33 Mike Hommey [:glandium] 2014-01-28 13:38:54 PST
(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.
Comment 34 Michael Wu [:mwu] 2014-01-28 13:46:51 PST
(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.
Comment 35 Michael Wu [:mwu] 2014-01-28 14:40:12 PST
Created attachment 8366926 [details] [diff] [review]
Implement VolatileBuffer for OSX and Ashmem, v4

This addresses most review comments except for using InitFromHeap - it doesn't seem to do enough to be worth splitting off and complicating things.
Comment 36 Mike Hommey [:glandium] 2014-01-28 15:42:24 PST
(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.
Comment 37 Michael Wu [:mwu] 2014-01-28 15:48:38 PST
(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.
Comment 38 Michael Wu [:mwu] 2014-01-31 11:52:57 PST
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.
Comment 39 Michael Wu [:mwu] 2014-02-02 19:12:50 PST
Created attachment 8369291 [details] [diff] [review]
Implement VolatileBuffer for OSX, Windows and Ashmem, v5

Figured out my Windows build problem and added the Windows implementation back in.
Comment 40 Mike Hommey [:glandium] 2014-02-03 17:05:03 PST
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 41 Mike Hommey [:glandium] 2014-02-03 17:05:53 PST
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 42 Michael Wu [:mwu] 2014-02-03 18:18:36 PST
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 43 Nicholas Nethercote [:njn] 2014-02-04 21:45:09 PST
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 44 Michael Wu [:mwu] 2014-02-05 07:23:38 PST
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.
Comment 45 Michael Wu [:mwu] 2014-02-06 09:27:01 PST
Created attachment 8371597 [details] [diff] [review]
Implement VolatileBuffer for OSX, Windows and Ashmem, v6
Comment 46 Michael Wu [:mwu] 2014-02-06 09:33:09 PST
(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 47 Michael Wu [:mwu] 2014-02-06 12:13:26 PST
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.
Comment 48 Nicholas Nethercote [:njn] 2014-02-06 19:30:43 PST
> 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 49 Nicholas Nethercote [:njn] 2014-02-06 19:48:01 PST
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.
Comment 50 Michael Wu [:mwu] 2014-02-07 14:15:51 PST
(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 51 Michael Wu [:mwu] 2014-02-07 14:34:27 PST
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.
Comment 52 Michael Wu [:mwu] 2014-02-07 14:49:04 PST
(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 53 Michael Wu [:mwu] 2014-02-07 15:51:27 PST
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.
Comment 54 Michael Wu [:mwu] 2014-02-07 15:55:50 PST
Created attachment 8372651 [details] [diff] [review]
Implement VolatileBuffer for OSX, Windows and Ashmem, v7
Comment 55 Michael Wu [:mwu] 2014-02-07 16:08:24 PST
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 56 Nicholas Nethercote [:njn] 2014-02-10 01:22:33 PST
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.
Comment 57 Michael Wu [:mwu] 2014-02-11 16:31:12 PST
Created attachment 8374521 [details] [diff] [review]
Implement VolatileBuffer for OSX, Windows and Ashmem, v8

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.
Comment 58 Michael Wu [:mwu] 2014-02-12 09:18:49 PST
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.
Comment 59 Mike Hommey [:glandium] 2014-02-12 22:41:34 PST
(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 60 Mike Hommey [:glandium] 2014-02-12 22:55:25 PST
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.
Comment 61 Michael Wu [:mwu] 2014-02-13 14:26:09 PST
Created attachment 8375824 [details] [diff] [review]
Implement VolatileBuffer for OSX, Windows and Ashmem, v9

Review comments addressed. Thanks for reviews!
Comment 63 Ryan VanderMeulen [:RyanVM] 2014-02-14 06:02:45 PST
https://hg.mozilla.org/mozilla-central/rev/ae11ce69621f
Comment 64 Landry Breuil (:gaston) 2014-02-14 15:29:40 PST
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.
Comment 65 (dormant account) 2014-02-16 13:37:04 PST
mwu, you should blog this. We should encourage more parts of the codebase to use this.
Comment 66 Jacek Caban 2014-02-20 08:20:59 PST
I landed a trivial cross compilation fixup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/997127a13fd5
Comment 67 Ryan VanderMeulen [:RyanVM] 2014-02-20 12:17:59 PST
https://hg.mozilla.org/mozilla-central/rev/997127a13fd5
Comment 68 Andreas Gal :gal 2014-03-06 11:06:58 PST
Can you please prep a patch for 1.3T?
Comment 69 Michael Wu [:mwu] 2014-03-07 16:07:53 PST
Created attachment 8387952 [details] [diff] [review]
Implement VolatileBuffer for OSX, Windows and Ashmem, v9 (1.3T)
Comment 70 Joe Cheng [:jcheng] 2014-03-09 23:51:50 PDT
hi Michael, do we know how much memory saving we can get from this one? thanks
Comment 71 Michael Wu [:mwu] 2014-03-10 14:28:04 PDT
(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.
Comment 72 Joe Cheng [:jcheng] 2014-03-10 23:46:20 PDT
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

Note You need to log in before you can comment on or make changes to this bug.