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

WIP: COMMON: Added tracy profiler support #2686

Closed
wants to merge 1 commit into from

Conversation

@mgerhardy
Copy link
Contributor

@mgerhardy mgerhardy commented Dec 15, 2020

Frame based, on-demand profiler: https://github.com/wolfpld/tracy

https://github.com/wolfpld/tracy/releases/download/v0.7.4/tracy.pdf

Open Tasks

  • Decide how to integrate the tracy code into the scummvm build
  • Integrate the traceFrameEnd() call into each backend to the "proper" place
  • Mark threads with their names
  • Implement MutexManger tracy integration
  • OpenGL frame markers
@mgerhardy mgerhardy force-pushed the mgerhardy:pr/tracy-profiler branch from 2482a7c to 331ea78 Dec 15, 2020
configure Outdated
fi
define_in_config_if_yes $_tracy 'ENABLE_TRACY'
define_in_config_if_yes $_tracy 'TRACY_ON_DEMAND'
define_in_config_if_yes $_tracy 'FORBIDDEN_SYMBOL_ALLOW_ALL'

This comment has been minimized.

@mgerhardy

mgerhardy Dec 15, 2020
Author Contributor

Hack until I know how to solve this in a better way.

This comment has been minimized.

@sev-

sev- Dec 17, 2020
Member

Ugh, indeed.

This comment has been minimized.

@mgerhardy

mgerhardy Dec 17, 2020
Author Contributor

Do you have any hint for me on how to solve this? I think that profiling should never get delivered in a release build anyway. So maybe activating this isn't a real problem for local builds?

On could just get rid of common/trace.h and do the

#ifdef TRACE_ENABLE
#define FORBIDDEN_SYMBOL_ALLOW_ALL
#include "common/tracy/Tracy.hpp"
#endif

everywhere in the c++ files where tracing is desired. But that would imo pollute the code with too much Tracy specific stuff. I'm really open for suggestions on how to solve this in a clean and nice way.

This comment has been minimized.

@mgerhardy

mgerhardy Jan 10, 2021
Author Contributor

I just can't find a better way to do this. Is it a problem to activate this for those builds where tracy is enabled? I mean, tracy wouldn't be enabled for the CI builds, so errors would directly pop up (as FORBIDDEN_SYMBOL_ALLOW_ALL wouldn't be defined here of course.

@@ -1070,6 +1070,7 @@ const Feature s_features[] = {
{ "translation", "USE_TRANSLATION", false, true, "Translation support" },
{ "vkeybd", "ENABLE_VKEYBD", false, false, "Virtual keyboard support"},
{ "eventrecorder", "ENABLE_EVENTRECORDER", false, false, "Event recorder support"},
{ "tracy", "ENABLE_TRACY", false, false, "Tracy profiler support"},

This comment has been minimized.

@mgerhardy

mgerhardy Dec 15, 2020
Author Contributor

This is also not yet done - will be finished if we can agree to include this - but TRACY_ENABLE must be given as define. That is not yet done here.

Copy link
Member

@sev- sev- left a comment

I think this could be a good addition. I have a couple of question and notes, though.

@@ -2,3 +2,6 @@
path = devtools/create_prince
url = https://github.com/scummvm/game-translations
branch = prince-and-the-coward
[submodule "common/tracy"]

This comment has been minimized.

@sev-

sev- Dec 17, 2020
Member

I'm not a big fan of throwing in code to the codebase like this. Would like to hear opinions of other devs.

I see that the lib author made it very straightforward by including everything into once source file, though.

This comment has been minimized.

@mgerhardy

mgerhardy Dec 17, 2020
Author Contributor

Absolutely true - this is just for letting people play with it to decide whether a proper integration is worth the work. Just like to get feedback if I should continue to integrate it.

This comment has been minimized.

@mgerhardy

mgerhardy Dec 17, 2020
Author Contributor

The "problem" that I see here is that it's not a single c++ source file - but a lot of different headers and a lot of c++ files. Also the capture files are save, but the network protocol might differ from version to version. So it's the best to keep the profiler ui and the profiler "server" in sync somehow.

This comment has been minimized.

@mgerhardy

mgerhardy Jan 10, 2021
Author Contributor

The author is suggesting this as integration method, too - https://github.com/wolfpld/tracy/blob/master/manual/tracy.tex#L388

One could also just assume that people who want to profile can copy the source files manually to the proper location and the build system will pick it up if that files exist. This wouldn't be a problem for make - but for msvc you would most likely have to re-run create_project for it.

common/trace.h Outdated Show resolved Hide resolved
common/trace.h Outdated Show resolved Hide resolved
configure Outdated
fi
define_in_config_if_yes $_tracy 'ENABLE_TRACY'
define_in_config_if_yes $_tracy 'TRACY_ON_DEMAND'
define_in_config_if_yes $_tracy 'FORBIDDEN_SYMBOL_ALLOW_ALL'

This comment has been minimized.

@sev-

sev- Dec 17, 2020
Member

Ugh, indeed.

engines/twine/animations.cpp Outdated Show resolved Hide resolved
@mgerhardy mgerhardy force-pushed the mgerhardy:pr/tracy-profiler branch 6 times, most recently from 8c9c01e to 493cbab Dec 17, 2020
@mgerhardy mgerhardy force-pushed the mgerhardy:pr/tracy-profiler branch from 493cbab to b0264e1 Feb 13, 2021
@mgerhardy mgerhardy force-pushed the mgerhardy:pr/tracy-profiler branch from b0264e1 to 190201b Feb 13, 2021
@mgerhardy mgerhardy closed this Feb 13, 2021
@mgerhardy
Copy link
Contributor Author

@mgerhardy mgerhardy commented Feb 13, 2021

Closing this one - the profiler doesn't play very well with "inner-frames" - e.g. TwinE has a lot of do-while loops inside the main engine loop. Tracy needs a clear start and end of a frame - this doesn't work very nice with those inner-frames.

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

Successfully merging this pull request may close these issues.

None yet

2 participants