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

Update and document Emscripten link-time options #174

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update and document Emscripten link-time options #174

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
15 changes: 15 additions & 0 deletions bazel/defs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,25 @@ def proxy_wasm_cc_binary(
"@proxy_wasm_cpp_sdk//:proxy_wasm_intrinsics_js",
],
linkopts = linkopts + [
# Setting to indicate module is a "reactor library" without a main() entry point:
# https://emscripten.org/docs/tools_reference/settings_reference.html#standalone-wasm
"--no-entry",
# File listing additional functions that Emscripten should expect to be implemented by the host:
# https://emscripten.org/docs/porting/connecting_cpp_and_javascript/Interacting-with-code.html#implement-c-in-javascript
"--js-library=$(location @proxy_wasm_cpp_sdk//:proxy_wasm_intrinsics_js)",
# Emit Wasm module that can run without JavaScript
"-sSTANDALONE_WASM",
# Give host code access to Emscripten's _malloc() function
"-sEXPORTED_FUNCTIONS=_malloc",
# Allow allocating memory past initial heap size
"-sALLOW_MEMORY_GROWTH=1",
# Total stack size (fixed). Emscripten default stack size changed from 5MiB to 64KiB in
# 3.1.27: https://github.com/emscripten-core/emscripten/blob/main/ChangeLog.md,
# https://github.com/emscripten-core/emscripten/pull/18191. We pick 1MiB somewhat
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of this reasoning. Perhaps elaborate based on the comments in this thread?

# arbitrarily, since it is gives a little more room and is easy to remember.
"-sSTACK_SIZE=1MB",
Copy link
Contributor

@martijneken martijneken Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Feels kind of wasteful for a function/plugin, but that's just a gut feeling.
Real world OS defaults have a wide range: https://stackoverflow.com/a/1826072

Do we have evidence that Emscripten's 64 KiB default is insufficient? If not I would vote to leave the default.

Copy link
Member

@PiotrSikora PiotrSikora Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Real world OS defaults have a wide range: https://stackoverflow.com/a/1826072

Those are default thread stack sizes, which are often an order of magnitude smaller.

The default process stack size on all my 64-bit systems seems to be 8 MiB (check with ulimit -s).

I believe that historically @kyessenov was opposed to changing this from the 5 MiB default. As far as I recall, the reason was mostly because Istio plugins are using protobuf library, which might have a deep recursive stack? Kuat, could you verify that they still work after the recent changes?

I know this is not apples-to-apples comparison, but Rust uses 1 MiB stack by default, and I don't recall anybody ever running into any issues with that in Proxy-Wasm Rust SDK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PiotrSikora We can put away the worries about protobuf (and CEL) in Wasm away from Istio perspective. Istio is not using Wasm for production and the Google filters are perma-null Wasm. I think smaller overhead is better to match Lua, so 1MB or even less makes sense to me. One thing to worry is that standard library functions hide some deep recursion that can manifest on the data plane in a surprising way. I don't know how to protect against that in a general purpose language.

# Initial amount of heap memory
"-sINITIAL_HEAP=1MB",
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, Rust SDK starts with a minimal 64 KiB heap (64 KiB is 1 Wasm page size), and only because it allocates read-only data with static strings, otherwise it would be zero.

],
tags = tags + [
"manual",
Expand Down
Loading