[GDScript] Perform update-and-assign operations in place when possible. #76496
Conversation
This turns two bytecode operations into one by using the assignment destination directly as the output of the binary operator. This manifests in operations like `+=`.
|
What does this mean in practice? Is this a performance buff for GDScript? I remember Array's documentation mentioned |
|
This is a reapplication of #72056 with fixes for the regressions it caused. It is a performance optimization for GDScript, though fairly narrow in scope. This does not on its own improve Array |
This comment was marked as outdated.
This comment was marked as outdated.
2 similar comments
This comment was marked as outdated.
This comment was marked as outdated.
|
Ping |
|
This is currently planned for 4.x, and we're in feature freeze and this is an enhancement with some uncertain possibilities of side effects, I think it's best to get this looked at after the release of 4.1, but you can ask over in the contributor chat to see as well |
Now that 4.1 has shipped, can this be reviewed? |
|
Ping |
| GDScriptDataType assignment_type = _gdtype_from_datatype(assignment->get_datatype(), codegen.script); | ||
| if (!has_setter && !assignment->use_conversion_assign && assignment_type.kind == GDScriptDataType::BUILTIN && og_value.type.kind == GDScriptDataType::BUILTIN && assignment_type.builtin_type == og_value.type.builtin_type) { | ||
| // If there's nothing special about the assignment, perform the assignment as part of the operator | ||
| gen->write_binary_operator(target, assignment->variant_op, og_value, assigned_value); |
There was a problem hiding this comment.
This will not work with static variables after #77129, since the member and target addresses are not initialized in this case (static variables are currently implemented using separate opcodes, not addressing modes).
static var a: Array = [1, 2]
static var b: Array = [3, 4]
func _run():
a += b
print(a) modules/gdscript/gdscript_byte_codegen.h:195 - Leaving block with non-zero temporary variables: 1
modules/gdscript/gdscript_byte_codegen.cpp:187 - Non-zero temporary variables at end of function: 1
There was a problem hiding this comment.
What is the correct way to detect this scenario? is_static doesn't seem to cover it.
There was a problem hiding this comment.
The && !is_static check should be enough to narrow it down to the current behavior. This means that static variables will not have this optimization.
But we also need to make sure that the code always pops all temporaries before the early return, just like in the code below. link
I think you need to check target. While this seems to work, I think the case with static variables has shown that there is probably a bug with temporaries in this change. I can't tell which one, because the branch above is huge and complex.
to_assign looks unnecessary to check at this point, as it is assigned below.
| GDScriptCodeGenerator::Address og_value = _parse_expression(codegen, r_error, assignment->assignee); | ||
| GDScriptDataType assignment_type = _gdtype_from_datatype(assignment->get_datatype(), codegen.script); | ||
| if (!has_setter && !assignment->use_conversion_assign && assignment_type.kind == GDScriptDataType::BUILTIN && og_value.type.kind == GDScriptDataType::BUILTIN && assignment_type.builtin_type == og_value.type.builtin_type) { | ||
| // If there's nothing special about the assignment, perform the assignment as part of the operator |
There was a problem hiding this comment.
| // If there's nothing special about the assignment, perform the assignment as part of the operator | |
| // If there's nothing special about the assignment, perform the assignment as part of the operator. |
Code style requires first letter of comment to be capitalized and comments to end in period
This turns two bytecode operations into one by using the assignment destination directly as the output of the binary operator. This manifests in operations like
+=.This updated version contains fixes for the bugs previously exposed. It does so by: