Celtoys / Remotery

Single C file, Realtime CPU/GPU Profiler with Remote Web Viewer
Apache License 2.0
3.11k stars 264 forks source link

Api for iterating the profiler info #190

Closed JCash closed 2 years ago

JCash commented 2 years ago

In our current profile library, we have support for iterating the samples and counters. This allows us to display the info quickly at runtime. E.g.:

image

Is this a feature you've considered before, and if so, what were/are your current thoughts around such an api in remotery?

Regards, Mathias

dwilliamson commented 2 years ago

Right now I'm not convinced this is a good way forward for the API as it would pull a lot of internal structures into the public API, such as the Sample Tree. Any attempt to make some separation between public and private versions of these would complicate it even more.

A lot of people wrap remotery in their own macros so that they can swap profilers in and out. So some might do something like:

#define MySample rmt_ScopedCPUSample

In this case I would suggest using MySample to push your samples into your own tree structure as well.

JCash commented 2 years ago

Yeah, for sure, keeping a stable api is the trickier part of it all. However, seeing as the update rate to the library is very stable it might be worth considering.

Definitely, keeping our current profiler is of course an option. It just feels strange to keep two profilers around. In any case thanks for the heads up!

dwilliamson commented 2 years ago

Oh, so you want to inspect the values recorded by Remotery as well? Hmmm... there is a precedent for this with people actually piggybacking the network stream to receive all the updates.

JCash commented 2 years ago

Ah, yes. So our current implementation is basically the same as Remotery, so we have all the defines and hooks as expected. But our lib lacks a few features, and I'd much rather use something commonly used (more modern UI, better license etc).

I'm not opposed to using a fork of Remotery either, where I can traverse the internal structures, at least to get a feel for it (I am currently in the due diligence phase). In reality, I don't expect the internals changing too much too often, and if they do, we'll have to pay that time it takes. As long as I know beforehand, I can at least make informed choices :)

dwilliamson commented 2 years ago

What some people have done is go into AddSampleTreeMessage and chain to a callback, passing in Sample* sample which is the root of the sample tree being sent to the viewer.

This is really nice and simple but it does require that these go into the public header:

I'm a bit nervous of doing that because the user base of Remotery is pretty huge and it becomes very difficult to make changes to the API after something new is introduced. In this case I'd be wary of ever changing the structure of Sample for fear somebody now depends on it.

Try it out for yourself and let me know. I might be over-thinking the problems of putting these in the public API.

JCash commented 2 years ago

Yeah, I wouldn't want to expose the internals of the Sampleeither, but more likely add it as an opaque pointer that can be used to query the info needed. It would then probably require the enum SampleType to be available as well.

The SAMPLE_NAME_LEN might as well be a customizable define in the header I think? That would also make it known to the user.

I was thinking something along these lines:

// Allows the caller to keep track of the hierarchy
// Based on Per Vognsen's gist:
// https://gist.github.com/pervognsen/744066869da509a15c4e08fc9408207e

struct rmtSampleIterator
{
    struct rmtSample* sample;
};

rmtSample*          rmt_GetTopSample();
rmtSampleIterator   rmt_IterateChildren(rmtSample* sample);
rmtBool             rmt_IterateNext(SceneNodeIterator* it);

// Accessors

const char*   rmt_GetName(rmtSample* sample);
SampleType    rmt_GetType(rmtSample* sample);
rmtU32        rmt_GetCallCount(rmtSample* sample);
rmtU64        rmt_GetTime(rmtSample* sample);
...

Used in the engine to show the callgraph and times:

void CollectProfileData(Presenter* p, int indent, rmtSample* sample)
{
    p->printItem(rmt_GetName(sample), rmt_GetCallCount(sample), indent);

    rmtSampleIterator iter = rmt_IterateChildren(sample);
    while (rmt_IterateNext(&iter)) {
        CollectProfileData(presenter, indent + 1, iter.sample);
    }
}

rmtSample* top = rmt_GetTopSample(g_Remotery);
CollectProfileData(presenter, 0, top);
dwilliamson commented 2 years ago

I think you'll need the callback method mixed with this. Remotery doesn't actually store any sample data in memory: it sends it to the viewer and records it to disk so you can load it in the viewer later.

So the callback could send you the opaque Sample pointer and an API just like you sguggested could be perfect.

JCash commented 2 years ago

Ah, good point! I'll investigate further!.

I think as long as the hierarchy is preserved, it should basically "just work" Btw, is it Remotery_SendSampleTreeMessage() you meant?

dwilliamson commented 2 years ago

Ah that function is gone now. The new equivalent is QueueSampleTree.

If you add the hook in QueueSampleTree then you will get a callback from whatever thread creates the sample point. This means you'll need to cater for multiple writer threads. It also means any processing you do with this callback will slow your calling thread and potentially interfere with your timing.

If you do it in Remotery_SendSampleTreeMessage then you will only get a callback from the main Remotery thread. This is probably a better idea (the old implementation only used to call this if a viewer was attached - that's not true anymore). In fact, you could add it right after the if (rmt->logFile != NULL) check: specify a callback in the rmtSettings object and chain to it if it's there.

JCash commented 2 years ago

Awesome, I'll try this out tomorrow!

JCash commented 2 years ago

I've added a first take on this now, in a PR (https://github.com/Celtoys/Remotery/pull/191). Please have a look to see what you think!