feat(server): add sync subscription API register_subscription_raw
#1182
Conversation
core/src/server/rpc_module.rs
Outdated
| // definition and not the as same when the subscription call has been completed. | ||
| // | ||
| // This runs until the subscription callback has completed. | ||
| let res = callback(params, sink, ctx.clone()); |
There was a problem hiding this comment.
To make sure I get this right, this avoids an allocation because we pass the parameters directly to the callback instead of calling into_owned, which can allocate quite a bit since the params might either be frequent or large?
There was a problem hiding this comment.
yupp, you got it right.
It's possible to parse the params outside the callback/async block and avoid that allocation with this....
This alone in my substrate storage subscription test brought down the memory allocations from 2.5GB to 2GB
| F: (Fn(Params, PendingSubscriptionSink, Arc<Context>) -> R) + Send + Sync + Clone + 'static, | ||
| R: IntoSubscriptionCloseResponse + Send + 'static, | ||
| { | ||
| if subscribe_method_name == unsubscribe_method_name { |
There was a problem hiding this comment.
nit: Since the beginning of this is similar to register_async_subscription would it be possible to move some code into a shared function? Maybe we could have a internal_verify_and_unsubscribe which does the initial validation and registers the unsubscribe?
There was a problem hiding this comment.
sure, I just want know what you think about the API but unfortunately a breaking change :(
There was a problem hiding this comment.
Yep, that makes sense! From my perspective, this gives a nice boost in terms of performance, users that may value more ergonomics still the old method 👍
There was a problem hiding this comment.
Looks good to me!
Personally I'd swap the names around sortof, and have something like:
register_subscription- the original one as it wasregister_subscription_sync/register_subscription_raw- the lower level synchronous/raw method (you still need to.awaitthings inside it but can decide how.
I might be tempted to also #[doc(hidden)] the sync/raw one because maybe better if people don't rely on it in the interface and then we can tune it however we need to work best with the macro without worrying about any breakages.
|
Cool, yeah ok the intention was the methods has this I think we can make it |
|
That's cool; |
|
Yeah, I think that makes sense because this added API isn't really ergonomic to use |
niklasad1
changed the title
register_subscription_raw
The motivation is behind this is because this is #1146 if a subscription is having params that are big or happen very frequently it may allocate.
Thus, we now exposes two different APIs for that and our proc macros API will use the lower-level/sync API to parse the params outside async block/callback to avoid that allocation.
After this PR we will have: