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

SCons: Add stack_size and default_pthread_stack_size options to Web target #75166

Merged
merged 1 commit into from Jan 3, 2024

Conversation

nikitalita
Copy link
Contributor

This adds the following compile options to web builds with emscripten:

  • stack_size: the stack size for the main thread (this cannot be adjusted at runtime), defaults to 5MB
  • default_pthread_stack_size: the default stack size for any pthreads spawned, defaults to 2MB

The reason for these options being added is because versions of emscripten prior to 3.1.27 had the above default values. emscripten 3.1.27 adjusted the defaults of these down to 64KiB. This ended up breaking a lot of things in Godot; the editor would simply crash due to a stack overflow upon load.

We may want to adjust the default values for these downwards for non-editor builds, but I figured I should just set it back to where it was for now.

@nikitalita
Copy link
Contributor Author

nikitalita commented Dec 11, 2023

@Faless can you please take a look at this? This is still an issue and there's no easy way to pass these flags into emscripten otherwise.

For verification purposes, you can see the default values for 3.1.26 vs. 3.1.27-current (3.1.50) here:

@akien-mga
Copy link
Member

Sorry for the delay. Seems like we overlooked this, or the fact that it's fixing a critical build issue. It's described in the OP but was overlooked as this looked more like adding a new feature, than fixing a bug. For next time I may suggest opening a bug report so it would have been clear to us (and notably me as buildsystem maintainer) that we have a critical compatibility issue with Emscripten >= 3.1.27.

We debugged this issue from another bug report #85564 and fixed it yesterday: #86036.

So this fix is no longer needed technically. But the added configurability makes sense to me, so I think it can be rebased to make the new link flags added in #86036 use the options you're adding here.

@akien-mga akien-mga changed the title build: Add stack_size and thread stack_size options to web target SCons: Add stack_size and default_pthread_stack_size options to Web target Dec 12, 2023
platform/web/detect.py Outdated Show resolved Hide resolved
@nikitalita nikitalita requested a review from a team as a code owner December 17, 2023 16:36
@Faless
Copy link
Collaborator

Faless commented Dec 18, 2023

This PR should be rebased on #86036 and updated to change the flags values in place.
Right now merging it will result in some flags being set two times / overridden by the other change.

@akien-mga
Copy link
Member

Rebased on the latest master myself to solve the double definition of those flags.

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 3, 2024
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 3, 2024
@akien-mga akien-mga merged commit dc0114f into godotengine:master Jan 3, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 16, 2024
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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

7 participants