Implement SkAshmemDiscardableMemory

Implement SkAshmemDiscardableMemory

-   Implement ashmem-backed SkDiscardableMemory subclass:
    This class in only accesible via the SkDiscardableMemory::Create()
    function, which replaces the mock implementation in
    SkDiscardableMemory_none.cpp

-   Added SkDiscardableMemory_ashmem.cpp to the Android port of Skia
    Removed SkDiscardableMemory_none.cpp from the Android port.

-   Added DiscardableMemoryTest.
    Still needs work.

-   SkDiscardablePixelRef Bugfix:
    onLockPixels() now calls SkDELETE on the SkDiscardableMemory pointer
    when it fails to unlock.

-   Improved documentation inside ashmem.h

BUG=
R=scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12608

[scroggo, 2013-11-22 14:34:14]

https://codereview.chromium.org/83563002/diff/2/gyp/ports.gyp
File gyp/ports.gyp (right):

https://codereview.chromium.org/83563002/diff/2/gyp/ports.gyp#newcode157
gyp/ports.gyp:157: '../src/ports/SkDiscardableMemory_none.cpp',
nit: not in alphabetical order.

https://codereview.chromium.org/83563002/diff/2/gyp/ports.gyp#newcode160
gyp/ports.gyp:160: '../src/ports/SkDiscardableMemory_ashmem.cpp',
nit: not in alphabetical order.

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
File src/ports/SkDiscardableMemory_ashmem.cpp (right):

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:28: bool fLocked;
nit: names should line up.

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:59: return fLocked = false;
I find this confusing to look at. Is it okay to say

