Adapt backend usage depending on wasm file executed #1004

d0iasm · 2019-11-22T11:17:49Z

Adapt backend usage depending on wasm file executed in issue #998.
Close #998

Description

Add auto backend into a runtime-core and use it as a default backend.
The auto backend is equivalent to:

singlepass if singlepass is enabled and the wasm file size is larger than 10MiB, or singlepass is enable and the target architecture is aarch64.
cranelift otherwise.

Hywan

Despites minor comments, the PR looks good. Great job 👍!

Hywan · 2019-11-22T13:11:32Z

src/bin/wasmer.rs

@@ -492,6 +492,20 @@ fn execute_wasm(options: &Run) -> Result<(), String> {
        None
    };

+    // Update backend when a backend flag is `auto` or a default value.


or a default value

Is it still true? By reading the code, I assume it's when the --backend option is set to auto only?

I overlooked the Backend::default(). I updated the return value from Cranelift to Auto in Backend::default(), so this comment should be true.

Hywan · 2019-11-22T13:11:55Z

src/bin/wasmer.rs

@@ -492,6 +492,20 @@ fn execute_wasm(options: &Run) -> Result<(), String> {
        None
    };

+    // Update backend when a backend flag is `auto` or a default value.
+    // Use a singlepass if it's enable and the file provided is larger


- a singlepass + the Singlepass backend

- it's enable + it's enabled

Hywan · 2019-11-22T13:12:33Z

src/bin/wasmer.rs

@@ -492,6 +492,20 @@ fn execute_wasm(options: &Run) -> Result<(), String> {
        None
    };

+    // Update backend when a backend flag is `auto` or a default value.
+    // Use a singlepass if it's enable and the file provided is larger
+    // than 10MiB (10485760 bytes), or it's enable and the target architecture


- it's enable + it's enabled

Hywan · 2019-11-22T13:13:02Z

src/bin/wasmer.rs

+    // Update backend when a backend flag is `auto` or a default value.
+    // Use a singlepass if it's enable and the file provided is larger
+    // than 10MiB (10485760 bytes), or it's enable and the target architecture
+    // is aarch64. Otherwise, use a cranelift as a backend.


- use a cranelift as a backend + use the Cranelift backend

Hywan · 2019-11-22T13:24:29Z

src/bin/wasmer.rs

+    // is aarch64. Otherwise, use a cranelift as a backend.
+    if options.backend == Backend::Auto {
+        if Backend::variants().contains(&Backend::Singlepass.to_string())
+            && (wasm_binary.len() > 10485760 || cfg!(target_arch = "aarch64"))


Based on my comprehension of #998, when the --backend option is set to auto, we want to use Cranelift by default, or Singlepass if the module is larger than 10MiB; which translates as:

if Backend::variants().containts(&Backend::Singlepass.to_string()) // Singlepass is available && wasm_binary.len() > 10485760 // Large module, be a good candidate for Singlepass { options.backend = Backend::Singlepass; } else { options.backend = Backend::Cranelift; }

The issue #998 only mentions the file size, but I heard from @syrusakbary on Slack that the Singlepass backend is used when the target arch is AArch64. Is it better to add about it to the issue #998, or to remove the condition relating to AArch64 for now?

Hywan · 2019-11-22T13:25:45Z

@d0iasm Don't forget to update the CHANGELOG.md file too :-).

MarkMcCaskey

Looks good!

But I do have a style comment: execute_wasm is a very large function. I think it would help keep the code more readable and maintainable if option: Run was not mutable here. If it's mutable, that means that we need to be aware of where it's changing which can be hard to keep track of and increases the mental burden of understanding on an already too-complex function.

I suggest:

use a helper function which takes option: Run as mutable and is called from run or main, which are smaller functions and can be more easily audited. That is: localize the mutability to prevent it from making the other logic more complex.
use an extra variable backend in execute_wasm instead of mutating it (this may not work because we may need to read the field from Run in a sub-function)

d0iasm · 2019-11-25T05:15:16Z

@MarkMcCaskey
Thank you for your suggestions! I chose option 1 because option 2 would affect many places since options is used in other functions such as execute_wasi. I added a new function update_backend called from run. I also reverted options: &mut Run to options: &Run in execute_wasm argument.

MarkMcCaskey

Looks good to me!

Hywan

Look s good to me!

Hywan · 2019-11-25T20:24:44Z

bors r+

1004: Adapt backend usage depending on wasm file executed r=Hywan a=d0iasm Adapt backend usage depending on wasm file executed in issue #998. Close #998 # Description Add `auto` backend into a runtime-core and use it as a default backend. The `auto` backend is equivalent to: * singlepass if singlepass is enabled and the wasm file size is larger than 10MiB, or singlepass is enable and the target architecture is aarch64. * cranelift otherwise. Co-authored-by: Asami Doi <doiasami1219@gmail.com> Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>

bors · 2019-11-25T21:09:07Z

Build failed

wasmerio.wasmer

Hywan · 2019-11-25T21:10:23Z

bors r+

1004: Adapt backend usage depending on wasm file executed r=Hywan a=d0iasm Adapt backend usage depending on wasm file executed in issue #998. Close #998 # Description Add `auto` backend into a runtime-core and use it as a default backend. The `auto` backend is equivalent to: * singlepass if singlepass is enabled and the wasm file size is larger than 10MiB, or singlepass is enable and the target architecture is aarch64. * cranelift otherwise. Co-authored-by: Asami Doi <doiasami1219@gmail.com> Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>

bors · 2019-11-25T21:50:17Z

Build failed

wasmerio.wasmer

d0iasm · 2019-11-27T04:41:51Z

bors r+

1004: Adapt backend usage depending on wasm file executed r=d0iasm a=d0iasm Adapt backend usage depending on wasm file executed in issue #998. Close #998 # Description Add `auto` backend into a runtime-core and use it as a default backend. The `auto` backend is equivalent to: * singlepass if singlepass is enabled and the wasm file size is larger than 10MiB, or singlepass is enable and the target architecture is aarch64. * cranelift otherwise. Co-authored-by: Asami Doi <doiasami1219@gmail.com> Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>

bors · 2019-11-27T05:24:00Z

Build failed

wasmerio.wasmer

d0iasm · 2019-11-27T11:19:06Z

The test cache::tests::test_file_system_cache_run fails because the default compiler is auto but there is no "real" compiler of auto. So, I reverted Backend::default() to return Backend::Cranelift.

d0iasm · 2019-11-27T11:19:25Z

bors try

bors · 2019-11-27T12:02:07Z

try

Build succeeded

wasmerio.wasmer

MarkMcCaskey

Seems reasonable to me! We can add some #[cfg] logic in Backend::default later for aarch64, but this looks good to ship now!

MarkMcCaskey · 2019-11-27T19:07:38Z

bors r+

1004: Adapt backend usage depending on wasm file executed r=MarkMcCaskey a=d0iasm Adapt backend usage depending on wasm file executed in issue #998. Close #998 # Description Add `auto` backend into a runtime-core and use it as a default backend. The `auto` backend is equivalent to: * singlepass if singlepass is enabled and the wasm file size is larger than 10MiB, or singlepass is enable and the target architecture is aarch64. * cranelift otherwise. Co-authored-by: Asami Doi <doiasami1219@gmail.com>

bors · 2019-11-27T20:22:55Z

Build succeeded

wasmerio.wasmer

d0iasm requested review from bjfish, Hywan, MarkMcCaskey and syrusakbary as code owners November 22, 2019 11:17

Hywan added 🎉 enhancement New feature! 📦 lib-cli About wasmer-cli labels Nov 22, 2019

Hywan suggested changes Nov 22, 2019

View reviewed changes

Hywan self-assigned this Nov 22, 2019

MarkMcCaskey reviewed Nov 22, 2019

View reviewed changes

d0iasm force-pushed the feature/autodetect-backend branch from fadc573 to 5f2854c Compare November 25, 2019 02:23

MarkMcCaskey approved these changes Nov 25, 2019

View reviewed changes

Hywan approved these changes Nov 25, 2019

View reviewed changes

d0iasm unassigned Hywan Nov 27, 2019

d0iasm force-pushed the feature/autodetect-backend branch from 9d958d7 to 757017b Compare November 27, 2019 11:02

d0iasm requested review from losfair and nlewycky as code owners November 27, 2019 11:02

Add "auto" backend to change the backend depending on the wasm file size

fd0df99

d0iasm force-pushed the feature/autodetect-backend branch from 757017b to fd0df99 Compare November 27, 2019 11:04

bors bot added a commit that referenced this pull request Nov 27, 2019

Try #1004:

ac94bc3

MarkMcCaskey approved these changes Nov 27, 2019

View reviewed changes

bors bot merged commit fd0df99 into master Nov 27, 2019

bors bot deleted the feature/autodetect-backend branch November 27, 2019 20:22

syrusakbary mentioned this pull request Dec 10, 2019

Can't run Golang with large binary #994

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt backend usage depending on wasm file executed #1004

Adapt backend usage depending on wasm file executed #1004

d0iasm commented Nov 22, 2019

Hywan left a comment

Hywan Nov 22, 2019

d0iasm Nov 25, 2019

Hywan Nov 22, 2019

Hywan Nov 22, 2019

Hywan Nov 22, 2019

Hywan Nov 22, 2019

Hywan Nov 22, 2019

d0iasm Nov 25, 2019

Hywan commented Nov 22, 2019

MarkMcCaskey left a comment •

edited

Loading

d0iasm commented Nov 25, 2019

MarkMcCaskey left a comment

Hywan left a comment

Hywan commented Nov 25, 2019

bors bot commented Nov 25, 2019

Hywan commented Nov 25, 2019

bors bot commented Nov 25, 2019

d0iasm commented Nov 27, 2019

bors bot commented Nov 27, 2019

d0iasm commented Nov 27, 2019

d0iasm commented Nov 27, 2019

bors bot commented Nov 27, 2019

MarkMcCaskey left a comment

MarkMcCaskey commented Nov 27, 2019

bors bot commented Nov 27, 2019

Adapt backend usage depending on wasm file executed #1004

Adapt backend usage depending on wasm file executed #1004

Conversation

d0iasm commented Nov 22, 2019

Description

Hywan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan commented Nov 22, 2019

MarkMcCaskey left a comment • edited Loading

Choose a reason for hiding this comment

d0iasm commented Nov 25, 2019

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Hywan left a comment

Choose a reason for hiding this comment

Hywan commented Nov 25, 2019

bors bot commented Nov 25, 2019

Build failed

Hywan commented Nov 25, 2019

bors bot commented Nov 25, 2019

Build failed

d0iasm commented Nov 27, 2019

bors bot commented Nov 27, 2019

Build failed

d0iasm commented Nov 27, 2019

d0iasm commented Nov 27, 2019

bors bot commented Nov 27, 2019

try

Build succeeded

MarkMcCaskey left a comment

Choose a reason for hiding this comment

MarkMcCaskey commented Nov 27, 2019

bors bot commented Nov 27, 2019

Build succeeded

MarkMcCaskey left a comment •

edited

Loading