From 8a9cbcfb99f21a65bc469df673afebefe7abb254 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 25 Oct 2023 22:10:51 -0700 Subject: [PATCH] fix a Transform component leak in CameraManager CameraManager creates a Transform component for each Camera component is not already present. However, it didn't destroy the transform component when it's itself destroyed. the leaked transform component would eventually be garbage collected, but caused significant slow down and memory pressure. This is because camera components are created every frame for the shadow maps. FIXES=[303914944] --- filament/src/components/CameraManager.cpp | 68 +++++++++++++---------- filament/src/components/CameraManager.h | 15 +++-- filament/src/details/Engine.cpp | 10 ++-- 3 files changed, 52 insertions(+), 41 deletions(-) diff --git a/filament/src/components/CameraManager.cpp b/filament/src/components/CameraManager.cpp index 21dd9e81d0..a34b376d48 100644 --- a/filament/src/components/CameraManager.cpp +++ b/filament/src/components/CameraManager.cpp @@ -23,70 +23,82 @@ #include #include -#include - using namespace utils; using namespace filament::math; namespace filament { -FCameraManager::FCameraManager(FEngine& engine) noexcept - : mEngine(engine) { +FCameraManager::FCameraManager(FEngine&) noexcept { } -FCameraManager::~FCameraManager() noexcept { -} +FCameraManager::~FCameraManager() noexcept = default; -void FCameraManager::terminate() noexcept { +void FCameraManager::terminate(FEngine& engine) noexcept { auto& manager = mManager; if (!manager.empty()) { #ifndef NDEBUG slog.d << "cleaning up " << manager.getComponentCount() << " leaked Camera components" << io::endl; #endif - while (!manager.empty()) { - Instance const ci = manager.end() - 1; - destroy(manager.getEntity(ci)); + utils::Slice const entities{ manager.getEntities(), manager.getComponentCount() }; + for (Entity const e : entities) { + destroy(engine, e); } } } -void FCameraManager::gc(utils::EntityManager& em) noexcept { +void FCameraManager::gc(FEngine& engine, utils::EntityManager& em) noexcept { auto& manager = mManager; - manager.gc(em, 4, [this](Entity e) { - destroy(e); + manager.gc(em, 4, [this, &engine](Entity e) { + destroy(engine, e); }); } -FCamera* FCameraManager::create(Entity entity) { - FEngine& engine = mEngine; +FCamera* FCameraManager::create(FEngine& engine, Entity entity) { auto& manager = mManager; + // if this entity already has Camera component, destroy it. if (UTILS_UNLIKELY(manager.hasComponent(entity))) { - destroy(entity); + destroy(engine, entity); } + + // add the Camera component to the entity Instance const i = manager.addComponent(entity); - FCamera* camera = engine.getHeapAllocator().make(engine, entity); + // For historical reasons, FCamera must not move. So the CameraManager stores a pointer. + FCamera* const camera = engine.getHeapAllocator().make(engine, entity); manager.elementAt(i) = camera; + manager.elementAt(i) = false; // Make sure we have a transform component - FTransformManager& transformManager = engine.getTransformManager(); - if (!transformManager.hasComponent(entity)) { - transformManager.create(entity); + FTransformManager& tcm = engine.getTransformManager(); + if (!tcm.hasComponent(entity)) { + tcm.create(entity); + manager.elementAt(i) = true; } return camera; } -void FCameraManager::destroy(Entity e) noexcept { +void FCameraManager::destroy(FEngine& engine, Entity e) noexcept { auto& manager = mManager; - Instance const i = manager.getInstance(e); - if (i) { - FCamera* camera = manager.elementAt(i); - assert_invariant(camera); - camera->terminate(mEngine); - mEngine.getHeapAllocator().destroy(camera); - manager.removeComponent(e); + if (Instance const i = manager.getInstance(e) ; i) { + // destroy the FCamera object + bool const ownsTransformComponent = manager.elementAt(i); + + { // scope for camera -- it's invalid after this scope. + FCamera* const camera = manager.elementAt(i); + assert_invariant(camera); + camera->terminate(engine); + engine.getHeapAllocator().destroy(camera); + + // Remove the camera component + manager.removeComponent(e); + } + + // if we added the transform component, remove it. + if (ownsTransformComponent) { + engine.getTransformManager().destroy(e); + } } } diff --git a/filament/src/components/CameraManager.h b/filament/src/components/CameraManager.h index c6753f9553..239790a399 100644 --- a/filament/src/components/CameraManager.h +++ b/filament/src/components/CameraManager.h @@ -44,9 +44,9 @@ public: ~FCameraManager() noexcept; // free-up all resources - void terminate() noexcept; + void terminate(FEngine& engine) noexcept; - void gc(utils::EntityManager& em) noexcept; + void gc(FEngine& engine, utils::EntityManager& em) noexcept; /* * Component Manager APIs @@ -64,25 +64,24 @@ public: return mManager.elementAt(i); } - FCamera* create(utils::Entity entity); + FCamera* create(FEngine& engine, utils::Entity entity); - void destroy(utils::Entity e) noexcept; + void destroy(FEngine& engine, utils::Entity e) noexcept; private: enum { - CAMERA + CAMERA, + OWNS_TRANSFORM_COMPONENT }; - using Base = utils::SingleInstanceComponentManager; + using Base = utils::SingleInstanceComponentManager; struct CameraManagerImpl : public Base { using Base::gc; using Base::swap; using Base::hasComponent; } mManager; - - FEngine& mEngine; }; } // namespace filament diff --git a/filament/src/details/Engine.cpp b/filament/src/details/Engine.cpp index a02e524539..84bb55f4a0 100644 --- a/filament/src/details/Engine.cpp +++ b/filament/src/details/Engine.cpp @@ -424,7 +424,7 @@ void FEngine::shutdown() { mDFG.terminate(*this); // free-up the DFG mRenderableManager.terminate(); // free-up all renderables mLightManager.terminate(); // free-up all lights - mCameraManager.terminate(); // free-up all cameras + mCameraManager.terminate(*this); // free-up all cameras driver.destroyRenderPrimitive(mFullScreenTriangleRph); destroy(mFullScreenTriangleIb); @@ -537,7 +537,7 @@ void FEngine::gc() { mRenderableManager.gc(em); mLightManager.gc(em); mTransformManager.gc(em); - mCameraManager.gc(em); + mCameraManager.gc(*this, em); } void FEngine::flush() { @@ -828,7 +828,7 @@ FSwapChain* FEngine::createSwapChain(uint32_t width, uint32_t height, uint64_t f FCamera* FEngine::createCamera(Entity entity) noexcept { - return mCameraManager.create(entity); + return mCameraManager.create(*this, entity); } FCamera* FEngine::getCameraComponent(Entity entity) noexcept { @@ -837,7 +837,7 @@ FCamera* FEngine::getCameraComponent(Entity entity) noexcept { } void FEngine::destroyCameraComponent(utils::Entity entity) noexcept { - mCameraManager.destroy(entity); + mCameraManager.destroy(*this, entity); } @@ -1053,7 +1053,7 @@ void FEngine::destroy(Entity e) { mRenderableManager.destroy(e); mLightManager.destroy(e); mTransformManager.destroy(e); - mCameraManager.destroy(e); + mCameraManager.destroy(*this, e); } bool FEngine::isValid(const FBufferObject* p) {