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

[3.x] Enable method type information on release builds #59793

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

vnen
Copy link
Member

@vnen vnen commented Apr 1, 2022

This is the same as #53545 but for 3.x.

As a result, there is an increase in the binary size in release builds. For reference, these are the sizes I get in my machine (Intel macOS) compiled using production=yes and them stripped:

       3.x: 39 282 568 (37.46 MiB)
   This PR: 40 192 544 (38.33 MiB)
Difference:    909 976 ( 0.87 MiB)

Which is about a 2.32% increase in size. The absolute increase is a bit bigger than the one in master which is likely because some of which I added here to release was already done in master before my PR. Nevermind, it is way smaller now. I'll do the same on master to reduce the size there too.

This also removes the differences in type checking between debug and release for GDScript, which fixes #59723.

@Calinou
Copy link
Member

Calinou commented Apr 1, 2022

Do you know if this type information could be made smaller somehow (e.g. by mangling argument names)?

core/method_bind.cpp Outdated Show resolved Hide resolved
vnen added 2 commits April 1, 2022 18:20
This is needed for GDScript (and potentially other scripting languages)
to properly identify type errors and avoid mismatch between release and
debug versions.

This increases the release bynary size by about 889 KiB.
This makes sure native methods and properties have the actual type
checked to compare for compatibility and inference.
@vnen
Copy link
Member Author

vnen commented Apr 1, 2022

I realized the D_METHOD was made a macro that ignores argument names when DEBUG_METHODS_ENABLED isn't defined, so my change added those to the binary. I now restored how it was before so the names are not included, which makes a lesser increase in size. I will update the OP with the new values.

@akien-mga
Copy link
Member

Went from 9.5% increase to 2.3%, very nice!
That's definitely an acceptable tradeoff to fix issues related to type inference on release.

@akien-mga akien-mga merged commit 99c07c9 into godotengine:3.x Apr 1, 2022
@akien-mga
Copy link
Member

Thanks!

@lufog
Copy link
Contributor

lufog commented Apr 2, 2022

@akien-mga, after this pr, on Windows, editor crash immediately on startup, with following backtrace:

Godot Engine v3.5.beta.custom_build.99c07c92e - https://godotengine.org
OpenGL ES 3.0 Renderer: NVIDIA GeForce GTX 1660/PCIe/SSE2
Async. shader compilation: OFF


================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v3.5.beta.custom_build (99c07c92eb901e021de45a34158ba0004f5962c2)
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] embree::TaskScheduler::wait
[1] embree::TaskScheduler::wait
[2] main
[3] main
[4] widechar_main
[5] _main
[6] main
[7] embree::TaskScheduler::wait
[8] BaseThreadInitThunk
-- END OF BACKTRACE --
================================================================

Previous build (v3.5.beta.custom_build [3855154]) works fine.

@smix8
Copy link
Contributor

smix8 commented Apr 2, 2022

Can confirm, (custom) builds compile without error but editor startup is completely broken on Windows with this / todays pr(s). The editor startup window crashes immediately which wasn't the case with yesterdays Windows build.

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.

5 participants