FIXES #137: make calling wasm from python 7x faster #139

muayyad-alsadi · 2023-03-25T22:05:23Z

this is a great enhancement from 43 ms to 6 ms in @alexprengere question in #96

0.0002523580042179674 # math_gcd
0.0014094869984546676 # python_gcd
0.043804362998344004  #  wasm_gcd
0.005873051006346941  # wasm_gcd_alt <-------- my change

muayyad-alsadi · 2023-03-25T23:07:27Z

Why I'm getting 'Func' object has no attribute '_params_n'
despite it's being defined in the constructor params_n = len(ty_params); self._params_n = params_n

_______________________ ERROR collecting examples/gcd.py _______________________
examples/gcd.py:10: in <module>
    print("gcd(6, 27) = %d" % gcd(store, 6, 27))
wasmtime/_func.py:105: in __call__
    if params_n > self._params_n:
E   AttributeError: 'Func' object has no attribute '_params_n'

muayyad-alsadi · 2023-03-26T09:14:35Z

the reason it failed was because it init was never called as it was constructed with Func._from_raw()
I would like to call _init_call(ty) but I don't have ty at that point.

muayyad-alsadi · 2023-03-26T10:39:24Z

all the test errors are in codegen, what are so special about those? why normal work and this does not work?
can anybody take it from here?

alexcrichton

This seems reasonable to me to add one way or another, but there's a number of soundness and safety issues to address before merging, in addition to the test failures.

alexcrichton · 2023-03-27T14:57:12Z

wasmtime/_func.py

+        for i, param_str in enumerate(self._params_str):
+            setattr(raw[i], param_str, params[i])


I don't think that this conversion is quite correct. The IntoVal type could also be Val, for example, to handle things like v128/funcref/externref/etc. In such a case the raw attribute of raw[i] can't be set to that value, and I'm not sure what happens otherwise during such a process.

Additionally it's not safe to simply set the externref and funcref fields. Those need to go through the methods in the Val type.

For an example you can see the logic in Value._convert

I have no idea about externref and funcref

the code related to v128 says it's not implemented

elif kind == WASMTIME_V128.value: raise Exception("unimplemented v128 type")

but no problem I've just pushed an example and make sure it works

https://github.com/muayyad-alsadi/wasmtime-py/blob/wip-speed-call/examples/simd_i8x16.py#L15

would you please give me examples about externref and funcref

wasmtime/_func.py

alexcrichton · 2023-03-27T14:58:36Z

wasmtime/_func.py

+        if self._results_n==1:
+            return getattr(vals_raw[0], self._results_str0)


It's not sufficient to simply call getattr here, namely for the funcref and externref types. You can see what the logic in Value._value does, for example.

I'm trying to move all the logic in the constructor (or in _init_call) so that this logic is run once the module is loaded not on every single call. and hence the speed boost.

I need a minimal reproducible example about the two missing types, in a way similar to my SIMD example.

I found the example it's test_refs.py I'll look into it and make sure it works

alexcrichton · 2023-03-28T14:27:49Z

Sorry I don't have a ton of time to sink into this myself. I can help out more once CI is passing perhaps, but until then I think it's probably best to fix those errors. Once CI is passing that should give a sense of everything needed to handle various edge cases correctly.

muayyad-alsadi · 2023-04-03T08:15:44Z

it was very difficult to know how extenref and funcref internally works
but I figured that out.

the code now works and passes all the tests except cosmetic tests
I'm not going to fix them right now, because I want to move some logic to _value.py and restructure the code related to wasmtime_val_raw_t and its helpers, this should not live in _func.py.

muayyad-alsadi · 2023-04-05T00:49:53Z

there seems to be an existing typing bug (not introduced by my commits)

_wasm_valtype_kind = dll.wasm_valtype_kind
_wasm_valtype_kind.restype = wasm_valkind_t
_wasm_valtype_kind.argtypes = [POINTER(wasm_valtype_t)]
def wasm_valtype_kind(arg0: Any) -> wasm_valkind_t:
    return _wasm_valtype_kind(arg0)  # type: ignore

the function wasm_valtype_kind returns int but is defined to return wasm_valkind_t = c_uint8
which is not compatible as c_uint8 is accessed via ret.value

# _types.py
class ValType:
    def __str__(self) -> str:
        assert(self._ptr is not None)
        kind = ffi.wasm_valtype_kind(self._ptr)
        if kind == ffi.WASM_I32.value:
            return 'i32'
        if kind == ffi.WASM_I64.value:
            return 'i64'
        if kind == ffi.WASM_F32.value:
            return 'f32'
        if kind == ffi.WASM_F64.value:
            return 'f64'
        if kind == ffi.WASM_ANYREF.value:
            return 'anyref'
        if kind == ffi.WASM_FUNCREF.value: # <--- as you can see it's not kind.value but just kind which means it's int not c_uint8
            return 'funcref'
        return 'ValType(%d)' % kind  # type: ignore  <----------- this was kind.value which is wrong

If I change wasm_valtype_kind type to be int CI breaks with strange message
and it seems that this is a known issue because "type: ignore" was there (not added by me)

muayyad-alsadi · 2023-04-05T00:58:30Z

I think I'm done and you can take it from here
since the typing issue is not introduced by me and all the CI passes.

muayyad-alsadi · 2023-04-05T09:40:38Z

I've kept old method of calling so that we can compare the performance of the two gcd._call_val(store, 6, 27) and gcd._call_raw(store, 6, 27)

alexcrichton

I think there are still some correctness issues related to funcref/externref. The C API has wasmtime_{func,externref}_{to,from}_raw which need to be used for the various conversions and should avoid the need to pass around a store_id which should be an internal implementation detail.

alexcrichton · 2023-04-10T14:56:10Z

examples/simd_i8x16.py

+)
+
+instance = Instance(store, module, [])
+vector_type = ctypes.c_uint8 * 16


This is not intended to be a public detail of this library that ctypes can be used to pass in data. Currently there's no support for v128 but I think that adding it should be done in a "first class" way that doesn't accidentally expose the internal representation at this time. For example I don't know that this array-based representation is the best for v128.

alexcrichton · 2023-04-10T14:56:41Z

examples/gcd.py

@@ -1,10 +1,13 @@
 # Example of instantiating a wasm module and calling an export on it

 from wasmtime import Store, Module, Instance
-
+from functools import partial


How come this is being imported and used in the examples? Is this required? If not I think it'd be best to leave this out.

alexcrichton · 2023-04-10T15:01:35Z

wasmtime/_value.py

+        ptr = ctypes.POINTER(wasmtime_externref_t)
+        if not val:
+            return None
+        ffi = ptr.from_address(val)
+        if not ffi:
+            return None
+        extern_id = wasmtime_externref_data(ffi)
+        return _unintern(extern_id)


Can this avoid duplicating Val._as_externref?

alexcrichton · 2023-04-10T15:01:55Z

wasmtime/_value.py

+        if val == 0:
+            return None
+        f = wasmtime_func_t()
+        f.store_id = store_id
+        f.index = val
+        return wasmtime.Func._from_raw(f)


Can this avoid duplicating Val._as_func?

alexcrichton · 2023-04-10T15:02:56Z

wasmtime/_value.py

@@ -25,6 +37,63 @@ def _unintern(val: int) -> typing.Any:
    return Val._id_to_extern.get(val)


+def get_valtype_attr(ty: ValType) -> str:
+    return val_id2attr[wasm_valtype_kind(ty._ptr)]  # type: ignore


How come this is type: ignore?

alexcrichton · 2023-04-10T15:11:24Z

wasmtime/_func.py

+    _ty: FuncType
+    _params_n: int
+    _results_n: int
+    _params_str: list[str]


Is it still performant to store strings here? I would expect naively that storing the integer kinds would be more efficient since then checking for funcref/externref would be integer comparisons instead of string comparisons.

alexcrichton · 2023-04-10T15:11:48Z

wasmtime/_func.py

+    _results_n: int
+    _params_str: list[str]
+    _results_str: list[str]
+    _results_str0: str


I think this would be best stored as Optional[str] to avoid having a dummy value listed for empty results.

alexcrichton · 2023-04-10T15:14:39Z

wasmtime/_func.py

+            return None
+        if self._results_n == 1:
+            return val_getter(self._func.store_id, vals_raw[0], self._results_str0)
+        # we can use tuple construct, but I'm using list for compatability


This comment can be removed. Either the result should continue to be a list in which case there's no need for the comment, or the result should change to a tuple, in which case there's no need for the comment.

alexcrichton · 2023-04-10T15:17:04Z

wasmtime/_value.py

+        elif isinstance(val, wasmtime.Func):
+            if val._func.store_id != store_id:
+                raise WasmtimeError("passed funcref does not belong to same store")
+            casted = val._func.index


Passing the index here is not correct. This needs to invoke wasmtime_func_to_raw. This also shows that this needs to have tests passing funcref, for example.

alexcrichton · 2023-04-10T15:17:38Z

wasmtime/_value.py

+        else:
+            ex = Val.externref(val)._raw
+            if ex:
+                casted = ctypes.addressof(ex.of.externref)


Like below for funcref I don't think that this is correct. This needs to call wasmtime_externref_to_raw.

muayyad-alsadi added 3 commits March 26, 2023 01:04

FIXES bytecodealliance#137: make calling wasm from python 7x faster

c724ce1

FIXES bytecodealliance#137: make calling wasm from python 7x faster

d0dcca2

FIXES bytecodealliance#137: make calling wasm from python 7x faster

52a0f1d

muayyad-alsadi added 2 commits March 26, 2023 12:07

FIXES bytecodealliance#137: make calling wasm from python 7x faster

da545aa

FIXES bytecodealliance#137: make calling wasm from python 7x faster

68fa2a8

FIXES bytecodealliance#137: make calling wasm from python 7x faster

f00d32a

alexcrichton reviewed Mar 27, 2023

View reviewed changes

muayyad-alsadi added 3 commits March 27, 2023 22:50

FIXES bytecodealliance#137: make calling wasm from python 7x faster

7bee159

FIXES bytecodealliance#137: make calling wasm from python 7x faster

d0913e8

FIXES bytecodealliance#137: make calling wasm from python 7x faster

d383a05

muayyad-alsadi added 2 commits April 3, 2023 01:16

FIXES bytecodealliance#137: make calling wasm from python 7x faster

383cad6

FIXES bytecodealliance#137: make calling wasm from python 7x faster

dc3d697

muayyad-alsadi added 2 commits April 5, 2023 01:56

FIXES bytecodealliance#137: make calling wasm from python 7x faster

1446d79

FIXES bytecodealliance#137: make calling wasm from python 7x faster

1ca73e9

muayyad-alsadi force-pushed the wip-speed-call branch from 9122bb4 to 1ca73e9 Compare April 4, 2023 23:16

muayyad-alsadi added 8 commits April 5, 2023 02:18

FIXES bytecodealliance#137: make calling wasm from python 7x faster

e6c054b

FIXES bytecodealliance#137: make calling wasm from python 7x faster

8bc29e6

FIXES bytecodealliance#137: make calling wasm from python 7x faster

37b6067

FIXES bytecodealliance#137: make calling wasm from python 7x faster

2b1d2c0

FIXES bytecodealliance#137: make calling wasm from python 7x faster

8417fba

FIXES bytecodealliance#137: make calling wasm from python 7x faster

5bb5c39

FIXES bytecodealliance#137: make calling wasm from python 7x faster

a67ecc0

FIXES bytecodealliance#137: make calling wasm from python 7x faster

0388b16

FIXES bytecodealliance#137: make calling wasm from python 7x faster

4cd48d3

FIXES bytecodealliance#137: make calling wasm from python 7x faster

9285e6c

FIXES bytecodealliance#137: make calling wasm from python 7x faster

818bb8d

This was referenced Apr 5, 2023

Performance questions #96

Open

performance: calling python from wasm is too slow #140

Open

alexcrichton reviewed Apr 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIXES #137: make calling wasm from python 7x faster #139

FIXES #137: make calling wasm from python 7x faster #139

muayyad-alsadi commented Mar 25, 2023

muayyad-alsadi commented Mar 25, 2023

muayyad-alsadi commented Mar 26, 2023

muayyad-alsadi commented Mar 26, 2023

alexcrichton left a comment

alexcrichton Mar 27, 2023

alexcrichton Mar 27, 2023

muayyad-alsadi Mar 27, 2023

alexcrichton Mar 27, 2023

muayyad-alsadi Mar 27, 2023

muayyad-alsadi Mar 27, 2023

alexcrichton commented Mar 28, 2023

muayyad-alsadi commented Apr 3, 2023

muayyad-alsadi commented Apr 5, 2023

muayyad-alsadi commented Apr 5, 2023

muayyad-alsadi commented Apr 5, 2023

alexcrichton left a comment

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

alexcrichton Apr 10, 2023

		for i, param_str in enumerate(self._params_str):
		setattr(raw[i], param_str, params[i])

		if self._results_n==1:
		return getattr(vals_raw[0], self._results_str0)

FIXES #137: make calling wasm from python 7x faster #139

Are you sure you want to change the base?

FIXES #137: make calling wasm from python 7x faster #139

Conversation

muayyad-alsadi commented Mar 25, 2023

muayyad-alsadi commented Mar 25, 2023

muayyad-alsadi commented Mar 26, 2023

muayyad-alsadi commented Mar 26, 2023

alexcrichton 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

alexcrichton commented Mar 28, 2023

muayyad-alsadi commented Apr 3, 2023

muayyad-alsadi commented Apr 5, 2023

muayyad-alsadi commented Apr 5, 2023

muayyad-alsadi commented Apr 5, 2023

alexcrichton 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment