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.

Sign up for GitHub

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

Jump to bottom

wasmtime 17 hangs while trying to load wasm file #7874

Closed
glebpom opened this issue Feb 5, 2024 · 5 comments · Fixed by #7882
Closed

wasmtime 17 hangs while trying to load wasm file #7874

glebpom opened this issue Feb 5, 2024 · 5 comments · Fixed by #7882
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift:E-compiler Compiler issues.

Comments

@glebpom
Copy link

glebpom commented Feb 5, 2024

Test Case

https://drive.google.com/file/d/1HYlz78KTuag9bTpNwGP_blp5yWo95i_7/view?usp=sharing

This wasm file successfully loads by version 16, but wasmtime 17 hangs and eats a lot of CPU while trying to load it

Steps to Reproduce

This is reproducible in python and ruby. Example ruby code:

      @engine = Wasmtime::Engine.new
      @mod = Wasmtime::Module.new(@engine, wasm)

Expected Results

Should work

Actual Results

It hangs eating ~170% of CPU

Versions and Environment

Wasmtime version or commit: 17

Operating system: MacOS

Architecture: aarch64
@glebpom glebpom added the bug Incorrect behavior in the current implementation that needs fixing label Feb 5, 2024
@pchickey
Copy link
Contributor

pchickey commented Feb 5, 2024

It appears this is some sort of codegen bug - I was able to reproduce this on main with cargo run --release -- compile v0.2.0.wasm, wasmtime consumes 2 cores for as long as I had the patience for (~4 mins)

@pchickey pchickey added the cranelift:E-compiler Compiler issues. label Feb 5, 2024
@glebpom
Copy link
Author

glebpom commented Feb 6, 2024 •
edited
Loading

I was able to bisect and find the breaking commit, which is:

fac3b9c2d1373c8bdf10b53f2f6e96c01c90afd8 is the first bad commit
commit fac3b9c2d1373c8bdf10b53f2f6e96c01c90afd8
Author: Alex Crichton <alex@alexcrichton.com>
Date:   Thu Jan 25 09:50:19 2024 -0800

    Enable the component model by default

    This commit enables the component model by default in the embedding API
    and the CLI. This means that an opt-in of `-W component-model` is no
    longer required and additionally `.wasm_component_model(true)` is no
    longer required. Note that this won't impact existing embeddings since
    the component model feature doesn't do much less `wasmtime::component`
    is used, and if that's being used this is probably good news for you.

 crates/wasmtime/src/config.rs |  1 +
 src/common.rs                 |  2 +-
 tests/all/cli_tests.rs        | 23 +++++++++++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

EDIT: I was bisecting this PR https://github.com/bytecodealliance/wasmtime/pull/7800/commits

@alexcrichton
Copy link
Member

@glebpom can you clarify how you bisected? My bisection points to #7719 and looking at a performance profile most of the time is spent in constructor_will_simplify_with_ireduce which helps point to that PR as well.

@alexcrichton alexcrichton mentioned this issue Feb 6, 2024
Merged
@glebpom
Copy link
Author

glebpom commented Feb 6, 2024

@alexcrichton yes, you are right. I can't reproduce this anymore, but the commit you pointed to clearly makes it hang.

elliottt added a commit to elliottt/wasmtime that referenced this issue Feb 7, 2024
@elliottt @fitzgen
Guard recursion in will_simplify_with_ireduce
c293ba1 
Add a test to expose issues with unbounded recursion through `iadd`
during egraph rewrites, and bound the recursion of
`will_simplify_with_ireduce`.

Fixes bytecodealliance#7874

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
@elliottt elliottt mentioned this issue Feb 7, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Feb 7, 2024
@elliottt @fitzgen
Guard recursion in will_simplify_with_ireduce (#7882)
74a303a 
Add a test to expose issues with unbounded recursion through `iadd`
during egraph rewrites, and bound the recursion of
`will_simplify_with_ireduce`.

Fixes #7874

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
elliottt added a commit to elliottt/wasmtime that referenced this issue Feb 7, 2024
@elliottt @fitzgen
Guard recursion in will_simplify_with_ireduce (bytecodealliance#7882)
45c8a5c 
Add a test to expose issues with unbounded recursion through `iadd`
during egraph rewrites, and bound the recursion of
`will_simplify_with_ireduce`.

Fixes bytecodealliance#7874

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
@elliottt elliottt mentioned this issue Feb 7, 2024
Open
elliottt added a commit that referenced this issue Feb 7, 2024
@elliottt @fitzgen
Guard recursion in will_simplify_with_ireduce (#7882) (#7885)
f3e15d5 
Add a test to expose issues with unbounded recursion through `iadd`
during egraph rewrites, and bound the recursion of
`will_simplify_with_ireduce`.

Fixes #7874

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
elliottt added a commit to elliottt/wasmtime that referenced this issue Feb 7, 2024
@elliottt @fitzgen
Guard recursion in will_simplify_with_ireduce (bytecodealliance#7882)
93c7563 
Add a test to expose issues with unbounded recursion through `iadd`
during egraph rewrites, and bound the recursion of
`will_simplify_with_ireduce`.

Fixes bytecodealliance#7874

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
elliottt added a commit that referenced this issue Feb 7, 2024
@elliottt @fitzgen
Backport egraph fixes to 17.0.1 (#7889)
fff54cb 
* Guard recursion in `will_simplify_with_ireduce` (#7882)

Add a test to expose issues with unbounded recursion through `iadd`
during egraph rewrites, and bound the recursion of
`will_simplify_with_ireduce`.

Fixes #7874

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

* Cranelift: Use a fixpoint loop to compute the best value for each eclass (#7859)

* Cranelift: Use a fixpoint loop to compute the best value for each eclass

Fixes #7857

* Remove fixpoint loop early-continue optimization

* Add document describing optimization rule invariants

* Make select optimizations use subsume

* Remove invalid debug assert

* Remove now-unused methods

* Add commutative adds to cost tests

* Add missing subsume uses in egraph rules (#7879)

* Fix a few egraph rules that needed `subsume`

There were a few rules that dropped value references from the LHS
without using subsume. I think they were probably benign as they
produced constant results, but this change is in the spirit of our
revised guidelines for egraph rules.

* Augment egraph rule guideline 2 to talk about constants

* Update release notes

---------

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
@elliottt
Copy link
Member

elliottt commented Feb 7, 2024

This should be fixed in the wasmtime 17.0.1 release, thank you again for reporting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift:E-compiler Compiler issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Guard recursion in will_simplify_with_ireduce elliottt/wasmtime
4 participants
@elliottt @glebpom @alexcrichton @pchickey