feat!: make callback opaque #1383
Conversation
Use an enum hidden to use channel senders directly. It should improve performances, and may allow later to use async sender methods.
|
PR missing one of the required labels: {'dependencies', 'documentation', 'breaking-change', 'bug', 'enhancement', 'internal', 'new feature'} |
wyfo
added
enhancement
| { | ||
| match &self.0 { | ||
| CallbackInner::Dyn(cb) => cb(arg), | ||
| CallbackInner::Flume(tx) => { |
There was a problem hiding this comment.
Are you sure that this kind of optimization is really justified given that flume/ring channels are just one of many that users can use in rust, and that many people are still going to just use function callbacks ?
Also to make use of such an optimization we would need to complicate zenoh-c interface since right now it only accepts a "closure" with pointer to c-function (which is transformed into Arc).
There was a problem hiding this comment.
Flume is not "just one of many that users can use in rust", it's the default one, and one of the few with a IntoHandler implementation (the others being RingChannel, which is also included in CallbackInner, std:sync::mpsc).
And the goal of this change is also to optimize callback use, see https://godbolt.org/z/G6vcT4b93.
But more than a performance optimization, the long-term vision is to be able to get flume sender directly in order to use its async method.
Also to make use of such an optimization we would need to complicate zenoh-c interface since right now it only accepts a "closure" with pointer to c-function (which is transformed into Arc).
I don't see the how this PR changes anything here
There was a problem hiding this comment.
I don't see the how this PR changes anything here
It adds an extra overhead due to enum match (not sure whether it is significant though). Which means that you probably improve performance of flume channel in rust, but maybe also make it worse for the whole zenoh-c/cpp + produce slightly more complex code.
This also means that if in several month we decide to have another "default" channel we would need to revisit this code.
What I mean is that I would really like to see the performance gain for channels / loss for callbacks if there is any.
There was a problem hiding this comment.
It adds an extra overhead due to enum match (not sure whether it is significant though).
Branch prediction enters the room.
Which means that you probably improve performance of flume channel in rust, but maybe also make it worse for the whole zenoh-c/cpp + produce slightly more complex code.
Still better than the previous "by value" callback, see godbolt. Why is the produced code more complex?
This also means that if in several month we decide to have another "default" channel we would need to revisit this code.
And then, we will change the internal enum, we don't care, it's internal, and it's only a dozen lines to change.
What I mean is that I would really like to see the performance gain for channels / loss for callbacks if there is any.
Trust branch prediction, especially if you are using only callbacks.
There was a problem hiding this comment.
till better than the previous "by value" callback, see godbolt. Why is the produced code more complex?
I do not argue that passing by ref is faster than by value, this can still be achieved through only Arc, without any enum matching.
The produced code is more complex since it adds more lines => higher maintenance cost (which is always better to be avoided if there is no any significant benefit)
There was a problem hiding this comment.
But more than a performance optimization, the long-term vision is to be able to get flume sender directly in order to use its async method.
This is the goal behind making the type opaque. Now, using the enum is not a high maintenance code, all the code is contained in one module and is only a few lines.
|
Using a immutable reference as callback argument prevent getting a mutable access to a shared memory segment contained in payload. |
Access by value is still available through handlers if needed. |
With current semantics you mean passing |
|
Yes |
|
Replaced by #1405 |
Use an enum hidden to use channel senders directly. It should improve performances, and may allow later to use async sender methods.