The Clue In The Code Comments

April 17, 2016

TLDR: with this single line fix, we saved 30-40mb of runtime memory use and improved overall performance.

UPDATE 3rd June 2016: the fixes below were added to the code base in UE4.12

Sometimes, the most innocent of code comments can make you immediately aware that something, somewhere, has gone awry… like when you find that there are millions of objects being created inside an area of code with the comment “this should not happen too often“. In the case we found, these objects would consume 30-40mb of memory before eventually being garbage collected – so the problem was causing a memory as well as performance drain.

Here’s the problematic function, GetReusableMID() from \Engine\Source\Runtime\Renderer\Private\ScenePrivate.h:-

virtual UMaterialInstanceDynamic* GetReusableMID(class UMaterialInterface* InSource) override
{    
  check(IsInGameThread());
  check(InSource);

  // 0 or MID (MaterialInstanceDynamic) pointer
  auto InputAsMID = Cast<UMaterialInstanceDynamic>(InSource);

  // fixup MID parents as this is not allowed, take the next MIC or Material.
  UMaterialInterface* ParentOfTheNewMID = InputAsMID ? InputAsMID->Parent : InSource;

  // this is not allowed and would cause an error later in the code
  check(!ParentOfTheNewMID->IsA(UMaterialInstanceDynamic::StaticClass()));

  UMaterialInstanceDynamic* NewMID = 0;

  if(MIDUsedCount < (uint32)MIDPool.Num())
  {
    NewMID = MIDPool[MIDUsedCount];

    if(NewMID->Parent != ParentOfTheNewMID)
    {
      // create a new one
      // garbage collector will remove the old one
      // this should not happen too often
      NewMID = UMaterialInstanceDynamic::Create(ParentOfTheNewMID, 0);
      MIDPool[MIDUsedCount] = NewMID;
    }

    // reusing an existing object means we need to clear out the Vector and Scalar parameters
    NewMID->ClearParameterValues();
  }
  else
  {
    NewMID = UMaterialInstanceDynamic::Create(ParentOfTheNewMID, 0);
    check(NewMID);

    MIDPool.Add(NewMID);
  }

  if(InputAsMID)
  {
    // parent is an MID so we need to copy the MID Vector and Scalar parameters over
    NewMID->CopyInterpParameters(InputAsMID);
  }
  check(NewMID->GetRenderProxy(false));
  return NewMID;
}

The section of interest is bang in the middle:-

if(NewMID->Parent != ParentOfTheNewMID)
{
  // create a new one
  // garbage collector will remove the old one
  // this should not happen too often
  NewMID = UMaterialInstanceDynamic::Create(ParentOfTheNewMID, 0);
  MIDPool[MIDUsedCount] = NewMID;
}

As a simple first test, I added some code to test how often this code was executed compared to how often it wasn’t. The result was amazing: if the function was executed 100 million times, the code above (which, remember, shouldn’t happen “too often”), was executed all but one of those times. So, yeah, something was broken – or the comment was a lie…

The reason is pretty obvious – MIDUsedCount never changes, it’s always zero. Incrementing the counter in the section of code above isn’t actually the right solution to the problem, either, as it’s really not that simple – note the “MIDPool.Add()” further down the function. The correct placement of the increment is at the very end:-

  MIDUsedCount++;
  check(NewMID->GetRenderProxy(false));
  return NewMID;
}

Oddly enough, I’ve run a few static code analysis programs over the UE4 codebase and they’ve never shown the problem above, even though it’s been there for over 7 months (at the time of writing).

Looking into the history of the file, we found that the problem actually crept into the codebase from an optimization that had been committed to the codebase – doh!

CPU optimized PostProcessMaterial blending, GetReusableMID() makes a strong commitment on the returned data which requires less fixup, by knowing the param hierarchy we don’t need to copy all paramaters, only the one from the last MID parent. The blueprint code for material copying can be optimized but it will be less general or needs a different implementation – that can be done later. [CL 2674528 by Martin Mittring in Main branch]

We can only assume that the optimization was a good one and hid the fact that an earlier optimization had been broken.

Credit(s): Robert Troughton (Coconut Lizard)
Status: Fixed in 4.12