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

[Debugger] Move most profilers to ServersDebugger, fix core includes. #57715

Merged

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Feb 6, 2022

In this PR:

  • New extensible EngineProfiler class.
    Uses GDExtension, replaces old Callable system for profilers, and is also used internally.
  • Move most profilers to ServersDebugger.
    Also splits bandwidth/rpc profiler (RPCProfiler is now in SceneDebugger).
  • Move servers-related behaviours to ServersDebugger.
    Forcing draw during debug break is now handled by ServersDebugger, and only happens when the proper message is sent from the EditorDebugger ("servers:draw").
    In a similar way, briging the window in foreground is now also handled by ServersDebugger upon receiving "servers:foreground" which is sent by the EditorDebugger when resuming from a break ("continue").

Should remove all remaining bogus include in core/debugger (#53295).

@Faless Faless added this to the 4.0 milestone Feb 6, 2022
@Faless Faless requested review from a team as code owners February 6, 2022 16:28
Uses GDExtension, replaces old Callable system for profilers, and is
also used internally.
Also splits bandwidth/rpc profiler (RPCProfiler is now in
SceneDebugger).
Forcing draw during debug break is now handled by ServersDebugger, and
only happens when the proper message is sent from the EditorDebugger
("servers:draw").
In a similar way, briging the window in foreground is now also handled
by ServersDebugger upon receiving "servers:foreground" which is sent by
the EditorDebugger when resuming from a break ("continue").
@Faless Faless force-pushed the debugger/4.x_core_includes_and_servers branch from 9c1d685 to 6583797 Compare February 6, 2022 16:37
@Faless
Copy link
Collaborator Author

Faless commented Feb 6, 2022

Note: this also improve the network profiler to track all multiplayer bandwidth.

I still find it a bit tricky, because it cannot give accurate results (and never did):

  • The reported values are for the uncompressed packets, which can result in it being from 20% to 100% more than what actually gets transmitted over the wire.
  • It does not take into account when the same packet is sent to multiple peers because it doesn't know if the underlying peer will send that packet once when sending to broadcast, or excluding only one peer (because it might be the server, a client, or a p2p mesh peer).

@Calinou
Copy link
Member

Calinou commented Feb 6, 2022

The reported values are for the uncompressed packets, which can result in it being from 20% to 100% more than what actually gets transmitted over the wire.

The reported values could be multiplied by 0.8 when compression is enabled. It won't always be accurate, but it will likely get closer to the real value in most situations.

@Faless
Copy link
Collaborator Author

Faless commented Feb 6, 2022

The reported values could be multiplied by 0.8 when compression is enabled. It won't always be accurate, but it will likely get closer to the real value in most situations.

The MultiplayerAPI doesn't know if the underlying network peer is applying compression or not.

@Faless
Copy link
Collaborator Author

Faless commented Feb 6, 2022

We might want to apply the compression at the MultiplayerAPI-level, that would give a bit more accurate results (still limited by the fact that the value will not include broadcast bandwidth accurately).
On that side, implementing peer signalling in the MultiplayerAPI instead of in MultiplayerPeers might help, but needs proper investigation as it doesn't seem trivial right now.

@reduz
Copy link
Member

reduz commented Feb 7, 2022

This looks good to me, I guess maybe it shuold ask the peer for the used bandwidth?

@Faless
Copy link
Collaborator Author

Faless commented Feb 7, 2022

Yeah, we can add a get_bandwidth_usage method to MultiplayerPeer which is supposed to return in/out bandwidth and leave the implementation to each lower lever peer I guess.
This kinda leaves the feature to be implemented at the lower level, but I agree that's probably the only right way to get accurate values.
Maybe we could actually keep the current calculation as a fallback (in case the peer does not support it) but show the other if available.

@reduz
Copy link
Member

reduz commented Feb 8, 2022

@Faless Makes sense

@Faless
Copy link
Collaborator Author

Faless commented Feb 9, 2022

@reduz can we still merge this? The code is pretty unrelated to the multiplayer API, I would rather make those changes in a separate PR. I can remove the extra bandwidth logs if you wish (it's just 3 lines, it might as well just go in as is for now).

@akien-mga akien-mga merged commit d22ac13 into godotengine:master Feb 9, 2022
@akien-mga
Copy link
Member

Thanks!

@MikeSchulze
Copy link

MikeSchulze commented Feb 4, 2023

@Faless Hi is there an tutorial how to implement your own EngineProfiler?
I can't find any example how to use it

I looking for a way to build code coverage, maybe a Profiler can help to track the executed functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants