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

Sending takes to long #7

Closed
TimonPost opened this issue Oct 6, 2018 · 6 comments
Closed

Sending takes to long #7

TimonPost opened this issue Oct 6, 2018 · 6 comments
Milestone

Comments

@TimonPost
Copy link
Owner

So when @torkleyy and I where adding examples we noticed some huge delays.

I did some tests to monitor how long it took to send a message and it took 1 second. So there is some bug somewhere :)

test results:

= Message took 72.982µs to send =
Moving to lat: 10.555, long: 10.55454, alt: 1.3
=Message took 990.276878ms to send =
Moving to lat: 5.4545, long: 3.344, alt: 1.33
= Message took 990.114451ms to send =
Received text: "Some information"

So you can see that messages take about 1 second to be processed when by the send method.

After debugging I found that create_connection_if_not_exists method takes 0.5 - 0.9 seconds to execute.

Where after I found out that acquiring the lock on connections is taking to long in create_connection_if_not_exists method.

  let mut lock = self
            .connections
            .write()
            .map_err(|_| NetworkError::AddConnectionToManagerFailed)?;

Why does acquiring the lock takes so long?

@fhaynes
Copy link
Collaborator

fhaynes commented Oct 6, 2018

Let's find out! =)

@fhaynes
Copy link
Collaborator

fhaynes commented Oct 6, 2018

@TimonPost and I tracked it down to a lock being hold too long here:

l.seq_num = l.seq_num.wrapping_add(1);
In the pre_process_packet function.

Scoping it resolves it:

{
        let mut l = connection
            .write()
            .map_err(|_| NetworkError::AddConnectionToManagerFailed)?;

        raw_packet = RawPacket::new(
            l.seq_num,
            &packet,
            l.their_acks.last_seq,
            l.their_acks.field,
        );
        // increase sequence number
        l.seq_num = l.seq_num.wrapping_add(1);
}

However, we do not know why wrapping_add was causing the issue. It takes ownership when used, and we suspect that is related.

We're going to leave this issue open until we know the root cause, but we'll push a fix in the meantime.

@msiglreith
Copy link

So serialization takes too long and the lock guard isn't dropped beforehand?

@TimonPost
Copy link
Owner Author

I see this is just part of the solution when sending about ten packets the same problem will still occure. Int the PR is an suggestion to fix this. I am going to try that out.

@TimonPost
Copy link
Owner Author

This will be fixed in #5

@fhaynes
Copy link
Collaborator

fhaynes commented Oct 7, 2018

This should be resolved now with #9. I'm going to open another issue for more rigorous testing, though, and close this one.

@fhaynes fhaynes closed this as completed Oct 7, 2018
@LucioFranco LucioFranco added this to the 0.1.0 milestone Oct 7, 2018
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

No branches or pull requests

4 participants