Add metered compile and check functions to c api #736

ethanfrey · 2019-08-30T11:11:44Z

Created metering.rs and exposed:

wasmer_compile_with_limit - hardcoded to singlepass backend now, as only stable one
wasmer_instance_get_points_used
wasmer_instance_set_points_used

I also added a metering feature flag. And the functions default to wasmer_compile and "noops" if the feature is disabled (but expose the same API).

I will work on testing an example in the go-ext-wasm repo, then submit this for review, but I am happy for any early feedback as well.

lib/runtime/src/cache.rs

Hywan · 2019-09-06T08:57:06Z

lib/runtime-c-api/src/lib.rs

@@ -96,6 +96,7 @@ pub mod global;
 pub mod import;
 pub mod instance;
 pub mod memory;
+pub mod metering;


You can add the attribute #[cfg(feature = "metering")] here, and remove all the same attributes in metering.rs. I believe it will simplify the code :-).

The problem is that it would expose a different C ABI then, and the go wrapper would fail to compile. Since go doesn't support conditional compilation feature tags as rust does, I think it would break the go bindings to have different ABIs (okay, there is some stuff for architecture, but nothing like rust's feature switches).

I can change that out if this is not a problem, but wanted the least breaking change possible (especially since the abi is committed to the go-ext-wasmer repo)

Golang build tags can be used to select the proper C ABI object to link against.

Hywan · 2019-09-06T08:59:28Z

lib/runtime-c-api/src/metering.rs

+
+/*** placeholder implementation if metering feature off ***/
+
+// Without metering, wasmer_compile_with_limit is a copy of wasmer_compile


I'm not sure we need fallbacks. If metering feature is on, then wasmer_compile_with_limit will exist, otherwise it will be absent.

See above point about the bindings - the ABI is copied into the other repo. I feel this option is more a way to merge for now without it being fully tested, then metering can be always on (remove conditions) when it is ready to release.

If this is fully supported in master of go-ext-wasmer, then it should be also in the default binary builds of the .so files

To elaborate, cgo will panic on link error (no build) if the linked dll doesn't have the referenced methods. This is why I need stubs (go doesn't have smart feature flags)

I don't think that wasmer_compile_with_limit should return without error if the feature is disabled. At least the function should inform the user that it can't enforce a limit.

Hywan · 2019-09-18T12:00:27Z

lib/runtime-c-api/Cargo.toml

 [features]
 default = ["cranelift-backend"]
 debug = ["wasmer-runtime/debug"]
 cranelift-backend = ["wasmer-runtime/cranelift", "wasmer-runtime/default-backend-cranelift"]
 llvm-backend = ["wasmer-runtime/llvm", "wasmer-runtime/default-backend-llvm"]
 singlepass-backend = ["wasmer-runtime/singlepass", "wasmer-runtime/default-backend-singlepass"]
+metering = ["wasmer-runtime/singlepass", "wasmer-middleware-common", "wasmer-singlepass-backend"]


Since singlepass-backend is already a feature, you can simplify the “feature dependency” by writing:

metering = ["singlepass-backend", "wasmer-middleware-common"]

I did that. It then sets wasmer-runtime/default-backend-singlepass.
and the default features set wasmer-runtime/default-backend-cranelift.

Setting two default backends caused a panic or compile issue. (I can look up the line if you want). I had to set this to avoid crashes. Ideal would be to disable the cranelift-backend completely for the metered build I guess. But I don't know how to disable default features.

Also note that singlepass-backend wouldn't even compile without an extra dependency added. Or maybe that is the problem. Hmmm...

lib/runtime-c-api/src/metering.rs

Hywan · 2019-09-18T12:10:25Z

The fact that only the Singlepass backend support gas metering annoys me. The go-ext-wasm language integration works with the Cranelift backend. To support the Singlepass backend, it means that the shared library we share with our CI must embed two backends. Or we should distribute 2 shared libraries: One with the Singlepass backend, and one with the Cranelift backend.

Thoughts @syrusakbary?

ethanfrey

Thanks for the feedback.

I think much would simplify if we could disable the default feature and just force default singlepass when metering was enabled.

This would provide consistency in each format, although two targetted builds:

a fast one (cranelift) with stubs for metering to be abi compatible, that don't meter.
a slow one (singlepass) with metering enabled.

I guess this leaves the go-ext-wasm on a branch for a while, but I am definitely happier to stick on a go branch that points to the runtime master than a branch off a branch that will be maintainability nightmare.

ethanfrey · 2019-09-18T12:55:57Z

Also, any idea how to get the C tests to run: eg. https://github.com/wasmerio/wasmer/pull/736/files#diff-105bd7892c64c7216319c0f4f58e0d18 ?

Hywan · 2019-09-18T13:05:01Z

You should update CMake, https://github.com/wasmerio/wasmer/blob/master/lib/runtime-c-api/tests/CMakeLists.txt.

ethanfrey · 2019-09-18T20:58:37Z

