Ensure sufficient stack size for Lua values #4482
Conversation
BMagnu
added
fix
There was a problem hiding this comment.
Looks ok, I guess, but I defer to @asarium.
Edit: He says he won't be able to look at this until Monday.
code/scripting/lua/LuaReference.h
Outdated
| @@ -101,7 +101,7 @@ class UniqueLuaReference { | |||
| * @brief Pushes the referenced value onto the stack. | |||
| * @param thread A specific thread state to push the value to. nullptr for the default state of this reference | |||
| */ | |||
| void pushValue(lua_State* thread) const; | |||
| void pushValue(lua_State* thread, bool guaranteeStack = false) const; | |||
There was a problem hiding this comment.
Adding a "random" boolean variable to this call would, in my opinion, make this API confusing to use. Furthermore, since you do the relevant call before the Lua function anyway, I do not think adding this parameter (and also to LuaValue) is necessary.
There was a problem hiding this comment.
While I make this call in this instance, it is necessary to do this for every case this function is called. Which is from quite a lot of places. You have to compromise at some point, either by asking the coder to always manually ensure the stack size before each call of pushValue (both for this and value), or eat the performance penalty of doing the allocation / check more than once if you push many values, or have the option to pass this as the API parameter.
Since I would really not want to have to rely on the programmer to always remember manually ensuring the stack size, it's basically a question of "do we want to waste performance in a low level function" vs "do we want one more parameter in the low level function".
IMO, with this case we should pick performance over pretty API.
There was a problem hiding this comment.
Ok, I can understand that reasoning.
One thing I noticed though is that the current name suggests that a true value will trigger the lua_checkstack call but that is not the case. Maybe a parameter name like checkStack would be more appropriate here.
There was a problem hiding this comment.
ok, clarified variable name and added documentation
code/scripting/lua/LuaReference.h
Outdated
| @@ -101,7 +101,7 @@ class UniqueLuaReference { | |||
| * @brief Pushes the referenced value onto the stack. | |||
| * @param thread A specific thread state to push the value to. nullptr for the default state of this reference | |||
| */ | |||
| void pushValue(lua_State* thread) const; | |||
| void pushValue(lua_State* thread, bool guaranteeStack = false) const; | |||
There was a problem hiding this comment.
Ok, I can understand that reasoning.
One thing I noticed though is that the current name suggests that a true value will trigger the lua_checkstack call but that is not the case. Maybe a parameter name like checkStack would be more appropriate here.
Fixes #4287.
Until now, when we pushed values to the Lua stack from C, we never checked that the stack allocated by the lua interpreter had space available, let alone reserve new space.
This PR now adds checks on each push to the stack. While this might slightly degrade performance, it is a necessary check.
To avoid unnecessary checks, there is infrastructure in place to pre-reserve multiple stack spaces and pass the information about guaranteed sufficient stack pre-allocation to the value push, enabling omission of the check in these instances.