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

Reduce number of addressing modes in GDScript VM #47727

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

vnen
Copy link
Member

@vnen vnen commented Apr 8, 2021

There's now only 3 addressing modes: stack, constant, and member.

Self, class, and nil are now present respectively in the first 3 stack slots. Global and class constants are moved to local constants when compiling. Named globals is only present on editor to use on tool singletons, so its use now emits a new instruction to copy the global to the stack.

This allow us to further optimize the VM later by embedding the addressing modes in the instructions themselves, which is better done with less permutations.

@vnen vnen added this to the 4.0 milestone Apr 8, 2021
@vnen vnen requested a review from a team as a code owner April 8, 2021 15:03
@qarmin
Copy link
Contributor

qarmin commented Apr 8, 2021

https://github.com/qarmin/Qarminer/archive/refs/heads/4.0.zip shows error

Invalid call. Nonexistent function 'find' in base 'bool'

https://github.com/qarmin/RegressionTestProject/archive/refs/heads/4.0.zip shows this errors

SCRIPT ERROR: Trying to call a function on a null value.
          at: _ready (res://Start.gd:17)
SCRIPT ERROR: Invalid type in function 'is_parent_class' in base '_ClassDB'. Cannot convert argument 1 from bool to StringName.
          at: _populate (res://Nodes/Nodes.gd:17)

Both projects works fine in master

@qarmin
Copy link
Contributor

qarmin commented Apr 8, 2021

Also with first project I have crash when running project without editor

core/variant/array.cpp:91:13: runtime error: applying non-zero offset to non-null pointer 0xfffffffffffffff8 produced null pointer
core/variant/array.cpp:91:27: runtime error: member call on null pointer of type 'struct Vector'
core/templates/vector.h:82:65: runtime error: member access within null pointer of type 'const struct Vector'
core/templates/vector.h:82:48: runtime error: member access within null pointer of type 'const struct Vector'
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /mnt/Miecz/mojgodot/bin/godot.linuxbsd.tools.64s() [0x1e3d532] (/mnt/Miecz/mojgodot/platform/linuxbsd/crash_handler_linuxbsd.cpp:54)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7fbf1f57c210] (??:0)
[3] CowData<Variant>::is_empty() const (/mnt/Miecz/mojgodot/./core/templates/cowdata.h:142)
[4] Vector<Variant>::is_empty() const (/mnt/Miecz/mojgodot/./core/templates/vector.h:82)
[5] Array::is_empty() const (/mnt/Miecz/mojgodot/core/variant/array.cpp:92)
[6] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/mnt/Miecz/mojgodot/modules/gdscript/gdscript_vm.cpp:2529)
[7] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (/mnt/Miecz/mojgodot/modules/gdscript/gdscript.cpp:1552)
[8] Object::call(StringName const&, Variant const**, int, Callable::CallError&) (/mnt/Miecz/mojgodot/core/object/object.cpp:765 (discriminator 1))
[9] Variant::call(StringName const&, Variant const**, int, Variant&, Callable::CallError&) (/mnt/Miecz/mojgodot/core/variant/variant_call.cpp:713)
[10] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/mnt/Miecz/mojgodot/modules/gdscript/gdscript_vm.cpp:1394)
[11] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (/mnt/Miecz/mojgodot/modules/gdscript/gdscript.cpp:1552)
[12] ScriptInstance::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) (/mnt/Miecz/mojgodot/core/object/script_language.cpp:311)
[13] Node::_notification(int) (/mnt/Miecz/mojgodot/scene/main/node.cpp:147)
[14] Node::_notificationv(int, bool) (/mnt/Miecz/mojgodot/./scene/main/node.h:45 (discriminator 14))
[15] Object::notification(int, bool) (/mnt/Miecz/mojgodot/core/object/object.cpp:795)
[16] Node::_propagate_ready() (/mnt/Miecz/mojgodot/scene/main/node.cpp:189)
[17] Node::_propagate_ready() (/mnt/Miecz/mojgodot/scene/main/node.cpp:179 (discriminator 2))
[18] Node::_set_tree(SceneTree*) (/mnt/Miecz/mojgodot/scene/main/node.cpp:2525)
[19] SceneTree::initialize() (/mnt/Miecz/mojgodot/scene/main/scene_tree.cpp:395)
[20] OS_LinuxBSD::run() (/mnt/Miecz/mojgodot/platform/linuxbsd/os_linuxbsd.cpp:256)
[21] /mnt/Miecz/mojgodot/bin/godot.linuxbsd.tools.64s(main+0x3fc) [0x1e3b012] (/mnt/Miecz/mojgodot/platform/linuxbsd/godot_linuxbsd.cpp:60)
[22] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7fbf1f55d0b3] (??:0)
[23] /mnt/Miecz/mojgodot/bin/godot.linuxbsd.tools.64s(_start+0x2e) [0x1e3ab5e] (??:?)

There's now only 3 addressing modes: stack, constant, and member.

Self, class, and nil are now present respectively in the first 3 stack
slots. Global and class constants are moved to local constants when
compiling. Named globals is only present on editor to use on tool
singletons, so its use now emits a new instruction to copy the global to
the stack.

This allow us to further optimize the VM later by embedding the
addressing modes in the instructions themselves, which is better done
with less permutations.
@vnen vnen force-pushed the gdscript-less-addressing branch from a6840b3 to cf4079c Compare April 8, 2021 17:30
@vnen
Copy link
Member Author

vnen commented Apr 8, 2021

@qarmin Thanks! It seems I had forgot to account for the new stack space in one case. Tested with both projects and I don't get script errors anymore nor crashes (with or without editor).

@akien-mga akien-mga merged commit 3aadbec into godotengine:master Apr 9, 2021
@akien-mga
Copy link
Member

Thanks!

@vnen vnen deleted the gdscript-less-addressing branch April 9, 2021 13:50
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.

None yet

4 participants