-
Notifications
You must be signed in to change notification settings - Fork 16
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
refactor(sv-webserver): Improve performance on subscribing #435
base: main
Are you sure you want to change the base?
Conversation
* feat(repo): Added rest endpoints Postman documentation * feat(repo): Added open-api documentation
* ci(release): Release v0.0.26 * ci(repo): Format files in the release PR --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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 left a few questions, but am already pre-approving it in case I wont be able to see your replies later. Feel free to merge it ofc ! Nice changes.
@@ -90,7 +90,8 @@ impl<R: Record> Stream<R> { | |||
response: &StreamResponse, | |||
) -> Result<(), StreamError> { | |||
let broker = self.broker.clone(); | |||
let payload = response.encode_json()?; | |||
let response = response.clone(); | |||
let payload = spawn_blocking(move || response.encode_json()).await??; |
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.
Why are we spawn_blocking here ? THis has a huge performance overhead. The whole method is async, cannot we just call response.encode_json()
?
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.
If its something I am not assessing correctly, please feel free to close this comment and move on with the merge. I dont want to hold it off by any means.
Err(err) => break Some(CloseAction::Error(err)), | ||
Ok(Some(close_action)) => break Some(close_action), | ||
Ok(None) => {} | ||
Ok(AggregatedMessage::Binary(bin)) => { |
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.
The AggregatedMessage is usually a bundle of X message received at a time from what I know about Actix, do we want to have this ? I was first using it and realized messages were being accumulated / buffered then sent over the wire to us.
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.
and there was indeed a performance delay on my end when I did the first ws impl .
No description provided.