RenderTargetPool's FindFreeElement leading to poor memory reuse

Development Platform & Builds
, ,
1

Hi all,

While investigating performance in our game, I found that there was a lot of calls to FRenderTargetPool::CreateRenderTarget.

Tracing further, what seems to be the pattern is that we have a view extension which uses a pooled render target with a dimension that depends on the current rendering resolution.

This, coupled with planar reflection causes that pooled render target to be re-queried via FindFreeElement several time in a frame with different resolution : once for each planar reflection, and once more for the final render.

That behavior makes sense, however, it was unexpected that this would lead to several calls to CreateRenderTarget every frame since it’s supposed to be pooled, and the code led me to believe that render targets allocated in such a way would stick around for up to 2 frames when not used, and at least a frame since things seems to be released only via FRenderTargetPool::TickPoolElements().

However, I don’t understand this part of FRenderTargetPool::FindFreeElement:

bool FRenderTargetPool::FindFreeElement(FRHICommandListBase& RHICmdList, const FRHITextureCreateInfo& Desc, TRefCountPtr<IPooledRenderTarget>& Out, const TCHAR* Name)
{
	...

	// if we can keep the current one, do that
	if (Out)
	{
		Current = (FPooledRenderTarget*)Out.GetReference();

		if (Translate(Out->GetDesc()) == Desc) <--- Ends up being false since the resolution changes
		{

		}
		else
		{
			// release old reference, it might free a RT we can use
			Out = 0;
			bFreeCurrent = Current->IsFree(); <--- This causes the RT to be marked as needed to be freed
		}
	}

	...

	if (bFreeCurrent)
	{
		...
		FreeElementAtIndex(Index); <--- This will effectively free the RT
	}

	Out = FindFreeElement(RHICmdList, Desc, Name); <--- No subsequent FindFreeElement will ever match that RT, because FreeElementAtIndex also clears the hash in PooledRenderTargetHashes which would be used to match that RT again in another call
	return false;
}

It appears that commenting out “bFreeCurrent = Current->IsFree();” does produce the expected outcome where a RT sticks around for a while, and in the planar reflections cases, the RT do end up being reused in the next frame.

Am I missing something or is it an oversight?

Thank you!

[Attachment Removed]

2

Steps to Reproduce
In a ticking subsystem

Declaration:

TRefCountPtr<IPooledRenderTarget> TestThing_RenderThread;

In the Tick function

ENQUEUE_RENDER_COMMAND(DoSomeAlloc)([this](FRHICommandList& RHICmdList)
{
    FRDGTextureDesc Desc
    (
       FRDGTextureDesc::Create2D
       (
          {1024, 1024},
          EPixelFormat::PF_R8G8B8A8,
          FClearValueBinding::Transparent,
          TexCreate_ShaderResource | TexCreate_RenderTargetable
       )
    );

    for (int i = 0; i < 4; i++)
    {
       GRenderTargetPool.FindFreeElement(Desc, TestThing_RenderThread, TEXT("Test"));
       Desc.Extent.X /= 2;
       Desc.Extent.Y /= 2;
    }
});

[Attachment Removed]

3

Hi,

Thanks for reaching out. I investigated this code and believe this immediate freeing logic is to prevent memory bloat by discarding render targets when the descriptors do not match. The render target pool assumes the caller is done with the old RT permanently and frees it immediately to recover memory as quickly as possible. While this logic works in the general case, it may not be optimal in the case of planar reflections.

I’m not sure how the view extension calculates the render target resolution, but if it’s done in a dynamic way, then a potential optimization might be to limit the possible render target resolutions to a small set of fixed resolutions (e.g. 1024x1024, 512x512, 256x256) to increase the chance that a call to FindFreeElement() will match an existing free render target (even if the one in the view extension was discarded a moment before). This technique is often used in rendering features that sample from the screen at different scales, to preventing new allocations for every minor change.

EDIT: I discussed this with a colleague of mine and another potential approach is to keep the render target’s resolution intact (ensuring reuse instead of invalidation) and draw only to the portion of a render target that’s needed.

I hope this helps, but if you have further questions or comments I can see if I can pass it on to a subject matter expert at Epic. Please let me know.

Thanks,

Sam

[Attachment Removed]

4

Hi,

>> Currently my workaround is to include a flag that effectively skips the release, maybe that could be a flag in the Description instead to be a more generic way to handle the problem?

Do you mean as part of ETextureCreateFlags to FRHITextureDesc? That might be an option, another option could be to overload FindFreeElement with an extra flag that can be used as a condition to wrap the bFreeCurrent = Current->IsFree(); statement in an if-block. That can be a feature request I can pass on to Epic if you think that would be valuable.

Thanks,

Sam

[Attachment Removed]

5

Hi guys,

I apologize, I just got around to reviewing this request. I do not think the free/reallocation of the render target pool is the best solution, but I went ahead and filed a feature request with the development team in charge of that part of the code, so that they could consider some alternate options. They will likely review this request in the new year, and I will try to get you some updates once I hear more. Since you already have a workaround of sorts, it sounds like we are in a " if " for now. If you have any further questions or concerns, please don’t hesitate to contact us.

[Attachment Removed]

6

> I’m not sure how the view extension calculates the render target resolution

In our case, it’s just the size of the scene color texture passed in

> draw only to the portion of a render target that’s needed.

This makes things complicated since things are drawn with the MeshBatch route

Currently my workaround is to include a flag that effectively skips the release, maybe that could be a flag in the Description instead to be a more generic way to handle the problem?

[Attachment Removed]

7

Yes, I think it’d be a useful little feature to have something of an opt out for that early release path.

Thank you!

[Attachment Removed]

8

Thanks, I have passed your request on to Epic, so someone who is familiar with this part of the engine can assess it.

Thanks,

Sam

[Attachment Removed]