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

GDScript: Typed VM Take 2 #43725

Merged
merged 9 commits into from
Nov 23, 2020
Merged

Conversation

vnen
Copy link
Member

@vnen vnen commented Nov 20, 2020

New take from #43004, since that had issues with resource usage during compilation.

This uses the new core utilities to get function pointers instead of having discrete instructions. This is in the same speed improvement ballpark as the previous PR.

This does not include the builtin GDScript functions since it will require further changes in the parser/compiler, so they are best suited to a different Pull Request (also I might take the chance to remove a couple of those, so reviewing it independently might be better).

@vnen vnen added this to the 4.0 milestone Nov 20, 2020
@vnen vnen mentioned this pull request Nov 20, 2020
@qarmin
Copy link
Contributor

qarmin commented Nov 20, 2020

Project - https://github.com/qarmin/RegressionTestProject/archive/master.zip - crashes with this backtrace

Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] bin/godot.linuxbsd.tools.64s() [0x1cac258] (/mnt/Miecz/mojgodot/platform/linuxbsd/crash_handler_linuxbsd.cpp:54)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7fa9adb06210] (??:0)

@qarmin
Copy link
Contributor

qarmin commented Nov 20, 2020

When running project - https://github.com/qarmin/The-worst-Godot-test-project/archive/master.zip - then I got this address sanitizer error

==121552==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000020000000 at pc 0x000004d2c538 bp 0x7ffd7e6fcd50 sp 0x7ffd7e6fcd40
READ of size 8 at 0x000020000000 thread T0
    #0 0x4d2c537 in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_vm.cpp:1679
    #1 0x4862859 in GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) modules/gdscript/gdscript.cpp:1322
    #2 0x12f22511 in ScriptInstance::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object/script_language.cpp:314
    #3 0xc323765 in Node::_notification(int) scene/main/node.cpp:147
    #4 0x2a60da5 in Node::_notificationv(int, bool) scene/main/node.h:45
    #5 0x3a5c7c8 in Node3D::_notificationv(int, bool) scene/3d/node_3d.h:52
    #6 0xd58c288 in Camera3D::_notificationv(int, bool) scene/3d/camera_3d.h:41
    #7 0x12eba42c in Object::notification(int, bool) core/object/object.cpp:793
    #8 0xc3259c7 in Node::_propagate_ready() scene/main/node.cpp:188
    #9 0xc32522d in Node::_propagate_ready() scene/main/node.cpp:180
    #10 0xc32522d in Node::_propagate_ready() scene/main/node.cpp:180
    #11 0xc32522d in Node::_propagate_ready() scene/main/node.cpp:180
    #12 0xc374f7a in Node::_set_tree(SceneTree*) scene/main/node.cpp:2561
    #13 0xc4116be in SceneTree::init() scene/main/scene_tree.cpp:395
    #14 0x1cb6a02 in OS_LinuxBSD::run() platform/linuxbsd/os_linuxbsd.cpp:249
    #15 0x1caa010 in main platform/linuxbsd/godot_linuxbsd.cpp:58
    #16 0x7f28e64be0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #17 0x1ca9b4d in _start (/mnt/Miecz/mojgodot/bin/godot.linuxbsd.tools.64s+0x1ca9b4d)

Address 0x000020000000 is a wild pointer.

@vnen
Copy link
Member Author

vnen commented Nov 20, 2020

@qarmin I believe I fixed both issues (didn't test the address sanitizer but I have an idea of what caused it)

There are still some issues with the validated getter instructions but it's because a bug in the core functions. They are setting the value but not the type, so you might get a Nil (default) when there should be something.

@qarmin
Copy link
Contributor

qarmin commented Nov 20, 2020

Also in https://github.com/qarmin/RegressionTestProject/archive/master.zip
there is error about non existent Vector2 constructor

move_vector = Vector2(move_vector.x, -1.0 * move_vector.y)

there are also other errors like Invalid operands 'Nil' and 'float' in operator *

This projects works fine in master

@vnen
Copy link
Member Author

vnen commented Nov 20, 2020

Also in https://github.com/qarmin/RegressionTestProject/archive/master.zip
there is error about non existent Vector2 constructor

move_vector = Vector2(move_vector.x, -1.0 * move_vector.y)

Yes, that's what I mentioned that is a problem in core. I'll fix it in another PR to get independent review.

EDIT: To clarify, the issue is that move_vector.x is not setting the type in the getter, so it thinks it's a null, and there's no Vector2 constructor with null as argument.

@vnen
Copy link
Member Author

vnen commented Nov 20, 2020

The fix for the core issue is in #43728.

- Allow getting an opaque pointer, no matter the type (for ptrcall).
- Allow setting object pointer and id directly.
- Allow initializing the data given a type, to allow properly setting
  return types on ptrcalls.
To improve organization and reduce the size of compilation units.
Almost all instructions need variant arguments. With this change they
are loaded in an array before each instruction call. This makes the
addressing code be localized to less places, improving compilation
overhead and binary size by a small margin.

This should not affect performance.
It now uses the direct operator function pointer, which increases
performance in evaluation.
When the base type is known at compile-time, we can get a direct
function pointer that is faster than the regular set/get paths.
Methods from builtin types can be called by using the function pointer
when the argument and base types are known at compile time.
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested locally after rebasing on latest master, seems to work OK.

RegressionTestProject doesn't crash anymore (before PR: 340 errors/warnings, after PR: 317 errors/warnings, though I didn't double check if it's related to this PR).

The-worst-Godot-test-project fails due to !bool being broken in master currently (both before and after this PR). Tried working it around but there are many more errors due to changes to type conversion of Strings (before and after PR).

Let's merge and refine further in master.

@akien-mga akien-mga merged commit 4ed42bf into godotengine:master Nov 23, 2020
@akien-mga
Copy link
Member

Thanks!

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.

3 participants