From 4815fb4e1e185a4af4f090ba146dfdd21c48858c Mon Sep 17 00:00:00 2001 From: legatt Date: Sat, 4 Jul 2026 14:29:42 +0300 Subject: [PATCH] Closing technical debt - Manage resources deallocations in AssetManager - Reformatting in AssetManager - Fix resource address synchronization in AssetManager - Windows native file system uses IOCP for performance --- engine/modules/AssetManager.cppm | 142 +++++++++++--------- engine/modules/FileSystem.cppm | 37 +++++- engine/src/FileSystem.cpp | 215 +++++++++++++++++++++++-------- 3 files changed, 272 insertions(+), 122 deletions(-) diff --git a/engine/modules/AssetManager.cppm b/engine/modules/AssetManager.cppm index dee210a..6ada97b 100644 --- a/engine/modules/AssetManager.cppm +++ b/engine/modules/AssetManager.cppm @@ -28,11 +28,11 @@ export ee_assets::AssetType type = ee_assets::BINARY; std::atomic isReady; - AssetAddress *resourceAddress; + std::atomic resourceAddress; explicit AssetInfo(void *resource) { this->resourceAddress = Allocate(); - this->resourceAddress->resource.store(resource); + this->resourceAddress.load(std::memory_order_relaxed)->resource.store(resource); } AssetInfo(const AssetInfo&) = delete; @@ -40,7 +40,7 @@ export AssetInfo(AssetInfo&& other) noexcept : type(other.type), - resourceAddress(other.resourceAddress) { + resourceAddress(other.resourceAddress.load(std::memory_order_relaxed)) { isReady.store(other.isReady.load(std::memory_order_relaxed), std::memory_order_relaxed); other.resourceAddress = nullptr; @@ -50,7 +50,7 @@ export AssetInfo& operator=(AssetInfo&& other) noexcept { if (this != &other) { type = other.type; - resourceAddress = other.resourceAddress; + resourceAddress = other.resourceAddress.load(std::memory_order_relaxed); isReady.store(other.isReady.load(std::memory_order_relaxed), std::memory_order_relaxed); other.resourceAddress = nullptr; @@ -105,7 +105,7 @@ export } } - void LoadAsync(ee_assets::AssetType type, const std::filesystem::path& path) { + void LoadAsync(const ee_assets::AssetType type, const std::filesystem::path& path) { AssetStreamCommand command; command.type = AssetStreamCommand::LOAD; command.targetPath = path; @@ -150,21 +150,22 @@ export return Get(Find(id)); } - void Redirect(const AssetHandle& handle, void *resource) const { + // ReSharper disable once CppMemberFunctionMayBeConst + void Redirect(const AssetHandle& handle, void *resource) { const AssetInfo& asset = activeAssets[handle.slot]; - if (asset.resourceAddress == nullptr) { + AssetAddress *address = asset.resourceAddress.load(std::memory_order_relaxed); + if (address == nullptr) { LOG(WARNING, "Attempt to redirect empty handle"); return; } - // TODO: LEAK! Free old resource after redirecting handler to other resource - if (asset.resourceAddress->resource.load() != nullptr) { - void *oldResource = asset.resourceAddress->resource.exchange(nullptr, std::memory_order_acq_rel); - //if (oldResource) - //Deallocate(oldResource); - } - - asset.resourceAddress->resource.store(resource); + if ( + void *oldResource = address->resource.exchange( + resource, + std::memory_order_acq_rel + ) + ) + Deallocate(oldResource); } // Returned resource reference should not be stored @@ -184,7 +185,7 @@ export void Free(const AssetHandle& handle) { AssetInfo& asset = activeAssets[handle.slot]; Redirect(handle, nullptr); - Deallocate(asset.resourceAddress); + Deallocate(asset.resourceAddress.load(std::memory_order_relaxed)); asset.resourceAddress = nullptr; freeAssetSlots.push_back(handle.slot); @@ -192,57 +193,24 @@ export protected: void Update() override { - // TODO: FinishLoadTasks() - for (AssetLoadingEntry& entry : loadingEntries) { - if (entry.fileOwner->PollReady(*entry.file)) { - AssetInfo& asset = activeAssets[entry.handle.slot]; - - switch (asset.type) { - case ee_assets::MAP_MESH: - FinishLoadCapnprotoDeserialize(entry, mapMeshesStorage); - break; - case ee_assets::SAVE: - FinishLoadCapnprotoDeserialize(entry, savesStorage); - break; - - case ee_assets::SHADER: - FinishLoadConstruct(entry, shadersStorage); - break; - case ee_assets::MATERIAL: - FinishLoadCapnprotoDeserialize(entry, materialsStorage); - break; - case ee_assets::IMAGE: - break; - - case ee_assets::BINARY: - default: - FinishLoadConstruct(entry, binaryStorage); - break; - } - - entry.fileOwner->Close(*entry.file); -#ifndef NDEBUG - LOG(INFO, "Asset loaded: [%hs]", entry.file->debugPath.generic_u8string().c_str()); -#endif - entry.file = nullptr; - asset.isReady.store(true); - } - } - - std::erase_if(loadingEntries, [](const AssetLoadingEntry& entry) { - return entry.file == nullptr; - }); + FinishLoadTasks(); } void Release() override { for (const AssetInfo& asset : activeAssets) { - //TODO: LEAK! Free resource - if (asset.resourceAddress->resource.load() != nullptr) { - void *resource = asset.resourceAddress->resource.exchange(nullptr, std::memory_order_acq_rel); - //if (resource) - //Deallocate(resource); + if ( + AssetAddress* address = asset.resourceAddress.load(std::memory_order_relaxed); + address != nullptr + ) { + if ( + void *resource = address->resource.exchange( + nullptr, + std::memory_order_acq_rel + ) + ) + Deallocate(resource); + Deallocate(address); } - Deallocate(asset.resourceAddress); } activeAssets.clear(); @@ -289,7 +257,7 @@ export std::size_t assetPos = 0; if (freeAssetSlots.size() > 0) { assetPos = freeAssetSlots.back(); - Deallocate(activeAssets[assetPos].resourceAddress); + Deallocate(activeAssets[assetPos].resourceAddress.load(std::memory_order_relaxed)); activeAssets[assetPos] = std::move(asset); freeAssetSlots.pop_back(); } @@ -342,11 +310,57 @@ export return; if (command.out != nullptr) - command.out->store(activeAssets[command.target.slot].resourceAddress->resource); + command.out->store( + activeAssets[command.target.slot].resourceAddress.load(std::memory_order_relaxed)->resource + ); command.type = AssetStreamCommand::COMPLETED; } + void FinishLoadTasks() { + diskSystem->PollLoads(); + + for (AssetLoadingEntry& entry : loadingEntries) { + if (entry.file->isCompleted) { + AssetInfo& asset = activeAssets[entry.handle.slot]; + + switch (asset.type) { + case ee_assets::MAP_MESH: + FinishLoadCapnprotoDeserialize(entry, mapMeshesStorage); + break; + case ee_assets::SAVE: + FinishLoadCapnprotoDeserialize(entry, savesStorage); + break; + + case ee_assets::SHADER: + FinishLoadConstruct(entry, shadersStorage); + break; + case ee_assets::MATERIAL: + FinishLoadCapnprotoDeserialize(entry, materialsStorage); + break; + case ee_assets::IMAGE: + break; + + case ee_assets::BINARY: + default: + FinishLoadConstruct(entry, binaryStorage); + break; + } + + entry.fileOwner->Close(*entry.file); +#ifndef NDEBUG + LOG(INFO, "Asset loaded: [%hs]", entry.file->debugPath.generic_u8string().c_str()); +#endif + entry.file = nullptr; + asset.isReady.store(true); + } + } + + std::erase_if(loadingEntries, [](const AssetLoadingEntry& entry) { + return entry.file == nullptr; + }); + } + template void FinishLoadConstruct(AssetLoadingEntry& loaded, std::vector& storage) { T *resource = Allocate(loaded.file->data, loaded.file->size); diff --git a/engine/modules/FileSystem.cppm b/engine/modules/FileSystem.cppm index c4a8457..b369930 100644 --- a/engine/modules/FileSystem.cppm +++ b/engine/modules/FileSystem.cppm @@ -15,19 +15,30 @@ export std::uint8_t *data = nullptr; std::uint8_t *buffer = nullptr; + bool isCompleted = false; + bool isClosing = false; + #ifndef NDEBUG std::filesystem::path debugPath; #endif #ifdef _WIN32 HANDLE win32Handle = INVALID_HANDLE_VALUE; - OVERLAPPED overlapped; #endif #ifdef __linux__ #endif }; +#ifdef _WIN32 + struct IOContext { + OVERLAPPED overlapped; + FileHandle *file; + std::size_t currentOffset; + }; +#endif + + class IFileSystem { public: static constexpr std::uint32_t FILE_OPEN_NONE = 0; @@ -41,7 +52,7 @@ export virtual void ReadAsync(FileHandle& file, std::uint32_t bufferSize, std::uint32_t flags) = 0; - virtual bool PollReady(FileHandle& file) = 0; + virtual void PollLoads() = 0; virtual void Close(FileHandle& file) = 0; @@ -55,12 +66,26 @@ export class NativeFileSystem final : public IFileSystem { public: - FileHandle& Open(const std::filesystem::path& path, std::uint32_t flags) override; + NativeFileSystem(); + ~NativeFileSystem() override; - void ReadAsync(FileHandle& file, std::uint32_t bufferSize, std::uint32_t flags) override; + FileHandle& Open(const std::filesystem::path &path, std::uint32_t flags) override; - bool PollReady(FileHandle& file) override; + void ReadAsync(FileHandle &file, std::uint32_t bufferSize, std::uint32_t flags) override; - void Close(FileHandle& file) override; + void PollLoads() override; + + void Close(FileHandle &file) override; + + private: + +#ifdef _WIN32 + HANDLE iocpHandle = INVALID_HANDLE_VALUE; + + bool HandleChunk(IOContext *context, DWORD bytesTransferred, bool success); + void IssueRead(IOContext *context); +#endif + + void FreeFile(FileHandle &file); }; } diff --git a/engine/src/FileSystem.cpp b/engine/src/FileSystem.cpp index e949f0e..469a3f2 100644 --- a/engine/src/FileSystem.cpp +++ b/engine/src/FileSystem.cpp @@ -5,8 +5,6 @@ module; module FileSystem; import utils; -//TOO: REWORK WITH IOCP - IFileSystem::~IFileSystem() { for (FileHandle *handle : activeFiles) Deallocate(handle); @@ -16,6 +14,27 @@ IFileSystem::~IFileSystem() { #ifdef _WIN32 +NativeFileSystem::NativeFileSystem() { + iocpHandle = CreateIoCompletionPort( + INVALID_HANDLE_VALUE, + nullptr, + 0, + 0 + ); + if (iocpHandle == nullptr) { + LOG(CRITICAL, "Failed to create IOCP. Error: %d", GetLastError()); + } +} + +NativeFileSystem::~NativeFileSystem() { + // I have no idea why ReSharper think that this condition is always false + // ReSharper disable once CppDFAConstantConditions + if (iocpHandle != INVALID_HANDLE_VALUE && iocpHandle != nullptr) { + // ReSharper disable once CppDFAUnreachableCode + CloseHandle(iocpHandle); + } +} + FileHandle& NativeFileSystem::Open(const std::filesystem::path& path, const std::uint32_t flags) { FileHandle *result = Allocate(); @@ -30,14 +49,14 @@ FileHandle& NativeFileSystem::Open(const std::filesystem::path& path, const std: if (flags & FILE_OPEN_WRITE) createFileMode |= GENERIC_WRITE; - HANDLE file = CreateFileW( + const HANDLE file = CreateFileW( path.c_str(), createFileMode, NULL, - NULL, + nullptr, OPEN_EXISTING, FILE_FLAG_OVERLAPPED, - NULL + nullptr ); if (file == INVALID_HANDLE_VALUE) { @@ -47,12 +66,23 @@ FileHandle& NativeFileSystem::Open(const std::filesystem::path& path, const std: } result->win32Handle = file; - result->size = GetFileSize(file, NULL); + result->size = GetFileSize(file, nullptr); if (result->size == INVALID_FILE_SIZE) LOG(ERROR, "Failed to get size of file %s. Error: %d", path.c_str(), GetLastError()); + if ( + CreateIoCompletionPort( + file, + iocpHandle, + NULL, + NULL + ) == nullptr + ) { + LOG(ERROR, "Failed to associate file with IOCP. Error: %d", GetLastError()); + } + if (freeFileSlots.size() > 0) { - std::size_t pos = freeFileSlots.back(); + const std::size_t pos = freeFileSlots.back(); result->id = pos; activeFiles[pos] = result; freeFileSlots.pop_back(); @@ -66,73 +96,140 @@ FileHandle& NativeFileSystem::Open(const std::filesystem::path& path, const std: } void NativeFileSystem::ReadAsync(FileHandle& file, const std::uint32_t bufferSize, const std::uint32_t flags) { - file.overlapped = { - .hEvent = CreateEvent(nullptr, TRUE, FALSE, nullptr) - }; - file.buffer = AllocateArray(bufferSize); + file.isCompleted = false; + file.isClosing = false; file.bufferSize = bufferSize; + file.buffer = AllocateArray(file.bufferSize); - BOOL isSync = ReadFile( - file.win32Handle, - file.buffer, - bufferSize, - NULL, - &file.overlapped - ); + IOContext *context = Allocate(); + context->file = &file; + context->currentOffset = 0; + context->overlapped = {}; - if (isSync || GetLastError() != ERROR_IO_PENDING) - LOG(ERROR, "File reading is synchronious while asynchronious is awaited"); + if (!ReadFile(file.win32Handle, file.buffer, bufferSize, nullptr, &context->overlapped)) { + if (DWORD error = GetLastError(); error != ERROR_IO_PENDING) { +#ifndef NDEBUG + LOG(ERROR, "Failed to start reading file [%s]. Error: %d", file.debugPath.c_str(), error); +#else + LOG(ERROR, "Failed to start reading file [%d]. Error: %d", file.id, error); +#endif + HandleChunk(context, 0, false); + } + } } -bool NativeFileSystem::PollReady(FileHandle& file) { - DWORD status = WaitForSingleObject(file.overlapped.hEvent, 0); +void NativeFileSystem::PollLoads() { + DWORD bytesTransferred = 0; + OVERLAPPED *overlapped = nullptr; + ULONG_PTR ck = 999; - switch (status) { - case WAIT_OBJECT_0: - { - if (file.size == file.bufferSize) { + while (true) { + const bool success = GetQueuedCompletionStatus( + iocpHandle, + &bytesTransferred, + &ck, + &overlapped, + 0 + ); + + if (!overlapped) { // No more ready files + if (DWORD error = GetLastError(); !success && error != WAIT_TIMEOUT) { + LOG(ERROR, "Can not get files loading status. Error: %d", error); + } + break; + } + + if ( + IOContext *context = reinterpret_cast(overlapped); + HandleChunk(context, bytesTransferred, success) + ) { + IssueRead(context); + } + } +} + +void NativeFileSystem::Close(FileHandle& file) { + if (file.win32Handle != INVALID_HANDLE_VALUE) { + CancelIoEx(file.win32Handle, nullptr); + + if (file.isCompleted) { + FreeFile(file); + } + else { + file.isClosing = true; + } + } +} + +bool NativeFileSystem::HandleChunk(IOContext *context, const DWORD bytesTransferred, const bool success) { + FileHandle &file = *context->file; + + if (!success) { + if (const DWORD error = GetLastError(); error != ERROR_OPERATION_ABORTED) { +#ifndef NDEBUG + LOG(ERROR, "IOCP read error [%s]. Error: %d", file.debugPath.c_str(), error); +#else + LOG(ERROR, "IOCP read error [%d]. Error: %d", file.id, error); +#endif + } + + if (file.isClosing) FreeFile(file); + + Deallocate(context); + file.isCompleted = true; + return false; + } + + if (file.data == nullptr) { // Initialize for first chunk + if (file.size <= file.bufferSize) { // File of one chunk, just target resource file.data = file.buffer; file.buffer = nullptr; } else { - /* TODO: reserve size for data, shift and move */ + file.data = AllocateArray(file.size); // Allocate memory for all chunks } - - break; } - case WAIT_ABANDONED: - case WAIT_TIMEOUT: - return false; - - default: - { - LOG(CRITICAL, "Failed to get file status. Error: %d", GetLastError()); + if (bytesTransferred == 0) { // Case of unexpected EOF + Deallocate(context); + if (file.isClosing) FreeFile(file); + file.isCompleted = true; return false; } + + if (file.buffer != nullptr) { + std::ranges::copy(file.buffer, file.buffer + bytesTransferred, file.data + context->currentOffset); + } + context->currentOffset += bytesTransferred; + + if (context->currentOffset >= file.size) { // Finish reading of completed file + Deallocate(context); + if (file.isClosing) FreeFile(file); + file.isCompleted = true; + return false; } return true; } -void NativeFileSystem::Close(FileHandle& file) { - if (file.overlapped.hEvent) - CloseHandle(file.overlapped.hEvent); +void NativeFileSystem::IssueRead(IOContext *context) { + // 64-bit offset is seperated for WinAPI + context->overlapped.Offset = static_cast(context->currentOffset); + context->overlapped.OffsetHigh = static_cast(context->currentOffset >> 32); - if (file.buffer != nullptr) { - DeallocateArray(file.buffer); - file.buffer = nullptr; + if ( + !ReadFile( + context->file->win32Handle, + context->file->buffer, + context->file->bufferSize, + nullptr, + &context->overlapped + ) + ) { + if (GetLastError() != ERROR_IO_PENDING) { + HandleChunk(context, 0, false); + } } - if (file.data != nullptr) { - DeallocateArray(file.data); - file.data = nullptr; - } - - if (file.win32Handle != INVALID_HANDLE_VALUE) { - CloseHandle(file.win32Handle); - file.win32Handle = INVALID_HANDLE_VALUE; - } - freeFileSlots.push_back(file.id); } #endif @@ -141,3 +238,17 @@ void NativeFileSystem::Close(FileHandle& file) { #endif + +void NativeFileSystem::FreeFile(FileHandle &file) { + if (file.buffer != nullptr) DeallocateArray(file.buffer); + if (file.data != nullptr) DeallocateArray(file.data); + file.buffer = nullptr; + file.data = nullptr; + +#ifdef _WIN32 + if (file.win32Handle != INVALID_HANDLE_VALUE) CloseHandle(file.win32Handle); + file.win32Handle = INVALID_HANDLE_VALUE; +#endif + + freeFileSlots.push_back(file.id); +} \ No newline at end of file