TimonPost opened PR #6547 from TimonPost:timon/allow-setting-unwind_info-to-false
to bytecodealliance:main
:
Regarding issue #6541, it was reported that unloading a large module on macOS can result in significantly long loading times. One suggestion was to disable unwind frames, but this didn't seem to have any effect. Upon further investigation, it appears that this issue may be a regression introduced in a commit made last year in August. The specific commit can be found here: link to commit.
Currently, there is only support for enabling the compiler flag, and there is no way to disable it. This means that the config flag is enabled by default, and there is no option to turn it off. The suggested pull request addresses this issue by checking if native_unwind_info is disabled and inserting the flag into the compiler config if necessary.
I would like to have it confirmed if this means that all crane lift users have been forced to compile with unwind frames, always, or perhaps I am missing something.
Also second question, disabling this flag, it would only affect GC tracing and debugging right? So should be fine to expose this to users if they use neither of those?
TimonPost requested pchickey for a review on PR #6547.
TimonPost requested wasmtime-core-reviewers for a review on PR #6547.
TimonPost updated PR #6547.
TimonPost edited PR #6547:
Regarding issue #6541, it was reported that unloading a large module on macOS can result in significantly long loading times. One suggestion was to disable unwind frames, but this didn't seem to have any effect. Upon further investigation, it appears that this issue may be a regression introduced in a commit made last year in August. The specific commit can be found here: link to commit.
Currently, there is only support for enabling the compiler flag, and there is no way to disable it. This means that the config flag is enabled by default, and there is no option to turn it off. The suggested pull request addresses this issue by checking if native_unwind_info is disabled and inserting the flag into the compiler config if necessary.
I would like to have it confirmed if this means that all crane lift users have been forced to compile with unwind frames, always, or perhaps I am missing something.
Also second question, disabling this flag, it would only affect GC tracing and debugging right? So should be fine to expose this to users if they use neither of those?
As temporary work around i can do:
unsafe { config.cranelift_flag_set("unwind_info", "false"); }
TimonPost edited PR #6547:
Regarding issue #6541, it was reported that unloading a large module on macOS can result in significantly long loading times. One suggestion was to disable unwind frames, but this didn't seem to have any effect. Upon further investigation, it appears that this issue may be a regression introduced in a commit made last year in August. The specific commit can be found here: link to commit.
Currently, there is only support for enabling the compiler flag, and there is no way to disable it. This means that the config flag is enabled by default, and there is no option to turn it off. The suggested pull request addresses this issue by checking if native_unwind_info is disabled and inserting the flag into the compiler config if necessary.
I would like to have it confirmed if this means that all crane lift users have been forced to compile with unwind frames, always, or perhaps I am missing something.
Also second question, disabling this flag, it would only affect GC tracing and debugging right? So should be fine to expose this to users if they use neither of those?
As temporary work around i can do:
unsafe { config.native_unwind_info(false); config.cranelift_flag_set("unwind_info", "false"); }
pchickey requested fitzgen for a review on PR #6547.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yeah, I think it would be slightly nicer to do something like
if self.profiling_strategy != ProfilingStrategy::None || target == target_lexicon::OperatingSystem::Windows { self.cranelift_flag_set("unwind_info", "true"); } if !self.compiler_config.ensure_setting_unset_or_given( "unwind_info", self.native_unwind_info.unwrap_or(DEFAULT_UNWIND_INFO), ) { bail!("`native_unwind_info` cannot be disabled when profiling or on Windows"); }
TimonPost created PR review comment:
- If profiling enabled, always include unwind info, and ignore
self.native_unwind_info
, even if set by user. We likely want to add this to the functoin comments?- I assume
DEFAULT_UNWIND_INFO
to true?
TimonPost edited PR review comment.
TimonPost updated PR #6547.
TimonPost updated PR #6547.
TimonPost updated PR #6547.
TimonPost edited PR review comment.
TimonPost edited PR review comment.
TimonPost has marked PR #6547 as ready for review.
TimonPost requested fitzgen for a review on PR #6547.
TimonPost edited PR #6547:
Regarding issue #6541, it was reported that unloading a large module on macOS can result in significantly long loading times. One suggestion was to disable unwind frames, but this didn't seem to have any effect. Upon further investigation, it appears that this issue may be a regression introduced in a commit made last year in August. The specific commit can be found here: link to commit.
Currently, the config flag is enabled by default, and there is no option to turn it off via the
native_unwind_info
function. This pull request addresses the issue.I would like to have it confirmed if this means that all crane lift users have been forced to compile with unwind frames, always, or perhaps I am missing something.
Also, second question, disabling this flag, it would only affect GC tracing and debugging right? So should be fine to expose this to users if they use neither of those?
As temporary workaround i can do:
unsafe { config.native_unwind_info(false); config.cranelift_flag_set("unwind_info", "false"); }
fitzgen submitted PR review:
LGTM, modulo the new revelations around profiling. Thanks!
jameysharp submitted PR review:
Okay, I think we have consensus on the conditions where this setting is needed, and now just need to agree on implementation.
jameysharp submitted PR review:
Okay, I think we have consensus on the conditions where this setting is needed, and now just need to agree on implementation.
jameysharp created PR review comment:
I don't think this
.clone()
is necessary after the various changes this PR has gone through, though I could be wrong.
jameysharp submitted PR review:
I failed to submit one of my review comments, whoops?
jameysharp submitted PR review:
I failed to submit one of my review comments, whoops?
jameysharp created PR review comment:
Given the new information that profiling doesn't affect this setting at all, I think the structure should be more like:
if let Some(unwind_requested) = self.native_unwind_info { if !self.compiler_config.ensure_setting_unset_or_given("unwind_info", unwind_requested.to_string()) { bail!("incompatible settings requested for Cranelift and Wasmtime unwind-info settings"); } } if target.operating_system == target_lexicon::OperatingSystem::Windows { if !self.compiler_config.ensure_setting_unset_or_given("unwind_info", "true") { bail!("`native_unwind_info` cannot be disabled on Windows"); } }
TimonPost created PR review comment:
Unfortunately seems like self.compiler_config.ensure_setting_unset_or_given still needs a mutable borrow while tripple would be borrowed immutability.
TimonPost updated PR #6547.
TimonPost created PR review comment:
Updated!
TimonPost requested fitzgen for a review on PR #6547.
TimonPost requested jameysharp for a review on PR #6547.
jameysharp created PR review comment:
Oops, you're right. We could fix that by moving the
ensure_setting_unset_or_given
method tocompiler_config.settings
, but cloning the triple isn't so bad either, so this is fine as-is.
jameysharp submitted PR review:
Great, I think this is all set. Thank you!
TimonPost updated PR #6547.
TimonPost updated PR #6547.
TimonPost updated PR #6547.
jameysharp submitted PR review:
This PR still looks good to me, thanks for fixing the Windows fuzzing issue!
The CI failure is probably because you accidentally changed
crates/wasi-nn/spec
. I suspect you need to rungit submodule update
to undo that, and then hopefully everything will be fine.
TimonPost updated PR #6547.
TimonPost updated PR #6547.
TimonPost updated PR #6547.
jameysharp has enabled auto merge for PR #6547.
jameysharp merged PR #6547.
Last updated: Dec 23 2024 at 13:07 UTC