Open McKillroy opened 1 year ago
McKillroy
commented
1 year ago I isolated the error in my vcode (used to work flawlessly in the past):
/// This first part works
auto eventtype_test_table = solstate_.new_usertype<TestMessage>(
"TestEvent",
sol::factories( &MessageFactory<TestMessage> )
);
/// This part breaks with the error message above (Only in release mode sol 2.3.2, C++20, MSVC latest)
eventtype_test_table[ "arg" ] = &TestMessage::args;It seems my build works again in release mode with SOL_SAFE_USERTYPE=1 which leaves me with one question: Do the various SOL safeties have any runtime performance penalty or are they just about compilation times?
Rochet2
commented
1 year ago Do the various SOL safeties have any runtime performance penalty or are they just about compilation times?
Some of them are definitely introducing runtime overhead. For example, normally a double value would be cast into an integer, but some safeties (SOL_SAFE_NUMERICS ?) introduce checks that make sure the number is an integer instead of a float like 0.5. Doing so introduces a lot of overhead for operations that use numbers. Casting is (obviously?) a lot faster than running several functions that check some constraints.
For SOL_SAFE_USERTYPE specifically, if you search it in the codebase you can see for example this part which introduces a stack check which is overhead compared to not doing it. Overall seems like most changes related to the setting are similar. Just a check-get instead of plain get, followed by code that does a few IF checks on the value to determine if it is nil.
McKillroy
commented
1 year ago Do the various SOL safeties have any runtime performance penalty or are they just about compilation times?
Some of them are definitely introducing runtime overhead. For example, normally a double value would be cast into an integer, but some safeties (SOL_SAFE_NUMERICS ?) introduce checks that make sure the number is an integer instead of a float like 0.5. Doing so introduces a lot of overhead for operations that use numbers. Casting is (obviously?) a lot faster than running several functions that check some constraints.
For
SOL_SAFE_USERTYPEspecifically, if you search it in the codebase you can see for example this part which introduces a stack check which is overhead compared to not doing it. Overall seems like most changes related to the setting are similar. Just a check-get instead of plain get, followed by code that does a few IF checks on the value to determine if it is nil.
Since I'm writing a gameserver, for us performance is critical. I guess I'll have to dig deeper. It's weird that suddenly this issue came up and maybe there's a fix for it that doesn't require changing safeties or introducing overhead.
McKillroy
commented
1 year ago So - the error lies here: https://github.com/ThePhD/sol2/blob/eba86625b707e3c8c99bbfc4624e51f42dc9e561/include/sol/usertype_container.hpp#L527-L543 The upper code branch compiles on debug and on release, while the lower branch for my code leads to an error in release mode. I have no idea why, or what to do to fix it.
/// This first part works
auto eventtype_test_table = solstate_.new_usertype<TestMessage>(
"TestEvent",
sol::factories( &MessageFactory<TestMessage> )
);
/// This part breaks with the error message
eventtype_test_table[ "arg" ] = &TestMessage::args;The error:
usertype_container.hpp(541,12): error : non-const lvalue reference to type 'basic_string<...>' cannot bind to a temporary of type 'basic_string<...>'
return stack::unqualified_get<T>(L_, 1);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~For some reason it has an issue with "arg" in release mode if no safeties are enabled.
McKillroy
commented
1 year ago Just for completeness - wondering if it is the member I am trying provide as "arg":
std::vector<vardata> args { };and
using vardata = std::variant< //
/* CAVEAT!
* int64_t MUST come before double, or Lua assignments
* may do unwanted conversions from int to double !!!!
* */
int64_t, // MUST be first
double, // MUST be AFTER int64_t !!!!!
std::shared_ptr<std::string>, //
hpx::id_type /* LAST */ // Could be anywhere, but better last
>;I just realized that removing the shared_ptr made that code compile and the same problem coming up at another location where I tried to expose some std::shared_ptr.
This attempt also failed:
auto shared_string_table = //
solstate_.new_usertype<std::shared_ptr<std::string>>( "SharedString", restricted_enrollments );
shared_string_table[ "new" ] = &std::make_shared<std::string>;
shared_string_table[ "get" ] = []( std::shared_ptr<std::string> p ) -> std::string //
{ //
return static_cast<std::string>( *p );
};So - could I expose shared_ptr or is there an issue?
McKillroy
commented
1 year ago So after removing the shared_ptr from the variant and removing the shared_string_table, now the compiles work without safeties again. But I don't really understand it - I'd love to have that functionality but don't know how to do it now. I still don't really understand the problem.
McKillroy
commented
1 year ago I tried something more intuitively than knowing what I was doing. I changed this line of code: https://github.com/ThePhD/sol2/blob/eba86625b707e3c8c99bbfc4624e51f42dc9e561/include/sol/usertype_container.hpp#L541 into this:
return *stack::unqualified_get<T*>(L_, 1); And my sorrows went away.
Please someone explain if this is OK and could be used as a fix or is just utter hack and will make things explode in the near future.
McKillroy
commented
1 year ago I resorted to asking ChatGPT and that's the answer I got. Is this correct or nonsensical? It kinda makes sense to me. Can someone chime in who really understands what's happening?
ChatGPT answer:
I see what you did there! By changing the return statement to return stack::unqualified_get<T>(L_, 1);, you're dereferencing the pointer returned by stack::unqualifiedget<T*>(L, 1) and returning a reference to the object pointed to by the pointer.
Here's why this works:
When you pass a usertype to a Lua function using Sol2, Sol2 stores a pointer to the object in the Lua registry. When the Lua function is called, Sol2 retrieves this pointer and passes it to the C++ function. By default, Sol2 uses stack::unqualified_get to retrieve the pointer from the Lua stack.
However, when Sol2 safeties are turned off, stack::unqualified_get returns a raw pointer to the object in the Lua registry. This means that if you have a usertype that uses a smart pointer to manage its lifetime, Sol2 won't be able to tell when the object has been deleted.
By changing the return statement to return stack::unqualified_get<T>(L_, 1);, you're dereferencing the raw pointer returned by stack::unqualifiedget<T*>(L, 1) and returning a reference to the object pointed to by the pointer. This means that Sol2 can properly manage the lifetime of the object using the smart pointer.
Note that this approach only works if your usertype is designed to work with raw pointers. If your usertype is designed to work with smart pointers, you should consider using std::shared_ptr or std::unique_ptr to manage the lifetime of your objects.
I hope this helps! Let me know if you have any more questions.
McKillroy
commented
1 year ago I finally worked around the issue by replacing the variant type stored in a vector with a variant wrapper with dedicated getters and constructors so Lua can understand it.
I had a project resting for a while and am picking it up again and I now have broken builds in release mode while debug builds still work. I tried using older versions of sol but it didn't change. Ofc there were some changes in the codebase - e.g. using C++20 now. I am not sure I'm able to produce a minimal example shortly, so I'm just providing the error message here, maybe it's sufficient for people in the know of the sol2 intestines .
Error message: