Make GrMemoryPool play nice with bucketing allocators.

Make GrMemoryPool play nice with bucketing allocators.

Some memory allocators have very coarse size buckets, so for example on
Android (jemalloc) an attempt to allocate 32 KiB + 1 byte will end up
allocating 40 KiB, wasting 8 KiB.

GrMemoryPool ctor takes two arguments that specify prealloc / block sizes,
and then inflates them to accommodate some bookkeeping structures. Since
most places create GrMemoryPools with pow2 numbers (which have buckets in
most allocators) the inflation causes allocator to select next size bucket,
wasting memory.

This CL makes GrMemoryPool to stop inflating sizes it was created with, and
allocate specified amounts exactly. Part of allocated memory is then used for
bookkeeping structures. Additionally, GrObjectMemoryPool template is provided,
which takes prealloc / block object counts (instead of sizes) and guarantees
that specified number of objects will fit in prealloc / block spaces.

BUG=651872
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2525773002

Committed: https://skia.googlesource.com/skia/+/e4cd00699167cefde9abedbd49ede64f82d552c7

[DmitrySkiba, 2016-11-22 22:09:51]

Description was changed from

==========
Make GrMemoryPool play nice with bucketing allocators.

Some memory allocators have very coarse size buckets, so for example on
Android (jemalloc) an attempt to allocate 32 KiB + 1 byte will end up
allocating 40 KiB, wasting 8 KiB.

GrMemoryPool ctor takes two arguments that specify prealloc / block sizes,
and then inflates them to accommodate some bookkeeping structures. Since
most places create GrMemoryPools with pow2 numbers (which have buckets in
most allocators) the inflation causes allocator to select next size bucket,
wasting memory.

This CL makes GrMemoryPool to stop inflating sizes it was created with, and
allocate specified amounts exactly. Part of allocated memory is then used for
bookkeeping structures. Additionally, GrObjectMemoryPool template is provided,
which takes prealloc / block object counts (instead of sizes) and guarantees
that specified number of objects will fit in prealloc / block spaces.

BUG=651872
==========

to

==========
Make GrMemoryPool play nice with bucketing allocators.

Some memory allocators have very coarse size buckets, so for example on
Android (jemalloc) an attempt to allocate 32 KiB + 1 byte will end up
allocating 40 KiB, wasting 8 KiB.

GrMemoryPool ctor takes two arguments that specify prealloc / block sizes,
and then inflates them to accommodate some bookkeeping structures. Since
most places create GrMemoryPools with pow2 numbers (which have buckets in
most allocators) the inflation causes allocator to select next size bucket,
wasting memory.

This CL makes GrMemoryPool to stop inflating sizes it was created with, and
allocate specified amounts exactly. Part of allocated memory is then used for
bookkeeping structures. Additionally, GrObjectMemoryPool template is provided,
which takes prealloc / block object counts (instead of sizes) and guarantees
that specified number of objects will fit in prealloc / block spaces.

BUG=651872
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2525773002
==========

[DmitrySkiba, 2016-11-22 22:10:49]

The CQ bit was checked by dskiba@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 22:10:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DmitrySkiba, 2016-11-22 22:11:22]

dskiba@chromium.org changed reviewers:
+ bsalomon@google.com

[commit-bot: I haz the power, 2016-11-22 22:44:44]

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

[commit-bot: I haz the power, 2016-11-22 22:44:45]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-11-28 16:03:31]

Thanks for looking at this and also for writing unit tests! Small stuff inline.

https://codereview.chromium.org/2525773002/diff/1/src/gpu/GrMemoryPool.h
File src/gpu/GrMemoryPool.h (right):

https://codereview.chromium.org/2525773002/diff/1/src/gpu/GrMemoryPool.h#newc...
src/gpu/GrMemoryPool.h:117: friend class GrObjectMemoryPool;
Could this friendship go away if we moved some more stuff from private to
protected?

https://codereview.chromium.org/2525773002/diff/1/src/gpu/GrMemoryPool.h#newc...
src/gpu/GrMemoryPool.h:169: constexpr static size_t countToSize(size_t count) {
nit: we capitalize the first char of a class static function.

[DmitrySkiba, 2016-11-29 01:08:02]

https://codereview.chromium.org/2525773002/diff/1/src/gpu/GrMemoryPool.h
File src/gpu/GrMemoryPool.h (right):

https://codereview.chromium.org/2525773002/diff/1/src/gpu/GrMemoryPool.h#newc...
src/gpu/GrMemoryPool.h:117: friend class GrObjectMemoryPool;
On 2016/11/28 16:03:31, bsalomon wrote:
> Could this friendship go away if we moved some more stuff from private to
> protected?

Done.

https://codereview.chromium.org/2525773002/diff/1/src/gpu/GrMemoryPool.h#newc...
src/gpu/GrMemoryPool.h:169: constexpr static size_t countToSize(size_t count) {
On 2016/11/28 16:03:31, bsalomon wrote:
> nit: we capitalize the first char of a class static function.

Done.

[bsalomon, 2016-11-29 14:18:37]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-11-29 14:18:38]

lgtm

[commit-bot: I haz the power, 2016-11-29 14:18:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 14:50:36]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480429117875730,
"parent_rev": "c51c18fd783737391e26541cbb5a72af2396ef7b", "commit_rev":
"e4cd00699167cefde9abedbd49ede64f82d552c7"}

[commit-bot: I haz the power, 2016-11-29 14:50:38]

Description was changed from

==========
Make GrMemoryPool play nice with bucketing allocators.

Some memory allocators have very coarse size buckets, so for example on
Android (jemalloc) an attempt to allocate 32 KiB + 1 byte will end up
allocating 40 KiB, wasting 8 KiB.

GrMemoryPool ctor takes two arguments that specify prealloc / block sizes,
and then inflates them to accommodate some bookkeeping structures. Since
most places create GrMemoryPools with pow2 numbers (which have buckets in
most allocators) the inflation causes allocator to select next size bucket,
wasting memory.

This CL makes GrMemoryPool to stop inflating sizes it was created with, and
allocate specified amounts exactly. Part of allocated memory is then used for
bookkeeping structures. Additionally, GrObjectMemoryPool template is provided,
which takes prealloc / block object counts (instead of sizes) and guarantees
that specified number of objects will fit in prealloc / block spaces.

BUG=651872
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2525773002
==========

to

==========
Make GrMemoryPool play nice with bucketing allocators.

Some memory allocators have very coarse size buckets, so for example on
Android (jemalloc) an attempt to allocate 32 KiB + 1 byte will end up
allocating 40 KiB, wasting 8 KiB.

GrMemoryPool ctor takes two arguments that specify prealloc / block sizes,
and then inflates them to accommodate some bookkeeping structures. Since
most places create GrMemoryPools with pow2 numbers (which have buckets in
most allocators) the inflation causes allocator to select next size bucket,
wasting memory.

This CL makes GrMemoryPool to stop inflating sizes it was created with, and
allocate specified amounts exactly. Part of allocated memory is then used for
bookkeeping structures. Additionally, GrObjectMemoryPool template is provided,
which takes prealloc / block object counts (instead of sizes) and guarantees
that specified number of objects will fit in prealloc / block spaces.

BUG=651872
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2525773002

Committed:
https://skia.googlesource.com/skia/+/e4cd00699167cefde9abedbd49ede64f82d552c7
==========

[commit-bot: I haz the power, 2016-11-29 14:50:38]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/e4cd00699167cefde9abedbd49ede64f82d552c7

[reed1, 2016-11-29 17:51:47]

reed@google.com changed reviewers:
+ herb@google.com, mtklein@google.com, reed@google.com

[reed1, 2016-11-29 17:51:48]

Can/should we look for ways to generalize this idea (avoiding wasted space in
chunky allocations) across skia? We certainly do other chunky allocs in other
places (resource cache, font cache, preallocs in collection-classes, etc.)

[herb_g, 2016-11-29 18:31:14]

SkFixedAlloc has 4 byte overhead, and squeezes additional space by taking
alignment into account. Maybe you can use some of the ideas.

https://codereview.chromium.org/2525773002/diff/20001/src/gpu/GrMemoryPool.h
File src/gpu/GrMemoryPool.h (right):

https://codereview.chromium.org/2525773002/diff/20001/src/gpu/GrMemoryPool.h#...
src/gpu/GrMemoryPool.h:27: *   1. they are are 8-byte aligned
Generally , 8-byte alignment is not enough. You should consult std::max_align_t.
For example on the mac, 16-byte alignment is required for a general allocator.
You could consult alignof() for the incoming type to make the right choice.

https://codereview.chromium.org/2525773002/diff/20001/src/gpu/GrMemoryPool.h#...
src/gpu/GrMemoryPool.h:162: T* allocate() { return
static_cast<T*>(GrMemoryPool::allocate(sizeof(T))); }
I think this is a very dangerous API. It is not RAII; therefore forces the
client to manage the destruction and type of the object. Memory should not be
returned with out being initialized, and ownership established.

Look at SkFixedAlloc one approach for doing this.

[DmitrySkiba, 2016-11-29 19:17:17]

On 2016/11/29 18:31:14, herb_g wrote:
> SkFixedAlloc has 4 byte overhead, and squeezes additional space by taking
> alignment into account. Maybe you can use some of the ideas.
> 
> https://codereview.chromium.org/2525773002/diff/20001/src/gpu/GrMemoryPool.h
> File src/gpu/GrMemoryPool.h (right):
> 
>
https://codereview.chromium.org/2525773002/diff/20001/src/gpu/GrMemoryPool.h#...
> src/gpu/GrMemoryPool.h:27: *   1. they are are 8-byte aligned
> Generally , 8-byte alignment is not enough. You should consult
std::max_align_t.
> For example on the mac, 16-byte alignment is required for a general allocator.
> You could consult alignof() for the incoming type to make the right choice.

I think those are two different things: alignof() applies to all allocators that
want to allocate C++ objects, while 16-byte alignment requirement applies only
to allocators that want to replace standard macOS facilities
(malloc_default_zone(), CF allocators). 

But I agree that in some cases default alignment of 8 (which BTW was already
there, my CL just reflected it in the comment) might not be enough. It's easy to
check / change for alignment in GrObjectMemoryPool, but not in GrMemoryPool,
since it allocates just raw bytes.

>
https://codereview.chromium.org/2525773002/diff/20001/src/gpu/GrMemoryPool.h#...
> src/gpu/GrMemoryPool.h:162: T* allocate() { return
> static_cast<T*>(GrMemoryPool::allocate(sizeof(T))); }
> I think this is a very dangerous API. It is not RAII; therefore forces the
> client to manage the destruction and type of the object. Memory should not be
> returned with out being initialized, and ownership established.
> 
> Look at SkFixedAlloc one approach for doing this.

Created https://bugs.chromium.org/p/skia/issues/detail?id=6010 for further
discussion.