Skip to content
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

runtime: Add env var for maximum stack size #2719

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

evaporei
Copy link
Contributor

@evaporei evaporei commented Aug 18, 2021

Just an environment variable to set the maximum stack size for the WASM runtime.
If not set, the default is 512KiB, half of what it was before (default from wasmtime crate was 1MiB).

@evaporei evaporei requested a review from leoyvens August 18, 2021 14:37
@evaporei evaporei self-assigned this Aug 18, 2021
Copy link
Collaborator

@leoyvens leoyvens left a comment

Choose a reason for hiding this comment

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

runtime/wasm/src/mapping.rs Outdated Show resolved Hide resolved
runtime/wasm/src/mapping.rs Outdated Show resolved Hide resolved
@evaporei
Copy link
Contributor Author

@leoyvens should I put it in the running mapping handlers section, or create another one about the runtime?

@evaporei evaporei force-pushed the otavio/runtime-max-stack-size branch from 8778668 to 37e27e2 Compare August 18, 2021 14:52
@@ -140,6 +149,8 @@ impl ValidModule {
config.interruptable(true); // For timeouts.
config.cranelift_nan_canonicalization(true); // For NaN determinism.
config.cranelift_opt_level(wasmtime::OptLevel::None);
config.max_wasm_stack(*MAX_STACK_SIZE)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it makes a difference to unwrap right here? The only error this can return is this one, which is when the size parameter is 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be unwrapped.

Copy link
Contributor Author

@evaporei evaporei Aug 19, 2021

Choose a reason for hiding this comment

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

Unwrapped (with comment 🙂 )!

.ok()
.and_then(|max_stack_size| max_stack_size.parse().ok())
// 512KiB
.unwrap_or((ONE_MIB as f32 * 0.5) as usize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's wrong with division by 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing wrong, for some reason I felt like 0.5 was easier to associate with 512KiB.

Also I think it's easier to change if we want something like 800KiB.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that it matters here, but integer arithmetic is much easier to reason about than float arithmetic, if we need to change this to something else we can introduce new constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@evaporei evaporei force-pushed the otavio/runtime-max-stack-size branch from 37e27e2 to fea87c0 Compare August 19, 2021 14:56
@evaporei evaporei force-pushed the otavio/runtime-max-stack-size branch from fea87c0 to 7319d7f Compare August 19, 2021 15:00
@evaporei evaporei merged commit 009b6f6 into master Aug 19, 2021
@evaporei evaporei deleted the otavio/runtime-max-stack-size branch August 19, 2021 17:10
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.

2 participants