diff --git a/.agent/plans/resource_refactor_prerequisite_bugfixes.md b/.agent/plans/resource_refactor_prerequisite_bugfixes.md index ec456a37a..d97ed9343 100644 --- a/.agent/plans/resource_refactor_prerequisite_bugfixes.md +++ b/.agent/plans/resource_refactor_prerequisite_bugfixes.md @@ -155,6 +155,22 @@ Regression coverage: ## 4. Priority C: loader and callback lifetime +### C0. MemoryManager first-use synchronization + +Current behavior: + +`MemoryManager::addPointer()` skips its mutex until `sHasInit` becomes true. Two concurrent first +tracked allocations can therefore race on the initialization flag, allocation map, and accounting +counters. + +Fix: + +- Use thread-safe function-local initialization for process-lifetime tracker state. +- Always lock map and accounting operations. +- Keep tracker state valid through process-static destruction. + +Status: fixed, 2026-07-13. Unsuppressed focused TSAN coverage passes after the change. + ### C1. TextureAtlasLoader member destruction order Current behavior: @@ -164,14 +180,17 @@ reverse declaration order, so that state dies before mRL joins its work. Fix: -- Add an explicit destructor shutdown/join before any callback-visible member is destroyed, or move - async operation state into a lifetime object that outlives execution. -- Do not rely only on member declaration order without an explicit invariant comment/test. +- Declare `mRL` last so its destructor joins before callback-visible members are destroyed. +- Document and regression-test the member-order invariant. Regression coverage: - Destroy a loader immediately with queued texture tasks and completion callbacks. +Status: fixed, 2026-07-13. `TextureAtlasLoader` now declares `mRL` last, causing its worker join to +run before callback-visible members are destroyed. Loader status/progress synchronization and a +focused ASAN lifetime test were added as part of the same fix. + ### C2. TextureLoader static callback registry is unsynchronized and process-persistent Current behavior: diff --git a/.agent/plans/resource_refactor_prerequisite_execution_plan.md b/.agent/plans/resource_refactor_prerequisite_execution_plan.md new file mode 100644 index 000000000..229da4abc --- /dev/null +++ b/.agent/plans/resource_refactor_prerequisite_execution_plan.md @@ -0,0 +1,413 @@ +# Resource-refactor prerequisite bug-fix execution plan + +Status: active; work packages 1 and 2 completed, 2026-07-13. + +This plan defines the bounded correctness work to complete before Stage 1 of the shared-resource +ownership refactor. It turns the findings in `resource_refactor_prerequisite_bugfixes.md` into an +ordered implementation sequence. The defect ledger remains the source of detailed evidence; this +document defines execution order, dependencies, validation, and the point at which prerequisite +work stops. + +Related documents: + +- `resource_refactor_prerequisite_bugfixes.md` +- `resource_shared_ownership_architecture.md` +- `resource_shared_ownership_stage0_inventory.md` + +## 1. Objective and boundary + +Fix current correctness defects that would make the ownership migration unsafe or unnecessarily +difficult, without introducing `ResourcePtr`, resource catalogs, scopes, deferred texture release, +or compatibility APIs. + +This is not a general cleanup phase. A defect belongs here only when at least one of these is true: + +- It can currently cause a deadlock, use-after-free, double deletion, stale cross-Engine work, or + invalid GL access. +- It prevents deterministic destruction of current Engine-owned systems. +- It prevents loaders and asynchronous producers from being stopped safely before resource + teardown. +- It is a small, independent correctness bug found during the audit and can be fixed without + designing an API that Stage 1 or a later migration will immediately replace. + +Once the work packages below satisfy their exit criteria, begin Stage 1. Do not delay Stage 1 for +raw resource ownership problems already assigned to Stages 2 through 7. + +## 2. Landing rules + +- Land one defect or one tightly coupled lifetime cluster per change. +- Add focused regression coverage before or with each fix. +- Preserve current TextureFactory ownership and current public resource APIs. +- Avoid temporary ownership abstractions that compete with the accepted final architecture. +- Do not invoke callbacks, destroy callback-visible objects, perform GL work, or join threads while + holding a registry/pool mutex. +- Regenerate the build before compiling, format modified C++ sources, and run the relevant focused + unit-test suite under `xvfb`. +- Use ASAN for lifetime/destruction changes and TSAN where concurrent state is changed. +- For Engine teardown changes, run repeated Engine creation/destruction in one process. + +## 3. Work package 1: small independent correctness fixes + +These fixes are low risk and do not depend on the larger lifetime changes. + +### 3.1 TextureAtlasLoader texture-filter count + +Current defect: + +```cpp +size_t count = getTextureAtlas()->getTexturesCount() == 0; +``` + +The expression stores a boolean instead of the texture count. When textures exist, `count` becomes +zero and no filter is applied. When no textures exist, it becomes one and the loop may request +texture index zero. + +Fix: + +- Store the actual texture count. +- Apply the filter to every loaded atlas texture. +- Handle a null or not-yet-created atlas consistently with the surrounding loader API. + +Validation: + +- An atlas with multiple textures updates every texture. +- An empty/not-yet-loaded atlas performs no invalid access. + +Status: implemented and covered by `ResourcePrerequisites` unit tests. Focused ASAN tests pass. + +### 3.2 Unsigned Models::Variant type + +Current defect: + +`Variant(const unsigned int&)` stores the value in `asUint` but sets `mType` to `Type::Int`. + +Fix: + +- Set `mType` to `Type::Uint`. +- Add construction, copy, move, assignment, `is(Type::Uint)`, `asUint()`, and `toString()` coverage. +- Include a value greater than `INT_MAX` so signed reinterpretation cannot pass unnoticed. + +Status: implemented and covered by `ResourcePrerequisites.unsignedVariantPreservesTypeAndValue`. +The focused test and existing `StringMapModel` tests pass under ASAN. + +### 3.3 Texture copy-construction trap + +`Texture` already inherits privately from `NonCopyable`, but it still implements a protected copy +constructor that copies the GL texture handle. This is dangerous if an internal/friend path ever +uses it. + +Fix: + +- Explicitly delete the Texture copy constructor and copy assignment in `texture.hpp`. +- Remove the copy-constructor implementation. +- Add compile-time non-copyability assertions. + +This is defensive cleanup rather than a currently observed public copy path and must not block the +following packages if it exposes unrelated legacy code. + +Status: implemented. The obsolete implementation was removed and compile-time non-copyability +checks cover both construction and assignment. + +### 3.4 Empty ResourceLoader progress + +Current defect: + +`ResourceLoader::getProgress()` divides by `mTasks.size()` without handling an empty loader. + +Fix: + +- Define empty-loader progress explicitly. Prefer `100%` when an empty load is considered complete; + otherwise use `0%` consistently with `isLoaded()` semantics. +- Add focused coverage for empty, partially completed, and completed loaders. + +Status: implemented. An empty loader reports `0%` before loading and `100%` after completing an +empty load. Focused unit coverage verifies both states. + +### 3.5 MemoryManager concurrent bootstrap + +Current defect found during Work Package 2 TSAN validation: + +`MemoryManager::addPointer()` conditionally skips `sAllocMutex` until a process-global `sHasInit` +flag is set. Concurrent first tracked allocations race on that flag and can enter the allocation +map and accounting counters without mutual exclusion. + +Fix: + +- Replace translation-unit bootstrap globals with one thread-safe function-local state. +- Keep that tracking state alive through process-static destruction so late `eeDelete()` calls do + not depend on static destruction order. +- Always lock allocation-map and accounting access, including metric getters. +- Return the biggest-allocation snapshot by value instead of exposing an unlocked mutable record. + +Status: implemented. The focused TSAN suite initially reproduced the race in +`MemoryManager::addPointer()`. After the fix, all six `ResourcePrerequisites` tests pass under +TSAN without suppressions. The ASAN build and focused tests also pass. + +Work-package exit criteria: + +- Each fix has an isolated regression test. +- No resource ownership API has changed. + +## 4. Work package 2: ResourceLoader and TextureAtlasLoader lifetime + +This package establishes reliable loader destruction before changing texture ownership. + +### 4.1 ResourceLoader synchronization audit + +Current worker and caller threads read and write `mLoaded`, `mLoading`, and `mTotalLoaded`. Treat +these accesses as shared state rather than relying on timing. + +Fix requirements: + +- Synchronize status and progress state with atomics or a narrowly scoped mutex. +- Define which thread invokes completion callbacks. Preserve current behavior unless deliberately + changing it with documented caller migration. +- Never invoke completion callbacks while holding the loader-state mutex. +- Ensure task and callback containers cannot be modified while worker execution reads them. + +### 4.2 ResourceLoader destruction contract + +`ResourceLoader` owns and joins its worker from its destructor. There is no consumer-facing need +for a separate terminal shutdown state or public shutdown operation. + +Contract: + +- The destructor waits for the runner and its internal ThreadPool work before clearing tasks and + callbacks. +- A loader is not destroyed from one of its own tasks or completion callbacks. +- Owners whose callbacks access sibling members must encode a destruction order that destroys the + loader before those sibling members. + +### 4.3 TextureAtlasLoader member order + +Current member order destroys callback-visible atlas state before `mRL`, whose destructor performs +the join. + +Fix: + +- Declare `mRL` as the final data member so it is destroyed first and joins before callback-visible + atlas state is destroyed. +- Document the required order beside the member. +- Keep a destruction regression test protecting the invariant. + +Validation: + +- Destroy a loader immediately after queuing several tasks. +- Verify under ASAN that no task or callback accesses destroyed loader members. +- Run synchronization coverage under TSAN where available. + +Work-package exit criteria: + +- TextureAtlasLoader's required destruction order is documented and covered by a regression test. +- ResourceLoader status/progress reads are data-race-free. +- No callbacks execute under internal synchronization locks. + +Status: implemented. Status/progress counters are atomic, task and callback container access is +synchronized, and callbacks execute after releasing internal locks. `ResourceLoader` joins its +worker in its destructor without exposing a terminal shutdown API. `TextureAtlasLoader` declares +`mRL` last, documents the order invariant, and publishes asynchronous status through atomics. +Focused ASAN coverage verifies atlas destruction while tasks and a completion callback are pending. +All six `ResourcePrerequisites` tests pass under both ASAN and unsuppressed TSAN. + +## 5. Work package 3: shared ThreadPool HTTP operation lifetime + +This follows the already fixed `Http::Pool::clear()` lock-order defect. + +Current defect: + +When `Http::setThreadPool()` is active, queued lambdas capture raw `Http*`. `Http` tracks and joins +only its privately created `AsyncRequest` threads, so a shared-pool operation may begin or continue +after its Http object has been destroyed. + +Fix requirements: + +- Register every asynchronous operation before publishing it to an executor. +- Give queued/running operations lifetime and cancellation state independent of raw `Http*`. +- `Http::~Http()` rejects new work, cancels all registered operations, and waits until no operation + can dereference the object. +- Handle destruction initiated from an operation callback without joining/waiting on the same + operation. +- Do not destroy or drain an externally owned shared ThreadPool. +- Do not wait while holding the global Http Pool mutex, operation-map mutex, or any lock visible to + callbacks. +- Preserve cancellation callback behavior deliberately and document it. +- Cover all three async forms: response in memory, external IOStream, and output path. + +Validation: + +- Queue a request behind blocked shared-pool work, destroy/clear its Http owner, then unblock it. +- Destroy while a request is running. +- Re-enter `Http::Pool` from a callback during concurrent pool clearing. +- Initiate final-owner release from a callback and verify no self-deadlock. +- Run under ASAN and TSAN. + +Work-package exit criteria: + +- No shared-pool lambda depends on an untracked raw Http lifetime. +- Http destruction provides a complete operation barrier without taking ownership of the executor. + +## 6. Work package 4: deterministic Engine shutdown + +Reorder shutdown only after HTTP and loader barriers are reliable. + +### 6.1 Required dependency order + +The exact implementation may group calls differently, but it must preserve this dependency graph: + +```text +mark Engine shutting down / reject new deliveries + -> stop and join HTTP and resource-producing work + -> invalidate and purge UI main-thread deliveries + -> destroy scenes + -> flush or discard GlobalBatchRenderer submissions + -> clear TextLayout and shaped-font caches + -> destroy UI/global drawable providers and resource managers + -> destroy fonts, atlases and textures in dependency order + -> destroy shader programs and shaders + -> destroy framebuffer and vertex-buffer registries/owners + -> destroy Renderer + -> destroy windows and GL contexts + -> destroy process utilities and backend state +``` + +Concrete corrections required: + +- Stop `Network::Http::Pool` and Engine-owned resource producers before Graphics consumers. +- Destroy `SceneManager` before `GlobalBatchRenderer` and `NinePatchManager` resources scenes can + reference. +- Flush or explicitly discard pending batches before releasing their borrowed dependencies. +- Call `TextLayout::clearLayoutCache()` before `FontManager::destroySingleton()`. +- Destroy `ShaderProgramManager` before `Renderer` while `GLi` and a valid context still exist. +- Keep windows/context state alive through every GPU-object destruction step. + +### 6.2 Validation + +- Engine destruction with a live scene using fonts, nine-patches, textures, batches, shaders, FBOs, + and vertex buffers. +- Engine destruction with pending HTTP and decode/resource-loader work. +- Multiple Engine create/destroy cycles in one test process. +- Assert no destructor recreates an Engine or manager singleton. +- ASAN/LSAN clean teardown in supported configurations. + +Work-package exit criteria: + +- All asynchronous producers are behind a shutdown barrier before their consumers are destroyed. +- Every GPU-owning manager is destroyed while its required Renderer/context services remain valid. +- Repeated Engine lifecycle tests pass. + +## 7. Work package 5: UISceneNode async delivery lifecycle + +Current behavior: + +Async resource deliveries are stored in a process-static queue. Generation/alive checks prevent +many stale callbacks from mutating a dead scene, but queued closures and their captures can survive +until an unrelated future scene update and cross an Engine test boundary. + +Fix requirements: + +- Add explicit accept/reject/drain/purge lifecycle operations for the queue. +- Reject new deliveries once UI/Engine shutdown begins. +- Invalidate scene generation/alive state before purging queued closures. +- Release captures on the main/update thread, following the existing project destruction contract. +- Re-open/reset the delivery mechanism deliberately for a recreated test Engine. +- Do not allow work queued by Engine lifecycle A to execute during lifecycle B. + +Validation: + +- Queue immediate and delayed deliveries, destroy the scene before update, and verify neither runs. +- Destroy and recreate Engine, then update a new UISceneNode and verify no old closure executes. +- Race worker submission with queue shutdown under TSAN. + +Work-package exit criteria: + +- The static queue has an explicit Engine lifecycle boundary. +- Purging releases all stale captures deterministically. + +## 8. Work package 6: FrameBufferFBO recreation correctness + +First determine the intended callers and semantics of `FrameBufferFBO::reload()`. + +Required distinction: + +- Live-context recreation must release the previous framebuffer/renderbuffer objects before + replacing their handles. +- Context-loss recreation must forget names from the lost namespace without issuing invalid delete + calls against the replacement context. + +Additional create-path audit: + +- Restore prior framebuffer/renderbuffer bindings on every failure return. +- Release partially created objects on live-context failure. +- Leave the object in a destructible, clearly invalid state after failure. +- Do not change texture-attachment ownership in this package; that belongs to TexturePtr Stage 2. + +Validation: + +- Repeated live-context recreation does not grow tracked GL object counts. +- Context-loss recreation performs no deletion in the lost namespace. +- Forced create failures restore previous bindings and do not leak partial objects. + +Work-package exit criteria: + +- `reload()` has an explicit live/lost-context contract. +- Replacement and failure paths have deterministic GL-handle cleanup. + +## 9. Work package 7: TextureLoader callback registry safety + +Current behavior: + +`TextureLoader::sCbs` is process-static and unsynchronized. Loading may notify from a worker while +UITextureViewer or another caller registers/removes callbacks. + +Fix requirements: + +- Synchronize callback registration and removal. +- Copy/snapshot callbacks under the lock and invoke the snapshot after unlocking. +- Define removal-during-notification behavior. +- Add an explicit test/Engine lifecycle reset operation if the registry remains process-static. +- Ensure callbacks from an old Engine lifecycle cannot target UI state in a recreated Engine. + +Validation: + +- Concurrent registration, removal, and notification under TSAN. +- Callback re-entry into registration/removal does not deadlock. +- Repeated Engine lifecycle does not inherit callback subscriptions. + +This package may be omitted only if Stage 1 immediately replaces this registry with the accepted +weak live-texture diagnostics mechanism. The omission must be an explicit Stage 1 scope decision, +not an assumption. + +## 10. Deferred findings: do not solve in this plan + +The following defects remain recorded but should normally be resolved by their owning migration +stage because a raw-pointer workaround would be short-lived or semantically incomplete: + +- `Variant` copying an owning Drawable pointer: resolve with the Stage 4 handle/`std::variant` + redesign unless a current reproducer requires an emergency restriction. +- `UISkin::clone()` and `StateListDrawable` duplicating ownership maps: resolve with Stage 4 + source/instance semantics unless a current owning-child clone path is demonstrated. +- FrameBuffer texture attachment direct/factory ownership: resolve in the complete TexturePtr + holder cut in Stage 2. +- TextureRegion, TextureAtlas, fonts, glyphs, sprites, particles, and batches borrowing textures: + resolve together in Stage 2. +- Mutable Drawable sharing and incorrect `isStateful()` classifications: resolve in Stage 4. +- TextureFactory lock scope, weak diagnostics, semantic lookup, metrics, and deferred release: + these are Stage 1 through Stage 3 architecture work, not prerequisite patches. + +## 11. Final prerequisite gate + +Stage 1 may begin when all mandatory packages satisfy these invariants: + +- HTTP and ResourceLoader work cannot outlive the objects they dereference. +- Engine shutdown rejects and joins asynchronous resource producers before destroying scenes or + Graphics systems. +- UISceneNode queued delivery cannot cross an Engine lifecycle boundary. +- Scenes and caches are destroyed before the resources they borrow. +- Shader, font-layout, Renderer, and context destruction order is valid. +- TextureAtlasLoader destruction is safe with active work. +- FrameBuffer recreation cannot leak or delete objects from the wrong context namespace. +- Focused ASAN tests and repeated Engine create/destroy tests pass. + +After this gate, start Stage 1 immediately. Any newly discovered issue is added to this prerequisite +track only if it violates one of these invariants; otherwise it is assigned to its resource-family +migration stage. diff --git a/include/eepp/core/memorymanager.hpp b/include/eepp/core/memorymanager.hpp index f43f7da19..f97a57bc6 100644 --- a/include/eepp/core/memorymanager.hpp +++ b/include/eepp/core/memorymanager.hpp @@ -64,7 +64,7 @@ class EE_API MemoryManager { static size_t getTotalMemoryUsage(); - static const AllocatedPointer& getBiggestAllocation(); + static AllocatedPointer getBiggestAllocation(); }; #if defined( __GNUC__ ) && __GNUC__ >= 12 #pragma GCC diagnostic pop diff --git a/include/eepp/graphics/texture.hpp b/include/eepp/graphics/texture.hpp index c40870e33..9d4b21386 100644 --- a/include/eepp/graphics/texture.hpp +++ b/include/eepp/graphics/texture.hpp @@ -340,7 +340,9 @@ class EE_API Texture : public DrawableResource, public Image, private NonCopyabl const Texture::ClampMode& clampMode, const bool& CompressedTexture, const Uint32& memSize = 0, const Uint8* data = NULL ); - Texture( const Texture& copy ); + Texture( const Texture& copy ) = delete; + + Texture& operator=( const Texture& copy ) = delete; void create( const Uint32& texture, const unsigned int& width, const unsigned int& height, const unsigned int& imgwidth, const unsigned int& imgheight, const bool& UseMipmap, diff --git a/include/eepp/graphics/textureatlasloader.hpp b/include/eepp/graphics/textureatlasloader.hpp index 5f9679f94..7a4dfd97e 100644 --- a/include/eepp/graphics/textureatlasloader.hpp +++ b/include/eepp/graphics/textureatlasloader.hpp @@ -120,10 +120,10 @@ class EE_API TextureAtlasLoader { void setThreaded( const bool& threaded ); /** @return True if the texture atlas is loaded. */ - const bool& isLoaded() const; + bool isLoaded() const; /** @return True if the texture atlas is loading. */ - const bool& isLoading() const; + bool isLoading() const; /** The function will check if the texture atlas is updated. Checks if all the images inside the * images path are inside the texture atlas, and if they have the same date and size, otherwise @@ -162,13 +162,12 @@ class EE_API TextureAtlasLoader { void setTextureFilter( const Texture::Filter& textureFilter ); protected: - ResourceLoader mRL; std::string mTextureAtlasPath; bool mThreaded; - bool mLoaded; + std::atomic mLoaded; Pack* mPack; bool mSkipResourceLoad; - bool mIsLoading; + std::atomic mIsLoading; TextureAtlas* mTextureAtlas; GLLoadCallback mLoadCallback; std::vector mTexturesLoaded; @@ -181,6 +180,10 @@ class EE_API TextureAtlasLoader { sTextureAtlasHdr mTexGrHdr; std::vector mTempAtlass; + // Must remain the last data member: it joins its worker in its destructor before any + // callback-visible loader state above is destroyed. + ResourceLoader mRL; + void createTextureRegions(); }; diff --git a/include/eepp/system/resourceloader.hpp b/include/eepp/system/resourceloader.hpp index dad8ff93f..108f02afd 100644 --- a/include/eepp/system/resourceloader.hpp +++ b/include/eepp/system/resourceloader.hpp @@ -1,8 +1,10 @@ #ifndef EE_SYSTEMCRESOURCELOADER #define EE_SYSTEMCRESOURCELOADER +#include #include #include +#include #include namespace EE { namespace System { @@ -61,12 +63,13 @@ class EE_API ResourceLoader { Uint32 getCount() const; protected: - bool mLoaded; - bool mLoading; - bool mThreaded; + std::atomic mLoaded; + std::atomic mLoading; + std::atomic mThreaded; Uint32 mThreads; - Uint32 mTotalLoaded; + std::atomic mTotalLoaded; Thread mThread; + mutable std::mutex mMutex; std::vector mLoadCbs; std::vector mTasks; diff --git a/include/eepp/ui/models/variant.hpp b/include/eepp/ui/models/variant.hpp index ff8ad7c3f..4c47e67c0 100644 --- a/include/eepp/ui/models/variant.hpp +++ b/include/eepp/ui/models/variant.hpp @@ -40,7 +40,9 @@ class EE_API Variant { explicit Variant( const String& string ) : mType( Type::String ) { mValue.asString = eeNew( String, ( string ) ); } - explicit Variant( const String* string ) : mType( Type::StringPtr ) { mValue.asStringPtr = string; } + explicit Variant( const String* string ) : mType( Type::StringPtr ) { + mValue.asStringPtr = string; + } Variant( Drawable* drawable, bool ownDrawable = false ) : mType( Type::Drawable ) { mValue.asDrawable = drawable; mOwnsObject = ownDrawable; @@ -54,7 +56,7 @@ class EE_API Variant { Variant( bool val ) : mType( Type::Bool ) { mValue.asBool = val; } Variant( const Float& val ) : mType( Type::Float ) { mValue.asFloat = val; } Variant( const int& val ) : mType( Type::Int ) { mValue.asInt = val; } - Variant( const unsigned int& val ) : mType( Type::Int ) { mValue.asUint = val; } + Variant( const unsigned int& val ) : mType( Type::Uint ) { mValue.asUint = val; } Variant( const Int64& val ) : mType( Type::Int64 ) { mValue.asInt64 = val; } Variant( const Uint64& val ) : mType( Type::Uint64 ) { mValue.asUint64 = val; } explicit Variant( const char* data ) : mType( Type::cstr ) { mValue.asCStr = data; } diff --git a/src/eepp/core/memorymanager.cpp b/src/eepp/core/memorymanager.cpp index dd6057eb7..c0a205d2a 100644 --- a/src/eepp/core/memorymanager.cpp +++ b/src/eepp/core/memorymanager.cpp @@ -23,20 +23,8 @@ void operator delete( void* p ) throw() { } #endif -#ifdef EE_MEMORY_MANAGER -static bool sHasInit = false; -#else -static bool sHasInit = true; -#endif - namespace EE { -static AllocatedPointerMap sMapPointers; -static size_t sTotalMemoryUsage = 0; -static size_t sPeakMemoryUsage = 0; -static AllocatedPointer sBiggestAllocation = AllocatedPointer( NULL, "", 0, 0 ); -static Mutex sAllocMutex; - AllocatedPointer::AllocatedPointer( void* data, const std::string& file, int line, size_t memory, bool track ) { mData = data; @@ -46,6 +34,26 @@ AllocatedPointer::AllocatedPointer( void* data, const std::string& file, int lin mTrack = track; } +namespace { + +struct MemoryManagerState { + AllocatedPointerMap pointers; + size_t totalMemoryUsage{ 0 }; + size_t peakMemoryUsage{ 0 }; + AllocatedPointer biggestAllocation{ NULL, "", 0, 0 }; + Mutex allocationMutex; +}; + +MemoryManagerState& getMemoryManagerState() { + // The tracker must remain valid through process-static destruction. Function-local + // initialization makes its first concurrent use safe; intentionally retaining the state avoids + // reintroducing a static-destruction-order dependency for late eeDelete() calls. + static MemoryManagerState* state = new MemoryManagerState; + return *state; +} + +} // namespace + void* MemoryManager::allocate( size_t size ) { return malloc( size ); } @@ -55,9 +63,11 @@ void* MemoryManager::reallocate( void* ptr, size_t size ) { } void* MemoryManager::addPointerInPlace( void* place, const AllocatedPointer& aAllocatedPointer ) { - AllocatedPointerMapIt it = sMapPointers.find( place ); + auto& state = getMemoryManagerState(); + Lock lock( state.allocationMutex ); + AllocatedPointerMapIt it = state.pointers.find( place ); - if ( it != sMapPointers.end() ) { + if ( it != state.pointers.end() ) { removePointer( place, aAllocatedPointer.mFile.c_str(), aAllocatedPointer.mLine ); } @@ -65,42 +75,40 @@ void* MemoryManager::addPointerInPlace( void* place, const AllocatedPointer& aAl } void* MemoryManager::addPointer( const AllocatedPointer& aAllocatedPointer ) { - ConditionalLock l( sHasInit, &sAllocMutex ); + auto& state = getMemoryManagerState(); + Lock lock( state.allocationMutex ); - sMapPointers.insert( + state.pointers.insert( AllocatedPointerMap::value_type( aAllocatedPointer.mData, aAllocatedPointer ) ); - sTotalMemoryUsage += aAllocatedPointer.mMemory; + state.totalMemoryUsage += aAllocatedPointer.mMemory; - if ( sPeakMemoryUsage < sTotalMemoryUsage ) { - sPeakMemoryUsage = sTotalMemoryUsage; + if ( state.peakMemoryUsage < state.totalMemoryUsage ) { + state.peakMemoryUsage = state.totalMemoryUsage; } - if ( aAllocatedPointer.mMemory > sBiggestAllocation.mMemory ) { - sBiggestAllocation = aAllocatedPointer; + if ( aAllocatedPointer.mMemory > state.biggestAllocation.mMemory ) { + state.biggestAllocation = aAllocatedPointer; } if ( aAllocatedPointer.mTrack ) eePRINTL( "Allocating pointer %p at '%s' %d", aAllocatedPointer.mData, aAllocatedPointer.mFile.c_str(), aAllocatedPointer.mLine ); -#ifdef EE_MEMORY_MANAGER - sHasInit = true; -#endif - return aAllocatedPointer.mData; } void* MemoryManager::reallocPointer( void* data, const AllocatedPointer& aAllocatedPointer ) { - Lock l( sAllocMutex ); + auto& state = getMemoryManagerState(); + Lock lock( state.allocationMutex ); - AllocatedPointerMapIt it = sMapPointers.find( data ); + AllocatedPointerMapIt it = state.pointers.find( data ); - if ( it != sMapPointers.end() && it->second.mTrack ) + if ( it != state.pointers.end() && it->second.mTrack ) eePRINTL( "Realloc pointer %p at '%s' %d", data, aAllocatedPointer.mFile.c_str(), aAllocatedPointer.mLine ); - if ( it == sMapPointers.end() ) + if ( it == state.pointers.end() ) return addPointer( aAllocatedPointer ); if ( aAllocatedPointer.mTrack ) @@ -111,20 +119,20 @@ void* MemoryManager::reallocPointer( void* data, const AllocatedPointer& aAlloca removePointer( data, aAllocatedPointer.mFile.c_str(), aAllocatedPointer.mLine ); addPointer( aAllocatedPointer ); } else { - sTotalMemoryUsage -= it->second.mMemory; + state.totalMemoryUsage -= it->second.mMemory; it->second.mMemory = aAllocatedPointer.mMemory; it->second.mFile = aAllocatedPointer.mFile; it->second.mLine = aAllocatedPointer.mLine; it->second.mTrack = aAllocatedPointer.mTrack; - sTotalMemoryUsage += aAllocatedPointer.mMemory; + state.totalMemoryUsage += aAllocatedPointer.mMemory; - if ( sPeakMemoryUsage < sTotalMemoryUsage ) { - sPeakMemoryUsage = sTotalMemoryUsage; + if ( state.peakMemoryUsage < state.totalMemoryUsage ) { + state.peakMemoryUsage = state.totalMemoryUsage; } - if ( aAllocatedPointer.mMemory > sBiggestAllocation.mMemory ) { - sBiggestAllocation = aAllocatedPointer; + if ( aAllocatedPointer.mMemory > state.biggestAllocation.mMemory ) { + state.biggestAllocation = aAllocatedPointer; } } @@ -132,11 +140,12 @@ void* MemoryManager::reallocPointer( void* data, const AllocatedPointer& aAlloca } bool MemoryManager::removePointer( void* data, const char* file, const size_t& line ) { - Lock l( sAllocMutex ); + auto& state = getMemoryManagerState(); + Lock lock( state.allocationMutex ); - AllocatedPointerMapIt it = sMapPointers.find( data ); + AllocatedPointerMapIt it = state.pointers.find( data ); - if ( it == sMapPointers.end() ) { + if ( it == state.pointers.end() ) { eePRINTL( "Trying to delete pointer %p created that does not exist!", data ); eeASSERT( false ); return false; @@ -145,23 +154,29 @@ bool MemoryManager::removePointer( void* data, const char* file, const size_t& l if ( it->second.mTrack ) eePRINTL( "Deleting pointer %p at '%s' %d", data, file, line ); - sTotalMemoryUsage -= it->second.mMemory; + state.totalMemoryUsage -= it->second.mMemory; - sMapPointers.erase( it ); + state.pointers.erase( it ); return true; } size_t MemoryManager::getPeakMemoryUsage() { - return sPeakMemoryUsage; + auto& state = getMemoryManagerState(); + Lock lock( state.allocationMutex ); + return state.peakMemoryUsage; } size_t MemoryManager::getTotalMemoryUsage() { - return sTotalMemoryUsage; + auto& state = getMemoryManagerState(); + Lock lock( state.allocationMutex ); + return state.totalMemoryUsage; } -const AllocatedPointer& MemoryManager::getBiggestAllocation() { - return sBiggestAllocation; +AllocatedPointer MemoryManager::getBiggestAllocation() { + auto& state = getMemoryManagerState(); + Lock lock( state.allocationMutex ); + return state.biggestAllocation; } void MemoryManager::showResults() { @@ -173,11 +188,13 @@ void MemoryManager::showResults() { } Engine::destroySingleton(); + auto& state = getMemoryManagerState(); + Lock lock( state.allocationMutex ); eePRINTL( "\n|--Memory Manager Report-------------------------------------|" ); eePRINTL( "|" ); - if ( sMapPointers.empty() ) { + if ( state.pointers.empty() ) { eePRINTL( "| No memory leaks detected." ); } else { eePRINTL( "| Memory leaks detected: " ); @@ -186,9 +203,9 @@ void MemoryManager::showResults() { // Get max length of file name int lMax = 0; - AllocatedPointerMapIt it = sMapPointers.begin(); + AllocatedPointerMapIt it = state.pointers.begin(); - for ( ; it != sMapPointers.end(); ++it ) { + for ( ; it != state.pointers.end(); ++it ) { AllocatedPointer& ap = it->second; if ( (int)ap.mFile.length() > lMax ) @@ -204,9 +221,9 @@ void MemoryManager::showResults() { eePRINTL( "|-----------------------------------------------------------|" ); - it = sMapPointers.begin(); + it = state.pointers.begin(); - for ( ; it != sMapPointers.end(); ++it ) { + for ( ; it != state.pointers.end(); ++it ) { AllocatedPointer& ap = it->second; eePRINT( "| %p\t %s", ap.mData, ap.mFile.c_str() ); @@ -220,12 +237,13 @@ void MemoryManager::showResults() { eePRINTL( "|" ); eePRINTL( "| Memory left: %s", - FileSystem::sizeToString( static_cast( sTotalMemoryUsage ) ).c_str() ); + FileSystem::sizeToString( static_cast( state.totalMemoryUsage ) ).c_str() ); eePRINTL( "| Biggest allocation:" ); eePRINTL( "| %s in file: %s at line: %d", - FileSystem::sizeToString( sBiggestAllocation.mMemory ).c_str(), - sBiggestAllocation.mFile.c_str(), sBiggestAllocation.mLine ); - eePRINTL( "| Peak Memory Usage: %s", FileSystem::sizeToString( sPeakMemoryUsage ).c_str() ); + FileSystem::sizeToString( state.biggestAllocation.mMemory ).c_str(), + state.biggestAllocation.mFile.c_str(), state.biggestAllocation.mLine ); + eePRINTL( "| Peak Memory Usage: %s", + FileSystem::sizeToString( state.peakMemoryUsage ).c_str() ); eePRINTL( "|------------------------------------------------------------|\n" ); #endif diff --git a/src/eepp/graphics/texture.cpp b/src/eepp/graphics/texture.cpp index dc85df87b..9cb5d3e61 100644 --- a/src/eepp/graphics/texture.cpp +++ b/src/eepp/graphics/texture.cpp @@ -41,24 +41,6 @@ Texture::Texture() : mFilter( Filter::Linear ), mCoordinateType( CoordinateType::Normalized ) {} -Texture::Texture( const Texture& Copy ) : - DrawableResource( Drawable::TEXTURE, Copy.mName ), - Image(), - mFilepath( Copy.mFilepath ), - mTexture( Copy.mTexture ), - mImgWidth( Copy.mImgWidth ), - mImgHeight( Copy.mImgHeight ), - mFlags( Copy.mFlags ), - mClampMode( Copy.mClampMode ), - mFilter( Copy.mFilter ) { - mWidth = Copy.mWidth; - mHeight = Copy.mHeight; - mChannels = Copy.mChannels; - mSize = Copy.mSize; - - setPixels( reinterpret_cast( &Copy.mPixels[0] ) ); -} - Texture::Texture( const Uint32& texture, const unsigned int& width, const unsigned int& height, const unsigned int& imgwidth, const unsigned int& imgheight, const bool& UseMipmap, const unsigned int& Channels, const std::string& filepath, diff --git a/src/eepp/graphics/textureatlasloader.cpp b/src/eepp/graphics/textureatlasloader.cpp index 5be442416..7709f9201 100644 --- a/src/eepp/graphics/textureatlasloader.cpp +++ b/src/eepp/graphics/textureatlasloader.cpp @@ -102,7 +102,7 @@ TextureAtlasLoader::TextureAtlasLoader( IOStream& IOS, const bool& Threaded, loadFromStream( IOS ); } -TextureAtlasLoader::~TextureAtlasLoader() {} +TextureAtlasLoader::~TextureAtlasLoader() = default; void TextureAtlasLoader::setLoadCallback( GLLoadCallback LoadCallback ) { mLoadCallback = LoadCallback; @@ -115,12 +115,12 @@ sTextureAtlasHdr TextureAtlasLoader::getTextureAtlasHeader() { void TextureAtlasLoader::setTextureFilter( const Texture::Filter& textureFilter ) { mTexGrHdr.TextureFilter = (char)textureFilter; - size_t count = getTextureAtlas()->getTexturesCount() == 0; + if ( NULL == mTextureAtlas ) + return; - if ( count > 0 ) { - for ( size_t i = 0; i < count; i++ ) - getTextureAtlas()->getTexture( i )->setFilter( textureFilter ); - } + const size_t count = mTextureAtlas->getTexturesCount(); + for ( size_t i = 0; i < count; i++ ) + mTextureAtlas->getTexture( i )->setFilter( textureFilter ); } void TextureAtlasLoader::loadFromStream( IOStream& IOS ) { @@ -319,12 +319,12 @@ void TextureAtlasLoader::setThreaded( const bool& threaded ) { mThreaded = threaded; } -const bool& TextureAtlasLoader::isLoaded() const { - return mLoaded; +bool TextureAtlasLoader::isLoaded() const { + return mLoaded.load(); } -const bool& TextureAtlasLoader::isLoading() const { - return mIsLoading; +bool TextureAtlasLoader::isLoading() const { + return mIsLoading.load(); } Texture* TextureAtlasLoader::getTexture( const Uint32& texnum ) const { diff --git a/src/eepp/system/resourceloader.cpp b/src/eepp/system/resourceloader.cpp index 628d55b2f..76cefc359 100644 --- a/src/eepp/system/resourceloader.cpp +++ b/src/eepp/system/resourceloader.cpp @@ -15,9 +15,8 @@ ResourceLoader::ResourceLoader( const Uint32& maxThreads ) : } ResourceLoader::~ResourceLoader() { - clear(); - mThread.wait(); + clear(); } void ResourceLoader::setThreads() { @@ -31,31 +30,35 @@ void ResourceLoader::setThreads() { } bool ResourceLoader::isThreaded() const { - return mThreaded; + return mThreaded.load(); } Uint32 ResourceLoader::getCount() const { + std::unique_lock lock( mMutex ); return mTasks.size(); } void ResourceLoader::setThreaded( const bool& threaded ) { + std::unique_lock lock( mMutex ); if ( !mLoading ) { mThreaded = threaded; } } void ResourceLoader::add( const ObjectLoaderTask& objectLoaderTask ) { + std::unique_lock lock( mMutex ); if ( !mLoading ) { mTasks.emplace_back( objectLoaderTask ); } } bool ResourceLoader::clear() { + std::unique_lock lock( mMutex ); if ( !mLoading ) { mLoaded = false; - mLoading = false; mTotalLoaded = 0; mTasks.clear(); + mLoadCbs.clear(); return true; } @@ -63,75 +66,88 @@ bool ResourceLoader::clear() { } void ResourceLoader::load( const ResLoadCallback& callback ) { - if ( callback ) - mLoadCbs.push_back( callback ); + { + std::unique_lock lock( mMutex ); + if ( callback ) + mLoadCbs.push_back( callback ); + } load(); } void ResourceLoader::load() { - if ( mLoaded ) - return; + bool serialized = false; + { + std::unique_lock lock( mMutex ); + if ( mLoaded || mLoading ) + return; - if ( mThreaded ) { - if ( !mLoading ) { - mLoading = true; + mLoading = true; + if ( mThreaded ) { mThread.launch(); + } else { + serialized = true; } - } else { - serializedLoad(); } + + if ( serialized ) + serializedLoad(); } bool ResourceLoader::isLoaded() { - return mLoaded; + return mLoaded.load(); } bool ResourceLoader::isLoading() { - return mLoading; + return mLoading.load(); } void ResourceLoader::setLoaded() { mLoaded = true; mLoading = false; - if ( mLoadCbs.size() ) { - for ( auto it = mLoadCbs.begin(); it != mLoadCbs.end(); ++it ) { - ( *it )( this ); - } - - mLoadCbs.clear(); + std::vector callbacks; + { + std::unique_lock lock( mMutex ); + callbacks.swap( mLoadCbs ); } + + for ( auto& callback : callbacks ) + callback( this ); } void ResourceLoader::taskRunner() { { auto pool = ThreadPool::createUnique( eemin( mThreads, (Uint32)mTasks.size() ) ); - for ( auto& task : mTasks ) { + for ( auto& task : mTasks ) pool->run( task, [this]( const auto& ) { mTotalLoaded++; } ); - } } - mLoading = false; setLoaded(); } void ResourceLoader::serializedLoad() { - mLoading = true; - for ( auto& task : mTasks ) { task(); mTotalLoaded++; } - mLoading = false; setLoaded(); } Float ResourceLoader::getProgress() { - return mTotalLoaded / (float)mTasks.size() * 100.f; + Uint32 taskCount; + { + std::unique_lock lock( mMutex ); + taskCount = mTasks.size(); + } + + if ( taskCount == 0 ) + return mLoaded ? 100.f : 0.f; + + return mTotalLoaded / (float)taskCount * 100.f; } }} // namespace EE::System diff --git a/src/tests/unit_tests/resource_prerequisite_tests.cpp b/src/tests/unit_tests/resource_prerequisite_tests.cpp new file mode 100644 index 000000000..06ef5240d --- /dev/null +++ b/src/tests/unit_tests/resource_prerequisite_tests.cpp @@ -0,0 +1,175 @@ +#include "utest.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace EE; +using namespace EE::Graphics; +using namespace EE::UI::Models; +using namespace EE::Window; + +namespace { + +struct LoaderGate { + std::mutex mutex; + std::condition_variable condition; + bool started{ false }; + bool release{ false }; + + void run() { + std::unique_lock lock( mutex ); + started = true; + condition.notify_all(); + condition.wait( lock, [this] { return release; } ); + } + + void waitUntilStarted() { + std::unique_lock lock( mutex ); + condition.wait( lock, [this] { return started; } ); + } + + void finish() { + std::unique_lock lock( mutex ); + release = true; + condition.notify_all(); + } +}; + +class TestTextureAtlas : public TextureAtlas { + public: + using TextureAtlas::setTextures; +}; + +class TestTextureAtlasLoader : public TextureAtlasLoader { + public: + void setTextureAtlas( TextureAtlas* textureAtlas ) { mTextureAtlas = textureAtlas; } + + void loadDelayed( const std::shared_ptr& gate, + const std::shared_ptr>& callbackCompleted ) { + mRL.setThreaded( true ); + mRL.add( [gate] { gate->run(); } ); + mRL.add( [] {} ); + mRL.load( [this, callbackCompleted]( ResourceLoader* ) { + mTempAtlass.emplace_back(); + *callbackCompleted = true; + } ); + } +}; + +} // namespace + +static_assert( !std::is_copy_constructible::value, "Texture must not be copyable" ); +static_assert( !std::is_copy_assignable::value, "Texture must not be copy-assignable" ); + +UTEST( ResourcePrerequisites, textureAtlasLoaderAppliesFilterToEveryTexture ) { + Engine::instance()->createWindow( WindowSettings( 64, 64, "TextureAtlasLoader filter test", + WindowStyle::Default, WindowBackend::Default, + 32, {}, 1, false, true ), + ContextSettings( false, 0, 0, GLv_default, true, false ) ); + + { + Texture* first = TextureFactory::instance()->createEmptyTexture( 1, 1 ); + Texture* second = TextureFactory::instance()->createEmptyTexture( 1, 1 ); + ASSERT_TRUE( first != NULL ); + ASSERT_TRUE( second != NULL ); + + TestTextureAtlas atlas; + atlas.setTextures( { first, second } ); + + TestTextureAtlasLoader loader; + loader.setTextureAtlas( &atlas ); + loader.setTextureFilter( Texture::Filter::Nearest ); + + EXPECT_EQ( first->getFilter(), Texture::Filter::Nearest ); + EXPECT_EQ( second->getFilter(), Texture::Filter::Nearest ); + } + + Engine::destroySingleton(); +} + +UTEST( ResourcePrerequisites, textureAtlasLoaderAcceptsFilterBeforeAtlasExists ) { + TestTextureAtlasLoader loader; + loader.setTextureFilter( Texture::Filter::Nearest ); + EXPECT_EQ( static_cast( loader.getTextureAtlasHeader().TextureFilter ), + Texture::Filter::Nearest ); +} + +UTEST( ResourcePrerequisites, unsignedVariantPreservesTypeAndValue ) { + const unsigned int value = std::numeric_limits::max(); + Variant original( value ); + + ASSERT_TRUE( original.is( Variant::Type::Uint ) ); + EXPECT_EQ( original.asUint(), value ); + EXPECT_TRUE( original.toString() == std::to_string( value ) ); + + Variant copied( original ); + EXPECT_TRUE( copied.is( Variant::Type::Uint ) ); + EXPECT_EQ( copied.asUint(), value ); + + Variant moved( std::move( copied ) ); + EXPECT_TRUE( moved.is( Variant::Type::Uint ) ); + EXPECT_EQ( moved.asUint(), value ); + EXPECT_TRUE( !copied.isValid() ); + + Variant copyAssigned; + copyAssigned = original; + EXPECT_TRUE( copyAssigned.is( Variant::Type::Uint ) ); + EXPECT_EQ( copyAssigned.asUint(), value ); + + Variant moveAssigned; + moveAssigned = std::move( copyAssigned ); + EXPECT_TRUE( moveAssigned.is( Variant::Type::Uint ) ); + EXPECT_EQ( moveAssigned.asUint(), value ); + EXPECT_TRUE( !copyAssigned.isValid() ); +} + +UTEST( ResourcePrerequisites, emptyResourceLoaderHasDefinedProgress ) { + ResourceLoader loader; + loader.setThreaded( false ); + + EXPECT_EQ( loader.getProgress(), 0.f ); + + loader.load(); + + EXPECT_TRUE( loader.isLoaded() ); + EXPECT_EQ( loader.getProgress(), 100.f ); +} + +UTEST( ResourcePrerequisites, resourceLoaderReportsPartialAndCompleteProgress ) { + ResourceLoader loader; + loader.setThreaded( false ); + Float progressDuringSecondTask = 0.f; + + loader.add( [] {} ); + loader.add( [&] { progressDuringSecondTask = loader.getProgress(); } ); + loader.load(); + + EXPECT_EQ( progressDuringSecondTask, 50.f ); + EXPECT_EQ( loader.getProgress(), 100.f ); +} + +UTEST( ResourcePrerequisites, textureAtlasLoaderWaitsBeforeDestroyingCallbackState ) { + auto gate = std::make_shared(); + auto callbackCompleted = std::make_shared>( false ); + auto loader = std::make_unique(); + loader->loadDelayed( gate, callbackCompleted ); + gate->waitUntilStarted(); + + std::thread destroyThread( [loader = std::move( loader )]() mutable { loader.reset(); } ); + gate->finish(); + destroyThread.join(); + + EXPECT_TRUE( *callbackCompleted ); +}