Update memory configurations and simplify specification #494
Conversation
Several changes to make memory allocation cleaner: 1) enable build-time configuration of the memory configuration for the eservice enclave (the pservice enclave does not have dynamic memory requirements so they remain fixed). 2) a single variable now controls the allocation for enclave and interpreter memory; PDO_MEMORY_CONFIG may be set to SMALL, MEDIUM, or LARGE. this replaces the old WASM_MEM_CONFIG which was used for interpreter memory configuration. The SMALL configuration sets up the enclave and the interpreter for 4MB, MEDIUM is for 8MB and LARGE is for 16MB. 3) remove the stack and heap configuration from the contract build scripts; this appears to be unnecessary though further testing is warranted. Signed-off-by: Mic Bowman <[email protected]>
…build variable Signed-off-by: Mic Bowman <[email protected]>
Signed-off-by: Mic Bowman <[email protected]>
There was a problem hiding this comment.
LGTM! Thanks @cmickeyb . I only have a couple of non-blocking questions regarding some of the test cases. We probably also want to open an new issue so we don't forget to do more testing/add more test cases.
There was a problem hiding this comment.
Why are these test cases being removed?
There was a problem hiding this comment.
mostly because they no longer fail and are redundant with other tests already in place.
There was a problem hiding this comment.
I suppose this is where we'd add more tests to push the higher memory size limits?
Writing the "negative" memory tests turns out to be harder than it looks.
At best we can say "the CI will pass only for the medium memory configuration". Figure out through trial & error what works and what fails. And then update the what works/what fails every time we make a change to the interpreter. Anyway... i removed the negative tests for out of memory because I couldn't get failure to be consistent. Would love to have another set of eyes on the problem. |
| LIST(APPEND WASM_LINK_OPTIONS "-Wl,--initial-memory=${LINEAR_MEMORY}") | ||
| LIST(APPEND WASM_LINK_OPTIONS "-Wl,--max-memory=${LINEAR_MEMORY}") | ||
| LIST(APPEND WASM_LINK_OPTIONS "-z stack-size=${INTERNAL_STACK_SIZE}") |
There was a problem hiding this comment.
I cannot find much documentation about this (i.e., how memory is handled initially and at runtime).
Can it be that the default settings make WAMR allocate static chunks of memory as the interpreter is instantiated?
If that's the case, this would raise the startup cost for any contract execution -- this similarly holds for the enclave as well, but that's a one-time cost.
There was a problem hiding this comment.
i think this was a requirement for earlier versions of clang. this was necessary for the combination of the compiler and the interpreter. the wamr documentation no longer talks about this as a requirement.
AND
making this explicit tend to make the contracts themselves very unstable.
There was a problem hiding this comment.
Regarding the allocation... this effectively has nothing to do with the allocation in the interpreter. The fields in the common/interpreter/wawaka cmake file are the only ones that matter for actual allocation.
| <StackMaxSize>${ENCLAVE_STACK_SIZE}</StackMaxSize> | ||
| <HeapMaxSize>${ENCLAVE_HEAP_SIZE}</HeapMaxSize> | ||
| <ReservedMemMaxSize>${ENCLAVE_RESERVED_SIZE}</ReservedMemMaxSize> | ||
| <ReservedMemInitSize>${ENCLAVE_RESERVED_SIZE}</ReservedMemInitSize> | ||
| <ReservedMemExecutable>1</ReservedMemExecutable> |
There was a problem hiding this comment.
i think thats a deeper discussion and requires additional research on its impact for different WAMR settings. this has been our setting all along. i would suggest that we add an issue to investigate the implications.
| @@ -80,11 +51,7 @@ LIST(APPEND WASM_BUILD_OPTIONS "-std=c++11") | |||
| LIST(APPEND WASM_BUILD_OPTIONS "-DUSE_WASI_SDK=1") | |||
|
|
|||
| SET(WASM_LINK_OPTIONS) | |||
| LIST(APPEND WASM_LINK_OPTIONS "-Wl,--initial-memory=${LINEAR_MEMORY}") | |||
| LIST(APPEND WASM_LINK_OPTIONS "-Wl,--max-memory=${LINEAR_MEMORY}") | |||
| LIST(APPEND WASM_LINK_OPTIONS "-z stack-size=${INTERNAL_STACK_SIZE}") | |||
| LIST(APPEND WASM_LINK_OPTIONS "-Wl,--allow-undefined") | |||
There was a problem hiding this comment.
These are build/link-time memory options, hence under the control of developers/users. I am wondering if any of these should be checked (somehow) by the eservice before running the related contract for safety or efficiency reasons.
The interpreter's memory configuration should define the boundaries of a contract's memory configuration, so safety should not be an issue. However, contracts could still always max out memory. Does that determine an efficiency issue for the eservice or an unfair allocation w.r.t. other contracts?
There was a problem hiding this comment.
if the numbers are too big then the interpreter will throw the error. and... in fact... i have a hard time making anything other than the default work with the current version of WAMR.
and to be clear... these are contract allocations. any developer can set them to whatever is required.
|
@bvavala FYI... I plan to add another PR shortly that puts memory configuration in the interpreter information string in the enclave. its not part of this PR because it isn't required. |
Several changes to make memory allocation cleaner:
enable build-time configuration of the memory configuration for the eservice enclave (the pservice enclave does not have dynamic memory requirements so they remain fixed).
a single variable now controls the allocation for enclave and interpreter memory; PDO_MEMORY_CONFIG may be set to SMALL, MEDIUM, or LARGE. this replaces the old WASM_MEM_CONFIG which was used for interpreter memory configuration. The SMALL configuration sets up the enclave and the interpreter for 4MB, MEDIUM is for 8MB and LARGE is for 16MB.
remove the stack and heap configuration from the contract build scripts; this appears to be unnecessary though further testing is warranted.