Skip to content

Conversation

@tomaka
Copy link
Contributor

@tomaka tomaka commented Feb 27, 2021

bytecodealliance/wasmtime#2434 has now been merged. This PR updates the code for this new feature and removes usage of corooteen.

Fix #276

One of the objectives would ultimately be to publish smoldot on crates.io, so depending on a git version is kind of "meh".
However, in terms of dependencies, we're trading one crates.io dependency and one git dependency for one git dependency, which makes this trade neutral.

@tomaka
Copy link
Contributor Author

tomaka commented Feb 27, 2021

The way it works is by having a Shared struct that is shared by Rc between the external functions that wasmtime calls and the Jit object.

When wasmtime calls an external function, we store the information about the call in this Shared and return Pending.
wasmtime in turns propagates this pending to the Jit, which reads the content of Shared.
Later, we put back the return value in Shared, and resume execution. This time, the inner function returns Ready.

While this is a bit hacky, it is already how corooteen was implemented.

@tomaka
Copy link
Contributor Author

tomaka commented Feb 27, 2021

cc @pepyakin (might be interesting)

@tomaka
Copy link
Contributor Author

tomaka commented Feb 28, 2021

It seems to sometimes freeze, which is worrisome.

_ => unreachable!(),
match self.instance.get_export(name) {
Some(wasmtime::Extern::Global(g)) => match g.get() {
wasmtime::Val::I32(v) => Ok(u32::from_ne_bytes(v.to_ne_bytes())),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the love of the god, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the only way I know to turn a i32 into a u32 with the same bits representation 🤷

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't this, as well as a regular cast via as, is just a noop? Endianness doesn't matter when you speak about primitive values like here and as would just reinterpret the same bits as an unsigned value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a noop. The point is to tell the Rust compiler that we want the u32 with the same bits as this i32.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 5, 2021

wasmtime 0.24 released. No longer need to depend on git.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 5, 2021

It seems to sometimes freeze, which is worrisome.

I think this was caused by #615, and not by wasmtime.
Can't reproduce any freeze after #621

@tomaka tomaka added the automerge Automatically merge pull request as soon as possible label Mar 5, 2021
@tomaka
Copy link
Contributor Author

tomaka commented Mar 5, 2021

Manage to reproduce a couple of freezes, but they were always networking-related.

@mergify mergify bot merged commit 820b8cf into paritytech:main Mar 5, 2021
@tomaka tomaka deleted the fix-276 branch March 5, 2021 09:07
tomaka added a commit that referenced this pull request Apr 8, 2021
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
tomaka added a commit to tomaka/substrate-lite-1 that referenced this pull request Apr 15, 2021
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically merge pull request as soon as possible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't use the corooteen library anymore

2 participants