-
-
Notifications
You must be signed in to change notification settings - Fork 20k
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
Add network profiler #31870
Add network profiler #31870
Conversation
This looks good 🙂 Would it be possible to estimate the compressed size if some compression method is enabled? Or at least, display a tooltip when compression is enabled to indicate that the actual bandwidth used is lower than the estimated bandwidth. |
There's a typo in the commit message:
|
f680874
to
f8eaa8a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! 🥇 This is a much needed feature. See my comment on units (nothing super important, but maybe something to be fixed before final release).
Another thing, any particular reason not to have the profiling functions in it's own class? Just wondering, I think it's fine this way too given it's few lines for now, but I would have probably split it.
Not a blocker though, refactoring could in case be done at a later time when/if we expand on that.
editor/editor_network_profiler.cpp
Outdated
unit = "MB"; | ||
v /= 1048576.0; | ||
} else if (v > 1024.0) { | ||
unit = "KB"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the cost of sounding pedantic, I think if we divide by 1024 we should use KiB/MiB/GiB (standard binary prefix), or divide by 1000 if we want to keep KB(kB actually)/MB/GB . See man(7) units and ISO 80000-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just use String::humanize_size()
here? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I didn't even know we had that function... still, the same considerations apply to that function I guess, which for now is using custom binary prefix 🤢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change that in a later PR 🙂 It's currently not exposed to GDScript, so that won't break compatibility to my knowledge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Faless I agree, it started with a couple of lines in a couple of methods but it eventually grew quite a bit. I will be making a new PR (probably after 3.2) with some UI improvements, so I will take the chance to split the code into its own class.
@Calinou I checked String::humanize_size()
while working on the PR but it uses "Bytes"
which was too long for the fields I was using. I can do the change to KiB, MiB, etc. in place. unless you prefer to add some options to humanize_size
and keep it centralized (note that the memory-related performance counters also use a custom "humanize" code which I copied for my use case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JFonS I see, I guess we could change "Bytes" to "B" in the future as well.
f8eaa8a
to
8244f53
Compare
Should be ready to merge once CI passes. |
Very nice! Would be cool to have it work with active TCP Sockets (StreamPeerTCP) as well |
This PR adds a new tab in the debugger panel that allows for some basic network profiling. It contains a list of all the nodes that communicate over the multiplayer API and, for each one, some counters on the amount of incoming and outgoing network interactions.
It also features a bandwidth meter that displays the total bandwidth usage at any given moment (computed using the size of the packets sent in the last second).
This work has been kindly sponsored by IMVU.