Refactoring Old Code, Part 1: Memory Leaks

Introduction

I decided to start a blog to start documenting my progress in my career as a game programmer. I am likely to post about refactors, experiements and projects I undertake in this section and make them publically avaliable in case anyone is interested.

Where better to start a blog section on documenting progress than digging up some code you wrote over two years ago and ripping it apart! I first started programming using DirectX in 2015, where the majority of this code was written. The project was to create a DirectX engine that utilises simple to moderate 3D graphics techniques, as well as AI and physics. The full project can be seen here.

A warning to the more experienced programmers, the code I am about to reveal is HORRIBLY hacked together and would likely even cause the strong willed to wince. The objective of this exercise is mainly to discover how far I have progressed since first using DirectX 2 years ago, while also giving me a lesson in refactoring evil rookie code. So, let's delve into the abyss!

Problem Identification

Immediately, upon loading the application, it was apparent that were was a huge issue with memory being leaked at runtime [Figure 1]. The diagnostic session lasted for 1 minute and 10 seconds and reached 399MB of processed memory. Can’t fault past me for creating a fun project for myself 2 years down the line...


Figure 1 - Memory Leaks!


Not to mention that most of the code for this application seems to be situated within the main "Application" class, consisting of 1530 lines. I can't bear to look at how many layers of nested if statements exist in this frankensteined code. With only a quick glace at the project, it is clear what problems need addressing in this first refactor. So this first blog will be dedicated to fixing the memory leaks.

Plugging the Memory Leaks

After overcoming the guilt and shame of ever having written this code, it was time to work on righting that wrong and removing the offending code from existence. First, we need to locate the source of the memory leak, which can be done by using Visual Studio's built in Performance Profiler.


Figure 2 - Allocations happening in Application::Update over the course of 6 seconds


A lot of allocations seem to be happening in Application::Update(), more than would be expected for a fairly small project [Figure 2].

Memory Leak: Billboard

So, let's look at what Billboard::Update() is doing!

