Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 13, 2021

See ChangeLog.md for rationale.

@sbc100 sbc100 force-pushed the reduce_default_stack branch from 38865bc to 6fa466a Compare May 13, 2021 18:25
@sbc100 sbc100 requested review from dschuff and kripken and removed request for dschuff and kripken May 13, 2021 18:25
@sbc100
Copy link
Collaborator Author

sbc100 commented May 13, 2021

I like that the default pthread stack size now matches the default stack size of the main thread. Seems like a good idea to have them be the same by default.

ChangeLog.md Outdated
than on native platforms (since its only used for address taken values) it
seems like 1Mb might still be on the high side here. For reference, llvm
(`wasm-ld`) use 64kb as the default stack size. `DEFAULT_PTHREAD_STACK_SIZE`
was also reduces from 2Mb to 1Mb to match primary stack.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be even better to match LLVM? It seems there is no reason to have differences in the wasm ecosystem here.

That is, maybe we should go to 64K here? Or maybe LLVM should go to 1MB? (I don't think there's any reason for 5MB, I agree with the logic there - I think it was originally because that's the linux stack size.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the linux stack is 8Mb.. so I'm not sure where 5Mb came from.. maybe asm.js needed a lot more of it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it would make sense to have emscripten and llvm agree on the default.. but I fear that we could break more folks if we go down to 64k. I'm trying to remember now why we chose 1 page (64K) in wasm-ld..

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, good question, yeah... Linux is 8. So not sure where 5 came from. Maybe a typo 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like the default of 64Kb in wasm-ld goes way back to the initial commit: https://reviews.llvm.org/D34851

@sunfishcode would you object to doubling the default stack stack in the linker to 128k to match musl's default for new thread. The goal would be to allow emscripten to use the same default as wasm-ld.

@sbc100 sbc100 force-pushed the reduce_default_stack branch 2 times, most recently from 29efc0b to 5d079e8 Compare May 19, 2021 14:55
@sbc100
Copy link
Collaborator Author

sbc100 commented May 20, 2021

Based on feedback from the mailing list I'd like to move forward with this change to go from 5Mb to 1Mb. We may wish to further reduce this to 64Kb at some point but it seems much more likely to trip up some developers if/when we make that switch. I don't see the cost of possibility updating this limit twice as very significant. Nothing about this change prevents a future reduction.

@kripken
Copy link
Member

kripken commented May 20, 2021

There is a cost to doing this twice, in that each time there will be some users hit by it. In particular, in debug builds the shadow stack may be used quite a lot - that's the main thing I worry about.

Checking our current errors, if I build a simple program with too little stack, then in -O0 I get

RuntimeError: abort(RuntimeError: abort(Stack overflow! Stack cookie has been overwritten, expected hex dwords 0x89BACDFE and 0x2135467, but received 0x4ec1 4ec0) at Error

Perhaps it's worth improving that error message before making this change? But even that message is not guaranteed to happen. Maybe we can document the symptoms of a too-small stack in our docs at least?

Another option would be to stack checks mode 2 in -O0. That is slower, but it would actually catch all such errors. We should at least make sure that mode is well documented I guess.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 20, 2021

There is a cost to doing this twice, in that each time there will be some users hit by it.

The set of users hit but this first change will by definition not be hit by the second (because they will have set TOTAL_STACK to whatever they need). So the two sets will be distinct and the total number of affected users will be unchanged. So the cost seems pretty small.

Indeed it could be informative to us to make this change first.. see how many folks report they were affected and use that as a data point when deciding if we should move forward with further reductions. So I think maybe we could say doing it in two phases might actually have a benefit rather than a cost.

As you suggest I will look into the improving the error reporting before moving forward with this.

@juj
Copy link
Collaborator

juj commented Mar 1, 2022

Checking our current errors, if I build a simple program with too little stack, then in -O0 I get

RuntimeError: abort(RuntimeError: abort(Stack overflow! Stack cookie has been overwritten, expected hex dwords 0x89BACDFE and 0x2135467, but received 0x4ec1 4ec0) at Error
Perhaps it's worth improving that error message before making this change? But even that message is not guaranteed to happen. Maybe we can document the symptoms of a too-small stack in our docs at least?

Checking today with

#include <memory.h>

int main()
{
	char str[1024*1024];
	memset(str, 0, 1024*1024);
}
em++ a.cpp -o a.html -g2 -sASSERTIONS=2 -sSTACK_OVERFLOW_CHECK=2 -s TOTAL_STACK=64KB

I get

Aborted(stack overflow (Attempt to set SP to 0xfff103d0, with stack limits [0x410 - 0x10410]))
/Users/clb/emsdk/emscripten/main/a.js:146
      throw ex;
      ^

RuntimeError: Aborted(stack overflow (Attempt to set SP to 0xfff103d0, with stack limits [0x410 - 0x10410]))
    at abort (/Users/clb/emsdk/emscripten/main/a.js:1491:11)
    at ___handle_stack_overflow (/Users/clb/emsdk/emscripten/main/a.js:1839:7)
    at __original_main (<anonymous>:wasm-function[2]:0x1f5)
    at main (<anonymous>:wasm-function[3]:0x24a)
    at /Users/clb/emsdk/emscripten/main/a.js:1556:22
    at callMain (/Users/clb/emsdk/emscripten/main/a.js:2201:15)
    at doRun (/Users/clb/emsdk/emscripten/main/a.js:2258:23)
    at run (/Users/clb/emsdk/emscripten/main/a.js:2273:5)
    at runCaller (/Users/clb/emsdk/emscripten/main/a.js:2179:19)
    at removeRunDependency (/Users/clb/emsdk/emscripten/main/a.js:1457:7)

Same error message occurs with alloca:

#include <memory.h>
#include <alloca.h>

int main()
{
	char *str = (char*)alloca(1024*1024);
	memset(str, 0, 1024*1024);
}

so looks like this has been improved at some point?

Although with -sSTACK_OVERFLOW_CHECK=1 or smaller, I do get quite a cryptic program abort:

Screen Shot 2022-03-01 at 12 49 40 PM

However with the improved message in stack check=2 mode, I would be happy to reduce stack size all the way to 64KB. I think it is good to be very conservative by default, since many applications do not need much stack, and many developers may not know about the stack size setting, leading them to suboptimal results by accident.

@juj
Copy link
Collaborator

juj commented Mar 1, 2022

If people need more, one way to improve optics would be to implement stack bump profiling hooks, and then update --memoryprofiler option to capture those stack bump events, and let it record and display a stack usage "high watermark".

That way developers would be able to report how much stack they are actually using.

That kind of visualization feature already exists there from the old asm.js days, but it no longer functions and always reports zero since we don't get that info from the wasm backend.

See ChangeLog.md for rationale.
@sbc100 sbc100 force-pushed the reduce_default_stack branch from 5d079e8 to d82bfd4 Compare October 10, 2022 22:47
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 10, 2022

Although with -sSTACK_OVERFLOW_CHECK=1 or smaller, I do get quite a cryptic program abort:

Screen Shot 2022-03-01 at 12 49 40 PM

This actually looks really bad.. it looks like node itself actually crashed on the OOB access :-/

sbc100 added a commit that referenced this pull request Oct 11, 2022
Previously we defaulted to 1 in this case, but we want to improve the
error reporting around stack overflow in preparation for lowering the
default stack size.  See #14177
sbc100 added a commit that referenced this pull request Oct 12, 2022
Previously we defaulted to 1 in this case, but we want to improve the
error reporting around stack overflow in preparation for lowering the
default stack size. See #14177

I ran the benchmark test suite under node and could not measure any
significant effect:
sbc100 added a commit that referenced this pull request Oct 12, 2022
Previously we defaulted to 1 in this case, but we want to improve the
error reporting around stack overflow in preparation for lowering the
default stack size. See #14177

I ran the benchmark test suite under node and could not measure any
significant effect:
sbc100 added a commit that referenced this pull request Nov 5, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 5, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 5, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 5, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 5, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 8, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 8, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 8, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 8, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 8, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 8, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 9, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 9, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 10, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 10, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 10, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 10, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows STACK_OVERFLOW_CHECK=1 to detect stack overflow conditions
better without relying on the STACK_OVERFLOW_CHECK=2 binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the STACK_OVERFLOW_CHECK=2
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
sbc100 added a commit that referenced this pull request Nov 10, 2022
Also, when not using santizers (which currently rely on using the start
of memory).

This allows `STACK_OVERFLOW_CHECK=1` to detect stack overflow conditions
better without relying on the `STACK_OVERFLOW_CHECK=2` binaryen pass.

This works because when stack is placed first in memory stack overflow
results in SP dropping below zero which is a runtime error.  We can
then distinguish such runtime errors by looking at the SP value at the
time of the crash.

This change is part of a sequence of the effort to reduce the default
stack size.  The hope here is that we will be able to accurately catch
overflows in debug builds even without the `STACK_OVERFLOW_CHECK=2`
binaryen pass, thus minimizing the impact of reducing the stack size.

See #14177
@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 11, 2022

Closing in favor of #18191

@sbc100 sbc100 closed this Nov 11, 2022
@sbc100 sbc100 deleted the reduce_default_stack branch March 7, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants