Fix OverBudget tests from never terminating.

[bbernhar]

Creating a resource which made the current usage equal to budget would result in IsOverBudget never terminating. This fix replaces IsOverBudget with GetBudgetLeft so created resources never exceed the budget.

[bjjones]

LGTM.

Note that I did find the the reason tests were looping forever on my machine: In D3D12ResidencyManagerTests.cpp L91, the return statement should be an OR, not an AND. If I'm remembering correctly - I think we may be seeing (valid) behavior differences between hardware. On my Nvidia dGPU, evicted local memory does not get counted towards the non-local memory budget. I think this is different from how Intel integrated GPUs do it and I think this might be why you used an AND here.

[bbernhar]

Question -

evicted local memory does not get counted towards the non-local memory budget.

We only evict using the same segment the budget uses, though.

GPGMM/src/gpgmm/d3d12/HeapD3D12.cpp

Line 48 in 7c8eec9

ReturnIfFailed(pResidencyManager->Evict(descriptor.SizeInBytes, memorySegmentGroup));

This check

GPGMM/src/tests/end2end/D3D12ResidencyManagerTests.cpp

Line 91 in 7c8eec9

return local->Budget <= local->CurrentUsage && nonLocal->Budget <= nonLocal->CurrentUsage;

The AND was because I don't care which segment goes over budget, so long all of them are under.

Does that sound correct to you?

[bjjones]

...nevermind. What I first said was wrong - I misread the code and jumped to a conclusion. Whats happening on my machine is that local->Budget is exactly equal to local->CurrentUsage - so we're stiil technically not exceeding the budget, and returning false.

I'm getting this in a loop:

GPGMM Info (tid:8740): GPU page-out(5): Number of allocations: 1 (1048576 bytes).
GPGMM Debug (tid:8740): SlabMemoryAllocator(0): Slab size exceeds available memory: 1048576 vs 0 bytes.
GPGMM Info (tid:8740): SlabCacheAllocator(3): Failed to allocate memory for request

The issue may be that we're calling Evict, and it doesn't immediately evict (as expected). Then in SlabMemoryAllocator.cpp L104 we bail out of allocating more because still availableForAllocation < slabSize.

[bbernhar]

Where do you see an error?

SlabMemoryAllocator.cpp bails out only when there are no slabs left to re-use from the cache.

GPGMM Info (tid:8740): SlabCacheAllocator(3): Failed to allocate memory for request // Not fatal!

GPGMM/src/gpgmm/common/SlabMemoryAllocator.cpp

Line 151 in 7c8eec9

uint64_t SlabMemoryAllocator::FindNextFreeSlabOfSize(uint64_t slabSize) const {

When it falls-back, you get a heap exactly sized to the resource, increasing CurrentUsage, which usually succeeds a bit longer.

GPGMM/src/gpgmm/common/StandaloneMemoryAllocator.h

Line 24 in 7c8eec9

// StandaloneMemoryAllocator sub-allocates memory with exactly one block.

In D3D12ResidencyTests.cpp, Evict always happens on creation:

GPGMM/src/tests/end2end/D3D12ResidencyManagerTests.cpp

Line 46 in 7c8eec9

desc.Flags |= gpgmm::d3d12::ALLOCATOR_FLAG_ALWAYS_IN_BUDGET;

I suspect either the segment wasn't being updated or we are allocating from the wrong segment. See if GetPreferredMemorySegmentGroup() returns a different segment type here:

GPGMM/src/gpgmm/d3d12/ResidencyManagerD3D12.cpp

Line 731 in 7c8eec9

DXGI_MEMORY_SEGMENT_GROUP ResidencyManager::GetPreferredMemorySegmentGroup(

[bjjones]

GetPreferredMemorySegmentGroup appears to return the correct thing (local).

I think what I may be seeing is that we're doing this:

while(currentUsage <= budget) // and currentUsage is exactly the budget here
{
    see that we can't allocate another resource and stay under budget, so evict one resource 
    allocate one resource //which is the exact same size as evicted resource
}

We'll never exceed the budget when doing this - usage will always be equal to the budget when we check (and I think this is correct behavior).

Is using IsOverBudget() the correct method for this test? The purpose of the residency manager is to ensure we don't go over budget. It seems like we should instead be testing that the segment budget isn't exceeded after we've allocated enough resources to exceed the budget.

[bbernhar]

The test only fails for notifications, correct?

If so, I suspect this is due to a race condition. If the notification occurred sometime after a CreateHeap [going over budget] but before Evict is called for the next CreateHeap then this loop cannot terminate. You can test this theory by setting a breakpoint here:

GPGMM/src/gpgmm/d3d12/ResidencyManagerD3D12.cpp

Line 489 in 40a6ba2

std::vector<ID3D12Pageable*> objectsToEvict;

If true, we can make the test become deterministic by changing the termination condition to use the same check in GPGMM_SKIP_TEST_IF_NOT_CREATED_IN_BUDGET.

[bjjones]

I've been referring to D3D12ResidencyManagerTests.OverBudget, which appears to be an infinite loop on my machine.

D3D12ResidencyManagerTests.OverBudgetUsingBudgetNotifications seems to loop forever too, but I am getting the additional message that is not present in the other test: GPGMM Debug (tid:20736): ResourceAllocator(0): Current usage exceeded budget (23275970560 vs 23274641408 bytes). so you could be right about the race condition for this test.

[bbernhar]

@bjjones Thanks, I see the issue now. Looks like I got lucky with my heap sizes. PTAL again.

[bjjones]

Note that D3D12ResidencyManagerTests.OverBudgetUsingBudgetNotifications fails for me due to allocations not being resident. I think it may just be that we're going very close to the budget and normal budget fluctuations are causing things to be evicted before we expect them to. I do see the test passing when I increased the allocation size to 50MB and made the change on the condition I suggested in this review. edit: the test is still flaky after making these changes.

[bjjones]

I was able to make D3D12ResidencyManagerTests.OverBudgetUsingBudgetNotifications pass reliably by changing
while (resourceAllocator->GetInfo().UsedMemoryUsage + GPGMM_MB_TO_BYTES(1) < memoryUnderBudget) { to
while (resourceAllocator->GetInfo().UsedMemoryUsage + bufferDesc.Width <= GetBudgetLeft(residencyManager.Get(), bufferMemorySegment))

[bbernhar]

Yup, good catch! I believe the underlying issue was the created heap sizes would not exactly fit the budget left and spilled a bit which caused the test to fail.

Thanks again.

[bbernhar]

@bjjones Could you PTAL again? Thanks!

[bjjones]

Still seeing flaky failures on the notification test. Everything else LGTM.

[bjjones]

LGTM

[bbernhar]

@bjjones Merging, thanks!