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

Add the ability to use Tracy plots directly from Lua via UnsyncedCtrl Spring.LuaTracyPlotConfig and Spring.LuaTracyPlot #1215

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
3 changes: 3 additions & 0 deletions rts/Lua/LuaIntro.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,9 @@ bool CLuaIntro::LoadUnsyncedCtrlFunctions(lua_State* L)

REGISTER_SCOPED_LUA_CFUNC(LuaUnsyncedCtrl, SetLogSectionFilterLevel);

REGISTER_SCOPED_LUA_CFUNC(LuaUnsyncedCtrl, LuaTracyPlotConfig);
REGISTER_SCOPED_LUA_CFUNC(LuaUnsyncedCtrl, LuaTracyPlot);
Copy link
Collaborator

@p2004a p2004a Jan 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've realized one more thing, if we want to maintain (And I think we want) that tracing annotations are 0 overhead in release build, those exports should be in #if defined(TRACY_ENABLE) block

The problem is that tracy::LuaRemove calls we have to drop tracing in the release mode, won't delete Spring.LuaTracyPlot calls, so we might need to add our own cleanup like the code in tracy to do that...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relatedly, maybe these should go under the tracy namespace in Lua?

I've seen that the tracy cleanup is actually just a dumb removal of tracy.Blabla from the Lua plaintext code which I expect would break if people did local tBla = tracy.Bla and then used tBla(). And with Spring.Bla there is a heavy cargo cult of localizing functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, what if we just added Spring.LuaTracyPlot to the tracy subtable if tracy is present in system.lua?
So in usage, Spring.LuaTracyPlot would instead be
tracy.LuaTracyPlot()
Would the tracy cleanup stuff also remove this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the tracy cleanup stuff also remove this?

Yes (it would require a one-liner change in the cleanup func to handle the new var but that's trivial)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So no, because we don't own cleanup function code, it lives in the tracy repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preferred solution would still be to have just the bodies of Spring.TracyLuaPlot functions gated within
#if defined(TRACY_ENABLE) blocks, effectively making them near noop's.

  1. Since none of the functions have any return values, it doesn't matter to Lua's perspective
  2. We don't want to modify tracy upstream, as every time we've done that to a library it comes back later to bite us in the ass when it needs to be updated (remember Assimp, SDL, Lua, etc ), so it cannot be placed in the tracy. namespace.

These plotting functions shouldn't be overused too much anyway, so while 100% free is not possible with them, its only a tiny overhead. There are way more egregious wastes of performance littered everywhere else by game developers compared to some empty function calls that provide highly valuable debugging insight when used.

Copy link
Collaborator

@sprunk sprunk Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to modify tracy upstream, as every time we've done that to a library it comes back later to bite us in the ass when it needs to be updated (remember Assimp, SDL, Lua, etc )

But none of those were upstreamed, i.e. we never sent patches to the official repos. We just had a patched internal copy of each of those, and that modified copy then got merge conflicts. The idea here would be to add a new Recoil-side function that wraps over tracy's replacer (without touching tracy itself, so no conflicts) and then actually send patches to the tracy repo (which we never did with the earlier libs).

so it cannot be placed in the tracy. namespace.

We can add to it from outside the tracy lib. Consider rts/Lua/LuaMathExtra.cpp which adds to the built-in Lua math. namespace, completely separate from lib/lua/src/lmathlib.cpp which is what creates the built-in math. namespace. Similarly the tracy. Lua namespace can be created and mostly filled inside the tracy lib somewhere in lib/tracy/blabla, and then we would have rts/Lua/LuaTracyExtra.cpp which adds the others.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to what Sprung said

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can write a PoC of my ideal solution on the weekend

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my PoC implementation: #1651

@p2004a @Beherith


return true;
}

Expand Down
4 changes: 4 additions & 0 deletions rts/Lua/LuaMenu.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,10 @@ bool CLuaMenu::LoadUnsyncedCtrlFunctions(lua_State* L)
REGISTER_SCOPED_LUA_CFUNC(LuaUnsyncedCtrl, SDLSetTextInputRect);
REGISTER_SCOPED_LUA_CFUNC(LuaUnsyncedCtrl, SDLStartTextInput);
REGISTER_SCOPED_LUA_CFUNC(LuaUnsyncedCtrl, SDLStopTextInput);

