haraldh opened PR #5255 from feature/atomic_wait_notify
to main
:
Use parking_lot_core, as this established crate does exactly what is needed here.
WARNING
memory_atomic_wait32()
andmemory_atomic_wait64()
are called with an unsignedtimeout: u64
.The Threading proposal for WebAssembly uses a signed
timeout: i64
. Where is the source of truth?
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
alexcrichton submitted PR review.
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh requested alexcrichton for a review on PR #5255.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
It's ok to remove these files since in in the wasmtime repo we only keep the licenses at the root.
alexcrichton created PR review comment:
For including in this repo could you rename this to
wasmtime-parking-spot
and set the version to4.0.0
as wasmtime's current version? We generally try to keep everything in sync if it's located here.Personally I'd be ok having this either here or externally, however. I have a slight preference for vendoring it given how small it is, but I'll leave that up to you.
alexcrichton created PR review comment:
This will require a new entry for
cargo vet
, but if you'd prefer you can also continue to reuse1.12.0
and then there'd be no need for a new vet entry.
alexcrichton created PR review comment:
Instead of having
unpark_all
vsunpark
could this thread through au32
count always? That matches what the wasm semantics of the notify instruction are more closely I think.
alexcrichton created PR review comment:
I think this comment may no longer be relevant.
alexcrichton created PR review comment:
It should be ok to drop the
return
here
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Could you document that the
SeqCst
here matches the ordering specified by wasm itself?
alexcrichton created PR review comment:
It's ok to leave this out since it's already depended on by other crates.
alexcrichton created PR review comment:
Could this perhaps return
&ParkingSpot
to avoid the need for cloning? I think that should work out for the definitions in the libcalls.
alexcrichton created PR review comment:
Could the need for
as _
be removed here? The underlying implementation could take a key ofu64
, for example, and the count could beu32
. Otherwise I'm worried about lossy casts here and accidentally losing data that wasn't intended.
alexcrichton created PR review comment:
I think there's a few issues with this that will need updating. Again though this is where I think that tests should be added to exercise all these cases.
- Switching to
defined_memory_index
only works if the memory is defined locally, not if the memory is imported. This will want to work with imported memories as well.- The wait/notify instructions work with unshared memories in addition to shared memories, so the unwrap for a shared memory needs to be handled. To handle this I'd recommend returning an
Option<T>
and handlingNone
in each instruction, for example by sleeping forwait
, perhaps trapping for an infinite sleep, and doing nothing fornotify
.
alexcrichton created PR review comment:
This is one reason I would like to require tests as part of this PR. The
addr
here is a wasm-relative address, not a host address, meaning that I think this will segfault or otherwise corrupt lower memory addresses. I suspect, however, that tests would uncover this relatively quickly.
alexcrichton created PR review comment:
Could you add comments here as to what these fields are tracking? For example I believe
to_unpark
is required per the spec since it doesn't allow for spurious wakeups.
alexcrichton created PR review comment:
I think it would be better to put the
ParkingSpot
adjacent to theRwLock
in memory to avoid allocating twoArc
s here. Additionally as this struct grows can you use named fields rather than tuple structs?
alexcrichton created PR review comment:
Could an optimization be added here to use
notify_all
ifn
is greater thanto_unpark
? That'll help I believe with the removal ofunpark_all
.
alexcrichton created PR review comment:
Could this unconditionally happen to double-check that the count can always be decremented? The conditional check for removal from the internal map could then happen afterwards.
haraldh submitted PR review.
haraldh created PR review comment:
oh, that actually can be removed now ...
haraldh deleted PR review comment.
haraldh created PR review comment:
oops, totally correct!!
haraldh submitted PR review.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
removed
haraldh submitted PR review.
haraldh created PR review comment:
changed
haraldh submitted PR review.
haraldh created PR review comment:
removed
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
changed to workspace version
haraldh submitted PR review.
haraldh created PR review comment:
removed
haraldh submitted PR review.
haraldh created PR review comment:
fixed
haraldh submitted PR review.
haraldh created PR review comment:
fixed
haraldh submitted PR review.
haraldh created PR review comment:
maybe fixed
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh created PR review comment:
done
haraldh submitted PR review.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh submitted PR review.
haraldh created PR review comment:
done
haraldh created PR review comment:
moved it to a named field in SharedMemoryInner
haraldh submitted PR review.
haraldh created PR review comment:
err.. did not succeed
haraldh submitted PR review.
haraldh submitted PR review.
haraldh created PR review comment:
done
haraldh submitted PR review.
haraldh created PR review comment:
done
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh edited PR review comment.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh created PR review comment:
done
haraldh submitted PR review.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh requested alexcrichton for a review on PR #5255.
haraldh created PR review comment:
I can remove this test module, if wanted.
haraldh submitted PR review.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh created PR review comment:
or guard it behind some feature flag
haraldh submitted PR review.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I think it should be ok to drop the
pub
here
alexcrichton created PR review comment:
It should be ok to drop the
#[inline]
here and largely let LLVM figure it out.
alexcrichton created PR review comment:
I think it would be better to detect QEMU and bail out. Currently we have
WASMTIME_NO_HOG_MEMORY=1
as an env var and that can be used as a rough proxy for "I'm in qemu" and the tests are skipped.
alexcrichton created PR review comment:
It appears I didn't read close enough, and according to the overview (which I would highly recommend reviewing/correlating with the implementation here) it says:
Wait operators additionally trap if used on an unshared linear memory
So this can't actually sleep it needs to always trap. Additionally you'll want to allocate a trap code instead of reusing a different trap code for this purpose.
alexcrichton created PR review comment:
If this returned
&Arc<ParkingSpot>
what other errors pop up?
alexcrichton created PR review comment:
I don't think there's any need to tag all functions in this file with
#[inline]
haraldh submitted PR review.
haraldh created PR review comment:
error[E0515]: cannot return value referencing temporary value --> crates/runtime/src/memory.rs:492:9 | 492 | &self.0.read().unwrap().spot | ^----------------------^^^^^ | || | |temporary value created here | returns a value referencing data owned by the current function
haraldh created PR review comment:
Also a reference to an
Arc
defies the purpose ofArc
here :)
haraldh submitted PR review.
haraldh submitted PR review.
haraldh created PR review comment:
removed
haraldh submitted PR review.
haraldh created PR review comment:
removed
haraldh submitted PR review.
haraldh created PR review comment:
removed
haraldh submitted PR review.
haraldh created PR review comment:
done
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
done
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
Activated the spec testsuite for threads.
Tests include:
- [ ]
wait32
doesn't block if the values don't match- [ ]
wait32
eventually times out if the values match- [ ]
wait32
traps on unshared memories- [ ]
notify
returns 0 on unshared memories- [ ]
notify
returns 0 with 0 waiters- [ ]
notify
returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)- [ ]
wait32
andnotify
require in-bounds and aligned addresses
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
Activated the spec testsuite for threads.
Tests include:
- [ ]
wait32
doesn't block if the values don't match- [ ]
wait32
eventually times out if the values match- [ ]
wait32
traps on unshared memories- [ ]
notify
returns 0 on unshared memories- [ ]
notify
returns 0 with 0 waiters- [ ]
notify
returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)- [x]
wait32
andnotify
require in-bounds and aligned addresses
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
nevertheless, by adding
Shared::Memory::atomic_*
methods, I got rid of it.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
Activated the spec testsuite for threads.
Tests include:
- [ ]
wait32
doesn't block if the values don't match- [ ]
wait32
eventually times out if the values match- [x]
wait32
traps on unshared memories- [ ]
notify
returns 0 on unshared memories- [ ]
notify
returns 0 with 0 waiters- [ ]
notify
returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)- [x]
wait32
andnotify
require in-bounds and aligned addresses
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
Activated the spec testsuite for threads.
Tests include:
- [ ]
wait32
doesn't block if the values don't match- [ ]
wait32
eventually times out if the values match- [x]
wait32
traps on unshared memories- [x]
notify
returns 0 on unshared memories- [ ]
notify
returns 0 with 0 waiters- [ ]
notify
returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)- [x]
wait32
andnotify
require in-bounds and aligned addresses
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
Activated the spec testsuite for threads.
Tests include:
- [ ]
wait32
doesn't block if the values don't match- [ ]
wait32
eventually times out if the values match- [x]
wait32
traps on unshared memories- [x]
notify
returns 0 on unshared memories- [x]
notify
returns 0 with 0 waiters- [ ]
notify
returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)- [x]
wait32
andnotify
require in-bounds and aligned addresses
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
Tests include:
- [ ]
wait32
doesn't block if the values don't match- [ ]
wait32
eventually times out if the values match- [x]
wait32
traps on unshared memories- [x]
notify
returns 0 on unshared memories- [x]
notify
returns 0 with 0 waiters- [ ]
notify
returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)- [x]
wait32
andnotify
require in-bounds and aligned addresses
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Would it work to use
anyhow::ensure
?
alexcrichton submitted PR review.
alexcrichton created PR review comment:
One thing I might recommend, could the
unsafe
functions here be removed? That way the libcalls to delegate validation to these functions and that would remove the need to haveunsafe
interfaces entirely ideally.
alexcrichton created PR review comment:
Thanks for adding these, and if you're interested the final step of exposing these would be to have them public in the
wasmtime::SharedMemory
structure as well. Embedders don't get access towasmtime_runtime::SharedMemory
at this time. Again though if you'd prefer it's ok to defer this to a future PR.
alexcrichton created PR review comment:
I think it's ok to leave this scoped to
SharedMemory
for now since that's all it's used for
alexcrichton created PR review comment:
This doesn't have the desired semantics for the
atomic_wait32
instruction unfortunately. I would recommend not storing theParkingSpot
state inside theRwLock
of a shared memory. The problem here is that memory is read-locked for the entire duration of the park, meaning that a concurrent memory growth is blocked until this wakes up.This would actually make a great test as well. It's ok to leave this sort of test to later though if you prefer. Basically though there could be tests that while a thread is parked all other operations can still happen concurrently.
alexcrichton created PR review comment:
I'll note this was not a problem earlier due to the
Arc<ParkingSpot>
being cloned out of the internal storage. That's another possible solution, but if the storage for the spot is stored outside theRwLock
it retains the ability to avoid extraneousArc
clones too.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
yes... fixed
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
removed totally
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Could this have a more descriptive name than
InnerInner
perhaps?
alexcrichton created PR review comment:
I do realize that this fixes the issue of "I added a
*.wast
file and would like it to get picked up", but personally I'd prefer to leave this out if you're ok with it. I find it much more useful to work on a*.wast
file and have a quick cycle time withcargo test --test all the_test
personally and having this here would force a rebuild of the test binary for any edit to the*.wast
files.
alexcrichton created PR review comment:
Could this logic, in addition to issues related to pooling and threads, all get resolved in
tests/all/wast.rs
? Thewast.rs
file already has a lot of logic for dealing with various special cases and ideally this build script is just "go call thewast.rs
function with all the found tests"
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
SharedMemoryInnerRwLocked
haraldh submitted PR review.
haraldh created PR review comment:
removed
haraldh created PR review comment:
done
haraldh submitted PR review.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
So, the spectests expect the alignment check before any other trap and even in the non-shared case.
Because I can't get to aSharedMemory
object in the non-shared case, thevalidate_addr
has to live elsewhere, so I moved it in the fast path toVMMemoryDefinition
.And in the shared memory case, I have already validated the Atomic address, so I thought, I create an unchecked version called with the already validated address to not do the same work twice.
haraldh submitted PR review.
haraldh created PR review comment:
See the comment on
unchecked_atomic_notify
.
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
Tests include:
- [x]
wait32
doesn't block if the values don't match- [ ]
wait32
eventually times out if the values match- [x]
wait32
traps on unshared memories- [x]
notify
returns 0 on unshared memories- [x]
notify
returns 0 with 0 waiters- [ ]
notify
returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)- [x]
wait32
andnotify
require in-bounds and aligned addresses
haraldh edited PR #5255 from feature/atomic_wait_notify
to main
:
Added the parking_spot crate, which provides the needed registry for the operations.
Tests include:
- [x]
wait32
doesn't block if the values don't match- [x]
wait32
eventually times out if the values match- [x]
wait32
traps on unshared memories- [x]
notify
returns 0 on unshared memories- [x]
notify
returns 0 with 0 waiters- [ ]
notify
returns the number woken (this one you can't precisely test for but you could add a sleep for ~100ms and otherwise retry the test N times until it works)- [x]
wait32
andnotify
require in-bounds and aligned addresses
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh updated PR #5255 from feature/atomic_wait_notify
to main
.
haraldh has marked PR #5255 as ready for review.
haraldh requested alexcrichton for a review on PR #5255.
alexcrichton submitted PR review.
alexcrichton has enabled auto merge for PR #5255.
alexcrichton merged PR #5255.
Last updated: Dec 23 2024 at 12:05 UTC