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

Enable method type information on release builds #53545

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

vnen
Copy link
Member

@vnen vnen commented Oct 7, 2021

This is needed to ensure GDScript compilation works properly on release builds and make use of optimized typed instructions.

Also fix a bug with ptrcalls in the GDScript VM.

This do increase the binary size. For comparison, here's the sizes I got in my machine for the Linux release build, compiled with production=yes and then stripped:

master: 60 868 416 bytes (58.05 MiB)
this pr: 64 202 560 bytes (61.23 MiB)
difference: 3 334 144 bytes (3.18 MiB)

I was able to run the regression test project without noticing any issue (considering that #53543 is also applied to avoid crashing).

@vnen vnen added this to the 4.0 milestone Oct 7, 2021
@vnen vnen requested review from a team as code owners October 7, 2021 18:25
This is needed to ensure GDScript compilation works properly on release
builds and make use of optimized typed instructions.
@akien-mga
Copy link
Member

akien-mga commented Oct 7, 2021

Does it fix #51060? (Well I guess we can ask there once merged, the two reproduction projects are fairly complex.)

@akien-mga akien-mga merged commit 6090f90 into godotengine:master Oct 7, 2021
@akien-mga
Copy link
Member

Thanks!

@vnen
Copy link
Member Author

vnen commented Oct 7, 2021

Does it fix #51060?

It should. I couldn't try out the projects because they are outdated and can't run on current master.

@unfa
Copy link

unfa commented Oct 15, 2021

I assume this chage did not go into the recent pre-alpha release?

We'll test this against current master, probably next week.

@neikeq
Copy link
Contributor

neikeq commented Mar 5, 2022

I understand this was needed, but I hope a better solution can be found in the future. Type information should not be included in release builds (because of its effect on binary size, which is mentioned here, but also memory and startup speed). I don't know how the GDScript VM works, but maybe this could be solved by the editor adding the required type information when exporting the game, or with AOT compilation.
Lastly, one question: are argument names (which this PR adds to release builds) actually needed by the GDScript VM?

@vnen
Copy link
Member Author

vnen commented Mar 23, 2022

@neikeq there's not much that can be done because scripts are compiled when loaded, even in release builds. My plan is to add an IR that would be created on export, so no compilation needed on export (could even remove the GDScript compiler but it doesn't do much of a difference). The only issue is that technically you can create scripts dynamically, which would need compilation, but we can forbid typing on those.

The argument names aren't needed, if I added them here was by an oversight.

@Calinou
Copy link
Member

Calinou commented Mar 23, 2022

The only issue is that technically you can create scripts dynamically, which would need compilation, but we can forbid typing on those.

Would type hints be forbidden entirely when compiling a script at run-time, or just ignored? I have a project that relies on run-time GDScript compilation, and I'd like it to be able to compile the same GDScript code as the editor.

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