Add generic RT/NRT shared resource system (#147)#152
Conversation
Introduces Methcla_ResourceDef plugin API for defining named resource types, OSC commands /resource/new and /resource/free, and /resource/ready, /resource/error, /resource/destroyed notifications. Supports freePending to handle races between in-flight construction and a concurrent free request. AsyncLatch condvar helper replaces sleepFor() in tests.
e9813b8 to
c05a501
Compare
| } | ||
| }; | ||
|
|
||
| inline static std::ostream& operator<<(std::ostream& out, |
There was a problem hiding this comment.
Consider implementing this in the Id base class, similar to how it's done in StrongId. Same for NodeId.
| void resourceNew(ResourceId id, const char* uri) | ||
| { | ||
| beginMessage(); | ||
|
|
There was a problem hiding this comment.
Consider getting rid of this newline (same in similar Request methods).
|
|
||
| oscPacket() | ||
| .openMessage("/resource/new", 3) | ||
| .int32(0) // requestId (unused) |
There was a problem hiding this comment.
requestId is not needed, remove it from the OSC protocol.
| return m_audioBusIds; | ||
| } | ||
|
|
||
| ResourceId allocResourceId() |
There was a problem hiding this comment.
Follow the existing pattern (see nodeIdAllocator).
There was a problem hiding this comment.
Follow the existing pattern (see nodeIdAllocator).
| const char* message); | ||
|
|
||
| //* Register a resource type definition. | ||
| void (*register_resource_def)(Methcla_Host* host, |
There was a problem hiding this comment.
Move this between register_synthdef and register_soundfile_api. Binary compatibility is not a concern at this point.
|
|
||
| static inline void | ||
| methcla_host_register_synthdef(Methcla_Host* host, | ||
| const Methcla_SynthDef* synthDef) |
There was a problem hiding this comment.
Rename to def for consistency.
There was a problem hiding this comment.
Rename to def for consistency.
| @@ -138,6 +139,9 @@ namespace Methcla { namespace Audio { | |||
| //* Register SynthDef. | |||
| void registerSynthDef(const Methcla_SynthDef* synthDef); | |||
There was a problem hiding this comment.
Rename to def for consistency.
| const int32_t resourceId = args.int32(); | ||
| if (resourceId < 0 || | ||
| static_cast<size_t>(resourceId) >= m_resources.size()) | ||
| return; |
There was a problem hiding this comment.
Avoid early returns.
| { | ||
| entry.freePending = true; | ||
| } | ||
| else if (entry.state == ResourceEntry::State::Live && |
There was a problem hiding this comment.
I think there's a state transition missing here, according to #147:
refcount>0 → free_pending=true,
└────────────────────────────────────── Free │ drain via release path
| } | ||
|
|
||
| namespace { | ||
| struct ResourceDestroyCommand |
There was a problem hiding this comment.
Prefer a class here, with members private where possible. Prefix members properly.
| } | ||
| else if (msg == "/resource/new") | ||
| { | ||
| class ResourceConstructCommand |
There was a problem hiding this comment.
I think it's more readable in the long run to avoid inline class definitions. Pull this and others out in an anonymous namespace.
| void EnvironmentImpl::scheduleResourceDestroy(int32_t resourceId) | ||
| { | ||
| auto& entry = m_resources[resourceId]; | ||
| sendToWorker( |
There was a problem hiding this comment.
Use the template version.
| entry.def = nullptr; | ||
| entry.data = nullptr; | ||
| self->impl | ||
| ->sendToWorker<EnvironmentImpl::ResourceDestroyedNotification>( |
There was a problem hiding this comment.
I think an extra memory allocation can be avoided if this class also functions as the notification.
| std::list<const Methcla_SoundFileAPI*> m_soundFileAPIs; | ||
|
|
||
| using ResourceDefMap = | ||
| std::unordered_map<std::string, const Methcla_ResourceDef*>; |
There was a problem hiding this comment.
We can't use std::string here because it allocates. See definition of SynthDefMap.
| { | ||
| auto* self = static_cast<ResourceDestroyCommand*>(d); | ||
| auto& entry = self->impl->m_resources[self->resourceId]; | ||
| entry.state = EnvironmentImpl::ResourceEntry::State::Free; |
There was a problem hiding this comment.
Better to fully initialize by assigning a default constructed entry.
| const Methcla_ResourceDef* def = it->second; | ||
| auto& entry = m_resources[resourceId]; | ||
| entry.state = ResourceEntry::State::Constructing; | ||
| entry.freePending = false; |
There was a problem hiding this comment.
Better to fully initialize by assigning a default constructed entry.
| } | ||
| } | ||
|
|
||
| class ResourceErrorNotification : public Notification |
There was a problem hiding this comment.
This class is probably not needed in the header and can be moved to the source (encapsulation).
| sendToWorker<ResourceReadyNotification>(resourceId); | ||
| } | ||
|
|
||
| void registerResourceDef(const Methcla_ResourceDef* def); |
There was a problem hiding this comment.
Add context comment (NRT)
| // notification. | ||
| void scheduleResourceDestroy(int32_t resourceId); | ||
|
|
||
| class ResourceDestroyedNotification : public Notification |
There was a problem hiding this comment.
This could be merged with the destroy command to avoid an allocation (see earlier comment). In any case, this is probably not needed in the header and can be moved to the source (encapsulation).
| size_t realtime_memory_size = 1024 * 1024; | ||
| size_t max_num_nodes = 1024; | ||
| size_t max_num_audio_buses = 1024; | ||
| size_t max_num_resources = 256; |
There was a problem hiding this comment.
These constants are duplicated in various places. I'm wondering whether there's a clean way to avoid the duplication.
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Slice 1: Unknown URI → /resource/error |
There was a problem hiding this comment.
Remove "Slice" comments. Add comments to explain tests where necessary.
| return false; | ||
| }); | ||
|
|
||
| // Send /resource/new and /resource/free back-to-back without waiting. |
There was a problem hiding this comment.
Could be sent in a single bundle. This guarantees they're executed in the same process callback.
- Add kMethcla_UnsupportedResourceTypeError; replace notifyResourceError with throwErrorWith so unknown-URI errors flow through the standard catch/log path instead of a bespoke /resource/error notification - Report out-of-range resource id in /resource/free as ArgumentError - Move ResourceReadyNotification, NodeTreeStatisticsCommand, and RTMemoryStatisticsCommand out of method bodies into the anonymous namespace; rename Command* → *Command for consistent naming - Consolidate operator bool() and operator<< into the IdBase template - Add resourceIdAllocator() to the Engine interface; rename audioBusId() → audioBusIdAllocator() for consistency - Fix Methcla_Host initializer order to match struct field order after register_resource_def was repositioned
Summary
Methcla_ResourceDefto the plugin C API (include/methcla/plugin.h) — a struct describing a named resource type withuri,instance_size,options_size, mutability hint, andconfigure/construct/destroycallbacks. Plugins register definitions viamethcla_host_register_resource_def.ResourceId,allocResourceId(),freeResourceId(),resourceNew(), andresourceFree()to the C++ client API (include/methcla/engine.hpp)./resource/new <requestId:i> <resourceId:i> <uri:s>and/resource/free <id:i>with notifications/resource/ready <id:i>,/resource/error <id:i> <msg:s>, and/resource/destroyed <id:i>.freePendinghandles the race where/resource/freearrives while construction is still in progress.AsyncLatch(condvar + timeout) totests/methcla_tests.hppas a robust replacement forsleepFor()in async tests.Deferred
kMethcla_ResourcePortintegrationperform_with_resourceshost primitive)Test plan
ResourceTests.UnknownUriReturnsError— unknown URI →/resource/errorResourceTests.KnownUriConstructsAndNotifiesReady— registered URI →/resource/readyResourceTests.FreeLiveResourceDestroysIt— free after ready →/resource/destroyedResourceTests.FreeDuringConstructSetsFreePending— free during construction →/resource/destroyedonly (no/resource/ready)ctest --preset debug): 19/19 passed