wasmtime / PR #6547 Set compiler flag 'unwind_info' to fa... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6547 Set compiler flag 'unwind_info' to fa...

Wasmtime GitHub notifications bot (Jun 09 2023 at 10:45):

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?

Wasmtime GitHub notifications bot (Jun 09 2023 at 10:45):

TimonPost requested pchickey for a review on PR #6547.

Wasmtime GitHub notifications bot (Jun 09 2023 at 10:45):

TimonPost requested wasmtime-core-reviewers for a review on PR #6547.

Wasmtime GitHub notifications bot (Jun 09 2023 at 10:48):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Jun 09 2023 at 11:31):

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");
        }

Wasmtime GitHub notifications bot (Jun 09 2023 at 11:33):

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");
}

Wasmtime GitHub notifications bot (Jun 09 2023 at 15:03):

pchickey requested fitzgen for a review on PR #6547.

Wasmtime GitHub notifications bot (Jun 09 2023 at 18:59):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 09 2023 at 18:59):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 09 2023 at 18:59):

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");
}

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:15):

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?

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:16):

TimonPost edited PR review comment.

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:18):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:25):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:26):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:27):

TimonPost edited PR review comment.

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:27):

TimonPost edited PR review comment.

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:31):

TimonPost has marked PR #6547 as ready for review.

Wasmtime GitHub notifications bot (Jun 09 2023 at 20:31):

TimonPost requested fitzgen for a review on PR #6547.

Wasmtime GitHub notifications bot (Jun 12 2023 at 18:15):

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");
}

Wasmtime GitHub notifications bot (Jun 13 2023 at 18:14):

fitzgen submitted PR review:

LGTM, modulo the new revelations around profiling. Thanks!

Wasmtime GitHub notifications bot (Jun 13 2023 at 19:49):

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.

Wasmtime GitHub notifications bot (Jun 13 2023 at 19:49):

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.

Wasmtime GitHub notifications bot (Jun 13 2023 at 19:49):

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.

Wasmtime GitHub notifications bot (Jun 13 2023 at 19:53):

jameysharp submitted PR review:

I failed to submit one of my review comments, whoops?

Wasmtime GitHub notifications bot (Jun 13 2023 at 19:53):

jameysharp submitted PR review:

I failed to submit one of my review comments, whoops?

Wasmtime GitHub notifications bot (Jun 13 2023 at 19:53):

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");
    }
}

Wasmtime GitHub notifications bot (Jun 14 2023 at 06:46):

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.

Wasmtime GitHub notifications bot (Jun 14 2023 at 06:47):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Jun 14 2023 at 06:48):

TimonPost created PR review comment:

Updated!

Wasmtime GitHub notifications bot (Jun 14 2023 at 06:48):

TimonPost requested fitzgen for a review on PR #6547.

Wasmtime GitHub notifications bot (Jun 14 2023 at 06:48):

TimonPost requested jameysharp for a review on PR #6547.

Wasmtime GitHub notifications bot (Jun 14 2023 at 15:22):

jameysharp created PR review comment:

Oops, you're right. We could fix that by moving the ensure_setting_unset_or_given method to compiler_config.settings, but cloning the triple isn't so bad either, so this is fine as-is.

Wasmtime GitHub notifications bot (Jun 14 2023 at 15:26):

jameysharp submitted PR review:

Great, I think this is all set. Thank you!

Wasmtime GitHub notifications bot (Aug 24 2023 at 09:10):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Aug 24 2023 at 09:15):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Aug 24 2023 at 09:16):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Aug 24 2023 at 19:58):

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 run git submodule update to undo that, and then hopefully everything will be fine.

Wasmtime GitHub notifications bot (Aug 24 2023 at 20:49):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Aug 24 2023 at 20:54):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Aug 24 2023 at 20:55):

TimonPost updated PR #6547.

Wasmtime GitHub notifications bot (Aug 24 2023 at 21:12):

jameysharp has enabled auto merge for PR #6547.

Wasmtime GitHub notifications bot (Aug 24 2023 at 22:19):

jameysharp merged PR #6547.

Last updated: Nov 22 2024 at 16:03 UTC