Stream: git-wasmtime

Topic: wasmtime / PR #4813 feat: add a knob for reset stack

Wasmtime GitHub notifications bot (Aug 30 2022 at 03:28):

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.

Wasmtime GitHub notifications bot (Aug 30 2022 at 03:38):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:34):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:39):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:39):

bjorn3 created PR review comment:

This can't be set by the user.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:40):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:40):

bjorn3 created PR review comment:

        }

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:42):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:45):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:47):

Duslia created PR review comment:

What do you think would be a better way?

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:47):

Duslia submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:51):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:51):

bjorn3 created PR review comment:

Please don't commit this file.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:52):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:52):

bjorn3 created PR review comment:

Adding a setter like https://github.com/bytecodealliance/wasmtime/blob/d1fb2405d769b3eb81873d2a77b6b75e8d931936/crates/wasmtime/src/config.rs#L598-L603

Wasmtime GitHub notifications bot (Aug 30 2022 at 07:56):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 09:18):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 09:20):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2022 at 09:20):

bjorn3 created PR review comment:

This needs to set reset_stack_page, not async_stack_size.

Wasmtime GitHub notifications bot (Aug 30 2022 at 09:47):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 09:51):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 09:55):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 10:27):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 14:45):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2022 at 14:45):

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 the call_async variant of calling WebAssembly is used then Wasmtime will create a separate runtime execution stack for each future produced by call_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"

Wasmtime GitHub notifications bot (Aug 30 2022 at 14:45):

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

Wasmtime GitHub notifications bot (Aug 30 2022 at 14:45):

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?

Wasmtime GitHub notifications bot (Aug 30 2022 at 14:45):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2022 at 10:09):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 31 2022 at 10:13):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 31 2022 at 10:15):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 31 2022 at 10:36):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Aug 31 2022 at 10:36):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Sep 01 2022 at 09:29):

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.

Wasmtime GitHub notifications bot (Sep 01 2022 at 09:45):

Duslia updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Sep 01 2022 at 15:26):

alexcrichton updated PR #4813 from feat/knob_for_reset_stack to main.

Wasmtime GitHub notifications bot (Sep 01 2022 at 15:26):

alexcrichton has enabled auto merge for PR #4813.

Wasmtime GitHub notifications bot (Sep 01 2022 at 15:27):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2022 at 16:09):

alexcrichton merged PR #4813.

Last updated: Nov 22 2024 at 17:03 UTC