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

Avoid unnecessary Dictionary conversions in GDScriptInstance::validate_property #98301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aaronp64
Copy link
Contributor

Updated GDScriptInstance::validate_property to only convert PropertyInfo to Dictionary if _validate_property function is found.

This can make Node.duplicate around 4-5x faster for nodes that have gdscript attached, when the script doesn't override _validate_property. Compared with scripts below:

TestClass:

class_name TestClass extends Node2D

var a
var b
var c
var d

#func _validate_property(property: Dictionary) -> void:
#	pass

Main Scene:

extends Node

func _ready():
	var node := TestClass.new()
	var start := Time.get_ticks_msec()
	for i in 1000:
		add_child(node.duplicate())
	var end := Time.get_ticks_msec()
	print("%dms" % (end - start))

Old:

139ms
238ms (_validate_property uncommented)

New:

28ms
239ms (_validate_property uncommented)

…e_property

Updated GDScriptInstance::validate_property to only convert PropertyInfo to Dictionary if _validate_property function is found.
@aaronp64 aaronp64 requested a review from a team as a code owner October 18, 2024 17:00
@dalexeev dalexeev added this to the 4.4 milestone Oct 18, 2024
@dalexeev
Copy link
Member

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I haven't tested it, but it seems reasonable in any case.

The only issue is the return condition of the function, _validate_property() is not a multi-level call, unlike _get_property_list() (see #81139). But the function call error is some kind of undefined behavior, so we can deal with it later (I mean move the return on line 1850 out of the conditional block).

C# and GDExtension seem to be unaffected by the bug.

void CSharpInstance::validate_property(PropertyInfo &p_property) const {
ERR_FAIL_COND(!script.is_valid());
Variant property_arg = (Dictionary)p_property;
const Variant *args[1] = { &property_arg };
Variant ret;
Callable::CallError call_error;
GDMonoCache::managed_callbacks.CSharpInstanceBridge_Call(
gchandle.get_intptr(), &SNAME("_validate_property"), args, 1, &call_error, &ret);
if (call_error.error != Callable::CallError::CALL_OK) {
return;
}
p_property = PropertyInfo::from_dict(property_arg);
}

virtual void validate_property(PropertyInfo &p_property) const override {
if (native_info->validate_property_func) {
// GDExtension uses a StringName rather than a String for property name.
StringName prop_name = p_property.name;
GDExtensionPropertyInfo gdext_prop = {
(GDExtensionVariantType)p_property.type,
&prop_name,
&p_property.class_name,
(uint32_t)p_property.hint,
&p_property.hint_string,
p_property.usage,
};
if (native_info->validate_property_func(instance, &gdext_prop)) {
p_property.type = (Variant::Type)gdext_prop.type;
p_property.name = *reinterpret_cast<StringName *>(gdext_prop.name);
p_property.class_name = *reinterpret_cast<StringName *>(gdext_prop.class_name);
p_property.hint = (PropertyHint)gdext_prop.hint;
p_property.hint_string = *reinterpret_cast<String *>(gdext_prop.hint_string);
p_property.usage = gdext_prop.usage;
}
}
}

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

Successfully merging this pull request may close these issues.

2 participants