feat(c-api) Introduce the middleware (specifically metering) C API #2103

nlewycky · 2021-02-09T09:21:05Z

Edit by @Hywan: Fixes #2025.

syrusakbary

I think the wasmer_metering_points_value, wasmer_metering_points_is_exhausted and wasmer_metering_get_remaining_points can be simplified.

Right know, to get the points you have to do: wasmer_metering_get_remaining_points, ask if wasmer_metering_points_is_exhausted and then wasmer_metering_get_remaining_points.
This ties very close to the inner Rust structures but made the usage harder.

I think one simpler thing that we can do is to have wasmer_metering_get_remaining_points return the remaining points integer directly if it's not exhausted and -1 in case is exhausted.

Tests

Apart from that, we will need to test the metering API. Probably you can use the inline-c crate macros for testing as we do it with other C-APi tests.

nlewycky · 2021-02-09T19:07:51Z

I think the wasmer_metering_points_value, wasmer_metering_points_is_exhausted and wasmer_metering_get_remaining_points can be simplified.

Right know, to get the points you have to do: wasmer_metering_get_remaining_points, ask if wasmer_metering_points_is_exhausted and then wasmer_metering_get_remaining_points.
This ties very close to the inner Rust structures but made the usage harder.

I think one simpler thing that we can do is to have wasmer_metering_get_remaining_points return the remaining points integer directly if it's not exhausted and -1 in case is exhausted.

Yep, that's a good change! The only adjustment to that is that instead of returning -1 when exhausted, it should return a value that the user passes in, which is what wasmer_metering_points_value currently does.

syrusakbary · 2021-02-09T19:11:32Z

The only adjustment to that is that instead of returning -1 when exhausted, it should return a value that the user passes in, which is what wasmer_metering_points_value currently does.

I think that still makes the API a bit more complicated, and I'm failing to see the use case for a custom number that is not the "special case".
Exhausted points can only be one thing (is a state value) and with a -1 value it doesn't collide with the remaining points (since they are always > 0?).
So I still think not having to provide an artificial exhausted value is simpler than having to provide one.

What is the use case that you think we will cover with having the user to provide the exhausted value?

…nts_unwrap_or`.

…` constant.

lib/c-api/src/wasm_c_api/engine.rs

…_t`. Also, move it into the `unstable::middleware` module.

jubianchi

Seems great! 👍

Last comment I would add is it would be awesome to have a cost function example.

lib/c-api/src/wasm_c_api/unstable/middlewares/metering.rs

Hywan · 2021-03-03T10:25:42Z

User-defined cost function isn't working yet :-). Working on it!

…Copy`. The `F` parameter for `Metering`, and therefore for `FunctionMetering`, no longer requires to implement the `Clone` and `Copy` traits.

Hywan · 2021-03-04T22:17:59Z

bors try

bors · 2021-03-04T22:35:28Z

try

Build failed:

Test on windows-x64

Hywan · 2021-03-04T22:36:46Z

bors try

bors · 2021-03-04T22:56:41Z

try

Build failed:

Test on windows-x64

lib/c-api/src/wasm_c_api/unstable/middlewares/metering.rs

Hywan · 2021-03-04T23:25:32Z

bors try

