feat: Implement Tox network profiler #1885

JFreegman · 2022-01-14T17:57:21Z

This change is

toxcore/net_profile.c

toxcore/net_profile.h

toxcore/TCP_client.h

toxcore/tox_private.h

codecov · 2022-01-15T00:13:44Z

Codecov Report

Attention: Patch coverage is 70.05348% with 56 lines in your changes missing coverage. Please review.

Project coverage is 72.99%. Comparing base (102a1fa) to head (e740b4e).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
toxcore/tox_private.c	57.40%	46 Missing ⚠️
toxcore/net_profile.c	90.69%	4 Missing ⚠️
toxcore/TCP_connection.c	71.42%	2 Missing ⚠️
toxcore/net_crypto.c	66.66%	2 Missing ⚠️
toxcore/TCP_server.c	83.33%	1 Missing ⚠️
toxcore/network.c	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1885      +/-   ##
==========================================
- Coverage   73.08%   72.99%   -0.09%     
==========================================
  Files         149      150       +1     
  Lines       30531    30714     +183     
==========================================
+ Hits        22313    22420     +107     
- Misses       8218     8294      +76

Flag	Coverage Δ
	`72.99% <70.05%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

iphydf

Reviewed 10 of 19 files at r1, 8 of 9 files at r2, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @JFreegman)

toxcore/tox_private.h, line 60 at r1 (raw file):

Previously, JFreegman wrote…

The reason I named it ZERO is because it has a different purpose depending on the packet type (see the above comment). Any suggestions?

Ok, that's fine then.

iphydf

Reviewed 1 of 19 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @JFreegman)

Green-Sky

For testing, we should be able to inject a custom struct Network and compare total packet numbers there.

Green-Sky · 2024-09-21T14:42:55Z

auto_tests/netprof_test.c

+ TOX_NETPROF_DIRECTION_RECV);
+ }
+
+ const unsigned long long total_packets = total_sent_count + total_recv_count;


generally prefer size named types like uint64_t over unsigned long long.

Green-Sky · 2024-09-21T14:55:44Z

toxcore/tox.c

@@ -18,6 +18,7 @@
 #include "DHT.h"
 #include "Messenger.h"
 #include "TCP_client.h"
+#include "TCP_server.h"


is this right?

Green-Sky · 2024-09-21T15:00:36Z

toxcore/TCP_client.c

@@ -20,6 +20,7 @@
 #include "logger.h"
 #include "mem.h"
 #include "mono_time.h"
+#include "net_profile.h"


Some places, like here, don't need to know the full type and a forward declaration instead would suffice.

Green-Sky · 2024-09-21T15:00:48Z

toxcore/TCP_client.h

@@ -15,6 +15,7 @@
 #include "logger.h"
 #include "mem.h"
 #include "mono_time.h"
+#include "net_profile.h"


Green-Sky · 2024-09-21T15:01:04Z

toxcore/TCP_common.h

@@ -10,6 +10,7 @@
 #include "crypto_core.h"
 #include "logger.h"
 #include "mem.h"
+#include "net_profile.h"


and here. (we only store a pointer)

Green-Sky · 2024-09-21T15:04:14Z

toxcore/TCP_server.h

@@ -15,6 +15,7 @@
 #include "logger.h"
 #include "mem.h"
 #include "mono_time.h"
+#include "net_profile.h"


Green-Sky · 2024-09-21T15:05:13Z

toxcore/net_crypto.h

@@ -20,6 +20,7 @@
 #include "logger.h"
 #include "mem.h"
 #include "mono_time.h"
+#include "net_profile.h"


here too ... i feel like we should forward declare it in some public places.

Green-Sky · 2024-09-21T15:12:43Z

toxcore/network.c

@@ -999,6 +1008,11 @@ int send_packet(const Networking_Core *net, const IP_Port *ip_port, Packet packe
 loglogdata(net->log, "O=>", packet.data, packet.length, ip_port, res);

 assert(res <= INT_MAX);
+
+ if (res == packet.length) {


i saw a data != nullptr earlier, should it be put everywhere, like here too?

Green-Sky · 2024-09-21T15:24:13Z

toxcore/tox_private.h

+ * Bootstrap info packet.
+ */
+ TOX_NETPROF_PACKET_ID_BOOTSTRAP_INFO = 0xf0,
+} Tox_Netprof_Packet_Id;


all public facing enums in toxcore now have a *_to_string() function. Since there are alot of values here, it would be very valuable.
Maybe it can also take in the type (tcp/udp), to print a sensible string? or maybe I am just dreaming here...

toxcore/tox_private.h

Green-Sky · 2024-09-22T12:18:18Z

I get a lot of 0x10 udp packets, which are

    /**
     * TCP data packet.
     */
    TOX_NETPROF_PACKET_ID_TCP_DATA             = 0x10,

supposed to be tcp according to your code.

Green-Sky · 2024-09-22T12:31:13Z

example table

Green-Sky · 2024-09-23T13:47:51Z

The beating heath of a toxcore:

tomato_netprof_tableheat2.mp4

JFreegman · 2024-09-24T02:42:59Z

I get a lot of 0x10 udp packets, which are
    /**
     * TCP data packet.
     */
    TOX_NETPROF_PACKET_ID_TCP_DATA             = 0x10,
supposed to be tcp according to your code.

Are you sure that's not a custom packet ID in your client/modified toxcore? I'm not seeing the same thing with Toxic. If you disable UDP you should see that it doesn't record any UDP packets at all, so if that's the case it definitely has to be a UDP packet with an invalid ID, in which case toxcore should be logging it.

Edit: It appears that you're recording 0x10 as a TCP ID as well in that bottom video.

Green-Sky · 2024-09-24T07:44:56Z

Are you sure that's not a custom packet ID in your client/modified toxcore? I'm not seeing the same thing with Toxic.

Yep, I am running master with netprof, no "modified" toxcore in that sense.

If you disable UDP you should see that it doesn't record any UDP packets at all, so if that's the case it definitely has to be a UDP packet with an invalid ID, in which case toxcore should be logging it.

Disabling UDP looks normal.

Edit: It appears that you're recording 0x10 as a TCP ID as well in that bottom video.

Well yes. BTW, I copied the names/descriptions from the enum and there are some inconsistencies.

Green-Sky · 2024-09-24T08:02:54Z

Ok, after setting a conditional breakpoint and not hitting any UDP, I checked your code and found this.

c-toxcore/toxcore/net_profile.c

Lines 83 to 86 in e740b4e

 // Special case - TCP data packets can have any ID between 0x10 and 0xff 

 if (id == NETPROF_TCP_DATA_PACKET_ID) { 

 return netprof_get_packet_count_id_range(profile, id, UINT8_MAX, dir); 

 }

I think the way this is done leads to confusion. It should not be done at the lower level, since profile does not know whether it is UDP or TCP...

JFreegman · 2024-09-24T13:49:27Z

Ok, after setting a conditional breakpoint and not hitting any UDP, I checked your code and found this.

c-toxcore/toxcore/net_profile.c

Lines 83 to 86 in e740b4e

// Special case - TCP data packets can have any ID between 0x10 and 0xff

if (id == NETPROF_TCP_DATA_PACKET_ID) {

return netprof_get_packet_count_id_range(profile, id, UINT8_MAX, dir);

}

I think the way this is done leads to confusion. It should not be done at the lower level, since profile does not know whether it is UDP or TCP...

I'm not sure what you mean. I'm not sure how your client ended up thinking those were UDP packets. It knows they're TCP packets because you passed a TCP packet ID and TCP profile to the function (netprof_get_packet_count_id()). If you had passed the UDP profile to that function with that same packet ID it would return 0.

Green-Sky · 2024-09-25T08:30:40Z

I'm not sure what you mean. I'm not sure how your client ended up thinking those were UDP packets. It knows they're TCP packets because you passed a TCP packet ID and TCP profile to the function (netprof_get_packet_count_id()). If you had passed the UDP profile to that function with that same packet ID it would return 0.

I prefer to use a simple for loop for 0-255, since it is forward compatible and covers custom packet ranges. Also your current solution would not allow us to use 0x10 for UDP packets in the future, since its just magic behavior, independent of whether it is UDP or TCP.
It should also be a simple code removal, so less code ...
I am also not a fan on how UDP and TCP share the same ID enum, it makes it less ergonomic to use imho.
I would however like to see the *_range() variants elevated to the "public" private api.

JFreegman · 2024-09-25T13:47:47Z

@Green-Sky

I prefer to use a simple for loop for 0-255, since it is forward compatible and covers custom packet ranges

0x00 through 0x09 are already in use. 0x10 through 0x255 are all data packet ID's, which includes custom packets. As far as forward compatibility goes, if we were to change TCP data packet ID's, we would completely break the TCP implementation regardless.

Also your current solution would not allow us to use 0x10 for UDP packets in the future, since its just magic behavior, independent of whether it is UDP or TCP.

I don't think you understand how recording/querying works. There are already numerous packet ID's that are used for UDP and TCP (which is why the API enums have shared ID's). There are three distinct profiles - TCP server, TCP client, and UDP, which are queried using the Tox_Netprof_Packet_Type enum. If you query packet ID 0x10 for UDP, then it will only give you information for UDP packets with the ID 0x10, while ignoring those ID's for the TCP profiles. A TCP packet with the ID 0x10 will not affect the UDP profile, regardless of whether or not there exists a UDP packet with the ID 0x10. That's why I'm not understanding how you ended up with your client thinking that 0x10 is a UDP packet ID.

JFreegman added the enhancement New feature for the user, not a new feature for build script label Jan 14, 2022

JFreegman added this to the v0.2.x milestone Jan 14, 2022

restyled-io bot mentioned this pull request Jan 14, 2022

Restyle feat: Implement Tox network profiler #1886

Closed

JFreegman force-pushed the netprof branch 11 times, most recently from b93009f to 75fd324 Compare January 14, 2022 20:13

iphydf requested changes Jan 14, 2022

View reviewed changes

pull-request-attention bot assigned JFreegman and iphydf and unassigned JFreegman Jan 14, 2022

JFreegman force-pushed the netprof branch 7 times, most recently from 84ab794 to 687d5b5 Compare January 15, 2022 00:07

iphydf approved these changes Jan 15, 2022

View reviewed changes

pull-request-attention bot assigned JFreegman and unassigned iphydf Jan 15, 2022

iphydf reviewed Jan 15, 2022

View reviewed changes

pull-request-attention bot assigned JFreegman and iphydf and unassigned iphydf and JFreegman Feb 12, 2024

JFreegman force-pushed the netprof branch 2 times, most recently from 7852d0b to a53b5b7 Compare February 28, 2024 15:58

JFreegman force-pushed the netprof branch 2 times, most recently from 39a7571 to f1bc5a4 Compare March 7, 2024 18:30

JFreegman force-pushed the netprof branch from f1bc5a4 to 5894cfb Compare March 28, 2024 01:49

JFreegman force-pushed the netprof branch 3 times, most recently from 5700751 to 53d50e4 Compare September 21, 2024 15:09

Green-Sky requested changes Sep 21, 2024

View reviewed changes

pull-request-attention bot assigned JFreegman and unassigned Green-Sky Sep 21, 2024

JFreegman force-pushed the netprof branch 2 times, most recently from 3972670 to e740b4e Compare September 21, 2024 17:15

Green-Sky reviewed Sep 22, 2024

View reviewed changes

toxcore/tox_private.h Show resolved Hide resolved

JFreegman force-pushed the netprof branch from e740b4e to 422c8bb Compare September 24, 2024 13:57

feat: Implement Tox network profiler

71bc9c1

JFreegman force-pushed the netprof branch from 422c8bb to 71bc9c1 Compare September 25, 2024 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement Tox network profiler #1885

feat: Implement Tox network profiler #1885

JFreegman commented Jan 14, 2022 •

edited by iphydf

Loading

codecov bot commented Jan 15, 2022 •

edited

Loading

iphydf left a comment

iphydf left a comment

Green-Sky left a comment

Green-Sky Sep 21, 2024

Green-Sky Sep 21, 2024

Green-Sky Sep 21, 2024

Green-Sky Sep 21, 2024

Green-Sky Sep 21, 2024

Green-Sky Sep 21, 2024

Green-Sky Sep 21, 2024

Green-Sky Sep 21, 2024

Green-Sky Sep 21, 2024

Green-Sky commented Sep 22, 2024

Green-Sky commented Sep 22, 2024

Green-Sky commented Sep 23, 2024

JFreegman commented Sep 24, 2024 •

edited

Loading

Green-Sky commented Sep 24, 2024

Green-Sky commented Sep 24, 2024

JFreegman commented Sep 24, 2024 •

edited

Loading

Green-Sky commented Sep 25, 2024

JFreegman commented Sep 25, 2024 •

edited

Loading

feat: Implement Tox network profiler #1885

Are you sure you want to change the base?

feat: Implement Tox network profiler #1885

Conversation

JFreegman commented Jan 14, 2022 • edited by iphydf Loading

codecov bot commented Jan 15, 2022 • edited Loading

Codecov Report

iphydf left a comment

Choose a reason for hiding this comment

iphydf left a comment

Choose a reason for hiding this comment

Green-Sky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Green-Sky commented Sep 22, 2024

Green-Sky commented Sep 22, 2024

Green-Sky commented Sep 23, 2024

JFreegman commented Sep 24, 2024 • edited Loading

Green-Sky commented Sep 24, 2024

Green-Sky commented Sep 24, 2024

JFreegman commented Sep 24, 2024 • edited Loading

Green-Sky commented Sep 25, 2024

JFreegman commented Sep 25, 2024 • edited Loading

JFreegman commented Jan 14, 2022 •

edited by iphydf

Loading

codecov bot commented Jan 15, 2022 •

edited

Loading

JFreegman commented Sep 24, 2024 •

edited

Loading

JFreegman commented Sep 24, 2024 •

edited

Loading

JFreegman commented Sep 25, 2024 •

edited

Loading