Stream: git-wasmtime

Topic: wasmtime / issue #5255 feat: implement memory.atomic.noti...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2022 at 14:32):

bjorn3 commented on issue #5255:

The rustix crate wasmtime already uses has futex support. memory.atomic.{notify,wait32,wait64} are modeled after the futex model afaik, not the thread parking model that parking_lot_core exposes. In addition parking_lot_core is modelled on top of futex on Linux. As such using rustix::thread::futex will likely require less code and be more efficient.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2022 at 14:35):

bjorn3 edited a comment on issue #5255:

The rustix crate wasmtime already uses has futex support. memory.atomic.{notify,wait32,wait64} are modeled after the futex model afaik, not the thread parking model that parking_lot_core exposes. In addition parking_lot_core is modelled on top of futex on Linux. As such using rustix::thread::futex will likely require less code and be more efficient.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2022 at 14:38):

haraldh commented on issue #5255:

@bjorn3
I had a futex solution here, but we need a solution for other OSes and AtomicU64, too.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2022 at 14:42):

haraldh commented on issue #5255:

@bjorn3 see also this comment

view this post on Zulip Wasmtime GitHub notifications bot (Nov 11 2022 at 14:47):

bjorn3 commented on issue #5255:

Didn't think of wait64. That is indeed an issue.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2022 at 15:18):

haraldh commented on issue #5255:

Implemented parking_spot as a parking_lot_core replacement.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2022 at 16:33):

haraldh commented on issue #5255:

Err... SIGKILL is hard in the CI :)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):

haraldh edited a comment on issue #5255:

Err... SIGKILL is hard in the CI :)
edit: maybe a timeout killer?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):

alexcrichton commented on issue #5255:

Oh, and to reiterate, this will require tests to merge.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2022 at 16:38):

haraldh commented on issue #5255:

Oh, and to reiterate, this will require tests to merge.

@alexcrichton wouldn't that need threading support first, to test it in full action?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 16 2022 at 16:56):

alexcrichton commented on issue #5255:

It would indeed, and everything necessary should be implemented in Wasmtime today. There are, for example, already tests with threads.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 17 2022 at 08:53):

haraldh commented on issue #5255:

Addressed some of the issues, working on the others. Now tested with my real pthread example.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 17 2022 at 10:35):

haraldh commented on issue #5255:

So, I setup the aarch64 cross compile locally and it passes... just takes some time:

wasmtime/crates/parking_spot on  feature/atomic_wait_notify via 🦀 v1.65.0 took 42s
[debian-toolbox:latest] ❯ cargo test --target aarch64-unknown-linux-gnu  --locked -p wasmtime-parking_spot --lib
   Compiling wasmtime-parking_spot v4.0.0 (/home/harald/git/wasmtime/crates/parking_spot)
    Finished test [optimized + debuginfo] target(s) in 0.78s
     Running unittests src/lib.rs (/home/harald/git/wasmtime/target/aarch64-unknown-linux-gnu/debug/deps/wasmtime_parking_spot-91a65de835bf666b)

running 13 tests
test tests::atomic_wait_notify ... ok
test tests::parking_lot::hundred_unpark_all_one ... ok
test tests::parking_lot::unpark_one_fifty ... ok
test tests::parking_lot::unpark_all_one ... ok
test tests::parking_lot::unpark_one_one ... ok
test tests::parking_lot::unpark_one_fifty_then_fifty_all ... ok
test tests::parking_lot::unpark_one_hundred_fast ... ok
test tests::parking_lot::unpark_one_one_fast ... ok
test tests::parking_lot::unpark_one_fifty_then_fifty_all_fast ... ok
test tests::parking_lot::unpark_all_hundred ... ok
test tests::parking_lot::unpark_all_hundred_fast ... ok
test tests::parking_lot::hundred_unpark_all_one_fast has been running for over 60 seconds
test tests::parking_lot::unpark_all_one_fast has been running for over 60 seconds
test tests::parking_lot::hundred_unpark_all_one_fast ... ok
test tests::parking_lot::unpark_all_one_fast ... ok

test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 98.13s

view this post on Zulip Wasmtime GitHub notifications bot (Nov 17 2022 at 15:10):

haraldh commented on issue #5255:

Oh, and to reiterate, this will require tests to merge.

@alexcrichton added one basic test... more to follow

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2022 at 08:49):

github-actions[bot] commented on issue #5255:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Nov 18 2022 at 13:05):

haraldh commented on issue #5255:

Looks reasonable to me, thanks!

One thing that we'll want to add is embedder APIs to interact with these instructions as well. For eample I think we'll want SharedMemory::{notify,wait32,wait64} which are basically the same wasm instructions but callable by wasmtime embedders, allowing programmatic interactions with threads as well. That doesn't need to be added necessarily in this PR, however, but it might motivate movement of the implementation of these instructions into wasmtime_runtime::SharedMemory instead of keeping it in libcalls in the long run.

added Shared::Memory::atomic_* methods.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 20 2022 at 06:25):

github-actions[bot] commented on issue #5255:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Last updated: Dec 23 2024 at 12:05 UTC