Skip to content
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

codec send Message instead of Vec<Message> #1466

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rukai
Copy link
Member

@rukai rukai commented Feb 8, 2024

blocked on:
#1467
#1465

The first commit is performance neutral but for some reason the 2nd commit is heavily regressive:
image
From looking at a profiler we spend a lot more time in the server.rs spawn_read_write_tasks writev

I think yielding control back to the executor is preventing us from properly batching our writes, resulting in more total time spent in writev.

I have no idea why its only the writev to the client that is affected.
Oh! Maybe the writes to the DB were never batched properly?
Yep thats it!
image
in_w is sent a vec of a single item here.
So, we really do want to send a full message batch to the encoder.
But! If we fix the encoding for the DB connection we will have a big performance win.

@rukai rukai force-pushed the codec_messages_to_message branch 5 times, most recently from 993b261 to 333c777 Compare February 9, 2024 06:45
@shotover shotover deleted a comment from github-actions bot Feb 18, 2024
@shotover shotover deleted a comment from github-actions bot Feb 18, 2024
@shotover shotover deleted a comment from github-actions bot Feb 18, 2024
@shotover shotover deleted a comment from github-actions bot Feb 18, 2024
@rukai
Copy link
Member Author

rukai commented Feb 18, 2024

I've reverted the 2nd commit since that was clearly a regression.
For the first commit I will keep this as a draft and evaluate this again after we resolve #1471

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant