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
easy_profiler integration #1531
easy_profiler integration #1531
Conversation
Great, thanks 👍, I'll give this a whirl later today once I can get back to macOS |
Great work! It is a lot more stable on macOS and the UI seems to be working other than the thread ID's which are still not quite being calculated properly. I did try and disconnect and then reconnect and the Qt profiler app hangs on macOS when doing this... can you disconnect/reconnect for you? EDIT: Force quitting the Profiler application and restarting it and I could reconnect to the player app and start capturing again, so is Profiler app specific EDIT 2: I get this crash quite a bit, seem to be able to trigger it with right click select of the timespan, though also believe I got it once when Profiler app was in background process:
|
@JoshEngebretson easy_profiler maintainers did some work on networking and threading. Our changes were upstreamed as well. I rebased this branch on to master, cleaned it up, updated easy_profiler, fixed crash with event profiling (its a bit dirty, but works great). I squashed all commits as they were more wandering in the woods than meaningful work.. Squashing ate your changes, but rest assured your mark in history of humanity was presedved :) Please test profiler tool on MacOS. I commented out that |
@rokups Awesome sauce! Thanks, and will test and review as soon as possible 👍 |
@JoshEngebretson Hello! I'm collaborator of easy_profiler. I saw you had a crash in GUI. Does the problem still exist? If yes, could you, please, send me .prof file with which you had the problem? |
Hello @cas4ey , thanks for your excellent work on easy_profiler! I have not reproduced any crashes in recent testing of the profiler branch, will let you know if I do 👍 |
@JoshEngebretson i updated to latest profiler code and rebased on to engine master. Connection log spam is no longer there and it seems network quirks are fixed. Please test how it fares on MacOS. Also please point me to some example of manual binding example for c# and to some example how to add extra code for js. I would like to try adding something like AtomicNET/Core/Profiler.cs |
Awesome, I'll be able to test this later today. I'll also provide some guidance for C#/JS 🐫 Edit: Looks like there are some include path issues with the profiler on Windows/macOS according to the logs, note CI builds in release |
Something weird is going on here and i cant figure it out. Could you please take a look? Search build log for |
I am not sure on the CMake workaround, 3.1.2 is what is on build box. If we're modifying the stock easy_profiler CMake, please mark with // ATOMIC BEGIN and // ATOMIC END block with a note about modification, this helps when updating and also removing changes when they are no longer necessary. I'll have a look at the build when I get a chance here, won't be too long ;) Great work! |
@AtomicBuildBot retest this please |
1 similar comment
@AtomicBuildBot retest this please |
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}/bin) | ||
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}/bin) | ||
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}/bin) | ||
|
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.
These output overrides cause an error when building for multiple platforms as each platform's binaries will be in the same location, in the source instead of build tree:
%ATOMIC_ROOT%/Source/ThirdParty/easy_profiler/easy_profiler_core/Release
instead of for example:
%ATOMIC_ROOT%/Artifacts/Build/Mac/Source/ThirdParty/easy_profiler/easy_profiler_core/Release
%ATOMIC_ROOT%/Artifacts/Build/IOS/Source/ThirdParty/easy_profiler/easy_profiler_core/Release
%ATOMIC_ROOT%/Artifacts/Build/Android/Source/ThirdParty/easy_profiler/easy_profiler_core/Release
Disabling these output directory overrides, and ./Build_AtomicEditor.sh --with-android --with-ios
works. I think this is a fine solution, though should put // ATOMIC BLOCKS and a description of why disabled. This isn't standard CMake for a third party library and should probably be addressed upstream.
Finally it builds \o/ @JoshEngebretson thank you very much for spotting that easy_profiler cmake quirk. Fresh set of eyeballs are a great thing o/ I will try to upstream that change as well. |
I am afraid at least on windows profiler deployment is complicated.
We can build We have to do one of the following on windows:
|
add_subdirectory(profiler_gui) | ||
# ATOMIC BEGIN | ||
if (MSVC AND "${CMAKE_BUILD_TYPE}" STREQUAL "Debug") | ||
message(STATUS "Profiler GUI can only be built in Release builds with MSVC compiler.") |
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.
Does this cause the easy profiler cmake to look for debug Qt libs? If so, maybe can be configured to use Release Qt libs regardless in easy profiler scripts? If not, or another reason, lemme know. I think this is acceptable, though would be good to have Profiler client available in debug builds, which could also be sorted later.
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.
I do not think it is possible to kick certain targets into "Release" build when everything else is building as "Debug". But maybe we could just remove -DDEBUG=1
(which is implicit) and use /MD
and have release build within debug build. That that might work, i will try.
@@ -143,6 +143,8 @@ else () | |||
endif () | |||
endif () | |||
|
|||
# ATOMIC BEGIN | |||
if (0) |
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.
Good catch on the install stuff
if (MSVC) | ||
add_library(easy_profiler_md ${EASY_OPTION_LIB_TYPE} ${SOURCES} resources.rc) | ||
foreach(prop COMPILE_DEFINITIONS COMPILE_OPTIONS CXX_STANDARD CXX_STANDARD_REQUIRED INCLUDE_DIRECTORIES INTERFACE_COMPILE_DEFINITIONS INTERFACE_COMPILE_OPTIONS INTERFACE_INCLUDE_DIRECTORIES INTERFACE_LINK_LIBRARIES) | ||
get_target_property(val easy_profiler ${prop}) |
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.
Fancy footwork, the cmake is strong with this one! 🐫
# ATOMIC BEGIN | ||
target_link_libraries(profiler_gui Qt5::Widgets) | ||
if (TARGET easy_profiler_md) | ||
target_compile_options(profiler_gui PRIVATE /MD) |
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.
Very nice and clean 👍
Again sorry for taking forever. Real life and summer caught up to me. Sometimes i can be idiot of epic proportions. I completely forgot the fact that Qt ships both release and debug builds of their libraries. Forcing a release ( To deploy profiler dlls on windows we are supposed to run this: @JoshEngebretson please give it a spin on MacOS and let me know if something is still missing or if is it complete now. #1566 makes Also please point me to some example how to add extra code for js. I would like to try adding something like AtomicNET/Core/Profiler.cs If profiling on MacOS works as expected and when i add this js bit i think this PR will be complete. |
@rokups Absolutely zero worries man, this is a mega-feature! 😎 I am really swamped here today, will test at first opportunity. That's good news on the Windows side of things, thanks for R&D'ing it Also, I haven't forgotten #1566. I'll look into whipping up a quick hand binding example for the profiler if that gets hairy (or for the meantime). |
This is insane. Just saw the Repo for easy profiler. Super happy to see this in AGE :) |
@rokups This is great! Good job! |
Thanks for applauds guys :) 👏 @JoshEngebretson optimized c# bindings are working. One more piece of the puzzle is that generated c# projects do not have |
@@ -12,12 +13,16 @@ public partial class Profiler : AObject | |||
{ | |||
#if ATOMIC_PROFILING | |||
var profiler = AtomicNET.GetSubsystem<Profiler>(); | |||
profiler?.BeginBlock(name, file, line, color, (byte)status); | |||
if (profiler != null) |
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 don't need AtomicNET here, AObject has GetSubsystem on it, the static AtomicNET flavor is only for getting a subsystem outside of a AObject derived instance. Also, it might be good to locally cache so doesn't look up per block.
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 don't need AtomicNET here, AObject has GetSubsystem on it
👍
locally cache
You mean Profiler.Block()
should not be static so that caller gets Profiler
and reuses that reference?
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.
I mean so don't need to call GetSubsystem per block, which is not a terribly expensive call, though still, if that gets ugly that's fine, can cross bridge if it is an issue 🐫
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.
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.
This caching of subsystem pointers seem to comes up rather often. Maybe it would be better idea for next PR to cache engine subsystems as weak pointers in Context
?
@@ -750,7 +751,15 @@ namespace Atomic | |||
} | |||
#endif | |||
|
|||
#if ATOMIC_PROFILING | |||
ATOMIC_EXPORT_API void csi_Atomic_Profiler_BeginBlock(Profiler* profiler, const char* name, const char* file, int line, unsigned int argb, unsigned char status) | |||
{ |
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.
Let's make sure the csi_Atomic_Profiler_BeginBlock is available in the shared library, whether profiling is enabled or not... will avoid missing symbols. Can ifdef the profiler usage, also could add a one time log warning (controlled by a local static bool), which warns if called without profiling support in binary
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.
Will do
The C# solution generator has support for adding constants, though not for debug vs release I don't think right now, example:
Would be a pretty easy add for supporting debug/release specific flags:
|
@@ -12,7 +12,7 @@ public partial class Profiler : AObject | |||
[CallerLineNumber] int line = 0) | |||
{ | |||
#if ATOMIC_PROFILING | |||
var profiler = GetSubsystem<Profiler>(); | |||
var profiler = AtomicNET.Context.GetProfiler(); |
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.
@JoshEngebretson i cleaned up commit history, now it is in a state to be merged. Updated easy_profiler to latest code on their |
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.
Very impressive work @rokups!!! I think we can get this landed in short order, I went through and did a final review with some comments. Let me know what you think, it is close, really close... a joy to review, A+ 😄
setup_target() | ||
if (ARG_TOOL) |
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.
Nice add and good standardization for tool bins 👍
{ | ||
#if ATOMIC_PROFILING | ||
var profiler = AtomicNET.Context.GetProfiler(); | ||
if (profiler != null) |
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.
Still buzzing on new Context getters, great work there, it did make a Context.Profiler
property too, right ;)
@@ -23,6 +23,7 @@ | |||
"name": "AtomicNET", | |||
"atomicNET" : true, | |||
"outputType" : "Library", | |||
"defineConstants" : ["ATOMIC_PROFILING"], |
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.
I think it is ok to define ATOMIC_PROFILING
by default for now, we're going to want to be able to define difference constants per build, though that is for another day.
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.
People would be profiling a release build anyhow, this flag missing on release builds would certainly cause some confusion.
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.
I mean the equivalent of ${ATOMIC_RELEASE_OFF}
, for release builds the ATOMIC_PROFILING define should default off in C#, no need to worry about it now.
Source/Atomic/CMakeLists.txt
Outdated
@@ -32,6 +32,7 @@ elseif (ATOMIC_DATABASE_ODBC) | |||
endif () | |||
|
|||
if (WIN32) | |||
option (ATOMIC_D3D11 "Use DirectX 11" ON) |
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're not quuuuite ready to switch the D3D11 switch on by default, is this doing that?
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.
Not sure how that happened, will fix ;)
target_compile_options(Atomic PUBLIC "$<$<CONFIG:Release>:${ATOMIC_MSVC_RUNTIME}>") | ||
target_compile_options(Atomic PUBLIC "$<$<CONFIG:RelWithDebInfo>:${ATOMIC_MSVC_RUNTIME}>") | ||
target_compile_options(Atomic PUBLIC "$<$<CONFIG:MinSizeRel>:${ATOMIC_MSVC_RUNTIME}>") | ||
target_compile_options(Atomic PUBLIC $<$<CONFIG:Debug>:${ATOMIC_MSVC_RUNTIME}d> $<$<NOT:$<CONFIG:Debug>>:${ATOMIC_MSVC_RUNTIME}>) |
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.
Sometimes CMake makes my eyes cross, nice work
@@ -43,20 +43,24 @@ const StringHash StringHash::ZERO; | |||
StringHash::StringHash(const char* str) : | |||
value_(Calculate(str)) | |||
{ | |||
#if ATOMIC_PROFILING | |||
RegisterSignificantString(str, *this); | |||
#endif |
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.
What is the reason for registering all StringHash strings as being significant with profiling enabled?
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 need this for being able to recognize event names. Events as you know are sent using StringHash
, but profiler needs actual strings for data to be useful. See 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.
Ok, this is definitely an area to keep an eye on, and maybe for future improvement. It registers every time a string is converted to a StringHash, ouch... I know this is the only way right now, hopefully this doesn't come up on the top of the profiler ;)
It it does, maybe we can introduce a separate ATOMIC_PROFILING_EVENTS
for enabling event profiling...
} | ||
|
||
StringHash::StringHash(const String& str) : | ||
value_(Calculate(str.CString())) | ||
{ | ||
#if ATOMIC_PROFILING | ||
RegisterSignificantString(str, *this); |
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.
This one too, just noting...
@@ -264,20 +264,12 @@ void BackgroundLoader::FinishBackgroundLoading(BackgroundLoadItem& item) | |||
// If BeginLoad() phase was successful, call EndLoad() and get the final success/failure result | |||
if (success) | |||
{ | |||
#ifdef ATOMIC_PROFILING | |||
#if ATOMIC_PROFILING |
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.
On these profiling changes, I don't think we need to pepper ATOMIC modification blocks everywhere, we've diverged... next update, I guess see what run into and maybe adjust, though think modification blocks would just add noise in these cases
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.
This was reason why i did not add these comments elsewhere. We know that profiler subsystem was replaced therefore profiler changes should be ignored when merging from upstream. I am adding them to requested spots anyhow ;) Maybe we can find a way for got to help us do merges and make these comments thing of the past. I plan to look into it next ;)
#ifdef ATOMIC_PROFILING | ||
if (profiler) | ||
profiler->EndBlock(); | ||
#endif | ||
} |
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.
Another sign of code improvement, not needing to explicitly EndBlock
, winning! 🏅
@@ -8,6 +8,7 @@ option(ATOMIC_EDITOR "Build Editor" ON) | |||
option(ATOMIC_DATABASE_SQLITE "Enable SQLite database subsystem" OFF) | |||
option(ATOMIC_DATABASE_ODBC "Enable ODBC database subsystem" OFF) | |||
option(ATOMIC_IK "Enable inverse kinematics subsystem" OFF) | |||
option(ATOMIC_PROFILING "Enable profiler" ${ATOMIC_RELEASE_OFF}) |
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.
${ATOMIC_RELEASE_OFF}
is very cool!
@JoshEngebretson requested changes landed. Once you review i will squash |
Great, cleared for landing, I'll just do some final binary testing once you have the commits arranged, thanks! |
This would also be a good PR to add yourself to AUTHORS.md 🌟 |
Already there, Rokas Kupstys ;) |
Haha, awesome, you're a Super Contributor :) If you want to add yourself to the Atomic Game Engine credits block, I think you should.. and technically, part of the contributing agreement: CONTRIBUTING.md |
Moved OpenGL config option to appropriate spot.
@AtomicBuildBot retest this please |
|
||
find_package(Qt5Widgets) | ||
|
||
if (NOT Qt5Widgets_FOUND) |
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.
This is supposed to be if (Qt5Widgets_FOUND)
? I think a crumb may have been introduced when testing the warning message.
@rokups I made some tweaks and added JS/TS Profiler.BeginBlock support, feel free to look over and cherry pick into PR: d826df8 I also had some problems on CI Mac, so: @AtomicBuildBot retest this please |
@rokups You absolutely shredded on the new Profiler subsystem 🎸 , very impressive work! It has also been a lot of fun working on this feature with you, LEVEL UP! 🌟 A hearty THANKS from 🐫 everywhere, and special thanks to @cas4ey for such an excellent profiling library and client, which Atomic now uses to profile C++, C#, JavaScript/TypeScript!!! LANDING! |
This is pretty awesome @rokups! |
Whoa! Great news! 👍 |
This PR integrates easy_profiler replacing old and not really useful on-screen profiler.
Features:
ATOMIC_PROFILING
.In the future easy_profiler maintainers plan support for dx11/12, opengl, vulkan and cuda profiling. Maintainers were very helpful and implemented non-scoped profiler blocks on my request. Tracking arbitrary values (my request as well) is already in the works, this will be useful for tracking c#/js object counts and what not.
I found a way to do reasonably low-overhead c# code block profiling using lambda. There is still a chance to optimize a little by defining custom bindings to
Profiler::BeginBlock()
which takesconst char*
as parameter. It looks like this:I did not add js support, however it is trivial. Is there a spot somewhere we could add some extra code for typescript to use much like
Script/AtomicNET/AtomicNET/Core/Profiler.cs
? In js it would look like so:Can we set
engineParameters_
from c#/js applications? If we can - how? We need this in order to make listening for profiler connections opt-in instead of opt-out.TODO:
ATOMIC_PROFILING
to debug configurations of C# projectsProfiler::BeginBlock()
Please review.