Reduce default stack size from 5MB to 64KB #18191

sbc100 · 2022-11-11T23:31:10Z

See ChangeLog.md for rationale.

ChangeLog.md

test/test_other.py

juj · 2022-11-12T13:08:39Z

LGTM from me, I'm very happy to see the default stacks get lowered to 64KB.

kripken · 2022-11-14T20:34:56Z

test/test_other.py

@@ -2620,6 +2620,8 @@ def test_embind(self, extra_args):
         '--pre-js', test_file('embind/test.pre.js'),
         '--post-js', test_file('embind/test.post.js'),
         '-sWASM_ASYNC_COMPILATION=0',
+         # The default stack size is not enough for these tests.


Do you know why that is? Does embind use lots of stack space for some reason? If so, in theory we could consider a different default stack size when --bind is set, though I don't know if that's a good idea.

This is one of our more heavy weight tests I think.. since it embeds an entire JS test suite inside a single unit test. I will try to figure out what is going on..

sbc100 · 2022-11-28T21:50:13Z

I think this is now ready to land. PTAL.

ChangeLog.md

See ChangeLog.md for rationale.

dasa · 2022-11-29T02:11:00Z

ChangeLog.md

@@ -48,6 +59,7 @@ See docs/process.md for more on how version tagging works.
  overflow will trap rather corrupting global data first).  This should not
  be a user-visible change (unless your program does something very odd such
  depending on the specific location of stack data in memory). (#18154)
+- Add support for `-sEXPORT_ES6`/`*.mjs` on Node.js. (#17915)


Was this line added by accident? It's still under 3.1.27 as well.

Oops, well spotted. Bad merge I assume

After emscripten-core#18191 this started to fail. This program does DFS, so it uses deep recursive calls. https://github.com/emscripten-core/emscripten/blob/02cc1eecd81d44e4456aa27a9ebddfe5f54c33e6/test/havlak.cpp#L497

After #18191 this started to fail. This program does DFS, so it uses deep recursive calls. https://github.com/emscripten-core/emscripten/blob/02cc1eecd81d44e4456aa27a9ebddfe5f54c33e6/test/havlak.cpp#L497

This was always my intention with #18191 but somehow it got left out. I think it makes sense that user to override `STACK_SIZE` also, by default, override the pthread stack size. Such users can always explicitly set the pthread stack size if they don't want this behaviour. See #18472

Emscripten's stack size was recently decreased to 64 kB from 5 MB, (emscripten-core/emscripten#18191). Stack overflow appears to be the cause of frequent crashes of Tracy in my browser, especially at start-up. This increase is modest, but seems to be enough to resolve the issue.

nikitalita · 2023-03-22T19:44:29Z

This was a pretty major, breaking change in a patch(!) point release. Was there a reason why this wasn't at least saved for a minor point release?

sbc100 · 2023-03-22T19:53:00Z

Agreed in theory yes.

However, we've mostly given up on the idea doing anything except patch version changes at this point. Some folks seem to prefer an monotonically increasing 3.1.XX version. At least @tlively prefers that. Our minor and major version never really have much significance.. except that one time we switch away from fastcomp and bumped to 2.0.

nikitalita · 2023-03-22T20:02:26Z

I mean, I'm sure it's pleasing for aesthetic reasons, but I'm fairly certain that an indicator when there's a potentially breaking API change would trump that for most users...

sbc100 · 2023-03-22T20:13:11Z

The problem we face is that pretty much all our releases contain potentially breaking changes. Even the most simple bug fixes can often break users in unexpected ways. I don't know that we have ever made a release that couldn't potentially break somebody.

That is not to say that certain changes are not more or less likely to be breaking folks, but I don't think there such a thing as completely safe change (outside of things like documentation-only changes).

I also agree with you that this change in particular is likely to effect more users than most, and that is why we did have a lot of discussions on the mailing list about it this prior to landing it.

Perhaps we should have a wider discussion about the value of bumping anything except the patch level.

nikitalita · 2023-03-22T20:40:21Z

The problem we face is that pretty much all our releases contain potentially breaking changes. Even the most simple bug fixes can often break users in unexpected ways. I don't know that we have ever made a release that couldn't potentially break somebody.

That is not to say that certain changes are not more or less likely to be breaking folks, but I don't think there such a thing as completely safe change (outside of things like documentation-only changes).

Certainly; it's an XKCD comic. However, when a change that is obviously breaking to many users ends up in a patch release, this breaks trust with users; it encourages them to never update. This would be pretty bad if there were critical security patches that users didn't end up getting.

I also agree with you that this change in particular is likely to effect more users than most, and that is why we did have a lot of discussions on the mailing list about it this prior to landing it.

Perhaps we should have a wider discussion about the value of bumping anything except the patch level.

I agree; Thanks for hearing me out.

juj · 2023-03-23T08:04:15Z

However, we've mostly given up on the idea doing anything except patch version changes at this point. Some folks seem to prefer an monotonically increasing 3.1.XX version. At least @tlively prefers that. Our minor and major version never really have much significance.. except that one time we switch away from fastcomp and bumped to 2.0.

That is interesting. I found it quite surprising that Emscripten jumped from version 2 to 3 on the basis of "just" a musl update, maybe this new philosophy is a counter-direction to that?

I don't see there being anything to make a Emscripten 4.0 any time soon, but we do occassionally do the deprecation-removal game on features, so I would recommend that the minor version be bumped at least when a feature is removed?

Emscripten changed the default stack size from 5 MiB to 64 KiB. Until we figure out what stack size we actually need, we have to explicitly restore the old default. Refs: emscripten-core/emscripten#18191

Newer emscripten versions now only give a 64 KB stack by default (emscripten-core/emscripten#18191) Older emscripten versions gave a 5 MB stack by default, so we can allocate 4 MB more for use by the game itself

zowers · 2024-04-26T17:24:05Z

This change is really a breaking change and should have been highlited in the changelog as such, it causes code using cereal to crash

cereal rapidxml has

#define CEREAL_RAPIDXML_STATIC_POOL_SIZE (64 * 1024)
```cpp
https://github.com/USCiLab/cereal/blob/d1fcec807b372f04e4c1041b3058e11c12853e6e/include/cereal/external/rapidxml/rapidxml.hpp#L120
and uses it as
```cpp
class memory_pool {
...
char m_static_memory[CEREAL_RAPIDXML_STATIC_POOL_SIZE];    // Static raw memory

https://github.com/USCiLab/cereal/blob/d1fcec807b372f04e4c1041b3058e11c12853e6e/include/cereal/external/rapidxml/rapidxml.hpp#L649

described here for android, but the issue is the same https://www.programmerall.com/article/4274630636/

same issue with boost property_tree, from which rapidxml originates https://github.com/boostorg/property_tree/blob/8080ecd14f2d952a4bb7ae869fc0f54f54f5a31f/include/boost/property_tree/detail/rapidxml.hpp#L91

sbc100 · 2024-04-26T17:42:29Z

This change is really a breaking change and should have been highlited in the changelog as such, it causes code using cereal to crash

Yes, this is no doubt a breaking change for code that uses a lot of stack space.

It did get a fairly long entry in the ChangeLog:

emscripten/ChangeLog.md

Lines 610 to 620 in 7c0d2e7

    
           - The default `STACK_SIZE` was reduced from 5MB to 64KB.  Projects that use more 
        
             than 64Kb of stack will now need specify `-sSTACK_SIZE` at link time.  For 
        
             example, `-sSTACK_SIZE=5MB` can be used to restore the previous behaviour. 
        
             To aid in debugging, as of #18154, we now also place the stack first in memory 
        
             in debug builds so that overflows will be immediately detected, and result in 
        
             runtime errors.  This change brings emscripten into line with `wasm-ld` and 
        
             wasi-sdk defaults, and also reduces memory usage by default.  In general, 
        
             WebAssembly stack usage should be lower than on other platforms since a lot of 
        
             state normally stored on the stack is hidden within the runtime and does not 
        
             occupy linear memory at all.  The default for `DEFAULT_PTHREAD_STACK_SIZE` was 
        
             also reduced from 2MB to 64KB to match.

Did the checks that were added in debug mode help you to identify and fix the issue?

zowers · 2024-04-27T06:48:49Z

Yes, this is no doubt a breaking change for code that uses a lot of stack space.

I'd suggest marking breaking changes as such.

Did the checks that were added in debug mode help you to identify and fix the issue?

yes, it did hint to te direction of research, thanks

sbc100 requested review from kripken, juj and dschuff November 11, 2022 23:31

sbc100 mentioned this pull request Nov 11, 2022

Reduce default stack size from 5Mb to 1Mb #14177

Closed

juj reviewed Nov 12, 2022

View reviewed changes

ChangeLog.md Outdated Show resolved Hide resolved

juj reviewed Nov 12, 2022

View reviewed changes

ChangeLog.md Outdated Show resolved Hide resolved

juj reviewed Nov 12, 2022

View reviewed changes

ChangeLog.md Outdated Show resolved Hide resolved

juj reviewed Nov 12, 2022

View reviewed changes

test/test_other.py Outdated Show resolved Hide resolved

sbc100 force-pushed the reduce_default_stack2 branch from 22ac53a to db1a859 Compare November 13, 2022 18:00

sbc100 changed the title ~~Reduce default stack size from 5Mb to 64Kb~~ Reduce default stack size from 5MB to 64KB Nov 14, 2022

kripken reviewed Nov 14, 2022

View reviewed changes

sbc100 force-pushed the reduce_default_stack2 branch 6 times, most recently from f5b6bfd to cc42bb1 Compare November 28, 2022 20:39

sbc100 requested review from kripken and juj November 28, 2022 21:49

kripken approved these changes Nov 28, 2022

View reviewed changes

ChangeLog.md Outdated Show resolved Hide resolved

ChangeLog.md Outdated Show resolved Hide resolved

sbc100 force-pushed the reduce_default_stack2 branch from cc42bb1 to 541b69d Compare November 28, 2022 22:30

Reduce default stack size from 5Mb to 64Kb

72c3c73

See ChangeLog.md for rationale.

sbc100 force-pushed the reduce_default_stack2 branch from 541b69d to 72c3c73 Compare November 28, 2022 22:30

sbc100 merged commit 157fcd4 into main Nov 28, 2022

sbc100 deleted the reduce_default_stack2 branch November 28, 2022 22:31

dasa reviewed Nov 29, 2022

View reviewed changes

sbc100 added a commit that referenced this pull request Nov 29, 2022

Remove extra changelog entry added in #18191. NFC

5dcbd93

sbc100 added a commit that referenced this pull request Nov 29, 2022

Remove extra changelog entry added in #18191. NFC (#18269)

afa75f3

aheejin mentioned this pull request Nov 30, 2022

[Benchmarks] Increase havlak's stack size #18274

Merged

sbc100 mentioned this pull request Jan 5, 2023

Stack size error message missing important setting #18472

Closed

sbc100 mentioned this pull request Jan 9, 2023

Set pthread stack size when -sSTACK_SIZE is set #18479

Merged

lewing mentioned this pull request Jan 23, 2023

[wasm] Update emsdk to latest dotnet/runtime#80784

Closed

sbc100 mentioned this pull request Mar 23, 2023

Create versioning policy for emscripten #19053

Open

deadprogram mentioned this pull request Sep 24, 2023

WASM: stack overflow when unmarshalling JSON tinygo-org/tinygo#3900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce default stack size from 5MB to 64KB #18191

Reduce default stack size from 5MB to 64KB #18191

sbc100 commented Nov 11, 2022

juj commented Nov 12, 2022

kripken Nov 14, 2022

sbc100 Nov 14, 2022

sbc100 commented Nov 28, 2022

dasa Nov 29, 2022

sbc100 Nov 29, 2022

nikitalita commented Mar 22, 2023

sbc100 commented Mar 22, 2023

nikitalita commented Mar 22, 2023

sbc100 commented Mar 22, 2023

nikitalita commented Mar 22, 2023 •

edited

Loading

juj commented Mar 23, 2023

zowers commented Apr 26, 2024

sbc100 commented Apr 26, 2024

zowers commented Apr 27, 2024

Reduce default stack size from 5MB to 64KB #18191

Reduce default stack size from 5MB to 64KB #18191

Conversation

sbc100 commented Nov 11, 2022

juj commented Nov 12, 2022

kripken Nov 14, 2022

Choose a reason for hiding this comment

sbc100 Nov 14, 2022

Choose a reason for hiding this comment

sbc100 commented Nov 28, 2022

dasa Nov 29, 2022

Choose a reason for hiding this comment

sbc100 Nov 29, 2022

Choose a reason for hiding this comment

nikitalita commented Mar 22, 2023

sbc100 commented Mar 22, 2023

nikitalita commented Mar 22, 2023

sbc100 commented Mar 22, 2023

nikitalita commented Mar 22, 2023 • edited Loading

juj commented Mar 23, 2023

zowers commented Apr 26, 2024

sbc100 commented Apr 26, 2024

zowers commented Apr 27, 2024

nikitalita commented Mar 22, 2023 •

edited

Loading