Optimize LuaPushNamedFoo using compile time key hashing#2986
Conversation
…e time hash. Remove legacy hashing macros and replace callsites with corresponding LuaPushNamedFoo
dargem
changed the title
sprunk
left a comment
sprunk
left a comment
There was a problem hiding this comment.
Looks good.
not sure what the proper way is to test my changes
Run a normal game, if it's not visibly broken then the patch is alright since it touches wide swathes of lua.
As for testing whether it's now actually done at compile time, perhaps the new hash function could be consteval?
| #include "System/StringHash.h" | ||
|
|
||
| template <unsigned N> | ||
| static inline void LuaPushHString(lua_State* L, const char (&str)[N]) |
There was a problem hiding this comment.
This should be next to the other LuaPushFoo functions in LuaUtils.cpp. Also perhaps it should just be LuaPushString without the H, since part of the point was so that we wouldn't need to worry about hashing.
I've had a look at a few ways and I can't change it to consteval since I'd be passing the compile time constant to my non consteval LuaPushNamedFoo, and since function parameters are not constexpr they can't be used to call a consteval function even if the function was called with a compile time constant. So it would rely on the compiler inlining it then doing the hash. I'm thinking to guarantee it hashes a better approach would've been for me to to just template it with the string literal. Don't mind changing it to this, callsite would just change a bit to |
|
Compiler should still handle it sensibly so let's keep it as a regular arg. |
2 participants
Addresses #2891
I've added an overload for the LuaPushNamedFoo functions changing the const std::string& param to a reference to char arrays. Then it calls the compileTimeHash structs hash function like the macros did prior. If passed a string literal it hashes at compile time properly which I've checked with godbolt. Similar to the old macros which had a warning not to pass in a c style char array calling lua push named foo with one can lead to bugs so I added a warning. I didn't see any callers doing this, passing in a std::string if not using a compile time constant should be used instead which it is. Used copilot for searching and updating the old callsites then I went through and verified it. First PR so not sure what the proper way is to test my changes, ran unit tests and these failed but they were also failing prior to my modifications.