-
Notifications
You must be signed in to change notification settings - Fork 172
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
feat!: make callback opaque #1383
Closed
Closed
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 beingRingChannel
, which is also included inCallbackInner
,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.
I don't see the how this PR changes anything here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Branch prediction enters the room.
Still better than the previous "by value" callback, see godbolt. Why is the produced code more complex?
And then, we will change the internal enum, we don't care, it's internal, and it's only a dozen lines to change.
Trust branch prediction, especially if you are using only callbacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.