bombomby / optick

C++ Profiler For Games
https://optick.dev
MIT License
2.95k stars 296 forks source link

Fiber support? #88

Open BrodyHiggerson opened 5 years ago

BrodyHiggerson commented 5 years ago

I'm using https://github.com/RichieSams/FiberTaskingLib throughout my code, and am trying to integrate Optick into the application.

Previously, I was using the OPTICK_THREAD macro when the worker threads started, but Optick seems to get confused when you resume a function on another thread after a switch where there is an OPTICK_EVENT at the beginning of that function (e.g. the scoped event starts and stops on two different threads).

This prompted me to go looking, and I found Optick::RegisterFiber, but there is no documentation on this function. How should one use this? Should it be used in addition to OPTICK_THREAD on the workers? What about combined with OPTICK_EVENT, OPTICK_CATEGORY, and OPTICK_TAG?

bombomby commented 5 years ago

Hi Brody. Optick indeed provides support for fibers. Here is a couple of steps that should allow you to get a quick start: 1) Make sure you grab the latest code (or pull latest version into your branch) 2) Run .\tools\Windows\premake5.exe --Fibers vs2017 command to generate solution with Fibers support 3) Open generated solution build\vs2017\Optick.sln, compile and run 4) Open gui\OptickApp_vs2017.sln with Optick GUI, compile and run 5) Take a capture - you'll see that ConsoleApp.exe now runs with a set of Fibers OptickFibers 6) Search for MT_INSTRUMENTED_BUILD in the solution - it will show you all the points required for integration with Optick 7) This example demonstrates integration with a very similar Fiber Scheduler (https://github.com/SergeyMakeev/TaskScheduler)

Sorry, I haven't had a chance yet to wrap up everything fiber-related into a nice API and document it. Please let me know if you have any further questions.

BrodyHiggerson commented 5 years ago

Hi,

I tried to follow your instructions but am finding that the ConsoleApp does not build on latest master.

TestEngine.h(7): "MAKE_CATEGORY" should be "OPTICK_MAKE_CATEGORY" I believe.

After fixing that:

1>------ Build started: Project: ConsoleApp, Configuration: Debug x64 ------
1>TestEngine.cpp
1>e:\_programming\repositories\optick\samples\common\testengine\testengine.cpp(148): error C2665: 'Optick::CreateDescription': none of the 2 overloads could convert all the argument types
1>e:\_programming\repositories\optick\src\optick.h(537): note: could be 'Optick::EventDescription *Optick::CreateDescription(const char *,const char *,int,const char *,const Optick::Category::Type)'
1>e:\_programming\repositories\optick\samples\common\testengine\testengine.cpp(148): note: while trying to match the argument list '(const char [87], const char [77], int, const char [13], unsigned __int64)'

But it's not essential that this works for my case - I will take a look at the example you linked and attempt the integration regardless :)

bombomby commented 5 years ago

These errors have been fixed in https://github.com/bombomby/optick/commit/786c5af2bdbf9eb1f31689f9ac60774c58e86143. Don't skip step #1 :)

BrodyHiggerson commented 5 years ago

Hi,

Thanks for your reply - that helped, and I've been looking at your example and trying to implement my own, but I'm having some troubles.

My version of your profiling interface (with simplified states) is as follows:

class ProfilingInterface : public ftl::IProfilingInterface
{

public:
    virtual void OnFibersCreated(size_t numFibers) override
    {
        m_numFibers = numFibers;
        m_fiberEventStorages.resize(m_numFibers);

        for (uint32_t i = 0; i < m_fiberEventStorages.size(); ++i)
        {
            Optick::RegisterFiber(i, &m_fiberEventStorages[i]);
        }
    }

    virtual void OnThreadsCreated(size_t numThreads) override
    {
        m_numThreads = numThreads;
    }

    virtual void OnThreadCreated(size_t workerIndex) override
    {
        OPTICK_START_THREAD("FiberWorker");
    }

    virtual void OnThreadStopped(size_t workerIndex) override
    {
        OPTICK_STOP_THREAD();
    }

    virtual void OnTaskChangeState(size_t fiberIndex, FiberState newState) override
    {
        Optick::EventStorage** currentThreadStorageSlot = Optick::GetEventStorageSlotForCurrentThread();

        // Profile session is not active
        if (*currentThreadStorageSlot == nullptr)
        {
            return;
        }

        if (fiberIndex > m_fiberEventStorages.size())
        {
            printf("fiberIndex was too large. Received %zd, expected <%zd", fiberIndex, m_fiberEventStorages.size());
            return;
        }

        Optick::EventStorage* fiberStorage = m_fiberEventStorages[fiberIndex];

        switch (newState)
        {
        case ftl::IProfilingInterface::FiberState::START:
            Optick::FiberSyncData::AttachToThread(fiberStorage, 
                static_cast<uint64_t>(ftl::GetCurrentThread().Id));
            break;
        case ftl::IProfilingInterface::FiberState::STOP:
            Optick::FiberSyncData::DetachFromThread(fiberStorage);
            break;
        }
    }

private:
    size_t m_numThreads = 0;
    size_t m_numFibers = 0;

    std::vector<Optick::EventStorage*> m_fiberEventStorages;
};

I'm making sure to call STOP before switching out a fiber, and START when returning. The other complexity worth mentioning is that all of my game's work is performed using fibers - am I able to call OPTICK_FRAME("Server::Tick") within a fiber task using the above method of attaching events?

image The "CPU Frame" often extends over the top of a task which follows it on the same fiber worker, meaning that their tagged attributes are all displayed together, and it makes things a bit confusing.

If my 'main' thread is just going to be another fiber worker, should I be registering it like the others via OPTICK_THREAD("FiberWorker"), and if so, how does this interact with OPTICK_FRAME? In my screenshot about,

image

I also sometimes get profile events stacked onto the same Fiber worker, which doesn't make much sense - in my tests, these are standalone fiber-based tasks which execute some busy-work without spawning tasks, and then yield before we swap to another one. It's basically a mock-up of a staged frame pipeline, similar to Naughty Dog's.

image

Some more overlaps. It looks like maybe one of these two overlapping events should've been shown on a separate thread and then it would all fit together nicely.

bombomby commented 5 years ago

OPTICK_FRAME macro is not compatible with Fibers at the moment.

Try to do the following: 1) Create a new thread which does the following:

while (true)
{
   OPTICK_FRAME("FakeMainThread");
   Sleep(33);
}

2) Comment out the other OPTICK_FRAME that you have 3) Check whether it works or not

If all works ok - that's good news, I am currently working on a change which allows to split OPTICK_FRAME into two calls: update and flip frame. As a side-effect it will allow running OPTICK_FRAME on multiple threads through the fibers.

If it doesn't work - well, we'll need to dig deeper :) Let me know whether FakeMainThread workaround works for you or not.

BrodyHiggerson commented 4 years ago

Hey! I've been away from this issue for a long time but will likely return soon. I wanted to ask; is the above still a reflection of the current state of Optick? E.g. the status of

am currently working on a change which allows to split OPTICK_FRAME into two calls,

and so on. No pressure, just want to make sure :)

Thanks for all your hard work!

BrodyHiggerson commented 3 years ago

Hey @bombomby! I've had another go with this, hoping that pinning my 'main' fiber to a single thread would help the use of OPTICK_FRAME in a fiber situation. You can see in this image that it's always appearing on the same row, so that's something, but Optick is definitely getting confused. In this case, UpdateClient is the function controlled the main loop, and it kicks off a bunch of fiber work (such as the "busywork" fake sleep tasks I threw together). Those busywork tasks should all fall under a frame, which blocks on them and thus in reality can't continue without their completion.

Did you end up splitting up OPTICK_FRAME? :)

image