Use LocalVector in Array

[Nazarwadim]

Due to the fact that Array behaves like RefCount (if we have two references to the object and change one array, the other also changes).

var arr:Array[int]
for i in 5:
	arr.push_back(i)
print(arr)
var arr2:Array[int] = arr
arr2[0] = 1231
print(arr, arr2)

Output:

[0, 1, 2, 3, 4]
[1231, 1, 2, 3, 4][1231, 1, 2, 3, 4]

Array has a built-in SafeRefcount for this. So, using COWVector provides overhead.

[AThousandShips]

This makes calls to Array::assign not a constant time operation when types are compatible, some benchmarking is in order for different cases

[Nazarwadim]

@AThousandShips This is copying. Immediately after copying, the user will want to make some changes to the array, which in any case will make a copy on write.

[AThousandShips]

That's not granted though, you can't assume that just because someone makes a copy they want to mutate it, it might be passing an argument

[Nazarwadim]

When passing arguments, we use this constructor:

Array::Array(const Array &p_from) {
	_p = nullptr;
	_ref(p_from);
}

Where no copying occurs.

[AThousandShips]

That's not the only case though, for example constructing a TypedArray from any other array with non-identical typed, or using assign directly, this might be used in various places internally as well so it'd be good to have a benchmark to consider, and to weigh against the benefits of this change

For example:

var my_base_array : Array[Node]
var my_derived_array : Array[Node2D]
...
my_base_array.assign(my_derived_array)

Will not invoke any copying currently, but will with this

Or any use of assign, it doesn't use _ref at all, and is a standard way to handle various cases

This is also used in VariantParser::parse_value where it creates a second array, parses into it, and then uses array.assign with the temporary value:

godot/core/variant/variant_parser.cpp

Lines 1351 to 1365 in f032af7

	Array values;
	err = _parse_array(values, p_stream, line, r_err_str, p_res_parser);
	if (err) {
	return err;
	}
	get_token(p_stream, token, line, r_err_str);
	if (token.type != TK_PARENTHESIS_CLOSE) {
	r_err_str = "Expected ')'";
	return ERR_PARSE_ERROR;
	}
	array.assign(values);
	value = array;

This will now always perform a full copy of the data instead of just referencing it

[AThousandShips]

There are two performance aspects here we need to benchmark and assess:

• The gain from removing any overhead from Vector
• The loss from losing COW, as Array does not do COW itself

[Nazarwadim]

Aha. I'll look into that.

For the first point, I think godot-benchmark will be enough.

For the second, I will do something like passing the parameters of various types of arrays and iteration or something like that and measuring time.

[Nazarwadim]

Benchmarks:
results_master.json
results_local_vector.json

About VariantParser::parse_value. Since array is usually typed and values is untyped, we will go to this part of the code.

godot/core/variant/array.cpp

Lines 248 to 275 in f032af7

	Vector<Variant> array;
	array.resize(size);
	Variant *data = array.ptrw();
	if (source_typed.type == Variant::NIL && typed.type != Variant::OBJECT) {
	// from variants to primitives
	for (int i = 0; i < size; i++) {
	const Variant *value = source + i;
	if (value->get_type() == typed.type) {
	data[i] = *value;
	continue;
	}
	if (!Variant::can_convert_strict(value->get_type(), typed.type)) {
	ERR_FAIL_MSG(vformat(R"(Unable to convert array index %d from "%s" to "%s".)", i, Variant::get_type_name(value->get_type()), Variant::get_type_name(typed.type)));
	}
	Callable::CallError ce;
	Variant::construct(typed.type, data[i], &value, 1, ce);
	ERR_FAIL_COND_MSG(ce.error, vformat(R"(Unable to convert array index %d from "%s" to "%s".)", i, Variant::get_type_name(value->get_type()), Variant::get_type_name(typed.type)));
	}
	} else if (Variant::can_convert_strict(source_typed.type, typed.type)) {
	// from primitives to different convertible primitives
	for (int i = 0; i < size; i++) {
	const Variant *value = source + i;
	Callable::CallError ce;
	Variant::construct(typed.type, data[i], &value, 1, ce);
	ERR_FAIL_COND_MSG(ce.error, vformat(R"(Unable to convert array index %d from "%s" to "%s".)", i, Variant::get_type_name(value->get_type()), Variant::get_type_name(typed.type)));
	}
	} else {

Where no COW occurs.

assign itself became slower due to copying(bigger array -> more time). In any case, assign with read only behavior is rarely used, so I don't think it will be a problem.