ThreadPool now working with no memory leaks

.vscode/launch.json

@@ -35,6 +35,7 @@
             "environment": [],
             "symbolSearchPath": "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Community\\VC\\Tools\\MSVC\\14.25.28610\\lib\\x64",
             "requireExactSource": false,
+            "visualizerFile": "${workspaceRoot}/Seele.natvis",
             "logging": {
                 "moduleLoad": false,
                 "exceptions": true,

CMakeLists.txt

@@ -75,6 +75,7 @@ if(WIN32)
 endif()
 if(CMAKE_DEBUG_POSTFIX)
   add_compile_definitions(ENABLE_VALIDATION)
+  add_compile_definitions(SEELE_DEBUG)
 endif()
 add_executable(SeeleEngine "")
 target_link_libraries(SeeleEngine ${Vulkan_LIBRARY})

Seele.natvis

@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="utf-8"?>
+<AutoVisualizer xmlns="http://schemas.microsoft.com/vstudio/debugger/natvis/2010">
+  <Type Name="Seele::Array&lt;*&gt;">
+    <DisplayString>{{ size={arraySize} }}</DisplayString>
+    <Expand>
+      <Item Name="[size]" ExcludeView="simple">arraySize</Item>
+      <Item Name="[allocated]" ExcludeView="simple">allocated</Item>
+      <ArrayItems>
+        <Size>arraySize</Size>
+        <ValuePointer>_data</ValuePointer>
+      </ArrayItems>
+    </Expand>
+  </Type>
+  <Type Name="Seele::List&lt;*&gt;">
+    <DisplayString>{{ size={_size} }}</DisplayString>
+    <Expand>
+      <Item Name="[size]" ExcludeView="simple">_size</Item>
+      <LinkedListItems>
+        <Size>_size</Size>
+        <HeadPointer>root</HeadPointer>
+        <NextPointer>next</NextPointer>
+        <ValueNode>data</ValueNode>
+      </LinkedListItems>
+    </Expand>
+  </Type>
+  <Type Name="Seele::Map&lt;*&gt;">
+    <DisplayString>{{size={_size}}</DisplayString>
+    <Expand>
+      <Item Name="[size]">_size</Item>
+      <Item Name="[comp]">comp</Item>
+      <ArrayItems>
+        <Size>nodeContainer.arraySize</Size>
+        <ValuePointer>nodeContainer._data</ValuePointer>
+      </ArrayItems>
+    </Expand>
+  </Type>
+  <Type Name="Seele::RefPtr&lt;*&gt;">
+    <DisplayString>RefPtr {*object->handle} {object->refCount.load()} references</DisplayString>
+    <Expand>
+      <ExpandedItem>object->handle</ExpandedItem>
+    </Expand>
+  </Type>
+</AutoVisualizer>

src/Engine/Containers/Array.natvis

@@ -1,13 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>
-<AutoVisualizer xmlns="http://schemas.microsoft.com/vstudio/debugger/natvis/2010">
-  <Type Name="Seele::Array&lt;*&gt;">
-    <DisplayString>{{ size={arraySize} }}</DisplayString>
-    <Expand>
-        <Item Name="[size]" ExcludeView="simple">arraySize</Item>
-        <Item Name="[allocated]" ExcludeView="simple">allocated</Item>
-        <ArrayItems>
-            <Size>arraySize</Size>
-            <ValuePointer>_data</ValuePointer>
-        </ArrayItems>
-  </Type>
-</AutoVisualizer>

src/Engine/Containers/CMakeLists.txt

@@ -1,6 +1,5 @@
 target_sources(SeeleEngine
 	PRIVATE
-		Array.natvis
 		Array.h
 		Map.h
 		List.h)

src/Engine/Scene/Components/MyComponent.cpp

@@ -14,13 +14,17 @@ void MyComponent::start()
 
 Job MyComponent::tick(float deltatime) const
 {
-    writable++;
+    //std::cout << "MyComponent::tick" << std::endl;
+    ++writable;
+    //std::cout << "MyComponent::tick finished" << std::endl;
     otherComp->data = *writable;
     co_return;
 }
 
 Job MyComponent::update() 
 {
+    //std::cout << "MyComponent::update" << std::endl;
     writable.update();
+    //std::cout << "MyComponent::update finished" << std::endl;
     co_return;
 }

src/Engine/Scene/Components/MyComponent.h

@@ -13,7 +13,7 @@ public:
     virtual Job update();
 private:
     Writable<PMyOtherComponent> otherComp;
-    Writable<uint32> writable = Writable<uint32>(0);
+    Writable<uint32> writable = 0;
     uint32 notWritable = 10;
 };
 DECLARE_REF(MyComponent);

src/Engine/Scene/Scene.cpp

@@ -43,18 +43,30 @@ void Scene::start()
 
 Job Scene::beginUpdate(double deltaTime)
 {
+    //std::cout << "Scene::beginUpdate" << std::endl;
+    Array<Job> jobs;
     for(auto actor : rootActors)
     {
-        co_await Job::all(actor->launchTick(static_cast<float>(deltaTime)));
+        jobs.addAll(actor->launchTick(static_cast<float>(deltaTime)));
+    }
+    co_await Job::all(jobs);
+    //std::cout << "Scene::beginUpdate finished waiting" << std::endl;
+    for(auto job : jobs)
+    {
+        assert(job.done());
     }
 }
 
 Job Scene::commitUpdate() 
 {
+    //std::cout << "Scene::commitUpdate" << std::endl;
+    Array<Job> jobs;
     for(auto actor : rootActors)
     {
-        co_await Job::all(actor->launchUpdate());
+        jobs.addAll(actor->launchUpdate());
     }
+    co_await Job::all(jobs);
+    //std::cout << "Scene::commitUpdate finished waiting" << std::endl;
 }
 
 void Scene::addActor(PActor actor)

src/Engine/Scene/Util.h

@@ -9,15 +9,15 @@ class Writable
 public:
     Writable()
     {}
-    explicit Writable(T initialData)
+    Writable(T initialData)
         : data(initialData)
         , toBeWritten(initialData)
     {}
     Writable(const Writable& other) = delete;
     Writable(Writable&& other) = default;
     ~Writable() = default;
-    Writable& operator=(const Writable& other) = delete;
+    Writable& operator=(const Writable& other) const = delete;
-    Writable& operator=(Writable&& other) = default;
+    Writable& operator=(Writable&& other) const = delete;
     const Writable& operator=(const T& other) const
     {
         deferWrite(other);

src/Engine/ThreadPool.cpp

@@ -6,7 +6,7 @@ using namespace Seele;
 std::atomic_uint64_t Seele::globalCounter;
 
 Event::Event()
-    : flag(std::make_shared<std::atomic_bool>())
+    : flag(std::make_shared<StateStore>())
 {
 }
 
@@ -17,23 +17,31 @@ Event::Event(nullptr_t)
 
 Event::Event(const std::string &name)
     : name(name)
-    , flag(std::make_shared<std::atomic_bool>())
+    , flag(std::make_shared<StateStore>())
 {
 }
 
 void Event::raise()
 {
-    flag->store(1);
+    std::scoped_lock lock(flag->lock);
+    flag->data = 1;
     getGlobalThreadPool().notify(*this);
 }
 void Event::reset()
 {
-    flag->store(0);
+    std::scoped_lock lock(flag->lock);
+    flag->data = 0;
 }
 
 bool Event::await_ready()
 {
-    return flag->load();
+    flag->lock.lock();
+    bool result = flag->data;
+    if(result)
+    {
+        flag->lock.unlock();
+    }
+    return result;
 }
 
 ThreadPool::ThreadPool(uint32 threadCount)
@@ -48,7 +56,15 @@ ThreadPool::ThreadPool(uint32 threadCount)
 
 ThreadPool::~ThreadPool()
 {
-    running.store(false);
+    cleanup();
+}
+
+void ThreadPool::cleanup()
+{
+    bool temp = true;
+    running.compare_exchange_strong(temp, false);
+    if(!temp)
+        return;
     {
         std::scoped_lock lock(jobQueueLock);
         jobQueueCV.notify_all();
@@ -71,6 +87,7 @@ void ThreadPool::enqueueWaiting(Event &event, Promise* job)
     std::scoped_lock lock(waitingLock);
     //std::cout << "Job " << job->finishedEvent.name << " waiting on event " << event.name << std::endl;
     waitingJobs[event].push_back(job);
+    job->addRef();
 }
 void ThreadPool::enqueueWaiting(Event &event, MainPromise* job)
 {
@@ -78,6 +95,7 @@ void ThreadPool::enqueueWaiting(Event &event, MainPromise* job)
     std::scoped_lock lock(waitingMainLock);
     //std::cout << job->finishedEvent.name << " waiting on event " << event.name << std::endl;
     waitingMainJobs[event].push_back(job);
+    job->addRef();
 }
 void ThreadPool::scheduleJob(Promise* job)
 {
@@ -86,6 +104,7 @@ void ThreadPool::scheduleJob(Promise* job)
     //std::cout << "Queueing job " << job->finishedEvent.name << std::endl;
     jobQueue.push_back(job);
     jobQueueCV.notify_one();
+    job->addRef();
 }
 void ThreadPool::scheduleJob(MainPromise* job)
 {
@@ -94,6 +113,7 @@ void ThreadPool::scheduleJob(MainPromise* job)
     //std::cout << "Queueing job " << job->finishedEvent.name << std::endl;
     mainJobs.push_back(job);
     mainJobCV.notify_one();
+    job->addRef();
 }
 void ThreadPool::notify(Event &event)
 {
@@ -149,10 +169,7 @@ void ThreadPool::threadLoop(const bool mainThread)
                 }
             }
             job->resume();
-            if (job->done())
+            job->removeRef();
-            {
-                job->finalize();
-            }
         }
         else
         {
@@ -175,14 +192,10 @@ void ThreadPool::threadLoop(const bool mainThread)
             }
             //std::cout << "Starting job " << job.id << std::endl;
             job->resume();
-            if(job->done())
+            job->removeRef();
-            {
-                job->finalize();
-            }
         }
     }
 }
