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

fix large msg sending logic #274

Merged
merged 2 commits into from
Aug 3, 2023
Merged

fix large msg sending logic #274

merged 2 commits into from
Aug 3, 2023

Conversation

al8n
Copy link
Contributor

@al8n al8n commented Jul 27, 2023

According to @bacv, the problem with the current network throughput is if the message size is bigger than the calculated step throughput, then it never gets sent.

The solution in this PR is "partial send":

e.g.

Let's say, if we have a large message with size 15, and the throughput is 5. In the first step, we mock partially send behavior, so send 5 (remaining message size is 10). In the second step, partially send another 5 (the remaining message size is 5). In the third step, we send the final 5, so the large message can be sent through 3 steps in total.

@al8n al8n added this to the Simulation App milestone Jul 27, 2023
@al8n al8n requested a review from bacv July 27, 2023 08:05
@al8n al8n self-assigned this Jul 27, 2023
@al8n al8n requested a review from danielSanchezQ July 27, 2023 08:06
Copy link
Member

@bacv bacv left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Comment on lines 285 to 296
if should_delay {
let mut cap = node_capacity.current_load.lock();
let sent = node_capacity.capacity_bps - *cap;
*cap = node_capacity.capacity_bps;
if message.partial_send(sent) == 0 {
let to_node = self.to_node_senders.get(&to).unwrap();
to_node
.send(message.clone())
.expect("node should have connection");
node_capacity.decrease_load(sent);
return false;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: can we put this in a named method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Collaborator

@danielSanchezQ danielSanchezQ left a comment

Choose a reason for hiding this comment

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

LGTM

@al8n al8n merged commit c16b794 into master Aug 3, 2023
2 checks passed
zeegomo pushed a commit that referenced this pull request Aug 3, 2023
* fix large msg sending logic
zeegomo pushed a commit that referenced this pull request Aug 7, 2023
* fix large msg sending logic
@jakubgs jakubgs deleted the simulation-msg branch February 15, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants