perf: WIP: Improve file transfer speed

[anthonybilinski]

• Don't only update connection RTT with shorter times - update with any
  new RTT
• Don't use only default value for TCP, use connection RTT
• Make lastest sent packet correctly measured

This is a purely sender-side fix. Sending from qTox with this change through a slow VPN, then to a TCP relay, then through Tor to Antox on my phone I get about 200KiB/s consistently for a 100MB file. Before the change, I got speeds of about 4KiB/s on the same setup.

---

This change is 

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[sudden6]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonybilinski)

---

toxcore/net_crypto.c, line 997 at r1 (raw file):

    const uint64_t temp_time = current_time_monotonic(mono_time);
    uint64_t l_sent_time = 0;

I think this variable can be removed completely, it seems to be never written and just used for comparison, which are then constant.

[anthonybilinski]

Testing these RTT changes more thoroughly and reproducibly, I don't think they improve transfer times. Building on them, I think another problem with RTT calculation is that a packets sent_time is reset when the packet is resent, but still used for RTT which can cause the following:
0ms: packet A is sent with RTT 500ms
550ms: request packet is received stating A wasn't received
550ms: A is resent, sent time is updated to now.
551ms: request packet is received stating A was received (from the first packet that was received).

which causes us to have an unreasonably short RTT of 51ms, and then think that all of our outstanding packets with a true RTT of ~500ms have been dropped because they haven't been delivered within 51ms. Using the original sent_time isn't necessarily valid either, since the packet may well have legitimately been dropped.

Tracking when packets are resent and not using their sent_time for RTT calculation reduces the number of packets we think are dropped and improves overall transfer speed by about 2x in my testing, and traffic shape is a little less saw-tooth-y, but still not great. It looks like after this change, the target speed coming out of congestion control is still very volatile, with packet_send_rate thrashing from ~0 to ~300 back and forth during transmission, I'll continue investigating.

[zoff99]

@anthonybilinski is toxcore actively measuring RTT? and can you tell me how?
because i need RTT for my stuff (and i reimplemented it since i could not find it)

[codecov]

Codecov Report

Merging #1323 (8b8d6ab) into master (0a2190f) will increase coverage by 0.26%.
The diff coverage is 100.00%.

❗ Current head 8b8d6ab differs from pull request most recent head bd283a9. Consider uploading reports for the commit bd283a9 to get more accurate results

@@            Coverage Diff             @@
##           master    #1323      +/-   ##
==========================================
+ Coverage   76.88%   77.15%   +0.26%     
==========================================
  Files         105      105              
  Lines       17912    17580     -332     
==========================================
- Hits        13772    13563     -209     
+ Misses       4140     4017     -123     

Impacted Files	Coverage Δ
toxcore/net_crypto.c	93.99% <100.00%> (-0.20%)	⬇️
toxcore/events/friend_status.c	65.71% <0.00%> (-10.38%)	⬇️
toxcore/events/friend_connection_status.c	66.66% <0.00%> (-9.93%)	⬇️
toxcore/events/friend_typing.c	65.71% <0.00%> (-9.29%)	⬇️
toxcore/events/friend_name.c	61.22% <0.00%> (-9.27%)	⬇️
toxcore/events/friend_status_message.c	62.00% <0.00%> (-8.97%)	⬇️
toxcore/events/self_connection_status.c	72.72% <0.00%> (-8.67%)	⬇️
auto_tests/tox_events_test.c	91.48% <0.00%> (-6.39%)	⬇️
toxcore/events/friend_message.c	69.23% <0.00%> (-4.62%)	⬇️
toxcore/events/conference_connected.c	17.39% <0.00%> (-4.04%)	⬇️
... and 25 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a2190f...bd283a9. Read the comment docs.

[anthonybilinski]

I did some further testing after having a couple people tell me this improved their transfer speeds significantly.

Test Setup

• Toxcore 0.2.12
• Run two qTox instances, one inside a linux network namespace with a VPN to France from West coast NA.
• VPN is using UDP, i.e. lossy connection.
• Distance between friends is effectively twice across (the atlanatic + North America), round trip is 4X atlantic + NA
• Network upload max is ~8 Mb/s, with fairly heavy usage by other protocols.
• Both clients were restarted between each test (to change toxcore lib)
• The same profile sent the file each time.
• Both profiles were using UDP.
• Results are in order of the test, all tested back-to-back, to reduce network variations impacting the results.

Results

With change?	File size	Time	Average speed	Rough traffic shape
No	85723211	523s	1.3 Mb/s	ramps up linearly, then drops quickly to zero once max speed is reached. Sawtooth wave.
Yes	85723211	152s	4.5 Mb/s	speed bounces around a bit between 600 KiB/s and 700 KiB/s, wave looks overall fairly flat with some random ripple
No	85723211	281s	3.1 Mb/s	Better than first test, finished about 1/3rd of the file before going to 0 speed. Still bounced up and down a few times.
Yes	85723211	172s	5.0 Mb/s	Sat at around 1Mb/s for a long time at start until rising to around 8 Mb/s and staying there.
No	85723211	235s	3.6 Mb/s	same as other tests
Yes	85723211	451s	1.9 Mb/s	Spiky. Looks like the first test without the patch.
No	85723211	391s	2.2 Mb/s	same
Yes	51878640	425s	1.2 Mb/s	Disconnected during transfer.
No	85723211	313s	2.7 Mb/s
Yes	85723211	160s	5.4 Mb/s
No	85723211	435s	2.0 Mb/s
Yes	85723211	439s	2.0 Mb/s
No	85723211	401s	2.1 Mb/s
Yes	85723211	140s	6.1 Mb/s
No	85723211	371s	2.3 Mb/s
Yes	85723211	387s	2.2 Mb/s	File transfer widget was frozen on both sender and receiver side, possibly indicative of a core issue, probably not.
No	85723211	362s	2.4 Mb/s
Yes	85723211	338s	2.5 Mb/s
No	85723211	366s	2.3 Mb/s
Yes	74711274	336s	2.2 Mb/s	Discnonected during transfer.
No	85723211	392s	2.2 Mb/s
Yes	45016785	227s	2.0 Mb/s	Disconnected during transfer.

With this I can confidently say that the PR in the state it's in now is not a reliable fix, and could be related to the 3 disconnects during transfers that I saw. I think my last comment still needs to be looked into.

ping @Monsterovich, I don't recommend including this in protox.

[zoff99]

@anthonybilinski can you write an auto_test for this?

[Monsterovich]

I don't recommend including this in protox.

I'm using this patch on my PC for a few months (since toxcore 0.2.9). Without it the transfer speed in qTox is complete garbage. This patch keeps my transfer speed at 9-11 mb/s. In normal toxcore qTox reaches the highest speed and then it dramatically drops down to zero, then it starts to grow up again, repeat. On portable devices which use Wi-fi or LTE it's twice that bad. I'm going to make a separate version of Protox which includes this patch. I also recommend to merge this patch ASAP.

My tests on protox (over Wi-fi):
P.S. The average upload speed without this patch is 50-70 kb/s due to frequent speed drops, with this patch it's stable 300-400 kb/s.

[zoff99]

testing this with a specific client (be it qtox or any other) always includes client behaviour aswell. and then we can not be sure whats actually happening.

we need to code a test for this, to test in a neutral environment.

[Monsterovich]

testing this with a specific client (be it qtox or any other) always includes client behaviour aswell

I have exactly the same behavior in qTox and Protox with or without this PR. I don't know what are you talking about.

[netlify]

✔️ Deploy Preview for c-toxcore ready!

🔨 Explore the source changes: bd283a9

🔍 Inspect the deploy log: https://app.netlify.com/sites/c-toxcore/deploys/620089d032ca040007f294d6

😎 Browse the preview: https://deploy-preview-1323--c-toxcore.netlify.app