…_points_are_exhausted`.

nlewycky · 2021-03-04T23:41:07Z

lib/c-api/src/wasm_c_api/unstable/middlewares/metering.rs

+//! // Define our “cost function”.
+//! uint64_t cost_function(wasmer_parser_operator_t operator) {
+//!     switch(operator) {
+//!         // `local.get` and `i32.const` cost 1 unit.
+//!         case LocalGet:
+//!         case I32Const:
+//!             return 1;
+//!
+//!         // `i32.add` costs 2 units.
+//!         case I32Add:
+//!             return 2;
+//!
+//!         // The other operations are free.
+//!         default:
+//!             return 0;
+//!     }
+//! }


Optional: It might be worth using a more realistic cost function for the example?
a) the four Const operations are 0
b) F32Div and F64Div are cost 2
c) everything else is cost 1
?

I've taken the example from examples/metering.rs. It fits well with the simple WAT module. Thoughts?

lib/c-api/src/wasm_c_api/unstable/middlewares/metering.rs

lib/c-api/src/wasm_c_api/unstable/middlewares/mod.rs

nlewycky · 2021-03-04T23:49:55Z

lib/c-api/src/wasm_c_api/unstable/parser/operator.rs

@@ -0,0 +1,1017 @@
+use wasmer::wasmparser::Operator;


Clearly this file should be produced by a proc-macro that runs over wasmparser.

No action required. Please.

nlewycky · 2021-03-04T23:50:45Z

lib/engine-native/src/artifact.rs

@@ -317,7 +317,7 @@ impl NativeArtifact {
        if is_cross_compiling {
            Self::from_parts_crosscompiled(metadata, shared_filepath)
        } else {
-            let lib = Library::new(&shared_filepath).map_err(to_compile_error)?;
+            let lib = unsafe { Library::new(&shared_filepath).map_err(to_compile_error)? };


Technically this is part of a different PR?

Yeah but I want to see green tests in this PR for the moment.

bors · 2021-03-04T23:51:10Z

try

Build failed:

Test on windows-x64

…n C++).

Hywan · 2021-03-04T23:57:58Z

bors try

lib/c-api/build.rs

bors · 2021-03-05T00:35:34Z

try

Build succeeded:

Hywan

bors r+

bors · 2021-03-05T09:53:07Z

Build succeeded:

MarkMcCaskey

looks good!

MarkMcCaskey · 2021-03-05T15:05:28Z

lib/c-api/src/wasm_c_api/unstable/middlewares/metering.rs

+/// See module's documentation.
+#[allow(non_camel_case_types)]
+pub type wasmer_metering_cost_function_t =
+    extern "C" fn(wasm_operator: wasmer_parser_operator_t) -> u64;


Technically should probably be an unsafe function, but it's a minor point here because we immediately wrap it anyways. Only really matters if it has greater contact with Rust code.

MarkMcCaskey · 2021-03-05T15:08:15Z

lib/c-api/src/wasm_c_api/unstable/middlewares/mod.rs

+///
+/// See the documentation of the [`metering`] module.
+#[no_mangle]
+pub extern "C" fn wasm_config_push_middleware(


I'd assume middleware happens at a higher level than wasm_config, as wasm_config is used for setting up the initial engine / store. Not a huge deal, but it means that it's harder to share a store with modules using different middlewares. (I'm just assuming we allow a more flexible usage in the Rust API)

Initial commit of C API for metering and middleware.

197d0f7

syrusakbary reviewed Feb 9, 2021

View reviewed changes

Hywan added 10 commits March 1, 2021 15:13

feat(c-api) Simplify &Box<T> to &T.

d660943

fix(c-api) Move the middleware API into the unstable module.

2bc502b

chore(c-api) Update headers.

fd5a9b5

temp

cbbdfda

Merge branch 'master' into feature/metering-c-api

0f43391

fix(c-api) Fix merge conflict.

db4286d

feat(c-api) Add _delete and rename _value to `wasmer_metering_poi…

8b30ae9

…nts_unwrap_or`.

test(c-api) Add tests for the wasmer_metering_* API.

cb02694

feat(makefile) Test the C API with the middlewares feature.

3b0572f

feat(c-api) Update headers + add the `MIDDLEWARES_FEATURE_AS_C_DEFINE…

e67fd6a

…` constant.

Hywan changed the title ~~Initial commit of C API for metering and middleware.~~ feat(c-api) Introduce the middleware (specifically metering) API Mar 1, 2021

Hywan changed the title ~~feat(c-api) Introduce the middleware (specifically metering) API~~ feat(c-api) Introduce the middleware (specifically metering) C API Mar 1, 2021

Hywan reviewed Mar 1, 2021

View reviewed changes

lib/c-api/src/wasm_c_api/engine.rs Outdated Show resolved Hide resolved

Hywan added 8 commits March 1, 2021 17:31

feat(c-api) Add wasmer_metering_delete.

8f1a4f3

fix(c-api) wasmer_module_middleware_t is an opaque type.

fb973b2

feat(c-api) Rename wasmer_module_middleware_t to `wasmer_middleware…

31669b8

…_t`. Also, move it into the `unstable::middleware` module.

feat(c-api) Simplify a compilation condition.

798dd88

doc(middlewares) Format.

e014807

doc(c-api) Write more documentation and tests.

19c0412

chore(c-api) Update headers.

0c4e063

!tmp

902f58e

jubianchi reviewed Mar 2, 2021

View reviewed changes

Hywan added 3 commits March 4, 2021 17:05

feat(middleware) Relax F in Metering<F> by removing Clone and `…

487c964

…Copy`. The `F` parameter for `Metering`, and therefore for `FunctionMetering`, no longer requires to implement the `Clone` and `Copy` traits.

feat(c-api) wasmer_metering_t contains a closure to the cost function.

7780423

feat(c-api) Rename Operator to wasmer_parser_operator_t.

e2c4f1e

bors bot added a commit that referenced this pull request Mar 4, 2021

Try #2103:

d05711f

fix(makefile) build-capi includes the middlewares feature.

df0bdeb

bors bot added a commit that referenced this pull request Mar 4, 2021

Try #2103:

aeeabd6

fix(c-api) Remove useless unsafe marker.

2d20f44

jubianchi reviewed Mar 4, 2021

View reviewed changes

lib/c-api/src/wasm_c_api/unstable/middlewares/metering.rs Outdated Show resolved Hide resolved

Hywan added 3 commits March 5, 2021 00:18

Merge branch 'master' into feature/metering-c-api

4136536

chore(c-api) Update headers.

96524b5

fix(engine-native) Fix Cargo.lock and Library::new is unsafe.

8517547

bors bot added a commit that referenced this pull request Mar 4, 2021

Try #2103:

f83d0d6

feat(c-api) Return ~1 if metering are exhausted. Add `wasmer_metering…

1700c2d

…_points_are_exhausted`.

nlewycky commented Mar 4, 2021

View reviewed changes

Hywan added 2 commits March 5, 2021 00:56

doc(c-api) Update documentation for clarity.

06a00d9

chore(c-api) Change operator to wasm_operator (reserved keyword i…

d3f7b7e

…n C++).

bors bot added a commit that referenced this pull request Mar 4, 2021

Try #2103:

c3dda42

Hywan reviewed Mar 5, 2021

View reviewed changes

lib/c-api/build.rs Outdated Show resolved Hide resolved

chore(c-api) Fix merge conflicts.

21ea09f

Hywan approved these changes Mar 5, 2021

View reviewed changes

bors bot merged commit 09aee94 into master Mar 5, 2021

bors bot deleted the feature/metering-c-api branch March 5, 2021 09:53

MarkMcCaskey reviewed Mar 5, 2021

View reviewed changes

Hywan mentioned this pull request Jul 15, 2021

Expose all CallError details to C API #1046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(c-api) Introduce the middleware (specifically metering) C API #2103

feat(c-api) Introduce the middleware (specifically metering) C API #2103

nlewycky commented Feb 9, 2021 •

edited by Hywan

Loading

syrusakbary left a comment •

edited

Loading

nlewycky commented Feb 9, 2021

syrusakbary commented Feb 9, 2021

jubianchi left a comment

Hywan commented Mar 3, 2021

Hywan commented Mar 4, 2021

bors bot commented Mar 4, 2021

Hywan commented Mar 4, 2021

bors bot commented Mar 4, 2021

Hywan commented Mar 4, 2021

nlewycky Mar 4, 2021

Hywan Mar 4, 2021

nlewycky Mar 4, 2021

Hywan Mar 4, 2021

nlewycky Mar 4, 2021

Hywan Mar 4, 2021

bors bot commented Mar 4, 2021

Hywan commented Mar 4, 2021

bors bot commented Mar 5, 2021

Hywan left a comment

bors bot commented Mar 5, 2021

MarkMcCaskey left a comment

MarkMcCaskey Mar 5, 2021

MarkMcCaskey Mar 5, 2021

feat(c-api) Introduce the middleware (specifically metering) C API #2103

feat(c-api) Introduce the middleware (specifically metering) C API #2103

Conversation

nlewycky commented Feb 9, 2021 • edited by Hywan Loading

syrusakbary left a comment • edited Loading

Choose a reason for hiding this comment

Tests

nlewycky commented Feb 9, 2021

syrusakbary commented Feb 9, 2021

jubianchi left a comment

Choose a reason for hiding this comment

Hywan commented Mar 3, 2021

Hywan commented Mar 4, 2021

bors bot commented Mar 4, 2021

try

Hywan commented Mar 4, 2021

bors bot commented Mar 4, 2021

try

Hywan commented Mar 4, 2021

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

bors bot commented Mar 4, 2021

try

Hywan commented Mar 4, 2021

bors bot commented Mar 5, 2021

try

Hywan left a comment

Choose a reason for hiding this comment

bors bot commented Mar 5, 2021

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlewycky commented Feb 9, 2021 •

edited by Hywan

Loading

syrusakbary left a comment •

edited

Loading