Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin API Extension: SendReply #5939

Open
weefuzzy opened this issue Dec 20, 2022 · 11 comments
Open

Plugin API Extension: SendReply #5939

weefuzzy opened this issue Dec 20, 2022 · 11 comments
Assignees

Comments

@weefuzzy
Copy link
Contributor

Motivation

Extending the Plugin API to allow sending OSC replies to clients directly from the Command FIFO thread.

Description of Proposed Feature

The Plugin API allows plugins to run custom jobs on the Command FIFO (i.e. not the audio thread) via fDoAsyncronousCommand. However, there is no publicly exposed way to send a reply to clients from this thread  – the relevant function (in scsynth), SendReply is not in the InterfaceTable. As a result one has to dispatch a reply back via the audio thread. This then creates problems for clients waiting on replies because an additional asynchronous indirection is introduced.

This proposes that the Plugin API is extended with three functions: SendReply and two extra to copy and delete a ReplyAddr that is hidden behind a void*. Something like

void  fSendReply(void* inReplyAddr, char* inBuf, int inSize); 
void* fCopyReplyAddress(void* inreply); 
void  fDeleteReplyAddress(void* inreply); 

In FluCoMa we've ended up doing this 'illegally' by linking to the ReplyAddr structure and making these functions ourselves, because the pain for users outweighed the pain of doing something brittle and bad. Following some discussion in #5347 and flucoma/flucoma-sc#149, it looks like the coming bump in Boost versions will result in our comeuppance, hence making an official request.

Plan for Implementation

The main thing is there being agreement on a mechanism for extending the plugin API robustly. If a mechanism were available, or in flight, I would happily and gratefully do the implementation for these functions.

For scsynth the implementation is trivial and FluCoMa's functions, which have been out in the world for some years without problems, can be transplanted directly. SendReply merely forwards to an already existing internal function in SC_ReplyImpl.hpp, and the other two functions are just casts and copies. In C++, from our code:

void* copyReplyAddress(void* inreply)
{
  if (!inreply) return nullptr;
  ReplyAddress* reply = new ReplyAddress();
  *reply = *(static_cast<ReplyAddress*>(inreply));
  return reply;
}

void deleteReplyAddress(void* inreply)
{
  if (!inreply) return;
  delete static_cast<ReplyAddress*>(inreply);
}

void SendReply(void* inReplyAddr, char* inBuf, int inSize)
{
  SendReply(static_cast<ReplyAddress*>(inReplyAddr), inBuf, inSize);
}

(These aren't / mustn't be calle on the audio thread, so no need to use the real-time allocator)

⚠️ I think supernova is perhaps slightly more complicated because it implements all that stuff quite differently.

@telephon
Copy link
Member

this sounds likea very good idea. The fact that this has been tested for a long time is also nice, so it will be well integrated. More opinions?

@Spacechild1
Copy link
Contributor

Spacechild1 commented Dec 21, 2022

(These aren't / mustn't be calle on the audio thread, so no need to use the real-time allocator)

Plugin commands are dispatched on the RT thread, so you definitely need to use the RT allocator.


But let's take a step back. I guess what you (and I) really want is the possibility to send arbitrary reply messages back to the client.

