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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion platform/javascript/detect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ def configure(env):
env.Append(CCFLAGS=["-fsanitize=leak"])
env.Append(LINKFLAGS=["-fsanitize=leak"])
if env["use_safe_heap"]:
env.Append(CCFLAGS=["-s", "SAFE_HEAP=1"])
env.Append(LINKFLAGS=["-s", "SAFE_HEAP=1"])

# Closure compiler
Expand Down Expand Up @@ -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.


# Wrap the JavaScript support code around a closure named Godot.
env.Append(LINKFLAGS=["-s", "MODULARIZE=1", "-s", "EXPORT_NAME='Godot'"])

Expand Down