Update and document Emscripten link-time options #174
Conversation
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]>
| # 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 | ||
| # arbitrarily, since it is gives a little more room and is easy to remember. | ||
| "-sSTACK_SIZE=1MB", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
| # 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 | ||
| # arbitrarily, since it is gives a little more room and is easy to remember. | ||
| "-sSTACK_SIZE=1MB", |
There was a problem hiding this comment.
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.
| # arbitrarily, since it is gives a little more room and is easy to remember. | ||
| "-sSTACK_SIZE=1MB", | ||
| # Initial amount of heap memory | ||
| "-sINITIAL_HEAP=1MB", |
There was a problem hiding this comment.
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.
| "-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 |
There was a problem hiding this comment.
I'm not a big fan of this reasoning. Perhaps elaborate based on the comments in this thread?
There was a problem hiding this comment.
Let's move this PR forward? Please add the same link-time options to the Makefile:
Line 40 in a982ad0
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.