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

Update and document Emscripten link-time options #174

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
Update and document Emscripten link-time options
Set INITIAL_HEAP=1MB and ALLOW_MEMORY_GROWTH=1 to reduce starting memory usage
of plugins, but allow the heap to grow. Explicitly set STACK_SIZE (previously
named TOTAL_STACK) to 1MiB. This is a middle ground between Emscripten's
default value prior to 3.1.27 of 5MiB and its default value since then of
64KiB.

Also add comments documenting the purpose of each link-time option.

Signed-off-by: Michael Warres <[email protected]>
mpwarres committed Oct 11, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 20545e5cbf6d8a749dac27eeb23d0a4012375684
15 changes: 15 additions & 0 deletions bazel/defs.bzl
Original file line number Diff line number Diff line change
@@ -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

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

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",