fLocked = false;
return fLocked; // or false

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:108: if (-1 == ashmem_pin_region(fd, 0,
0)) {
Is this step necessary? It seems that SkImageRef_ashmem skips this step.

https://codereview.chromium.org/83563002/diff/2/tests/DiscardableMemoryTest.cpp
File tests/DiscardableMemoryTest.cpp (right):

https://codereview.chromium.org/83563002/diff/2/tests/DiscardableMemoryTest.c...
tests/DiscardableMemoryTest.cpp:17: if (NULL == dm)
Our style guide suggests using braces like so:

if (NULL == dm) {
    return;
}

https://codereview.chromium.org/83563002/diff/2/tests/DiscardableMemoryTest.c...
tests/DiscardableMemoryTest.cpp:25: if (!success)
braces

https://codereview.chromium.org/83563002/diff/2/tests/DiscardableMemoryTest.c...
tests/DiscardableMemoryTest.cpp:26: return;
dm is leaked here. Maybe wrap it in an SkAutoTDelete?

[hal.canary, 2013-11-22 15:17:54]

https://codereview.chromium.org/83563002/diff/2/gyp/ports.gyp
File gyp/ports.gyp (right):

https://codereview.chromium.org/83563002/diff/2/gyp/ports.gyp#newcode157
gyp/ports.gyp:157: '../src/ports/SkDiscardableMemory_none.cpp',
On 2013/11/22 14:34:14, scroggo wrote:
> nit: not in alphabetical order.

Done.

https://codereview.chromium.org/83563002/diff/2/gyp/ports.gyp#newcode160
gyp/ports.gyp:160: '../src/ports/SkDiscardableMemory_ashmem.cpp',
On 2013/11/22 14:34:14, scroggo wrote:
> nit: not in alphabetical order.

Done.

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
File src/ports/SkDiscardableMemory_ashmem.cpp (right):

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:28: bool fLocked;
On 2013/11/22 14:34:14, scroggo wrote:
> nit: names should line up.

Done.

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:59: return fLocked = false;
On 2013/11/22 14:34:14, scroggo wrote:
> I find this confusing to look at. Is it okay to say
> 
> fLocked = false;
> return fLocked; // or false

Done.

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:108: if (-1 == ashmem_pin_region(fd, 0,
0)) {
On 2013/11/22 14:34:14, scroggo wrote:
> Is this step necessary? It seems that SkImageRef_ashmem skips this step.

SkImageRef_ashmem does not begin in a locked state.  the SkDiscardableMemory
contract is different.

https://codereview.chromium.org/83563002/diff/2/tests/DiscardableMemoryTest.cpp
File tests/DiscardableMemoryTest.cpp (right):

https://codereview.chromium.org/83563002/diff/2/tests/DiscardableMemoryTest.c...
tests/DiscardableMemoryTest.cpp:17: if (NULL == dm)
On 2013/11/22 14:34:14, scroggo wrote:
> Our style guide suggests using braces like so:
> 
> if (NULL == dm) {
>     return;
> }

Done.

https://codereview.chromium.org/83563002/diff/2/tests/DiscardableMemoryTest.c...
tests/DiscardableMemoryTest.cpp:25: if (!success)
On 2013/11/22 14:34:14, scroggo wrote:
> braces

Done.

https://codereview.chromium.org/83563002/diff/2/tests/DiscardableMemoryTest.c...
tests/DiscardableMemoryTest.cpp:26: return;
On 2013/11/22 14:34:14, scroggo wrote:
> dm is leaked here. Maybe wrap it in an SkAutoTDelete?

Done.

[scroggo, 2013-11-22 15:37:05]

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
File src/ports/SkDiscardableMemory_ashmem.cpp (right):

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:108: if (-1 == ashmem_pin_region(fd, 0,
0)) {
On 2013/11/22 15:17:54, Hal Canary wrote:
> On 2013/11/22 14:34:14, scroggo wrote:
> > Is this step necessary? It seems that SkImageRef_ashmem skips this step.
> 
> SkImageRef_ashmem does not begin in a locked state.  the SkDiscardableMemory
> contract is different.

SkImageRef_ashmem does not begin in a locked state, but its analogous function
to this one, allocPixelRef(), is only called inside onLockPixels, so it is being
set to a locked state.

Does your test pass if you remove this call to ashmem_pin_region? (I'm not sure
that's a good indicator of what is correct; your test only covers a pretty
simple case. That said, I've yet to figure out how to write a good test for
Ashmem. I wonder if Android has any interesting tests we could mimic...)

https://codereview.chromium.org/83563002/diff/130001/tests/DiscardableMemoryT...
File tests/DiscardableMemoryTest.cpp (right):

https://codereview.chromium.org/83563002/diff/130001/tests/DiscardableMemoryT...
tests/DiscardableMemoryTest.cpp:14: static const char testString[] = "HELLO,
WORLD!";
nit: Do either of these need to be static?

[hal.canary, 2013-11-22 16:37:26]

https://codereview.chromium.org/83563002/diff/130001/tests/DiscardableMemoryT...
File tests/DiscardableMemoryTest.cpp (right):

https://codereview.chromium.org/83563002/diff/130001/tests/DiscardableMemoryT...
tests/DiscardableMemoryTest.cpp:14: static const char testString[] = "HELLO,
WORLD!";
On 2013/11/22 15:37:05, scroggo wrote:
> nit: Do either of these need to be static?

Done.

[hal.canary, 2013-12-05 22:17:30]

PTAL

[reed1, 2013-12-06 15:09:46]

I think you can close the FD as soon as you create the mmap

[scroggo, 2013-12-06 16:07:50]

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
File src/ports/SkDiscardableMemory_ashmem.cpp (right):

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:108: if (-1 == ashmem_pin_region(fd, 0,
0)) {
On 2013/11/22 15:37:05, scroggo wrote:
> On 2013/11/22 15:17:54, Hal Canary wrote:
> > On 2013/11/22 14:34:14, scroggo wrote:
> > > Is this step necessary? It seems that SkImageRef_ashmem skips this step.
> > 
> > SkImageRef_ashmem does not begin in a locked state.  the SkDiscardableMemory
> > contract is different.
> 
> SkImageRef_ashmem does not begin in a locked state, but its analogous function
> to this one, allocPixelRef(), is only called inside onLockPixels, so it is
being
> set to a locked state.
> 
> Does your test pass if you remove this call to ashmem_pin_region? (I'm not
sure
> that's a good indicator of what is correct; your test only covers a pretty
> simple case. That said, I've yet to figure out how to write a good test for
> Ashmem. I wonder if Android has any interesting tests we could mimic...)

Did you see my last comment? I don't think this recreates the same behavior that
SkImageRefAshmem had.

[hal.canary, 2013-12-06 20:14:41]

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
File src/ports/SkDiscardableMemory_ashmem.cpp (right):

https://codereview.chromium.org/83563002/diff/2/src/ports/SkDiscardableMemory...
src/ports/SkDiscardableMemory_ashmem.cpp:108: if (-1 == ashmem_pin_region(fd, 0,
0)) {
On 2013/12/06 16:07:51, scroggo wrote:
> Did you see my last comment? I don't think this recreates the same behavior
that
> SkImageRefAshmem had.

I had missed your last comment.  Thanks for reminding me.  Yes, you are correct:
that first call to pin the memory was unnecessary, as I verified by checking the
documentation.  I am removing that call and adding a comment to the constructor
to justify why I can initialize isLocked to true.

[scroggo, 2013-12-09 20:39:36]

lgtm

[hal.canary, 2013-12-10 18:33:14]

Committed patchset #8 manually as r12608 (presubmit successful).