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

feat: increase NGC lossy custom packet size #2384

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

Green-Sky
Copy link
Member

@Green-Sky Green-Sky commented Jun 15, 2023

since toxcore places the responsibility to ensure the data does not get lost on the user (programmer), this can be seen as backwards compatible.


This change is Reviewable

@iphydf
Copy link
Member

iphydf commented Aug 16, 2023

Why 1000? Why is lossless larger?

@Green-Sky
Copy link
Member Author

  1. this is a draft, to get my point across :)
  2. lossless packets are split into parts of 500
  3. the point: during filetransfers for ngc experimentation i discovered that my home network equipment did not like the spam of small packets at all, it stayed in low power mode, and only woke up when i started a tcp download simultaneously or increased toxcores packet size
    1000 here is just and arbitrary number that was large enough for me. It made filetransfers 2x faster.
    i cant imagine being the only one that is going to come across this issue. besides, larger payload means less overhead per transmitted byte :)

@Green-Sky
Copy link
Member Author

quote from https://www.libtorrent.org/utp.html

The other approach of picking a very conservative packet size, that would be very unlikely to get fragmented has the following drawbacks:

  1. People on good, normal, networks will be penalized with a small packet size. Both in terms of router load but also bandwidth waste.
  2. Software routers are typically not limited by the number of bytes they can route, but the number of packets. Small packets means more of them, and more load on software routers.

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (9b3c108) 71.52% compared to head (15ee46d) 71.65%.

Files Patch % Lines
toxcore/group_chats.c 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2384      +/-   ##
==========================================
+ Coverage   71.52%   71.65%   +0.12%     
==========================================
  Files          75       75              
  Lines       25195    25200       +5     
==========================================
+ Hits        18020    18056      +36     
+ Misses       7175     7144      -31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zoff99
Copy link

zoff99 commented Oct 14, 2023

still same question why 1000 ?

since tox already uses 1400 for everything else.

#define TOX_MAX_MESSAGE_LENGTH 1372

#define TOX_MAX_CUSTOM_PACKET_SIZE 1373

#define ONION_MAX_PACKET_SIZE 1400

#define MAX_CRYPTO_PACKET_SIZE (uint16_t)1400

1000 (and also 500) does not make any sense

@Green-Sky
Copy link
Member Author

still same question why 1000 ?

Treat this pr as a proof of concept, until we decide how large exactly.

  1. and 3. from my comment still apply feat: increase NGC lossy custom packet size #2384 (comment)

@iphydf iphydf added this to the v0.2.19 milestone Nov 10, 2023
toxcore/group_common.h Outdated Show resolved Hide resolved
@pull-request-attention pull-request-attention bot assigned Green-Sky and iphydf and unassigned Green-Sky and iphydf Nov 12, 2023
@Green-Sky Green-Sky changed the title wip increase NGC lossy custom packet size to 1000 wip increase NGC lossy custom packet size Nov 24, 2023
@Green-Sky Green-Sky changed the title wip increase NGC lossy custom packet size feat: increase NGC lossy custom packet size Nov 24, 2023
@Green-Sky Green-Sky force-pushed the ngc_lossy_1000 branch 2 times, most recently from ca97d7b to 919e946 Compare November 24, 2023 21:38
@Green-Sky Green-Sky marked this pull request as ready for review November 24, 2023 21:42
toxcore/group_chats.c Show resolved Hide resolved
toxcore/group_chats.c Outdated Show resolved Hide resolved
toxcore/group_chats.c Outdated Show resolved Hide resolved
toxcore/group_chats.c Outdated Show resolved Hide resolved
@pull-request-attention pull-request-attention bot assigned Green-Sky and unassigned iphydf Nov 24, 2023
@Green-Sky Green-Sky marked this pull request as draft November 25, 2023 00:48
@Green-Sky Green-Sky force-pushed the ngc_lossy_1000 branch 2 times, most recently from fd7aad5 to f2f4a3d Compare November 25, 2023 16:10
@Green-Sky Green-Sky marked this pull request as ready for review November 25, 2023 16:15
@pull-request-attention pull-request-attention bot assigned iphydf and unassigned Green-Sky Nov 25, 2023
@pull-request-attention pull-request-attention bot assigned Green-Sky and iphydf and unassigned iphydf and Green-Sky Dec 4, 2023
@Green-Sky Green-Sky force-pushed the ngc_lossy_1000 branch 2 times, most recently from 7b7c8fb to 161e11f Compare December 4, 2023 22:58
@pull-request-attention pull-request-attention bot assigned Green-Sky and iphydf and unassigned iphydf and Green-Sky Dec 5, 2023
@Green-Sky Green-Sky force-pushed the ngc_lossy_1000 branch 2 times, most recently from 4a356ec to fbe4832 Compare December 9, 2023 20:07
@pull-request-attention pull-request-attention bot assigned Green-Sky and unassigned iphydf Dec 14, 2023
@Green-Sky Green-Sky force-pushed the ngc_lossy_1000 branch 3 times, most recently from a534f55 to 38d17b2 Compare December 14, 2023 23:52
@Green-Sky Green-Sky merged commit 15ee46d into TokTok:master Dec 15, 2023
53 checks passed
@Green-Sky Green-Sky deleted the ngc_lossy_1000 branch December 23, 2023 19:24
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants