Create free list heap for suballocation

Create free list heap for suballocation

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

Committed: https://skia.googlesource.com/skia/+/6b6ffc4ab6ecee9f46425a5467ef0fbebbb8c9cd

[jvanverth1, 2016-06-01 20:54:10]

Description was changed from

==========
Create free list heap for suballocation

BUG=skia:
==========

to

==========
Create free list heap for suballocation

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

[jvanverth1, 2016-06-09 20:16:06]

jvanverth@google.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com

[jvanverth1, 2016-06-09 20:16:07]

I may do some tweaking tomorrow, but I think this is pretty much done. PTAL.

[bsalomon, 2016-06-10 15:14:53]

Do you think having some unit tests for the block list stuff would be helpful?
E.g. run some alloc / free passes and validate the the heap is self consistent.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkGpu.h
File src/gpu/vk/GrVkGpu.h (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkGpu.h#n...
src/gpu/vk/GrVkGpu.h:131: kVertexBuffer_Heap,
Why separate index and vertex heaps? What's the optimal vs small optimal
distinction?

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory.cpp
File src/gpu/vk/GrVkMemory.cpp (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:33: const GrVkGpu::Heap kBufferToHeap[]{
Should we have some static asserts somewhere that these are consistent?

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:95: static uint64_t gTotalImageMemory = 0;
Is this for debugging?

[jvanverth1, 2016-06-10 15:41:21]

On 2016/06/10 15:14:53, bsalomon wrote:
> Do you think having some unit tests for the block list stuff would be helpful?
> E.g. run some alloc / free passes and validate the the heap is self
consistent.
> 

Yes, I meant to do that and forgot.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkGpu.h
File src/gpu/vk/GrVkGpu.h (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkGpu.h#n...
src/gpu/vk/GrVkGpu.h:131: kVertexBuffer_Heap,
On 2016/06/10 15:14:53, bsalomon wrote:
> Why separate index and vertex heaps? What's the optimal vs small optimal
> distinction?

I'll add some comments. In Vulkan, the index of the heap depends on the buffer
type. So in theory, a driver's implementation could have different heaps for
index and vertex buffers. The small vs. non-small distinction is to reduce
fragmentation in the texture heap.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory.cpp
File src/gpu/vk/GrVkMemory.cpp (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:33: const GrVkGpu::Heap kBufferToHeap[]{
On 2016/06/10 15:14:53, bsalomon wrote:
> Should we have some static asserts somewhere that these are consistent?

Ok, I'll add those.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:95: static uint64_t gTotalImageMemory = 0;
On 2016/06/10 15:14:53, bsalomon wrote:
> Is this for debugging?

Yes, I'd rather leave it in for now.

[jvanverth1, 2016-06-10 18:31:55]

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkGpu.h
File src/gpu/vk/GrVkGpu.h (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkGpu.h#n...
src/gpu/vk/GrVkGpu.h:131: kVertexBuffer_Heap,
On 2016/06/10 15:41:20, jvanverth1 wrote:
> On 2016/06/10 15:14:53, bsalomon wrote:
> > Why separate index and vertex heaps? What's the optimal vs small optimal
> > distinction?
> 
> I'll add some comments. In Vulkan, the index of the heap depends on the buffer
> type. So in theory, a driver's implementation could have different heaps for
> index and vertex buffers. The small vs. non-small distinction is to reduce
> fragmentation in the texture heap. 

As a side note, I'd like to incorporate the small-large distinction within the
heap (I think malloc does something similar), but would rather do that as a
separate change.

[jvanverth1, 2016-06-10 18:39:28]

jvanverth@google.com changed reviewers:
+ brianosman@google.com

[jvanverth1, 2016-06-10 18:39:29]

Adding brianosman@ for comments.

[egdaniel, 2016-06-13 16:44:44]

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory.cpp
File src/gpu/vk/GrVkMemory.cpp (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:242: &allocInfo,
align these will allocate memory?

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:287: if (blockAlignedSize >= alignedSize &&
blockAlignedSize < bestFitSize) {
from our conversation, I think a better criteria for bestFitSize is to minimize
the total size of unused memory (both before and after this alloaction), and to
break ties maximize the difference in size between the left over left and right
memory in the block.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory.h
File src/gpu/vk/GrVkMemory.h (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.h:95: bool alloc(VkDeviceSize size, VkDeviceSize
alignment, uint32_t memoryTypeIndex, GrVkAlloc* alloc) {
100 chars

[jvanverth1, 2016-06-13 19:59:23]

Comments addressed.

Given the deadline, I'd prefer to defer the unit tests.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkGpu.h
File src/gpu/vk/GrVkGpu.h (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkGpu.h#n...
src/gpu/vk/GrVkGpu.h:131: kVertexBuffer_Heap,
On 2016/06/10 15:41:20, jvanverth1 wrote:
> On 2016/06/10 15:14:53, bsalomon wrote:
> > Why separate index and vertex heaps? What's the optimal vs small optimal
> > distinction?
> 
> I'll add some comments. In Vulkan, the index of the heap depends on the buffer
> type. So in theory, a driver's implementation could have different heaps for
> index and vertex buffers. The small vs. non-small distinction is to reduce
> fragmentation in the texture heap. 

Done.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory.cpp
File src/gpu/vk/GrVkMemory.cpp (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:33: const GrVkGpu::Heap kBufferToHeap[]{
On 2016/06/10 15:41:20, jvanverth1 wrote:
> On 2016/06/10 15:14:53, bsalomon wrote:
> > Should we have some static asserts somewhere that these are consistent?
> 
> Ok, I'll add those.

Done.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:95: static uint64_t gTotalImageMemory = 0;
On 2016/06/10 15:41:20, jvanverth1 wrote:
> On 2016/06/10 15:14:53, bsalomon wrote:
> > Is this for debugging?
> 
> Yes, I'd rather leave it in for now.

Added comment.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:242: &allocInfo,
On 2016/06/13 16:44:44, egdaniel wrote:
> align these will allocate memory?

Done.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.cpp:287: if (blockAlignedSize >= alignedSize &&
blockAlignedSize < bestFitSize) {
On 2016/06/13 16:44:44, egdaniel wrote:
> from our conversation, I think a better criteria for bestFitSize is to
minimize
> the total size of unused memory (both before and after this alloaction), and
to
> break ties maximize the difference in size between the left over left and
right
> memory in the block.

I've modified it to have a fixed alignment for a given subheap. We have separate
subheaps for each resource type, so the alignment should be the same for all
allocations. Since all allocations have the same alignment, we don't need the
extra padding in front.

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory.h
File src/gpu/vk/GrVkMemory.h (right):

https://codereview.chromium.org/2029763002/diff/120001/src/gpu/vk/GrVkMemory....
src/gpu/vk/GrVkMemory.h:95: bool alloc(VkDeviceSize size, VkDeviceSize
alignment, uint32_t memoryTypeIndex, GrVkAlloc* alloc) {
On 2016/06/13 16:44:44, egdaniel wrote:
> 100 chars

Done.

[bsalomon, 2016-06-13 20:04:54]

lgtm

[jvanverth1, 2016-06-13 20:49:37]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2016-06-13 21:02:12]

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

[commit-bot: I haz the power, 2016-06-13 21:28:11]

Description was changed from

==========
Create free list heap for suballocation

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

to

==========
Create free list heap for suballocation

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

Committed:
https://skia.googlesource.com/skia/+/6b6ffc4ab6ecee9f46425a5467ef0fbebbb8c9cd
==========

[commit-bot: I haz the power, 2016-06-13 21:28:12]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/6b6ffc4ab6ecee9f46425a5467ef0fbebbb8c9cd

[commit-bot: I haz the power, 2016-06-13 21:28:15]

CQ bit was unchecked