Fix performance issues after migration to sol2 3.3.0

[SiarheiFedartsou]

Issue

People are reporting performance issues after merge of #6279, I reproduced it and it turned out that now we are copying the whole relations container on each invocation of node_function 🤦🏻

Some discussions in:

#6302
#6297

Tasklist

• CHANGELOG.md entry (How to write a changelog entry)
• update relevant Wiki pages
• add tests (see testing documentation)
• review
• adjust for comments
• cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

[KaSakee]

It makes sense to use reference_wrapper, Nice job.

[mjjbell]

Thanks, I missed this in the original PR.

To confirm my understanding:

• the sol2 upgrade has changed the behaviour of passing const references to user-defined types. It will now copy the value (as indicated by the Node copy constructor error)
• ExtractionRelationContainer had a copy constructor, so this is the performance regression.
• Wrapping non-const references is not strictly necessary as the behaviour has not changed, but it makes the arguments consistent / future-proofed.

Is this right?

[SiarheiFedartsou]

the sol2 upgrade has changed the behaviour of passing const references to user-defined types. It will now copy the value

Tbh I haven't seen it in sol2's changelog, but I assume so.

(as indicated by the Node #6279 (comment))

Yes, and it is my fault that I didn't pay enough attention to it 🤦🏻

ExtractionRelationContainer had a copy constructor, so this is the performance regression.

Yes.

Wrapping non-const references is not strictly necessary as the behaviour has not changed, but it makes the arguments consistent / future-proofed.

In other words you are curious here why didn't we have problems for the cases when something was supposed to be changed by Lua script? I.e. if it was copied, then we would change the copy in the script -> behaviour was expected to be changed in this case? Tbh I didn't think about that. Probably worth looking a bit deeper into it to be 100% confident.
There is a paragraph about this in sol2 docs. They say it copies C++ types by default 🤔

I'll probably be able to dig into it in the nearest days and have better answers. WDYT?

[mjjbell]

Wrapping non-const references is not strictly necessary as the behaviour has not changed, but it makes the arguments consistent / future-proofed.

In other words you are curious here why didn't we have problems for the cases when something was supposed to be changed by Lua script? I.e. if it was copied, then we would change the copy in the script -> behaviour was expected to be changed in this case? Tbh I didn't think about that. Probably worth looking a bit deeper into it to be 100% confident. There is a paragraph about this in sol2 docs. They say it copies C++ types by default thinking

I'll probably be able to dig into it in the nearest days and have better answers. WDYT?

Yes, although it has the exact same wording in the old docs, but we know something has changed since then.
I think it's fine as all of the tests would have failed. Judging by this relatively recent comment, what we are seeing is now aligned with the intended behaviour.

[mjjbell]

And the std::ref wrapping ensures it's not a problem in any case 👍