HRESULT Billboard::Update(float delta, ID3D11Device * pd3dDevice, XMFLOAT3 eyePos) { HRESULT hr; // convert data into usable format XMFLOAT3 worldPos = XMFLOAT3(GetTransform()->GetWorld()._41, GetTransform()->GetWorld()._42, GetTransform()->GetWorld()._43); // Calculate the normal vector which will make the billboard face the camera XMFLOAT3 planeNormal; // = worldPos - camEyePos; if (_xAxisLock) planeNormal.x = 0.0f; else planeNormal.x = (worldPos.x * 2) - eyePos.x; if (_yAxisLock) planeNormal.y = 0.0f; else planeNormal.y = (worldPos.y * 2) - eyePos.y; if (_zAxisLock) planeNormal.z = 0.0f; else planeNormal.z = (worldPos.z * 2) - eyePos.z; // Translate normal float to vector and normalize XMVECTOR vectorNormal = XMLoadFloat3(&planeNormal); vectorNormal = XMVector3Normalize(vectorNormal); // Translate up float to vector XMFLOAT3 floatUp = XMFLOAT3(0.0f, 1.0f, 0.0f); XMVECTOR vectorUp = XMLoadFloat3(&floatUp); XMVECTOR vectorRight = XMVector3Normalize(XMVector3Cross(vectorNormal, vectorUp)); vectorRight = vectorRight * (GetAppearance()->GetWidth() / 2); floatUp = XMFLOAT3(0.0f, (GetAppearance()->GetHeight() / 2), 0.0f); vectorUp = XMLoadFloat3(&floatUp); // Build Vertex buffer XMFLOAT3 vert[4]; XMFLOAT3 vertToConvert[4] = { XMFLOAT3(0.0f, 0.0f, 0.0f), XMFLOAT3(0.0f, 0.0f, 0.0f), XMFLOAT3(0.0f, 0.0f, 0.0f), XMFLOAT3(0.0f, 0.0f, 0.0f) }; XMStoreFloat3(&vertToConvert[0], (-vectorRight)); XMStoreFloat3(&vertToConvert[1], vectorRight); XMStoreFloat3(&vertToConvert[2], (-vectorRight) + vectorUp); XMStoreFloat3(&vertToConvert[3], vectorRight + vectorUp); vert[0] = XMFLOAT3(worldPos.x + vertToConvert[0].x, worldPos.y + vertToConvert[0].y, worldPos.z + vertToConvert[0].z); vert[1] = XMFLOAT3(worldPos.x + vertToConvert[1].x, worldPos.y + vertToConvert[1].y, worldPos.z + vertToConvert[1].z); vert[2] = XMFLOAT3(worldPos.x + vertToConvert[2].x, worldPos.y + vertToConvert[2].y, worldPos.z + vertToConvert[2].z); vert[3] = XMFLOAT3(worldPos.x + vertToConvert[3].x, worldPos.y + vertToConvert[3].y, worldPos.z + vertToConvert[3].z); // Pine Tree // Create vertex buffer SimpleVertex vertices[] = { { vert[0], vert[0], XMFLOAT2(0.0f, 1.0f) }, { vert[1], vert[1], XMFLOAT2(1.0f, 1.0f) }, { vert[2], vert[2], XMFLOAT2(0.0f, 0.0f) }, { vert[3], vert[3], XMFLOAT2(1.0f, 0.0f) }, }; D3D11_BUFFER_DESC bd; ZeroMemory(&bd, sizeof(bd)); bd.Usage = D3D11_USAGE_DEFAULT; bd.ByteWidth = sizeof(SimpleVertex) * 4; bd.BindFlags = D3D11_BIND_VERTEX_BUFFER; bd.CPUAccessFlags = 0; D3D11_SUBRESOURCE_DATA InitData; ZeroMemory(&InitData, sizeof(InitData)); InitData.pSysMem = vertices; // Update Stride and offset GetAppearance()->GetGeometryData()->VBOffset = 0; GetAppearance()->GetGeometryData()->VBStride = sizeof(SimpleVertex); hr = pd3dDevice->CreateBuffer(&bd, &InitData, &GetAppearance()->GetGeometryData()->VertexBuffer); GameObject::Update(delta); if (FAILED(hr)) return hr; return S_OK; }

What a mess. Let's start by moving some of this logic out into different methods so we can understand it better. This will also have the added benefit of narrowing down the exact section of code causing the leak in the performance profiler. We can also pass the eyePos by const reference as we do not change the data and we want to avoid copying data wherever possible. The new code looks something like this:

HRESULT Billboard::Update(const float delta, ID3D11Device* pd3dDevice, const XMFLOAT3& eyePosition) { // Extracting world position from matrix const XMFLOAT3 worldPosition = XMFLOAT3(GetTransform()->GetWorld()._41, GetTransform()->GetWorld()._42, GetTransform()->GetWorld()._43); const XMVECTOR vectorNormal = CalculateBillboardNormal(worldPosition, eyePosition); XMFLOAT3 floatUp = XMFLOAT3(0.0f, 1.0f, 0.0f); XMVECTOR vectorUp = XMLoadFloat3(&floatUp); XMVECTOR vectorRight = XMVector3Normalize(XMVector3Cross(vectorNormal, vectorUp)); vectorRight = vectorRight * (GetAppearance()->GetWidth() / 2); floatUp = XMFLOAT3(0.0f, GetAppearance()->GetHeight() / 2, 0.0f); vectorUp = XMLoadFloat3(&floatUp); HRESULT hr = CreateVertexBuffer(pd3dDevice, worldPosition, vectorRight, vectorUp); if (FAILED(hr)) return hr; GameObject::Update(delta); return S_OK; }

Much better! As expected, this has helped the Memory Profiler narrow down the offending piece of code [Figure 3].


Figure 3 - Seperating the code out into methods further reveals the offending code


Looking at the code that constructs the Vertex Buffer, it is fairly obvious where the leak is occuring. The VertexBuffer we pass into the pd3dDevice->CreateBuffer() asks for a pointer to a pointer of type ID3D11Buffer. This is problematic when we are constantly changing this value each frame, as the previous pointer allocated on the previous frame is never cleared. A naive fix to this would be to clean up the memory at the top of the method:

HRESULT Billboard::CreateVertexBuffer(ID3D11Device* pd3dDevice, const XMFLOAT3& worldPosition, const XMVECTOR& vectorRight, const XMVECTOR& vectorUp) { GetAppearance()->GetGeometryData()->VertexBuffer->Release(); GetAppearance()->GetGeometryData()->VertexBuffer = nullptr; ... return pd3dDevice->CreateBuffer(&bd, &InitData, &GetAppearance()->GetGeometryData()->VertexBuffer); }

While this works, it is failing to address a fairly important flaw in the code. In games programming, a billboarded object is often a simple 2D sprite that rotates to always face the player. The vertex buffer is updated each frame to place the vertices in a way that cause the billboard to face the player, but what happens if the player doesn't move?

In this code, the application will simply plough on and reconstruct the vertex buffer regardless. Let's add a check in Billboard::Update() to prevent this:

HRESULT Billboard::Update(const float delta, ID3D11Device* pd3dDevice, const XMFLOAT3& eyePosition) { // Extracting world position from matrix const XMFLOAT3 worldPosition = XMFLOAT3(GetTransform()->GetWorld()._41, GetTransform()->GetWorld()._42, GetTransform()->GetWorld()._43); const XMFLOAT3 vectorNormalVector = CalculateBillboardNormal(worldPosition, eyePosition); if (_vectorNormal.x == vectorNormalVector.x && _vectorNormal.y == vectorNormalVector.y && _vectorNormal.z == vectorNormalVector.z) { GameObject::Update(delta); return S_OK; } _vectorNormal = vectorNormalVector; ... HRESULT hr = CreateVertexBuffer(pd3dDevice, worldPosition, vectorRight, vectorUp); if (FAILED(hr)) return hr; ... }

Now, the code will check if any changes occured to the normal vector drawn between the world position and eye position. The call to CreateVertexBuffer(...) is therefore only called if the camera or billboard have moved since the last frame. A few changes were made to the new CalculateBillboardNormal(...) method to return an object of type XMFLOAT3 opposed to XMVECTOR as they are easier to work with. It is later updated to an XMVECTOR in the Billboard::Update(...) method.

This improved the severity of the memory leak quite significantly, although it is clear the application still has some leaky pipes [Figure 4].


Figure 4 - Still leaking... but getting there!



Memory Leak: Particle Emitter

The second big worry in the memory leak department was the amount of allocations being done by the Particle Emitter. This class is responsible, surprisingly enough, for emitting particles. As a result, it shouldn't be too odd to expect such a class to allocate a lot of memory to generate a large amount of particles. What is worrying, however, is that this memory never appears to be deallocated. Let's investigate where the particles are allocated and how the application stores them, with the intention of discovering how the application deals with the deletion of a particle once it's energy has been expended.

The method ParticleEmitter::Update() appears to have code that deals with the deletion of particles already:

void ParticleEmitter::Update(float delta, ID3D11Device * pd3dDevice) { ... // Update particles for (int i = 0; i < _particles.size(); i++) { SmokeParticle* particle = (SmokeParticle*)_particles.at(i); if (particle != nullptr) { particle->Update(delta); if (particle->GetHealth() < 0.0f) { delete particle; _particles.at(i) = nullptr; } } } GameObject::Update(delta); }

This code looks like it does the intended job, so let's check the deconstructor...

SmokeParticle::~SmokeParticle() { }

Again, although the deconstructor is empty, this code should be fine as the class SmokeParticle containts no memory allocated on the heap. So where does the problem occur?

It turns out that one of the problems is related to the class it inherets from, GameObject. This class is inhereted by every other object that has a visual presence in the 3D world (ugh) and is therefore a memory leak that occurs everywhere. Here is the guilty code:

GameObject::~GameObject() { if (_appearance != nullptr) delete _appearance; if (_particleModel != nullptr) delete _particleModel; _waypointManager = nullptr; _parent = nullptr; _transform = nullptr; _particleModel = nullptr; }

Can you spot the rookie mistake? If not, the problem is that simply setting some of the pointer variables to nullptr is not equivalent to deleting them from the heap. In this scenareo, it doesn't make much sense to delete _waypointManager and _parent as they are pieces of code still being used elsewhere. The rest however, should be removed like so:

GameObject::~GameObject() { if (_appearance) { delete _appearance; _appearance = nullptr; } if (_particleModel) { delete _particleModel; _particleModel = nullptr; } if (_transform) { delete _transform; _transform = nullptr; } if (_particleModel) { delete _particleModel; _particleModel = nullptr; } }

This is not the only problem however. Another issue remains where the particle emitters themselves are never deleted when their parent is destroyed. In the application, each plane has a particle emitter that fires smoke particles when the plane is damaged. Once the plane has crashed, it is deleted from the world. However, no checks are put into place to delete the particle emitter, meaning it and all its particles are left suspended in memory with nothing to call it. There are multiple ways to fix this, and it is most tempting to redesign the entire system. However, for now, let's give the plane a list of child objects that it is responsible for deleting.

class GameObject { protected: ... GameObject* _parent; std::vector<GameObject*> _children; std::string _name; ... };

void PlaneSpawner::Update(float delta, ID3D11Device * pd3dDevice, vector<GameObject*>* objList) { ... ParticleEmitter* newSmoke = new ParticleEmitter(team + "_Spawned_Smoke_" + to_string(_planeCount), nullptr, smokeTransform, smokeParticle, pd3dDevice, 100.0f, 0.01f, true); newSmoke->SetParent(newPlane); newPlane->AddChild(newSmoke); ... }

ParticleEmitter::~ParticleEmitter() { for(SmokeParticle* particle : _particles) delete particle; for(BulletParticle* particle : _bullets) delete particle; }

This seems to have made a small difference in the performance profiler now, with drop offs in memory being detectable [Figure 5]. However, the graph still follows an upwards trend suggesting memory leaks are still present elsewhere in the application.


Figure 5 - Drops in memory now visible.



Locating the smaller leaks

To locate where the other leaks are occuring, the heap profiler provided by Visual Studio can be used to located which methods allocate the most things to the heap [Figure 6]. Leaving a gap of 40 seconds between the first and second heap profiles should give an approximate overview of where most allocations are happening [Figure 7]. This seems to suggest that most allocations are still occuring on the ParticleEmitter::Update(...) method, which can be proven by inspecting the Allocation Call Stack on the Heap Profiler [Figure 8].


Figure 6 - Heap Profile.


Figure 7 - Allocations made to the heap over 40 seconds of runtime.


Figure 8 - Allocation Call Stack for all Transform class heap allocations.


It would appear that, even with the changes made previously, the particles are still not being deleted correctly. It was also noticed that a simular issue was occuring when a Plane was spawned, which happens far less frequently, but was still a memory leak that was occuring. Then, it occured to me to check that the deconstructors in the classes derived from GameObject were actually being called and... lo and behold, in most cases, they were not.

In the main application file, all objects are stored in a vector list of GameObjects. The Application class is responsible for deleting these when appropiate, so it calls delete _objects.at(i);, for example. Except, in the code, the deconstructor was not made virtual, meaning that when the deconstructor is called, it only calls it on the object it is stored as, which was of type GameObject. Therefore, all data stored in GameObject's derived classes were not deleted. To fix this, the keyword virtual was inserted before the GameObject deconstructor and it's derived classes so the virtual table can be constructed, allowing the code to know what deconstructor to call at runtime.

virtual ~GameObject();


This leveled out the processed memory graph and appeared to have fixed the memory leak for the most part [Figure 9]. Other small issues remained with a few classes not correctly deallocating memory, but nothing outrageous enough to be worth noting in this blog.


Figure 9 - Much better!


I hope you have enjoyed this first blog post! It's not the most interesting of topics, but it has been a good exercise to delve into how poor my understanding was 2 years ago and fix some of the more sinister issues. I may pick this up again in the future for further refactoring exercises as there is a LOT I can do... But I may opt to cover some more interesting topics that may be of use to aspiring DirectX developers.

Thank you for your time!

Comments