-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Reduce default stack size from 5MB to 64KB #18191
Conversation
LGTM from me, I'm very happy to see the default stacks get lowered to 64KB. |
22ac53a
to
db1a859
Compare
test/test_other.py
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
f5b6bfd
to
cc42bb1
Compare
I think this is now ready to land. PTAL. |
cc42bb1
to
541b69d
Compare
See ChangeLog.md for rationale.
541b69d
to
72c3c73
Compare
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this line added by accident? It's still under 3.1.27 as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
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.
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? |
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. |
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... |
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. |
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 agree; Thanks for hearing me out. |
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
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
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 |
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: Lines 610 to 620 in 7c0d2e7
Did the checks that were added in debug mode help you to identify and fix the issue? |
I'd suggest marking breaking changes as such.
yes, it did hint to te direction of research, thanks |
See ChangeLog.md for rationale.