REGISTER_SCOPED_LUA_CFUNC(LuaUnsyncedCtrl, LuaTracyPlotConfig);
REGISTER_SCOPED_LUA_CFUNC(LuaUnsyncedCtrl, LuaTracyPlot);

return true;
}

Expand Down
69 changes: 69 additions & 0 deletions rts/Lua/LuaUnsyncedCtrl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@
#undef CreateDirectory
#undef Yield

#include <tracy/Tracy.hpp>
#include <common/TracyQueue.hpp>

/******************************************************************************
* Callouts to set state
Expand Down Expand Up @@ -325,6 +327,9 @@ bool LuaUnsyncedCtrl::PushEntries(lua_State* L)

REGISTER_LUA_CFUNC(Yield);

REGISTER_LUA_CFUNC(LuaTracyPlotConfig);
REGISTER_LUA_CFUNC(LuaTracyPlot);

return true;
}

Expand Down Expand Up @@ -4864,3 +4869,67 @@ int LuaUnsyncedCtrl::Yield(lua_State* L)
lua_pushboolean(L, true); //hint Lua should keep calling Yield
return 1;
}

/* Tracy seems to want unique, unchanging strings to be passed to
* its API, so we need to immanentize the ephemeral Lua strings
* and store them.
*
* NB: strings here are never cleaned up, but the use case assumes
* that they live a long time and there's just a handful of them. */
std::set <std::string, std::less<>> tracyLuaPlots;

/*** Configure custom appearence for a Tracy plot for use in debugging or profiling
*
* @function Spring.LuaTracyPlotConfig
* @string plotName which should be customized
* @string[opt] plotFormatType "Number"|"Percentage"|"Memory", default "Number"
* @bool[opt] step stepwise chart, default true is stepwise
* @bool[opt] fill color fill, default false is no fill
* @number[opt] color unit32 number as BGR color, default white
* @treturn nil
*/

int LuaUnsyncedCtrl::LuaTracyPlotConfig(lua_State* L)
{
const auto plotName = luaL_checkstring(L, 1);
const auto plotFormatTypeString = luaL_optstring(L, 2, "");
const auto step = luaL_optboolean(L, 3, true); // stepwise default
const auto fill = luaL_optboolean(L, 4, false); // no fill default
const uint32_t color = luaL_optint(L, 5, 0xFFFFFF); // white default

tracy::PlotFormatType plotFormatType;
switch (plotFormatTypeString[0]) {
case 'p': case 'P': plotFormatType = tracy::PlotFormatType::Percentage; break;
case 'm': case 'M': plotFormatType = tracy::PlotFormatType::Memory; break;
default: plotFormatType = tracy::PlotFormatType::Number; break;
}

auto plot = tracyLuaPlots.find(plotName);
if (plot == tracyLuaPlots.end())
plot = tracyLuaPlots.emplace(plotName).first;

TracyPlotConfig(plot->c_str(), plotFormatType, step, fill, color);
return 0;
}


/*** Update a Tracy Plot with a value
*
* @function Spring.LuaTracyPlot
* @string plotName which LuaPlot should be updated
* @number plotvalue the number to show on the Tracy plot
* @treturn nil
*/
int LuaUnsyncedCtrl::LuaTracyPlot(lua_State* L)
{
const auto plotName = luaL_checkstring(L, 1);
const auto plotValue = luaL_checkfloat(L, 2);

auto plot = tracyLuaPlots.find(plotName);
if (plot == tracyLuaPlots.end())
plot = tracyLuaPlots.emplace(plotName).first;

TracyPlot(plot->c_str(), plotValue);
return 0;
}

3 changes: 3 additions & 0 deletions rts/Lua/LuaUnsyncedCtrl.h
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ class LuaUnsyncedCtrl {
static int SetWindowMaximized(lua_State* L);

static int Yield(lua_State* L);

static int LuaTracyPlotConfig(lua_State* L);
static int LuaTracyPlot(lua_State* L);
};

#endif /* LUA_UNSYNCED_CTRL_H */