Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GDExtension: PtrToArg::convert() creates unnecessary copies #80074

Closed
Bromeon opened this issue Jul 30, 2023 · 0 comments · Fixed by #80075
Closed

GDExtension: PtrToArg::convert() creates unnecessary copies #80074

Bromeon opened this issue Jul 30, 2023 · 0 comments · Fixed by #80075

Comments

@Bromeon
Copy link
Contributor

Bromeon commented Jul 30, 2023

Godot version

4.2-dev (branched off 75f9c97)

System information

Windows 10

Issue description

Stumbled upon this while debugging equality between StringName instances (for unrelated issue #78580).

The GDExtension function pointer for equality calls OperatorEvaluatorEqual<StringName, StringName>::ptr_evaluate().
This one checks PtrToArg<A>::convert(left) == PtrToArg<B>::convert(right).

The function PtrToArg::convert() is defined as follows (substituted m_type with StringName):

template <>                                                  
struct PtrToArg<m_type> {                                    
    _FORCE_INLINE_ static StringName convert(const void *p_ptr) {
        return *reinterpret_cast<const StringName *>(p_ptr);     
    }
}

reinterpret_cast returns an expression of type const StringName*.
Dereferencing that yields const StringName& (and not StringName).

The return type of convert() however is StringName (by value).
Unless I'm missing something, the C++ compiler cannot apply RVO here because we don't have a value, only a const-reference.

So this calls the copy-constructor StringName::StringName(const StringName &p_name) -- I verified this via debugger.

This copy is unnecessary for many operations which only have read access, like equality here. Even if StringName is cheap to copy, this still comes with refcount inc/dec + locking, when it would mostly need to compare two hashes.

Steps to reproduce

Equality-compare two StringName objects in a GDExtension binding and step in via debugger. Copy constructor of StringName is invoked.

Minimal reproduction project

N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants