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.

Sign up for GitHub

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

Jump to bottom

[HTML5] Fix builds with emcc >= 2.0.17 on Chrome for Android. #49589

Closed
wants to merge 1 commit into from
Closed

[HTML5] Fix builds with emcc >= 2.0.17 on Chrome for Android. #49589

wants to merge 1 commit into from

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Jun 14, 2021

Disables a new default behaviour in emscripten that always inlines functions with only one caller.

This has been reported to cause performance issued on some browsers (emscripten-core/emscripten#13899), but more importantly, breaks Godot builds on Chrome for Android (not Firefox for Android, not Chrome for Desktop).

Also fix a warning about SAFE_HEAP being a linker only flag.

@Faless
[HTML5] Fix builds with emcc >= 2.0.17 on Chrome for Android.
cb63b4d 
Disables a new default behaviour in emscripten that always inlines
functions with only one caller.

This has been reported to cause performance issued on some browsers, but
more importantly, breaks Godot builds on Chrome for Android (not Firefox
for Android, not Chrome for Desktop).

Also fix a warning about SAFE_HEAP being a linker only flag.
@Faless Faless added bug platform:web topic:buildsystem cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jun 14, 2021
@Faless Faless added this to the 4.0 milestone Jun 14, 2021
@Faless Faless requested a review from a team as a code owner June 14, 2021 10:28
akien-mga
akien-mga reviewed Jun 14, 2021
View reviewed changes
platform/javascript/detect.py
@@ -213,6 +212,9 @@ def configure(env):
# Reduce code size by generating less support code (e.g. skip NodeJS support).
env.Append(LINKFLAGS=["-s", "ENVIRONMENT=web,worker"])

# Break Chrome for android? Need to report upstream.
env.Append(LINKFLAGS=["-s", "BINARYEN_EXTRA_PASSES=--one-caller-inline-max-function-size=1"])
Copy link
Member

Choose a reason for hiding this comment

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

In emscripten-core/emscripten#13899 they tested different values and it seems all users had performance regressions with 19307 and not 19306. Maybe forcing it to 1 is a bit overkill? Should we try something safe-ish like 4096 or 8192?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did some tests and found 2048 was working on my phone while results with 4096 were a bit inconsistent.
We can use that, but maybe we should test it on some (low-end) devices?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For reference, the upstream issue could be this: https://bugs.chromium.org/p/v8/issues/detail?id=11863

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, I've loaded some tests here (Chrome for Android 91.0.4472.101):

We could alternatively wait a little bit more and see it the upstream bug gets fixed instead.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can play it safe with 512 or something, and link the upstream report in a comment, so we know to revert or increase it once the bug is fixed.

@akien-mga akien-mga mentioned this pull request Jun 29, 2021
Closed
@Faless
Copy link
Collaborator Author

Faless commented Jul 13, 2021

Superseded by #50422.
The performance issues are being addressed upstream in binaryen WebAssembly/binaryen#3959 , and I would rather not introduce more custom values until that lands (then we should probably try to compare performances).

@Faless Faless closed this Jul 13, 2021
@akien-mga akien-mga added archived and removed cherrypick:3.3 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga left review comments

Assignees
No one assigned
Labels
archived bug platform:web topic:buildsystem
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Faless @akien-mga