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

add microbatching to proxy forward loop #206

Closed
wants to merge 5 commits into from

Conversation

grooviegermanikus
Copy link
Collaborator

@grooviegermanikus grooviegermanikus commented Sep 21, 2023

add 40ms between next message in channel and draining the channel; limit the batch size to 50

this chart (metric literpcproxy_batch_size) shows the difference between no microbatching and 40ms: batch size of 1 is without microbatching:

Bildschirmfoto 2023-09-21 um 11 52 18

bench/transactions.csv Outdated Show resolved Hide resolved
let mut transactions_batch: Vec<Vec<u8>> = packet.transactions.clone();

'more: while let Ok(more) = per_connection_receiver.try_recv() {
'drain_loop: while let Ok(more) = per_connection_receiver.try_recv() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need to specify drain_loop, seems like goto which is not required right ?

Copy link
Collaborator Author

@grooviegermanikus grooviegermanikus Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not needed but reads more clearly IMO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo this is a bad design. if u need labels to break out loops. could be done better

Copy link
Collaborator Author

@grooviegermanikus grooviegermanikus Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I do not need them - it just reads better!

compare it with this - when I scan that code I constantly look for the scope of breaks:
image

break+continue are so hard to read because they are so contextual IMO - this can be solved by labeling the scope

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was staring a bit on the code, but so little room for improvement - can you suggest something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I read more clearly when i read continue and break logic.

@grooviegermanikus
Copy link
Collaborator Author

Max advices to not do that because transactions are MTU sized

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.

None yet

3 participants