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 <mpw@google.com>
| # 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.
| # 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?
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.