-
 ThreadPool &Seele::getGlobalThreadPool()
 {
     static ThreadPool threadPool;

src/Engine/ThreadPool.h

@@ -31,7 +31,8 @@ public:
     }
     operator bool()
     {
-        return flag->load();
+        std::scoped_lock lock(flag->lock);
+        return flag->data;
     }
     
     void raise();
@@ -42,7 +43,12 @@ public:
     constexpr void await_resume() {}
 private:
     std::string name;
-    std::shared_ptr<std::atomic_bool> flag;
+    struct StateStore
+    {
+        std::mutex lock;
+        bool data;
+    };
+    std::shared_ptr<StateStore> flag;
     friend class ThreadPool;
 };
 
@@ -67,25 +73,21 @@ struct JobPromiseBase
         finishedEvent = Event(std::format("Job {}", id));
     }
     ~JobPromiseBase()
-    {
+    {}
-        if (handle)
-        {
-            handle.destroy();
-        }
-    }
     JobBase<MainJob> get_return_object() noexcept;
 
     inline auto initial_suspend() noexcept;
-    inline auto final_suspend() const noexcept;
+    inline auto final_suspend() noexcept;
 
     void return_void() noexcept {}
     void unhandled_exception() noexcept {
-        std::cerr << "Unhandled exception" << std::endl;
+        std::cerr << "Unhandled exception! Exiting" << std::endl;
         exit(1);
     };
 
     void resume()
     {
+        std::scoped_lock lock(promiseLock);
         if(!handle || handle.done() || executing())
         {
             return;
@@ -93,15 +95,26 @@ struct JobPromiseBase
         state = State::EXECUTING;
         handle.resume();
     }
+    void markDone() 
+    {
+        state = State::DONE;
+        finishedEvent.raise();
+        if(continuation)
+        {
+            getGlobalThreadPool().scheduleJob(continuation);
+            continuation->removeRef();
+        }
+    }
     void setContinuation(JobPromiseBase* cont)
     {
         std::scoped_lock lock(promiseLock, cont->promiseLock);
-        continuation = cont->handle;
+        continuation = cont;
         cont->state = State::SCHEDULED;
+        cont->addRef();
     }
     bool done()
     {
-        return handle.done();
+        return state == State::DONE;
     }
     bool scheduled()
     {
@@ -119,12 +132,6 @@ struct JobPromiseBase
     {
         return state == State::READY;
     }
-    void finalize()
-    {
-        std::scoped_lock lock(promiseLock);
-        state = State::DONE;
-        finishedEvent.raise();
-    }
     void reset()
     {
         std::scoped_lock lock(promiseLock);
@@ -132,7 +139,6 @@ struct JobPromiseBase
     }
     void enqueue(Event& event)
     {
-        // no need to lock here, as it is only called while executing, where the lock is already held
         if(!handle || handle.done() || waiting() || scheduled())
         {
             return;
@@ -142,6 +148,7 @@ struct JobPromiseBase
     }
     void schedule()
     {
+        //dont lock here, as it is already locked from outside
         std::scoped_lock lock(promiseLock);
         if(!handle || done() || !ready())
         {
@@ -149,11 +156,23 @@ struct JobPromiseBase
         }
         getGlobalThreadPool().scheduleJob(this);
     }
+    void addRef()
+    {
+        numRefs++;
+    }
+    void removeRef()
+    {
+        if(--numRefs < 1 && done())
+        {
+            handle.destroy();
+        }
+    }
 
     std::mutex promiseLock;
     std::coroutine_handle<JobPromiseBase> handle;
-    std::coroutine_handle<> continuation = std::noop_coroutine();
+    JobPromiseBase* continuation = nullptr;
     uint64 id;
+    std::atomic_uint64_t numRefs = 0;
     Event finishedEvent;
     State state = State::READY;
 };
@@ -172,13 +191,46 @@ public:
         : promise(promise)
     {
     }
+    JobBase(const JobBase& other)
+    {
+        std::scoped_lock lock(other.promise->promiseLock);
+        promise = other.promise;
+        promise->addRef();
+    }
+    JobBase(JobBase&& other)
+    {
+        std::scoped_lock lock(other.promise->promiseLock);
+        promise = other.promise;
+        other.promise = nullptr;
+    }
     ~JobBase()
     {
         if(promise)
         {
             promise->schedule();
+            promise->removeRef();
         }
     }
+    JobBase& operator=(const JobBase& other)
+    {
+        if(this != &other)
+        {
+            std::scoped_lock lock(other.promise->promiseLock);
+            promise = other.promise;
+            promise->addRef();
+        }
+        return *this;
+    }
+    JobBase& operator=(JobBase&& other)
+    {
+        if(this != &other)
+        {
+            std::scoped_lock lock(other.promise->promiseLock);
+            promise = other.promise;
+            other.promise = nullptr;
+        }
+        return *this;
+    }
     void resume()
     {
         promise->resume();
@@ -203,23 +255,32 @@ public:
     }
     Event operator co_await()
     {
+        Event result = promise->finishedEvent;
+        // if we schedule the promise now, it might finish and self destruct
+        // but there is no way for us to know that
+        // but as the co_await operator keeps a reference to this, we it won't 
+        // be scheduled from the destructor
         promise->schedule();
-        return promise->finishedEvent;
+        // so we reset the promise so we don't schedule it when this destructs
+        promise->removeRef();
+        promise = nullptr;
+        // but we still have to provide with the event, so we get that first
+        return result;
     }
     static JobBase all() = delete;
-    template<typename... Awaitable>
-    static JobBase all(Awaitable... jobs)
-    {
-        (co_await jobs, ...);
-    }
     template<iterable Iterable>
-    static JobBase all(Iterable&& collection)
+    static JobBase all(Iterable collection)
     {
-        for(auto&& it : collection)
+        for(auto it : collection)
         {
             co_await it;
         }
     }
+    template<typename... Awaitable>
+    static JobBase all(Awaitable&&... jobs)
+    {
+        return JobBase::all(Array{jobs...});
+    }
     template<std::invocable JobFunc, iterable IterableParams>
     static JobBase launchJobs(JobFunc&& func, IterableParams params)
     {
@@ -245,8 +306,9 @@ using Promise = JobPromiseBase<false>;
 class ThreadPool
 {
 public:
-    ThreadPool(uint32 threadCount = 1);
+    ThreadPool(uint32 threadCount = std::thread::hardware_concurrency());
     virtual ~ThreadPool();
+    void cleanup();
     // Adds a job to the waiting queue for event
     void enqueueWaiting(Event& event, Promise* job);
     // Adds a job to the waiting queue for event
@@ -277,6 +339,7 @@ private:
 template<bool MainJob>
 inline JobBase<MainJob> JobPromiseBase<MainJob>::get_return_object() noexcept 
 {
+    numRefs++;
     return JobBase<MainJob>(this);
 }
 
@@ -287,24 +350,29 @@ inline auto JobPromiseBase<MainJob>::initial_suspend() noexcept
 }
 
 template<bool MainJob>
-inline auto JobPromiseBase<MainJob>::final_suspend() const noexcept
+inline auto JobPromiseBase<MainJob>::final_suspend() noexcept
 {
-    struct JobAwaitable
+    /*struct JobAwaitable
     {
         constexpr bool await_ready() const noexcept { return false; }
         constexpr auto await_suspend(std::coroutine_handle<JobPromiseBase<MainJob>> h) const noexcept
         {
-            return h.promise().continuation;
+            auto continuation = h.promise().continuation;
+            h.destroy();
+
         }
         constexpr void await_resume() const noexcept {}
     };
-    return JobAwaitable{};
+    return JobAwaitable{};*/
+    markDone();
+    return std::suspend_always{};
 }
 
 template<bool MainJob>
 inline constexpr void Event::await_suspend(std::coroutine_handle<JobPromiseBase<MainJob>> h)
 {
     h.promise().enqueue(*this);
+    flag->lock.unlock();
 }
 
 } // namespace Seele

src/Engine/Window/SceneView.cpp

@@ -40,12 +40,15 @@ Seele::SceneView::SceneView(Gfx::PGraphics graphics, PWindow owner, const Viewpo
     plane->setWorldScale(Vector(100, 100, 100));
     scene->addPrimitiveComponent(plane);
 
-    PMyComponent myComp = new MyComponent();
+    for(uint32 i = 0; i < 1; ++i)
-    PMyOtherComponent myOtherComp = new MyOtherComponent();
+    {
-    PActor actor = new Actor();
+        PMyComponent myComp = new MyComponent();
-    actor->setRootComponent(myComp);
+        PMyOtherComponent myOtherComp = new MyOtherComponent();
-    myComp->addChildComponent(myOtherComp);
+        PActor actor = new Actor();
-    scene->addActor(actor);
+        actor->setRootComponent(myComp);
+        myComp->addChildComponent(myOtherComp);
+        scene->addActor(actor);
+    }
     scene->start();
     
     PRenderGraphResources resources = new RenderGraphResources();

src/Engine/Window/Window.cpp

@@ -75,8 +75,8 @@ void Window::setFocused(PView view)
 Job Window::viewWorker(size_t viewIndex)
 {
     WindowView* windowView = views[viewIndex];
-    windowView->view->beginUpdate();
+    co_await windowView->view->beginUpdate();
-    windowView->view->update();
+    co_await windowView->view->update();
     {
         std::scoped_lock lock(windowView->workerMutex);
         windowView->view->commitUpdate();

test/Engine/EngineTest.h

@@ -1,5 +1,5 @@
 #pragma once
-#include <vld.h>
+//#include <vld.h>
 #include "ThreadPool.h"
 
 namespace Seele
@@ -8,10 +8,11 @@ namespace Seele
     {
         GlobalFixture()
         {
-            _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
+            //_CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
         }
         ~GlobalFixture()
         {
+            getGlobalThreadPool().cleanup();
         }
         void setup()
         {

test/Engine/ThreadPool.cpp

@@ -4,51 +4,107 @@
 
 BOOST_AUTO_TEST_SUITE(ThreadPool)
 
-uint64 basicJobState = 0;
+uint64 basicAwaitState = 0;
+
+Job basicAwaitFirst()
+{
+    BOOST_REQUIRE_EQUAL(basicAwaitState, 5);
+    basicAwaitState = 10;
+    co_return;
+}
+Job basicAwaitSecond()
+{
+    BOOST_REQUIRE_EQUAL(basicAwaitState, 15);
+    basicAwaitState = 20;
+    co_return;
+}
+Job basicAwaitThird()
+{
+    BOOST_REQUIRE_EQUAL(basicAwaitState, 25);
+    basicAwaitState = 30;
+    co_return;
+}
+
+Job basicAwaitBase()
+{
+    basicAwaitState = 5;
+    co_await basicAwaitFirst();
+    BOOST_REQUIRE_EQUAL(basicAwaitState, 10);
+    basicAwaitState = 15;
+    co_await basicAwaitSecond();
+    BOOST_REQUIRE_EQUAL(basicAwaitState, 20);
+    basicAwaitState = 25;
+    co_await basicAwaitThird();
+    BOOST_REQUIRE_EQUAL(basicAwaitState, 30);
+}
+
+BOOST_AUTO_TEST_CASE(basic_coawait)
+{
+    basicAwaitBase();
+}
+
+uint64 basicThenState = 5;
 
 Job basicThenFirst()
 {
-    basicJobState = 10;
+    BOOST_REQUIRE_EQUAL(basicThenState, 5);
+    basicThenState = 10;
     co_return;
 }
 Job basicThenSecond()
 {
-    BOOST_REQUIRE_EQUAL(basicJobState, 10);
+    BOOST_REQUIRE_EQUAL(basicThenState, 15);
-    basicJobState = 20;
+    basicThenState = 20;
     co_return;
 }
 Job basicThenThird()
 {
-    BOOST_REQUIRE_EQUAL(basicJobState, 20);
+    BOOST_REQUIRE_EQUAL(basicThenState, 25);
+    basicThenState = 30;
     co_return;
 }
 
-Job basicThenBase()
+BOOST_AUTO_TEST_CASE(basic_thenchain)
 {
-    co_await basicThenFirst();
+    basicThenFirst()
-    co_await basicThenSecond();
+        .then([]() -> Job
-    co_await basicThenThird();
+        {
+            BOOST_REQUIRE_EQUAL(basicThenState, 10);
+            basicThenState = 15;
+            co_return;
+        })
+        .then(basicThenSecond())
+        .then([]() -> Job
+        {
+            BOOST_REQUIRE_EQUAL(basicThenState, 20);
+            basicThenState = 25;
+            co_return;
+        })
+        .then(basicThenThird())
+        .then([]() -> Job
+        {
+            BOOST_REQUIRE_EQUAL(basicThenState, 30);
+            co_return;
+        });
 }
 
-BOOST_AUTO_TEST_CASE(basic_then)
-{
-    basicThenBase();
-}
-/*
 uint64 basicAllState1 = 0;
 uint64 basicAllState2 = 0;
 Job basicAllFirst()
 {
     basicAllState1 = 10;
+    std::cout << "AllFirst" << std::endl;
     co_return;
 }
 Job basicAllSecond()
 {
     basicAllState2 = 10;
+    std::cout << "AllSecond" << std::endl;
     co_return;
 }
 Job basicAllThen()
 {
+    std::cout << "AllThen" << std::endl;
     BOOST_REQUIRE_EQUAL(basicAllState1, 10);
     BOOST_REQUIRE_EQUAL(basicAllState2, 10);
     co_return;
@@ -70,10 +126,12 @@ Job basicCallableFunc()
 BOOST_AUTO_TEST_CASE(basic_callable)
 {
     basicCallableFunc()
-        .then([=]() -> Job{
+        .then([=]() -> Job
+        {
             BOOST_REQUIRE_EQUAL(basicCallable, 10);
+            std::cout << "callable" << std::endl;
             co_return;
         });
-}*/
+}
 
 BOOST_AUTO_TEST_SUITE_END()