refactor: run SaveGameStats in a job #5934
Conversation
There was a problem hiding this comment.
Initial feedback - instead of adding the new LuaEvent class to piecewise construct an argument table, construct a LuaTable object inside of l_game_savegame_stats_callback, add values to it as normal, and pass that LuaTable handle as the args parameter to LuaEvent::Queue. (Don't forget to dispose of the underlying table from the stack - a LuaTable doesn't auto-pop its table.)
If that doesn't currently compile, let me know and I'll do the template metaprogrammery to get it to work.
A note - we tend to reserve the l_<namespace>_<method> naming convention for functions following the Lua_Function API prototype, i.e. static int l_name(lua_State *l). This is just a "regular" callback and can have any particular name.
Regarding profiling, we have an in-process profiler for C++ which can be used via the PROFILE_SCOPED() macro (with the appropriate include of profiler/Profiler.h). Details on usage are in the dev docs.
|
Thanks for working on this! We unfortunately don't have good (or any) Lua-side profiling utilities at current, but I'll pull the branch and take a closer look to see what's responsible for the stutters. |
|
Finally, as a suggestion - you may want to move the JobSet into a more central API like |
|
Hacked in a manual lua-side profiler to test performance. Note that printing to the console has an overhead of about With hot caches: With fully cold cache ( Looking at the profile from that run ( With the manual rate-limiting removed from This does raise the follow-on question of also moving Would you like me to push my profiler implementation to this branch? |
|
Thank you for having a look and providing feedback!
I'm afraid I still have extremely limited knowledge of how the Lua stack works and what can and can't be constructed on it and passed around. It's not really made any clearer (to me) by the wrappers you guys have developed which have various restrictions such as the ScopedTable. If there is a book or other training material you can point me at so I can learn more about it that would be appreciated. So I didn't think a LuaTable could be constructed on the stack and then passed around and modified etc before invoking a Lua function while other parts of the code also potentially modify the Lua stack (ie, another Job in another thread). Inside In the "normal" functions the This is why I made that LuaEvent class to construct the table and manage the
???
Noted.
I saw the macro, but haven't had time to look into it. Thanks for the link, I'll have a read. |
Yes please. No need for me to reinvent the wheel.. |
This is partially why the Lua*** wrappers were developed, to simplify interacting with Lua from the programmer's perspective. I'm sorry they're not clear and easy to use either - we (at this point, I) probably should write a primer on interacting with Lua and add it to the dev docs.
Lua is implicitly and explicitly single-threaded only. Only the main thread is allowed to call any Lua API functions on the main Lua state ( I should have mentioned this when I was originally describing the problem to you, sorry. 😅 The Other Lua states may be constructed for use on separate threads, but they operate under a share-nothing model in relation to the main Lua state from both a C and Lua-side perspective. As mentioned in another reply, from the "main" thread you can * Startup and shutdown excluded, for obvious reasons.
Don't worry about it at all - this is why I'm taking the time to review and provide feedback in such depth on your PRs. My hope is to help smooth along the learning process; feel free to mention me (as sturnclaw) in IRC with questions if that's a more expedient way to learn.
Here's an annotated example: // Create a new table on the Lua stack.
// This is a shorthand to call lua_newtable(l); LuaTable(l, -1).
// Passing a stack index to the constructor instead aliases an existing table.
LuaTable tab { l };
// Interact with the table as normal, don't need to bother with the underlying Lua API.
tab.Set("myKey", myVal);
// Pass this table to a Lua event. The LuaTable wrapper aliases the underlying table on
// the Lua stack when copied.
// LuaEvent::Queue pushes all parameters to Lua and then destroys its arguments. You
// generally don't have to worry about lifetime concerns as long as you're not passing
// a ScopedTable handle.
// LuaEvent::Queue is guaranteed to leave the stack in the same state as when you called it.
LuaEvent::Queue("myEvent", tab);
// Because LuaTable aliases a table on the stack, it does not remove that value from
// the stack when it is destroyed. Instead, you have to manually call lua_pop() to remove
// the table implicitly created via LuaTable(lua_State *l).
lua_pop(l, 1); |
Sure; I first wanted to see if it would work at all so just hacked it into the I moved the Job itself into the I moved the jobset into The issue is that the |
Looks good. Naming is a little stilted, I might personally try to name it something like
More seriously, this isn't ideal. I suspect the editor will likely crash on startup because of this, as Preferably, the |
This is proving to be marginally challenging as the other Applications do not have any Job Queues.. |
|
pioneer/src/core/Application.h Line 89 in 2884328 |
mwerle
force-pushed
the
refactor/savegame_stats_job
branch
from
20a7010 to
63c95d1
Compare
No idea how or why I missed this.. I was grubbing around inside the implementation files and must have missed the base class. Proposed fix in 63c95d1 |
Fixed in 1a871c8 |
Looks good. Will give this a whirl tomorrow. |
There was a problem hiding this comment.
So far this looks good! I've left one comment where a bit more cleanup is needed, and I've pushed a commit to the branch removing the manual rate-limiting code in SaveLoadWindow:update() (and that overridden function entirely). Feel free to rebase/squash/etc. that commit - I'm not worried about the author/committer information.
data/pigui/modules/saveloadgame.lua
Outdated
| @@ -243,6 +258,7 @@ end | |||
| --============================================================================= | |||
|
|
|||
| function SaveLoadWindow:makeFilteredList() | |||
| local profileEndScope = utils.profile("SaveLoadWindow::makeFilteredList()") | |||
There was a problem hiding this comment.
Leftover profiling call here.
There was a problem hiding this comment.
The change is impressive, even Debug builds now load the list of saves almost instantly, down from about 14 seconds, on my PC with about 40 saves.
I'll squash/merge/etc and mark the PR as ready then. Thank you for all of your input!
- Simple manual profiler for checking the time cost of certain Lua methods - Add Engine.nowTime, which returns the exact time since an unspecified epoch at the moment it's evaluated
The idea is to run this expensive task in an asynchronous job and notify the Lua side once it is completed. This is to prevent the stutter from occurring every time the SaveGameStats API is called from Lua. This commit implements this idea by: * add a job manager to the LuaManager class which can run any job on the Lua job queue. * add a LoadGameToJsonJob implementation in SaveGameManager, and a new function, LoadGameToJsonAsync(), which schedules an instance of this job on the Lua job queue. * call this new function from Lua, as well as registering a callback for when the job completes, in saveloadgame.lua. Anecdotal testing in debug builds shows an impressive speed increase when loading the list of savegames.
- Prior code limited synchronous Game.SaveGameStats calls to once-per-frame - Since the function is now async, we can dispatch as many jobs as we want and their results will be processed on completion
mwerle
force-pushed
the
refactor/savegame_stats_job
branch
from
6ebf296 to
f20eb57
Compare
|
Squashed commits, rebased on main, ready for review, thank you. |
|
My prior approval stands. Good work, and thank you for implementing this! Hopefully it's given you a bit better visibility into a "vertical slice" of the codebase - I'll chalk that up as a win for shamelessly asking other contributors to do my TODO items for me. 😄 In all seriousness, this is a good implementation and I'm glad you proposed the idea of the SaveGameManager - I likely wouldn't have come up with that on my own. |
|
Thank you, yes it was a great learning exercise and I think I gained some insight into how the codebase, especially interactions with Lua, work. |
##' Bugs / Feature requests
Attempts to fix #5929
Background
The idea is to run this expensive task in an asynchronous job and notify the Lua side once it is completed. This is to prevent the stutter from occurring every time the SaveGameStats API is called from Lua.
Details
The code was modified to call the expensive part of the SaveGameStats API (loading and parsing the game file) into a Job. Once finished, the data is marshalled into a LuaTable and a LuaEvent is triggered to pass the data to the Lua side.
The Lua side registers an Event listener, which parses the data and updates the SaveGame cache entry for the data.
Practically, this is achieved by the following changes :
the Lua job queue.
Many thanks to @Web-eWorks for feedback throughout the development of this change (details of which have been squashed, but the implementation went through several iterations) as well as providing the Lua profiler and removing the no longer required
SaveLoadWindow::update()callback.