Added sample interator api #191

JCash · 2022-04-07T10:19:03Z

Fixes #190

JCash · 2022-04-07T10:20:00Z

lib/Remotery.c

@@ -4309,22 +4309,12 @@ static void Server_Update(Server* server)

 #define SAMPLE_NAME_LEN 128

-typedef enum SampleType


Moved to the header, and renamed to rmtSampleType

JCash · 2022-04-07T10:21:54Z

lib/Remotery.c

+    if (g_Settings.sampletree_handler != NULL)
+    {
+        g_Settings.sampletree_handler(rmt, g_Settings.sampletree_context, sample_tree);
+    }


I had to put it before the FreeSamples() as the sample pointers would be invalid after that call.

JCash · 2022-04-07T10:24:16Z

lib/Remotery.c

+RMT_API rmtSampleIterator _rmt_IterateChildren(rmtSample* sample)
+{
+    rmtSampleIterator iterator;
+    iterator.sample = 0;
+    iterator.initial = sample != NULL ? sample->first_child : 0;
+    return iterator;
+}
+
+RMT_API rmtBool _rmt_IterateNext(rmtSampleIterator* iter)
+{
+    if (iter->initial != NULL)
+    {
+        iter->sample = iter->initial;
+        iter->initial = 0;
+    }
+    else
+    {
+        if (iter->sample != NULL)
+            iter->sample = iter->sample->next_sibling;
+    }
+
+    return iter->sample != NULL ? RMT_TRUE : RMT_FALSE;
+}


Following the pattern from this gist:
https://gist.github.com/pervognsen/744066869da509a15c4e08fc9408207e

JCash · 2022-04-07T10:25:48Z

lib/Remotery.c

+RMT_API const char* _rmt_SampleTreeGetThreadName(rmtMsgSampleTree* sample_tree)
+{
+    return sample_tree->threadName;
+}
+
+RMT_API rmtSample* _rmt_SampleTreeGetRootSample(rmtMsgSampleTree* sample_tree)
+{
+    return sample_tree->rootSample;
+}


At first, I passed the threadName explicitly to the callback function, but later thought that passing the opaque message pointer fits the api a bit better:

Easier to add more accessor functions later

Matches the Sample accessor api used in the callback api

Hmmm... yes... that's a good counter argument. I'll edit my comment.

JCash · 2022-04-07T10:26:51Z

lib/Remotery.c

+RMT_API const char* _rmt_SampleGetName(Remotery* rmt, rmtSample* sample)
+{
+    const char* name = StringTable_Find(rmt->string_table, sample->name_hash);
+    if (name == NULL)
+    {
+        return "null";
+    }
+    return name;
+}


Needs the Remotery pointer, so I explicitly pass that in the callback.

You should use g_Remotery in the implementation instead.

I was thinking that too, but I was perhaps trying to remove the need to use a global variable.

The current setup in the PR was to address the initialization problem:

The problem with the user context and the remotery pointer is a catch 22:

// Setting up settings before creating the context Remotery *rmt; rmtSettings* settings = rmt_Settings(); settings->sampletree_handler = dumpTree; settings->sampletree_context = rmt; // Not valid yet rmt_CreateGlobalInstance(&rmt);

An alternative, albeit not very clean code flow either:

// Setup some setting variables before creating the Remotery instance... Remotery *rmt; rmtSettings* settings = rmt_Settings(); settings->port = port; rmt_CreateGlobalInstance(&rmt); // ... and set some settings after settings->sampletree_handler = dumpTree; settings->sampletree_context = rmt; // Not valid yet

But, I have no examples of other users use case, and they can for sure create their own custom contexts etc.

So I'll go ahead with removing the Remotery pointer from the callback function.

Most of the internal code uses the global variable g_Remotery. The API explicitly avoids requesting it from the user. It even has a DLL API to cater for this where the use calls rmt_SetGlobalInstance.

Ah, now I finally see what you mean. Fixing...

JCash · 2022-04-07T10:28:05Z

lib/Remotery.h

+    RMT_SampleType_OpenGL,
+    RMT_SampleType_Metal,
+    RMT_SampleType_Count,
+} rmtSampleType;


Given the other public interfaces in the header, I prefixed it with RMT_.

Might want to be upper case, to match style of rmtError enum?

This is fine. I wanted to remove all error-messages-as-enums, anyway and replace them with 4 or 5 broader error enums that give you optional message retrieval.

JCash · 2022-04-07T10:29:19Z