Now, the async command already has a copy of the reply address (otherwise it wouldn't be able to send a "done" message). Instead of requiring users to make their own (redundant) copies, we only need to pass the reply address to the callback functions. Therefore I would rather propose an extension to the fDoAsynchronousCommand function:

typedef bool (*AsyncStageFnEx)(World* inWorld, void* cmdData, void* replyAddr);

int (*fDoAsynchronousCommandEx)(
        World* inWorld, void* replyAddr, const char* cmdName, void* cmdData,
        AsyncStageFnEx stage2,
        AsyncStageFnEx stage3,
        AsyncStageFnEx stage4,
        AsyncFreeFn cleanup, int completionMsgSize, void* completionMsgData);

void  fSendReply(void* inReplyAddr, char* inBuf, int inSize); 

Then users can just call fSendReply in the stage2 or stage4 callback with the replyAddr argument.

@weefuzzy
Copy link
Contributor Author

He he! The copying was actually following a suggestion you made in this forum thread, because we have the additional need to dispatch long-running tasks to a worker thread (to avoiding blocking the command queue overly long) and so need to persist a reply address until the job has finished some time later.

@Spacechild1
Copy link
Contributor

Ha! I certainly did not remember that :-D

because we have the additional need to dispatch long-running tasks to a worker thread (to avoiding blocking the command queue overly long) and so need to persist a reply address until the job has finished some time later.

Right, makes sense. I guess we could additionally offer fCopyReplyAddress(World* inWorld, void* inReply) for cases where replies are sent outside of asynchronous commands. Note that there is no need for fFreeReplyAddress, the reply address can be simply freed with RTFree. (Of course, this needs to be documented.)

@Spacechild1
Copy link
Contributor

As a side note, I would also like to add an extension to the Unit command interface:

typedef void (*UnitCmdFuncEx)(struct Unit* unit, struct sc_msg_iter* args, void* replyAddr);

bool (*fDefineUnitCmdEx)(const char* inUnitClassName, const char* inCmdName, UnitCmdFuncEx inFunc);

Then we could also use the new SendReply method in the context of unit commands. (Currently, I have to abuse fSendNodeReply to send replies back to the client.)

@weefuzzy
Copy link
Contributor Author

Ha! I certainly did not remember that :-D

😄

FWIW, my reason for not using RTAlloc for this to date (I always do the copy in stage 2) is to avoid making potentially many non-essential demands on the memory pool, especially if they're going to be long-lived.

(Also, I can't remember off-hand if this was an issue, but our buffer processing stuff is also designed to be impervious to cmd-. from the client – /g_freeAll doesn't clear the pool, but I don't know if fragmentation nastiness would set in if there were lots of little left over bits floating around).

And your new post just appeared whilst I was typing this! I see you have a real-time use case...

@Spacechild1
Copy link
Contributor

Spacechild1 commented Dec 21, 2022

FWIW, my reason for not using RTAlloc for this to date ... is to avoid making potentially many non-essential demands on the memory pool, especially if they're going to be long-lived.

I wouldn't worry too much about that. After all, every asynchronous command has to hit the RT memory allocator several times.

would set in if there were lots of little left over bits floating around).

I'm not sure what you mean by "little left overs". Every bit of memory should be freed when it is no longer needed, otherwise you have a memory leak :-)

(I always do the copy in stage 2)

Uhhh, I think this just works by sheer accident. AFAICT, there is no guarantee that the reply address outlives the plugin command callback...

@Spacechild1
Copy link
Contributor

As another side note, I was wondering whether we should also include a method to send a reply from the RT thread. I know it could be done with fSendMsgFromRT, but it is not particularly obvious and also rather awkward.

Instead, I would like to have something like
fSendReplyFromRT(World* inWorld, void* inReplyAddr, char* inBuf, int inSize)
that would take care of all the details.

This would be handy for synchronous plugin/unit commands where you don't need to go through the NRT thread.

@weefuzzy
Copy link
Contributor Author

I wouldn't worry too much about that. After all, every asynchronous command has to hit the RT memory allocator several times.

It's the longevity and quantity that concern me. If we have clients batch processing then lots of long-lived small allocations can add up (we already have users running into occasional problems like this if they throw a load of jobs on to the queue, and they start to exhaust the pool).

I'm not sure what you mean by "little left overs". Every bit of memory should be freed when it is no longer needed, otherwise you have a memory leak :-)

I mean that if the pool allocator is going to get confounded sooner or later by a lack of contiguous blocks, because lots of little chunks of ReplyAddr size are out there waiting for worker threads to complete (in our case). Not that I wasn't freeing memory 😸

Uhhh, I think this just works by sheer accident. AFAICT, there is no guarantee that the reply address outlives the plugin command callback...

Yikes. I see now that the enclosing SC_SequencedCommand class copies the ReplyAddr by value, and – indeed – there's no guarantee that the original pointer will outlive the original dispatch function. Bummer. Ah well.

This would be handy for synchronous plugin/unit commands where you don't need to go through the NRT thread.

But it still would go through the NRT thread under the hood, right? If fSendReplyFromRT is doing essentially what SendReplyCmd does

@Spacechild1
Copy link
Contributor

Spacechild1 commented Dec 22, 2022

It's the longevity and quantity that concern me. If we have clients batch processing then lots of long-lived small allocations can add up (we already have users running into occasional problems like this if they throw a load of jobs on to the queue, and they start to exhaust the pool).

The size of the ReplyAddr struct is about 64 bytes. With the default memory pool size of 8192 kB, you would need to create 130,000 instances to exhaust the pool... I don't think there is a real issue.

I mean that if the pool allocator is going to get confounded sooner or later by a lack of contiguous blocks, because lots of little chunks of ReplyAddr size are out there waiting for worker threads to complete (in our case)

I wouldn't worry too much about fragmentation. Think about it: every single Synth occupies RT memory and peoply are using hundreds or thousands of long-lived Synths just fine. If you hit a wall, you can always increase the memory pool size.

Also keep in mind that the RT memory pool uses free lists for each size categories. With very small objects chances are very high that you can take an existing slot before hitting the main linear allocator.

But it still would go through the NRT thread under the hood, right? If fSendReplyFromRT is doing essentially what SendReplyCmd does

Exactly. At least for scsynth. Supernova uses a dedicated thread for sending client replies (see fire_io_callback).

@weefuzzy
Copy link
Contributor Author

Cool, thanks – I'll stop worrying. And fix the existing lifetime bug in FluCoMa in the meantime!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants