Core: Reduce assignments in Variant Object constructor - #121834
Conversation
There was a problem hiding this comment.
Makes sense, but "Removes redundant assignments" is not a sufficient motivation for a PR.
I assume this is for performance purposes? If yes, we'd need to see some measurements as per our optimization guidelines.
I agree the new code should be faster; the question is if it matters in any practical case (so a profile would be enough for me).
|
Yes this was identified through GDScript function call profiling. Setup: calling an empty instance function in a loop. However I am unable to get results as high in release templates. Maybe MinGW vs MSVC differences? In a C++ benchmark, I only notice a difference when build with MSVC. Test snippet const int N = 100'000'000;
std::vector<Object *> objs;
for (int i = 0; i < 1024; i++) {
objs.push_back(memnew(CustomObject));
}
auto t = std::chrono::high_resolution_clock::now();
for (int i = 0; i < N; i++) {
Variant a(objs[i & 1023]);
if (!a) { // So the construction doesn't get optimized out
break;
}
}
auto t2 = std::chrono::high_resolution_clock::now();
auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(t2 - t).count();
print_line(duration / 1000.);I am not sure if this is change is desired if GCC already optimizes it. I haven't tested clang/LLVM. |
Ivorforce
left a comment
There was a problem hiding this comment.
Yea, it's possible compilers will be able to reason away most of the code, but I wouldn't always rely on it. MSVC especially isn't very good at it.
We optimize most constructors for this reason, and I think it's warranted to do it here too, since it's a hot path between C++ and Scripts/GDExtension.
96f32d0 to
1343950
Compare
Ivorforce
left a comment
There was a problem hiding this comment.
Technically more correct than the assignment path.
1343950 to
98ddee4
Compare



What problem(s) does this PR solve?
VariantObject*constructor.Additional information
Variant(Object *),ObjDatais empty-initialized becauseref_pointerunrefs the old value. This is not needed in a constructor so the new implementation extracts the main body fromref_pointer.