lib/Remotery.h

@@ -363,6 +388,7 @@ typedef void* (*rmtMallocPtr)(void* mm_context, rmtU32 size);
 typedef void* (*rmtReallocPtr)(void* mm_context, void* ptr, rmtU32 size);
 typedef void (*rmtFreePtr)(void* mm_context, void* ptr);
 typedef void (*rmtInputHandlerPtr)(const char* text, void* context);
+typedef void (*rmtSampleTreeHandlerPtr)(Remotery* rmt, void* cbk_context, rmtMsgSampleTree* sample_tree);


The callback type and its arguments.

JCash · 2022-04-07T10:31:38Z

lib/Remotery.h

+RMT_API rmtU64              _rmt_SampleGetTime(rmtSample* sample);
+RMT_API rmtU64              _rmt_SampleGetSelfTime(rmtSample* sample);
+RMT_API void                _rmt_SampleGetColour(rmtSample* sample, rmtU8* r, rmtU8* g, rmtU8* b);
+RMT_API rmtSampleType       _rmt_SampleGetType(rmtSample* sample);


Currently, they all are "private", with the "_" prefix.

I guess they should also be wrapped with the "RMT_OPTIONAL" define.
Should it also be contained withing a new tag, e.g. RMT_ENABLED_ITERATORS ? Might be a bit overkill?

Don't worry about adding a RMT_ENABLED_ITERATORS - just RMT_ENABLED will do.

JCash · 2022-04-07T10:32:18Z

sample/dump.c

+void dumpTree(Remotery* rmt, void* ctx, rmtMsgSampleTree* sample_tree)
+{
+    rmtSample* root = _rmt_SampleTreeGetRootSample(sample_tree);
+    const char* thread_name = _rmt_SampleTreeGetThreadName(sample_tree);
+
+    printf("// ********************   DUMP TREE: %s   ************************\n", thread_name);
+
+    printTree(rmt, root, 0);
+}


Example callback

JCash · 2022-04-07T10:32:27Z

sample/dump.c

+void printTree(Remotery* rmt, rmtSample* sample, int indent)
+{
+    printSample(rmt, sample, indent);
+
+    rmtSampleIterator iter = _rmt_IterateChildren(sample);
+    while (_rmt_IterateNext(&iter)) {
+        printTree(rmt, iter.sample, indent+1);
+    }
+}


Example of iterator

dwilliamson · 2022-04-07T12:04:57Z

Really great stuff.

I started reviewing this before you left your comments so I'll address them now.

dwilliamson · 2022-04-07T11:54:04Z

lib/Remotery.c

@@ -6115,6 +6105,11 @@ static rmtError Remotery_SendSampleTreeMessage(Remotery* rmt, Message* message)
    error = bin_SampleTree(bin_buf, sample_tree);
    rmt_EndCPUSample();

+    if (g_Settings.sampletree_handler != NULL)
+    {
+        g_Settings.sampletree_handler(rmt, g_Settings.sampletree_context, sample_tree);


Rather than passing sample_tree and having to create the opaque type rmtMsgSampleTree, can you pass sample_tree->rootSample instead? Then you only need to expose the rmtSample type to the public API.

There is also then no need for _rmt_SampleTreeGetRootSample and _rmt_SampleTreeGetThreadName can be replaced with passing the thread name to your sampletree_handler.

Leave as is but could you rename to rmtSampleTree?

dwilliamson · 2022-04-07T11:58:23Z

lib/Remotery.h

+RMT_API rmtSample*          _rmt_SampleTreeGetRootSample(rmtMsgSampleTree* sample_tree);
+
+// Sample accessors
+RMT_API const char*         _rmt_SampleGetName(Remotery* rmt, rmtSample* sample);


Need a comment that accessing all this data should only be done from the callback and not another thread elsewhere. This is because functions like _rmt_SampleGetName use StringTable_Find which is currently only safe to access from the main Remotery thread.

dwilliamson · 2022-04-07T12:04:07Z

lib/Remotery.h

@@ -661,6 +691,23 @@ RMT_API void _rmt_BeginMetalSample(rmtPStr name, rmtU32* hash_cache);
 RMT_API void _rmt_EndMetalSample(void);
 #endif

+// Iterator


Does this API compile and link when RMT_ENABLED=0? Prefix-underscores shouldn't be used here.

As an example:

RMT_API void _rmt_LogText(rmtPStr text);

This is the function signature that is part of the Private interface and this is the macro which gets compiled-out when RMT_ENABLED=0:

#define rmt_LogText(text) \ RMT_OPTIONAL(RMT_ENABLED, _rmt_LogText(text))

I have now added this, but I'm wondering if the use case is a bit different.
The client code will fail to compile on code like this if we simply omit the info:

error: expected expression const char* name = rmt_SampleGetName(ctx->rmt, sample);

I see two options:

Each such interface needs to produce "valid" values, which is counter intuitive when using the RMT_OPTIONAL + RMT_ENABLED

Letting the client control their code using #ifdef RMT_ENABLED explicitly around this particular piece of code.

Yes, there's going to be a lot of errors if you implement some of them in terms of RMT_OPTIONAL as they need to return a value even when RMT_ENABLED=0.

One option is to return nothing and specify the output as an address parameter to the function call but that would then prevent you from initialising at the declaration point in C when multiple calls are involved.

Have you looked at using RMT_OPTIONAL_RET?

I updated the api now to use the RMT_OPTIONAL+RMT_OPTIONAL_RET and the dump.c example builds and runs in -DRMT_ENABLED=0 as well.

dwilliamson · 2022-04-07T12:06:23Z

lib/Remotery.c

+RMT_API const char* _rmt_SampleTreeGetThreadName(rmtMsgSampleTree* sample_tree)
+{
+    return sample_tree->threadName;
+}
+
+RMT_API rmtSample* _rmt_SampleTreeGetRootSample(rmtMsgSampleTree* sample_tree)
+{
+    return sample_tree->rootSample;
+}


Hmmm... yes... that's a good counter argument. I'll edit my comment.

Renamed to rmtSampleTree Added RMT_OPTIONAL functions for the iterator/sample api

Tweaked api for the iterator to make it work better with c89 and the RMT_OPTIONAL

JCash · 2022-04-07T14:41:40Z

lib/Remotery.h

+#define rmt_IterateChildren(iter, sample)                                           \
+    RMT_OPTIONAL(RMT_ENABLED, _rmt_IterateChildren(iter, sample))


Tweaked the api from

rmtSampleIterator iter = rmt_IterateChildren(sample);

to

rmtSampleIterator iter; rmt_IterateChildren(&iter, sample);

in order to both make it more C89 compatible, and also make it work with RMT_OPTIONAL better.

dwilliamson · 2022-04-07T14:45:44Z

I love this... going to get some tea and make sure I understand the implications of adding this before merge.

Thanks so much.

Added sample interator api

2ddcad9

JCash commented Apr 7, 2022

View reviewed changes

JCash mentioned this pull request Apr 7, 2022

Api for iterating the profiler info #190

Closed

dwilliamson reviewed Apr 7, 2022

View reviewed changes

Removed rmt pointer from callback

93f6998

Renamed to rmtSampleTree Added RMT_OPTIONAL functions for the iterator/sample api

JCash requested a review from dwilliamson April 7, 2022 13:56

Added RMT_OPTIONAL_RET where needed

76220fc

Tweaked api for the iterator to make it work better with c89 and the RMT_OPTIONAL

JCash commented Apr 7, 2022

View reviewed changes

dwilliamson approved these changes Apr 7, 2022

View reviewed changes

dwilliamson merged commit 5c44432 into Celtoys:main Apr 7, 2022

JCash deleted the iterator-api branch April 13, 2022 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added sample interator api #191

Added sample interator api #191

JCash commented Apr 7, 2022 •

edited

Loading

JCash Apr 7, 2022

JCash Apr 7, 2022

JCash Apr 7, 2022

JCash Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022

JCash Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022

JCash Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022

JCash Apr 7, 2022

dwilliamson commented Apr 7, 2022

dwilliamson Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022 •

edited

Loading

dwilliamson Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022

dwilliamson Apr 7, 2022

JCash Apr 7, 2022

dwilliamson commented Apr 7, 2022

		@@ -4309,22 +4309,12 @@ static void Server_Update(Server* server)

		#define SAMPLE_NAME_LEN 128

		typedef enum SampleType

		#define rmt_IterateChildren(iter, sample) \
		RMT_OPTIONAL(RMT_ENABLED, _rmt_IterateChildren(iter, sample))

Added sample interator api #191

Added sample interator api #191

Conversation

JCash commented Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwilliamson commented Apr 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JCash Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwilliamson commented Apr 7, 2022

JCash commented Apr 7, 2022 •

edited

Loading

JCash Apr 7, 2022 •

edited

Loading