Initial new INITIAL_HEAP setting
#21071
Conversation
SingleAccretion
force-pushed
the
Initial-Heap
branch
4 times, most recently
from
6be67f3 to
eb88bb2
Compare
Changes in default behavior: 1) INITIAL_HEAP is the new default for most builds. This means that there is an increase in the effective initial memory used by "sizeof(stack) + sizeof(static data)". In typical small applications this should be on the order of half a megabyte. 2) Because we cannot precisely calculate the amount of initial memory now, ASAN support will use the conservative upper estimate of MAXIMUM_MEMORY. This only affects ALLOW_MEMORY_GROWTH=0 builds. This change does not yet enable INITIAL_HEAP for builds that instantiate the memory in JS, e. g. with threading.
It seems this wasn't working for not-MINIMAL_RUNTIME for similar reasons for some time already.
The tests hardcode the initial memory amount. Make them robust against changes in defaults.
1 byte increase due to this diff: - (memory (;0;) 256 256) + (memory (;0;) 258 32768) We no longer cap the maximum memory.
SingleAccretion
force-pushed
the
Initial-Heap
branch
from
eb88bb2 to
5ddd199
Compare
|
|
tools/link.py
Outdated
| settings.INITIAL_HEAP = -1 | ||
| else: | ||
| # Some of these could (and should) be implemented. | ||
| exit_with_error('INITIAL_HEAP is currently not compatible with IMPORTED_MEMORY, SHARED_MEMORY, RELOCATABLE, ASYNCIFY_LAZY_LOAD_CODE, WASM2JS') |
There was a problem hiding this comment.
IMPORTED_MEMORY- this is just harmless, but I didn't want to complicate this further trying to detect ifIMPORTED_MEMORYis implicit or not.SHARED_MEMORY- ?.RELOCATABLE- not sure how the memory scheme here works. Do the side modules import the main module's memory?ASYNCIFY_LAZY_LOAD_CODE- ?.WASM2JS- apparently it is a code size regression to define memory in WASM. It is also a legacy mode.
There was a problem hiding this comment.
This seems like a new error? Right now I can do emcc -sINITIAL_MEMORY=32mb -sIMPORTED_MEMORY hello.c without any error.
There was a problem hiding this comment.
The error is under if 'INITIAL_HEAP' in user_settings, so it should only apply if you try to use INITIAL_HEAP with an unsupported (unimplemented) configuration.
There was a problem hiding this comment.
Right, I'm just confirming that that was not previously an error.
There was a problem hiding this comment.
How about INITIAL_HEAP is currently not compatible with imported memoy?
Also, I think WASM2JS should still supported INITIAL_HEAP even through the memory is technically imported (since we still pragmatically create the memory with the correct size). Can you explain why this is not possible?
I wonder if we should switch wasm2js to not use imported memory before we land this change?
There was a problem hiding this comment.
How about
INITIAL_HEAP is currently not compatible with imported memory?
I think it would technically work, but it's a bit misleading. There are two cases:
IMPORTED_MEMORYis set because the user set it, and they want to provide the memory object themselves. In this case, bothINITIAL_MEMORYandINITIAL_HEAPare basically meaningless. The only reason this scenario is blocked in this change is for simplicity: I didn't want to write code to detect whetherIMPORTED_MEMORYis set by the user, or implicitly, or both. I presume that explicitIMPORTED_MEMORYis rare.IMPORTED_MEMORYis set as an internal implementation detail of Emscripten, where it is convenient/necessary to instantiate the memory object in JS. 'Leaking' that into documentation would seem odd.
Also, I think WASM2JS should still supported INITIAL_HEAP even through the memory is technically imported (since we still pragmatically create the memory with the correct size). Can you explain why this is not possible?
I believe it is possible to switch all INITIAL_MEMORY cases to INITIAL_HEAP and delete the error. It is just work I have not done yet. For it to be done, I basically need to understand what are the reasons for implicit IMPORTED_MEMORY in all the cases that it is set, and what is the optimal solution in each case.
For WASM2JS specifically, it seems like there is a code size optimization in place where doing imported memory "manually" saves code size compared to the code generated by the wasm2js tool for WASM modules that define their own memory. So here the fix could be to improve the wasm2js tool, or read out the defined memory size from the linked WASM binary (this could be the solution to other cases too), or something else.
|
...ping... |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Can you remind me again of the use cases for INITIAL_MEMORY once we have this new setting?
Should we at least start to recommend this setting over INITIAL_MEMORY and try to push folks towards using it?
Can you add a ChangeLog entry?
Otherwise lgtm with some comments.
test/test_other.py
Outdated
| if expected_initial_heap != 0: | ||
| self.assertContained('--initial-heap=' + str(expected_initial_heap), out.stderr) | ||
| else: | ||
| self.assertNotContained('--initial-heap=', out.stderr) |
There was a problem hiding this comment.
If possible, I'd prefer not to test this by peeking inside like this but I guess there is no other easy way?
I suppose we could compare __heap_base with __builtin_wasm_memory_size within the code? But that might not work for == 0 case here.
There was a problem hiding this comment.
It was an intentional decision do this sort of "white box" testing. There were two reasons:
- It's easier. As you mention, it is somewhat non-obvious how to test the
0case. - It's composable in a sense that it tests only the functionality we're adding (i. e. we're not adding the feature that lays out a memory in a particular way, we're adding a feature that makes it such that a particular argument is passed to the linker).
The upside is that it is more resilient against changes in the other parts of the system. The downside is that it assumes the process involves invoking the linker, but it seems unlikely that the linker will go away any time soon (interestingly, writing a test with __heap_base would also assume a very particular linker implementation).
Two cases:
The latter is the bigger of the two of course, as it prevents using the setting with e. g. threading.
I had some reservations about it initially, due to the above issue, but have gone ahead and added some text to this effect. |
|
Looks like |
This is suspect...
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
lgtm with a couple of final comments.
Note that this will ignore user-supplied MAXIMUM_MEMORY when growth is disabled. This is consistent with existing behavior.
Implementing ALLOW_MEMORY_GROWTH=0 properly revealed an invalid assumption in the test: that MAXIMUM_MEMORY is respected under ALLOW_MEMORY_GROWTH=0 (default). Since this is not true, pass -sALLOW_MEMORY_GROWTH=1 alongside MAXIMUM_MEMORY.
|
@sbc100 thank you for all the support in getting this through! |
Thanks for contributing! |
2 participants
This introduces the
INITIAL_HEAPsetting originally discussed in #20888 to Emscripten. This setting is intended to supersedeINITIAL_MEMORY, which requires all consumers of the Emscripten toolchain to guesstimate its intended value. E. g. this was just recently hit again in dotnet/runtime#96840, dotnet/runtime#97634.This change uses
INITIAL_HEAPby default in the following configurations:INITIAL_MEMORYnorMAXIMUM_MEMORY. SinceINITIAL_HEAPadds to the static data and stack, and those settings place an upper bound on the size of initial memory, enablingINITIAL_HEAPfor users who specify these settings would be a breaking change, with the possible consequence of a compilation failure.Since
INITIAL_HEAPis additive, using it by default increases the initial memory by "size of stack + size of static data" (due to backwards compatibility, we cannot set defaultINITIAL_HEAPlower than the previous default forINITIAL_MEMORY). I do not expect this increase to be noticeable in typical applications, but in certain edge cases (no memory growth, very large static data), it can be. In the worst case of an application using the whole 16MB for static data, this would double the memory usage. We can consider only enablingINITIAL_HEAPinALLOW_MEMORY_GROWTH=1builds if we consider it unacceptable to regress these kinds of applications.Another effect of
INITIAL_HEAP-by-default is initial memory forALLOW_MEMORY_GROWTH=0address-sanitized builds. Due to the uncertainty of static data size, the code now falls back to estimating usingMAXIMUM_MEMORY, which results in the same experience (of large initial memory usage) as withALLOW_MEMORY_GROWTH=1. Here, we can consider falling back toINITIAL_MEMORYif the user has this configurations.