789f2ce fixes the features so metering will disable cranelift and enable singlepass as default compiler for a smoother experience. This requires running tests with the --no-default--features flag, like: cargo test --features metering --no-default-features

This fails on a serialization issue, but notably the same issue exists without metering: cargo test --features singlepass-backend --no-default-features. I will check master, but I think singlepass-backend is not properly handled in the c api (although worked fine in pure rust I believe). Guess non-cranelift path is relatively untested here.

ethanfrey · 2019-09-20T10:35:43Z

I addressed all issues and added a section to the README on feature flags.
The c test for testing metering and serialization of metered code is also enabled.

Note that this currently only passes all c tests for:

cargo test --no-default-features --features llvm-backend,metering

test-module-metering-serialize requires metering to be enabled (and working) and serialization to be working. serialization is known to be broken on singlepass backend (see #811) and gas metering / middleware is known to be broken on the cranelift backend (see #819)

AdamSLevy · 2019-11-20T01:27:42Z

I rebased this PR on the latest master: https://github.com/AdamSLevy/wasmer/tree/metering

details here:
https://github.com/confio/wasmer/pull/1

Add wasmer_compile_with_limit api Try adding metering functions - need help to compile Metering api compiles with feature flag on and off Fixed linter errors in metering.rs Added some TODOs to check serialization failure Fix u32 -> u64 in gas limit Try to make C test case for using wasmer_compile_with_limit Run cargo +stable fmt --all Clean up feature setup with --no-default-features, singlepass-backend fails get_metered_compiler uses whatever default backend Handle null input in metering api wasmer_compile_with_limit -> wasmer_compile_with_gas_metering Update deps to 0.7.0 after rebase Use llvm backend by default for metering - all tests pass Add basic test-module-metering-serialize test Only passes with --features metering Expose CraneliftModuleCodeGenerator to test with --metering, document issues Fix linter issue Do not export undocumented Cranelift MCG

Hywan · 2020-12-18T13:48:45Z

Hello,

Now that Wasmer 1.0.0-beta2 is released, which contains a new middleware API, do you still need the features from the PR? If yes, can you rebase it, or do you want us to do that?

Note that we now align our C API onto the official standard Wasm C API. Middleware and metering are not part of this standard. That's why we have a vendor-specific API and I think such features will fit there.

ethanfrey · 2020-12-18T17:38:30Z

Oh, we can close this. I stopped using the c api a year ago to write my logic in rust and use custom high level ffi bindings to go.

I like the new Middleware design btw

This was referenced Aug 30, 2019

Run cargo fmt --all to make the ci linter happy #737

Closed

[WIP] Expose gas metering in the API wasmerio/wasmer-go#68

Open

Hywan added 🎉 enhancement New feature! 📦 lib-c-api About wasmer-c-api labels Sep 6, 2019

Hywan reviewed Sep 6, 2019

View reviewed changes

Hywan self-assigned this Sep 6, 2019

Hywan suggested changes Sep 18, 2019

View reviewed changes

ethanfrey commented Sep 18, 2019

View reviewed changes

ethanfrey mentioned this pull request Sep 20, 2019

Add support for gas metering to cranelift backend #819

Closed

ethanfrey marked this pull request as ready for review September 20, 2019 10:32

ethanfrey requested review from bjfish and nlewycky as code owners September 20, 2019 10:32

ethanfrey mentioned this pull request Oct 7, 2019

new feature flag: deterministic #709

Merged

ethanfrey mentioned this pull request Dec 10, 2019

Expose compile_with to C API #605

Closed

ethanfrey closed this Dec 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metered compile and check functions to c api #736

Add metered compile and check functions to c api #736

ethanfrey commented Aug 30, 2019

Hywan Sep 6, 2019

ethanfrey Sep 9, 2019

AdamSLevy Nov 20, 2019

Hywan Sep 6, 2019

ethanfrey Sep 9, 2019

ethanfrey Sep 18, 2019

AdamSLevy Nov 20, 2019

Hywan Sep 18, 2019

ethanfrey Sep 18, 2019

ethanfrey Sep 18, 2019

Hywan commented Sep 18, 2019

ethanfrey left a comment

ethanfrey commented Sep 18, 2019

Hywan commented Sep 18, 2019

ethanfrey commented Sep 18, 2019

ethanfrey commented Sep 20, 2019

AdamSLevy commented Nov 20, 2019

Hywan commented Dec 18, 2020

ethanfrey commented Dec 18, 2020


		/* placeholder implementation if metering feature off */

		// Without metering, wasmer_compile_with_limit is a copy of wasmer_compile

Add metered compile and check functions to c api #736

Add metered compile and check functions to c api #736

Conversation

ethanfrey commented Aug 30, 2019

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

Choose a reason for hiding this comment

Hywan commented Sep 18, 2019

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey commented Sep 18, 2019

Hywan commented Sep 18, 2019

ethanfrey commented Sep 18, 2019

ethanfrey commented Sep 20, 2019

AdamSLevy commented Nov 20, 2019

Hywan commented Dec 18, 2020

ethanfrey commented Dec 18, 2020