-
-
Notifications
You must be signed in to change notification settings - Fork 23.7k
Add support for profiling GDScript with tracy. #113279
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
base: master
Are you sure you want to change the base?
Conversation
471292f to
f9cec74
Compare
f9cec74 to
60cfe84
Compare
|
Note that Godot currently crashes on exit (when tracy is used). This doesn't affect the ability of the PR to trace events. |
60cfe84 to
65af0fc
Compare
This adds macro `GodotProfileZoneGroupedFirstScript`, and uses interning for speedy lookups. Co-authored-by: Samuel Nicholas <nicholas.samuel@gmail.com>
65af0fc to
acefbbb
Compare
|
Tested this for a bit. I think it's pretty good but it fails to capture engine calls from GDScript. So if a script function calls something else and that something is slow, it's not really possible to tell. I wonder if you can hook some of the API into the |
Right, since tracy is a tracing profiler, by nature we only trace what we explicitly annotate. (Most) engine functions aren't annotated, so they aren't traced.
This should be possible — and indeed, #112707 had some support for it. |
I put a lot of effort into capturing all calls from gdscript , specifically tailoring it for capturing engine calls as well. I was originally capturing all opcodes separately, and figured out which ones could be captured at the start, and which ones i needed to keep. I havent had time to look over and test the rest yet, but this makes me sad. |
It's not a problem, we can add those back in a follow-up PR. But I much prefer separation of concerns in PRs, to give each added feature the attention that's needed to review it. |
|
To go a little bit more in-depth with my response: First off, thank you for putting in the time to add support for tracing system calls. Judging by @vnen's and @AdriaandeJongh's reaction, this is a feature that is anticipated, and will be useful for profiling GDScript. Still, I'll try to explain why I decided to remove it from this PR. The reason for that is simply that I want to keep the complexity of the PR as low as possible. I strongly believe in the power of code reviews, and code reviews are most effective when the code is easy to understand, and focuses on one change at a time. In short, I'm not removing system call tracing because I believe it should not be added — I'm just removing it temporarily to make this PR easier to review and merge. I'm looking forward to your follow-up PR to re-add those calls! You've also asked why I've added interning for Use Tracy's dynamic source location API: This is you did in your PR #112707. However, using this API makes Tracy copy and leak the strings. In bigger projects, this can lead to large amounts of data being leaked, effectively making us unable to trace for longer than a few seconds (as described in the Halls of Torment retrospective). Use and leak SourceLocationData: I (inadvertently) tested this. Tracy supports a maximum of 32k Considering this, we need a way to keep track of |
profiler_pathoption toSConsbuilds, with support fortracyandperfetto. #104851This PR allows users to profile their games' GDScript functions using the tracy profiler.
It is a collaborative work between @enetheru and me.
Users can use the tracy instructions on the docs (currently in PR form, soon available here) to start profiling their games. Note that profiling with tracy requires recompiling the engine.
I've profiled tps-demo for about ~10 minutes straight, and both Godot and tracy stay responsive, without any apparent slowdowns or leaks.
Approach
Tracy profile zones generally make use of
constexpr tracy::SourceLocationData *, which are passed to the profiling macros. With this trick, tracy can introspect millions of events, because it only needs to store a start location, and end location, and a pointer to theSourceLocationData.While tracy supports dynamic source locations / strings, effectively it just copies the contents and leaks it. This works, but is wasteful, because tons of data is leaked. A previous implementation (by the developers of Halls of Torment) had trouble with this approach, because it led to gigabytes of data filling up after a few seconds of profiling (I don't know for sure they used tracy's leaky approach, but it would match the observations).
Instead of using tracy's dynamic source location API, this PR interns source locations for
GDScriptFunctionobjects, and leaks those instead. This means that every function is only leaked once, instead of once per call, which allows us to profile for much, much longer. This takes inspiration fromStringName's interning approach.The implementation is fully contained in the tracy glue internals, and doesn't affect games that don't compile with tracy. It is further contained to
.cppfiles includingprofiling.h, so recompiling with a profiler attached stays trivial.(it would be possible to attach a
void *toGDScriptFunctioninstances instead of interningSourceLocationData. This would be faster, but it wouldn't be self-contained anymore. In the future, if people start running into performance overhead from the profiler, we can amend the implementation to do this instead).