Duslia opened PR #4813 from feat/knob_for_reset_stack
to main
:
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
This PR has been discussed in #4637. I add a knob for reset stack.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
This can't be set by the user.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
}
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia created PR review comment:
What do you think would be a better way?
Duslia submitted PR review.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Please don't commit this file.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Adding a setter like https://github.com/bytecodealliance/wasmtime/blob/d1fb2405d769b3eb81873d2a77b6b75e8d931936/crates/wasmtime/src/config.rs#L598-L603
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
This needs to set
reset_stack_page
, notasync_stack_size
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I would suggest this documentation perhaps for this method:
Configures whether or not stacks used for async futures are reset to zero after usage.
When the
async_support
method is enabled for Wasmtime and thecall_async
variant of calling WebAssembly is used then Wasmtime will create a separate runtime execution stack for each future produced bycall_async
. When using the pooling instance allocator this allocation will happen from a pool of stacks and additionally deallocation will simply release the stack back to the pool. During the deallocation process Wasmtime will by default reset the contents of the stack back to zero.This reset back to zero, however, is a defense-in-depth mechanism and is not required for correctness. The operation to reset a stack back to zero can be a costly operation in highly concurrent environments. By setting this option to
false
this zero-ing operation will not occur which may prove beneficial to throughput in these environments.Additionally I might recommend renaming the method/option
async_stack_zeroing
or something like that to make sure the word "async" is somewhere in there along with "zero"
alexcrichton created PR review comment:
To avoid some extraneous
#[cfg]
below I think it's ok to remove this#[cfg]
while still leaving the#[cfg]
on the setter method
alexcrichton created PR review comment:
Here and in some other files there are a lot of changes to the imports here, but I don't think that anything substantively changed? Could the changes you're adding here be backed out to avoid churn in this commit?
alexcrichton submitted PR review.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
Duslia edited PR #4813 from feat/knob_for_reset_stack
to main
:
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
This PR has been discussed in #4637. I add a knob for reset stack.
Duslia updated PR #4813 from feat/knob_for_reset_stack
to main
.
alexcrichton updated PR #4813 from feat/knob_for_reset_stack
to main
.
alexcrichton has enabled auto merge for PR #4813.
alexcrichton submitted PR review.
alexcrichton merged PR #4813.
Last updated: Dec 23 2024 at 12:05 UTC