Wasmtime GitHub notifications bot (Nov 11 2022 at 14:07): Wasmtime GitHub notifications bot (Nov 11 2022 at 14:07):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?
Wasmtime GitHub notifications bot (Nov 11 2022 at 14:16):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 11 2022 at 14:27):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 14 2022 at 16:36):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 16 2022 at 15:16):haraldh edited PR #5255 from feature/atomic_wait_notify to main:
Added the parking_spot crate, which provides the needed registry for the operations.
Wasmtime GitHub notifications bot (Nov 16 2022 at 15:17):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 16 2022 at 15:18):haraldh requested alexcrichton for a review on PR #5255.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):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.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
For including in this repo could you rename this to
wasmtime-parking-spotand set the version to4.0.0as 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.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):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.0and then there'd be no need for a new vet entry.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
Instead of having
unpark_allvsunparkcould this thread through au32count always? That matches what the wasm semantics of the notify instruction are more closely I think.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
I think this comment may no longer be relevant.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
It should be ok to drop the
returnhere
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
Could you document that the
SeqCsthere matches the ordering specified by wasm itself?
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
It's ok to leave this out since it's already depended on by other crates.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
Could this perhaps return
&ParkingSpotto avoid the need for cloning? I think that should work out for the definitions in the libcalls.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):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.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):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_indexonly 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 handlingNonein each instruction, for example by sleeping forwait, perhaps trapping for an infinite sleep, and doing nothing fornotify.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
This is one reason I would like to require tests as part of this PR. The
addrhere 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.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
Could you add comments here as to what these fields are tracking? For example I believe
to_unparkis required per the spec since it doesn't allow for spurious wakeups.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
I think it would be better to put the
ParkingSpotadjacent to theRwLockin memory to avoid allocating twoArcs here. Additionally as this struct grows can you use named fields rather than tuple structs?
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):alexcrichton created PR review comment:
Could an optimization be added here to use
notify_allifnis greater thanto_unpark? That'll help I believe with the removal ofunpark_all.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:35):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.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:36):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 16 2022 at 16:36):haraldh created PR review comment:
oh, that actually can be removed now ...
Wasmtime GitHub notifications bot (Nov 16 2022 at 20:04):haraldh deleted PR review comment.
Wasmtime GitHub notifications bot (Nov 16 2022 at 20:05):haraldh created PR review comment:
oops, totally correct!!
Wasmtime GitHub notifications bot (Nov 16 2022 at 20:05):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:42):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:42):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:42):haraldh created PR review comment:
removed
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:42):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:42):haraldh created PR review comment:
changed
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:43):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:43):haraldh created PR review comment:
removed
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:44):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:44):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:44):haraldh created PR review comment:
changed to workspace version
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:44):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:44):haraldh created PR review comment:
removed
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:45):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:45):haraldh created PR review comment:
fixed
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:45):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:45):haraldh created PR review comment:
fixed
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:45):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:45):haraldh created PR review comment:
maybe fixed
Wasmtime GitHub notifications bot (Nov 17 2022 at 08:57):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 09:00):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 09:51):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 11:36):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 11:57):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:00):haraldh created PR review comment:
done
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:00):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:28):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:28):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:28):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:28):haraldh created PR review comment:
done
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:28):haraldh created PR review comment:
moved it to a named field in SharedMemoryInner
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:28):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:29):haraldh created PR review comment:
err.. did not succeed
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:29):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:29):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:29):haraldh created PR review comment:
done
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:29):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:29):haraldh created PR review comment:
done
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:40):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 12:48):haraldh edited PR review comment.
Wasmtime GitHub notifications bot (Nov 17 2022 at 13:57):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 13:58):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 13:58):haraldh created PR review comment:
done
Wasmtime GitHub notifications bot (Nov 17 2022 at 13:58):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 15:10):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 15:45):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 15:46):haraldh requested alexcrichton for a review on PR #5255.
Wasmtime GitHub notifications bot (Nov 17 2022 at 15:48):haraldh created PR review comment:
I can remove this test module, if wanted.
Wasmtime GitHub notifications bot (Nov 17 2022 at 15:48):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 17 2022 at 15:54):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 17 2022 at 16:43):haraldh created PR review comment:
or guard it behind some feature flag
Wasmtime GitHub notifications bot (Nov 17 2022 at 16:43):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 04:32):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 04:32):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 04:32):alexcrichton created PR review comment:
I think it should be ok to drop the
pubhere
Wasmtime GitHub notifications bot (Nov 18 2022 at 04:32):alexcrichton created PR review comment:
It should be ok to drop the
#[inline]here and largely let LLVM figure it out.
Wasmtime GitHub notifications bot (Nov 18 2022 at 04:32):alexcrichton created PR review comment:
I think it would be better to detect QEMU and bail out. Currently we have
WASMTIME_NO_HOG_MEMORY=1as an env var and that can be used as a rough proxy for "I'm in qemu" and the tests are skipped.
Wasmtime GitHub notifications bot (Nov 18 2022 at 04:32):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.
Wasmtime GitHub notifications bot (Nov 18 2022 at 04:32):alexcrichton created PR review comment:
If this returned
&Arc<ParkingSpot>what other errors pop up?
Wasmtime GitHub notifications bot (Nov 18 2022 at 04:32):alexcrichton created PR review comment:
I don't think there's any need to tag all functions in this file with
#[inline]
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:25):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:25):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
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:26):haraldh created PR review comment:
Also a reference to an
Arcdefies the purpose ofArchere :)
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:26):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh created PR review comment:
removed
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh created PR review comment:
removed
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh created PR review comment:
removed
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh created PR review comment:
done
Wasmtime GitHub notifications bot (Nov 18 2022 at 08:29):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 11:13):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 11:15):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 11:15):haraldh created PR review comment:
done
Wasmtime GitHub notifications bot (Nov 18 2022 at 11:29):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:
- [ ]
wait32doesn't block if the values don't match- [ ]
wait32eventually times out if the values match- [ ]
wait32traps on unshared memories- [ ]
notifyreturns 0 on unshared memories- [ ]
notifyreturns 0 with 0 waiters- [ ]
notifyreturns 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)- [ ]
wait32andnotifyrequire in-bounds and aligned addresses
Wasmtime GitHub notifications bot (Nov 18 2022 at 11:29):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:
- [ ]
wait32doesn't block if the values don't match- [ ]
wait32eventually times out if the values match- [ ]
wait32traps on unshared memories- [ ]
notifyreturns 0 on unshared memories- [ ]
notifyreturns 0 with 0 waiters- [ ]
notifyreturns 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]
wait32andnotifyrequire in-bounds and aligned addresses
Wasmtime GitHub notifications bot (Nov 18 2022 at 13:04):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 13:06):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 13:06):haraldh created PR review comment:
nevertheless, by adding
Shared::Memory::atomic_*methods, I got rid of it.
Wasmtime GitHub notifications bot (Nov 18 2022 at 13:24):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 14:02):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 14:03):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:
- [ ]
wait32doesn't block if the values don't match- [ ]
wait32eventually times out if the values match- [x]
wait32traps on unshared memories- [ ]
notifyreturns 0 on unshared memories- [ ]
notifyreturns 0 with 0 waiters- [ ]
notifyreturns 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]
wait32andnotifyrequire in-bounds and aligned addresses
Wasmtime GitHub notifications bot (Nov 18 2022 at 14:42):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 14:42):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:
- [ ]
wait32doesn't block if the values don't match- [ ]
wait32eventually times out if the values match- [x]
wait32traps on unshared memories- [x]
notifyreturns 0 on unshared memories- [ ]
notifyreturns 0 with 0 waiters- [ ]
notifyreturns 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]
wait32andnotifyrequire in-bounds and aligned addresses
Wasmtime GitHub notifications bot (Nov 18 2022 at 14:42):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:
- [ ]
wait32doesn't block if the values don't match- [ ]
wait32eventually times out if the values match- [x]
wait32traps on unshared memories- [x]
notifyreturns 0 on unshared memories- [x]
notifyreturns 0 with 0 waiters- [ ]
notifyreturns 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]
wait32andnotifyrequire in-bounds and aligned addresses
Wasmtime GitHub notifications bot (Nov 18 2022 at 14:50):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:
- [ ]
wait32doesn't block if the values don't match- [ ]
wait32eventually times out if the values match- [x]
wait32traps on unshared memories- [x]
notifyreturns 0 on unshared memories- [x]
notifyreturns 0 with 0 waiters- [ ]
notifyreturns 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]
wait32andnotifyrequire in-bounds and aligned addresses
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:36):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:36):alexcrichton created PR review comment:
Would it work to use
anyhow::ensure?
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:36):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:36):alexcrichton created PR review comment:
One thing I might recommend, could the
unsafefunctions here be removed? That way the libcalls to delegate validation to these functions and that would remove the need to haveunsafeinterfaces entirely ideally.
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:36):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::SharedMemorystructure as well. Embedders don't get access towasmtime_runtime::SharedMemoryat this time. Again though if you'd prefer it's ok to defer this to a future PR.
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:36):alexcrichton created PR review comment:
I think it's ok to leave this scoped to
SharedMemoryfor now since that's all it's used for
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:36):alexcrichton created PR review comment:
This doesn't have the desired semantics for the
atomic_wait32instruction unfortunately. I would recommend not storing theParkingSpotstate inside theRwLockof 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.
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:36):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 theRwLockit retains the ability to avoid extraneousArcclones too.
Wasmtime GitHub notifications bot (Nov 18 2022 at 15:47):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 16:10):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 16:10):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 16:10):haraldh created PR review comment:
yes... fixed
Wasmtime GitHub notifications bot (Nov 18 2022 at 16:20):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 16:20):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 16:20):haraldh created PR review comment:
removed totally
Wasmtime GitHub notifications bot (Nov 18 2022 at 16:22):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 18 2022 at 17:20):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 17:20):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 18 2022 at 17:20):alexcrichton created PR review comment:
Could this have a more descriptive name than
InnerInnerperhaps?
Wasmtime GitHub notifications bot (Nov 18 2022 at 17:20):alexcrichton created PR review comment:
I do realize that this fixes the issue of "I added a
*.wastfile 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*.wastfile and have a quick cycle time withcargo test --test all the_testpersonally and having this here would force a rebuild of the test binary for any edit to the*.wastfiles.
Wasmtime GitHub notifications bot (Nov 18 2022 at 17:20):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.rsfile already has a lot of logic for dealing with various special cases and ideally this build script is just "go call thewast.rsfunction with all the found tests"
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:06):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:07):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:07):haraldh created PR review comment:
SharedMemoryInnerRwLocked
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:07):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:07):haraldh created PR review comment:
removed
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:07):haraldh created PR review comment:
done
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:07):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:15):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 20 2022 at 06:45):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 21 2022 at 07:58):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 21 2022 at 08:03):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 21 2022 at 08:03):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 aSharedMemoryobject in the non-shared case, thevalidate_addrhas 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.
Wasmtime GitHub notifications bot (Nov 21 2022 at 08:03):haraldh submitted PR review.
Wasmtime GitHub notifications bot (Nov 21 2022 at 08:03):haraldh created PR review comment:
See the comment on
unchecked_atomic_notify.
Wasmtime GitHub notifications bot (Nov 21 2022 at 13:33):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]
wait32doesn't block if the values don't match- [ ]
wait32eventually times out if the values match- [x]
wait32traps on unshared memories- [x]
notifyreturns 0 on unshared memories- [x]
notifyreturns 0 with 0 waiters- [ ]
notifyreturns 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]
wait32andnotifyrequire in-bounds and aligned addresses
Wasmtime GitHub notifications bot (Nov 21 2022 at 13:34):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]
wait32doesn't block if the values don't match- [x]
wait32eventually times out if the values match- [x]
wait32traps on unshared memories- [x]
notifyreturns 0 on unshared memories- [x]
notifyreturns 0 with 0 waiters- [ ]
notifyreturns 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]
wait32andnotifyrequire in-bounds and aligned addresses
Wasmtime GitHub notifications bot (Nov 21 2022 at 13:36):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 21 2022 at 17:00):haraldh updated PR #5255 from feature/atomic_wait_notify to main.
Wasmtime GitHub notifications bot (Nov 21 2022 at 17:01):haraldh has marked PR #5255 as ready for review.
Wasmtime GitHub notifications bot (Nov 21 2022 at 17:01):haraldh requested alexcrichton for a review on PR #5255.
Wasmtime GitHub notifications bot (Nov 21 2022 at 17:52):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 21 2022 at 17:52):alexcrichton has enabled auto merge for PR #5255.
Wasmtime GitHub notifications bot (Nov 21 2022 at 18:23):alexcrichton merged PR #5255.
Last updated: Nov 22 2024 at 17:03 UTC