Skip to content

Commit

Permalink
Fix memory leak of resources caused by incorrect external references. (
Browse files Browse the repository at this point in the history
domchen committed Nov 11, 2024
1 parent 5b060b4 commit 619297c
Showing 7 changed files with 62 additions and 70 deletions.
3 changes: 2 additions & 1 deletion src/gpu/DrawingManager.h
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@
#pragma once

#include <unordered_map>
#include <unordered_set>
#include <vector>
#include "gpu/tasks/OpsRenderTask.h"
#include "gpu/tasks/RenderTask.h"
@@ -49,7 +50,7 @@ class DrawingManager {
void closeActiveOpsTask();

Context* context = nullptr;
UniqueKeyMap<ResourceTask*> resourceTaskMap = {};
ResourceKeyMap<ResourceTask*> resourceTaskMap = {};
std::vector<std::shared_ptr<ResourceTask>> resourceTasks = {};
std::vector<std::shared_ptr<RenderTask>> renderTasks = {};
OpsRenderTask* activeOpsTask = nullptr;
2 changes: 1 addition & 1 deletion src/gpu/ProxyProvider.cpp
Original file line number Diff line number Diff line change
@@ -200,7 +200,7 @@ std::shared_ptr<RenderTargetProxy> ProxyProvider::wrapBackendRenderTarget(
}

void ProxyProvider::purgeExpiredProxies() {
std::vector<const UniqueKey*> keys = {};
std::vector<const ResourceKey*> keys = {};
for (auto& pair : proxyMap) {
if (pair.second.expired()) {
keys.push_back(&pair.first);
2 changes: 1 addition & 1 deletion src/gpu/ProxyProvider.h
Original file line number Diff line number Diff line change
@@ -117,7 +117,7 @@ class ProxyProvider {

private:
Context* context = nullptr;
UniqueKeyMap<std::weak_ptr<ResourceProxy>> proxyMap = {};
ResourceKeyMap<std::weak_ptr<ResourceProxy>> proxyMap = {};

static UniqueKey GetProxyKey(const UniqueKey& uniqueKey, uint32_t renderFlags);

4 changes: 2 additions & 2 deletions src/gpu/ResourceCache.h
Original file line number Diff line number Diff line change
@@ -112,8 +112,8 @@ class ResourceCache {
size_t purgeableBytes = 0;
std::list<Resource*> nonpurgeableResources = {};
std::list<Resource*> purgeableResources = {};
ScratchKeyMap<std::vector<Resource*>> scratchKeyMap = {};
UniqueKeyMap<Resource*> uniqueKeyMap = {};
ResourceKeyMap<std::vector<Resource*>> scratchKeyMap = {};
ResourceKeyMap<Resource*> uniqueKeyMap = {};

static void AddToList(std::list<Resource*>& list, Resource* resource);
static void RemoveFromList(std::list<Resource*>& list, Resource* resource);
36 changes: 23 additions & 13 deletions src/gpu/ResourceKey.cpp
Original file line number Diff line number Diff line change
@@ -52,25 +52,28 @@ ResourceKey::ResourceKey(ResourceKey&& key) noexcept : data(key.data), count(key
key.count = 0;
}

bool ResourceKey::equal(const ResourceKey& that) const {
if (count != that.count) {
return false;
ResourceKey& ResourceKey::operator=(const ResourceKey& that) {
if (this == &that) {
return *this;
}
return memcmp(data, that.data, count * sizeof(uint32_t)) == 0;
delete[] data;
data = CopyData(that.data, that.count);
count = that.count;
return *this;
}

void ResourceKey::copy(const ResourceKey& that) {
if (data == that.data) {
return;
bool ResourceKey::operator==(const ResourceKey& that) const {
if (count != that.count) {
return false;
}
data = CopyData(that.data, that.count);
count = that.count;
return memcmp(data, that.data, count * sizeof(uint32_t)) == 0;
}

ScratchKey::ScratchKey(uint32_t* data, size_t count) : ResourceKey(data, count) {
}

ScratchKey& ScratchKey::operator=(const BytesKey& that) {
delete[] data;
data = CopyData(that.data(), that.size(), 1);
if (data != nullptr) {
data[0] = HashRange(that.data(), that.size());
@@ -89,11 +92,18 @@ UniqueKey UniqueKey::Combine(const UniqueKey& uniqueKey, const BytesKey& bytesKe
if (uniqueKey.empty()) {
return {};
}
auto data = CopyData(bytesKey.data(), bytesKey.size(), 2);
if (bytesKey.size() == 0) {
return uniqueKey;
}
auto offset = std::max(uniqueKey.count, static_cast<size_t>(2));
auto data = CopyData(bytesKey.data(), bytesKey.size(), offset);
if (data == nullptr) {
return uniqueKey;
}
auto count = bytesKey.size() + 2;
if (uniqueKey.count > 2) {
memcpy(data + 2, uniqueKey.data + 2, (uniqueKey.count - 2) * sizeof(uint32_t));
}
auto count = bytesKey.size() + offset;
auto domain = uniqueKey.uniqueDomain;
data[1] = domain->uniqueID();
data[0] = HashRange(data + 1, count - 1);
@@ -156,7 +166,7 @@ UniqueKey& UniqueKey::operator=(const UniqueKey& key) {
if (uniqueDomain != nullptr) {
uniqueDomain->addReference();
}
copy(key);
ResourceKey::operator=(key);
return *this;
}

@@ -193,7 +203,7 @@ LazyUniqueKey::~LazyUniqueKey() {
reset();
}

UniqueKey LazyUniqueKey::get() {
UniqueKey LazyUniqueKey::get() const {
auto domain = uniqueDomain.load(std::memory_order_acquire);
if (domain == nullptr) {
auto newDomain = new UniqueDomain();
61 changes: 28 additions & 33 deletions src/gpu/ResourceKey.h
Original file line number Diff line number Diff line change
@@ -30,6 +30,10 @@ namespace tgfx {
*/
class ResourceKey {
public:
ResourceKey() = default;

ResourceKey(const ResourceKey& that);

virtual ~ResourceKey();

/**
@@ -46,23 +50,32 @@ class ResourceKey {
return data == nullptr ? 0 : data[0];
}

protected:
ResourceKey() = default;
ResourceKey& operator=(const ResourceKey& that);

ResourceKey(uint32_t* data, size_t count);
bool operator==(const ResourceKey& that) const;

ResourceKey(const ResourceKey& that);

ResourceKey(ResourceKey&& key) noexcept;
bool operator!=(const ResourceKey& that) const {
return !(*this == that);
}

bool equal(const ResourceKey& that) const;
protected:
ResourceKey(uint32_t* data, size_t count);

void copy(const ResourceKey& that);
ResourceKey(ResourceKey&& key) noexcept;

uint32_t* data = nullptr;
size_t count = 0;
};

struct ResourceKeyHasher {
size_t operator()(const ResourceKey& key) const {
return key.hash();
}
};

template <typename T>
using ResourceKeyMap = std::unordered_map<ResourceKey, T, ResourceKeyHasher>;

/**
* A key used for scratch resources. There are three important rules about scratch keys:
*
@@ -91,33 +104,24 @@ class ScratchKey : public ResourceKey {
}

ScratchKey& operator=(const ScratchKey& that) {
copy(that);
ResourceKey::operator=(that);
return *this;
}

ScratchKey& operator=(const BytesKey& that);

bool operator==(const ScratchKey& that) const {
return equal(that);
return ResourceKey::operator==(that);
}

bool operator!=(const ScratchKey& that) const {
return !equal(that);
return !(*this == that);
}

private:
ScratchKey(uint32_t* data, size_t count);
};

struct ScratchKeyHasher {
size_t operator()(const ScratchKey& scratchKey) const {
return scratchKey.hash();
}
};

template <typename T>
using ScratchKeyMap = std::unordered_map<ScratchKey, T, ScratchKeyHasher>;

class UniqueDomain;

/**
@@ -179,11 +183,11 @@ class UniqueKey : public ResourceKey {
UniqueKey& operator=(UniqueKey&& key) noexcept;

bool operator==(const UniqueKey& that) const {
return equal(that);
return ResourceKey::operator==(that);
}

bool operator!=(const UniqueKey& that) const {
return !equal(that);
return !(*this == that);
}

private:
@@ -201,15 +205,6 @@ class UniqueKey : public ResourceKey {
friend class LazyUniqueKey;
};

struct UniqueKeyHasher {
size_t operator()(const UniqueKey& uniqueKey) const {
return uniqueKey.hash();
}
};

template <typename T>
using UniqueKeyMap = std::unordered_map<UniqueKey, T, UniqueKeyHasher>;

/**
* LazyUniqueKey defers the acquisition of the UniqueKey until it is actually needed.
*/
@@ -222,14 +217,14 @@ class LazyUniqueKey {
* immediately. Calling this method from multiple threads will not create multiple UniqueKeys.
* This method is thread-safe as long as there is no concurrent reset() call.
*/
UniqueKey get();
UniqueKey get() const;

/**
* Resets the LazyUniqueKey to an empty state. This method is not thread-safe.
*/
void reset();

private:
std::atomic<UniqueDomain*> uniqueDomain = nullptr;
mutable std::atomic<UniqueDomain*> uniqueDomain = nullptr;
};
} // namespace tgfx
24 changes: 5 additions & 19 deletions src/opengl/GLBuffer.cpp
Original file line number Diff line number Diff line change
@@ -22,14 +22,6 @@
#include "utils/UniqueID.h"

namespace tgfx {
static ScratchKey ComputeScratchKey(BufferType bufferType) {
static const uint32_t Type = UniqueID::Next();
BytesKey bytesKey(2);
bytesKey.write(Type);
bytesKey.write(static_cast<uint32_t>(bufferType));
return bytesKey;
}

std::shared_ptr<GpuBuffer> GpuBuffer::Make(Context* context, const void* buffer, size_t size,
BufferType bufferType) {
if (buffer == nullptr || size == 0) {
@@ -39,19 +31,13 @@ std::shared_ptr<GpuBuffer> GpuBuffer::Make(Context* context, const void* buffer,
CheckGLError(context);

unsigned target = bufferType == BufferType::Index ? GL_ELEMENT_ARRAY_BUFFER : GL_ARRAY_BUFFER;
auto scratchKey = ComputeScratchKey(bufferType);
auto glBuffer = Resource::Find<GLBuffer>(context, scratchKey);
auto gl = GLFunctions::Get(context);
if (glBuffer == nullptr) {
unsigned bufferID = 0;
gl->genBuffers(1, &bufferID);
if (bufferID == 0) {
return nullptr;
}
glBuffer = Resource::AddToCache(context, new GLBuffer(bufferType, size, bufferID), scratchKey);
} else {
glBuffer->_sizeInBytes = size;
unsigned bufferID = 0;
gl->genBuffers(1, &bufferID);
if (bufferID == 0) {
return nullptr;
}
auto glBuffer = Resource::AddToCache(context, new GLBuffer(bufferType, size, bufferID));
gl->bindBuffer(target, glBuffer->_bufferID);
gl->bufferData(target, static_cast<GLsizeiptr>(size), buffer, GL_STATIC_DRAW);
if (!CheckGLError(context)) {

0 comments on commit 619297c

Please sign in to comment.