Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Wait/Notify opcode, the waiters hashmap is now on the Memory itself #3723

Merged
merged 17 commits into from
Apr 13, 2023

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Mar 30, 2023

The Wait/Notify opcodes needs a list a waiters to works.
This list (an hashmap in fact) needs to be on a common object. It makes more sense to have the hashmap being part of the Memory object.

The LinearMemory memory trait has been expanded accordingly, with sensible default, that will behave as if the memory was not shared.
The actuall hashmap in only implemented for VMSharedMemory

Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

One thing to fix, and some recommendations.

I like moving this to the memory.

Note: this can be a separate PR, but we also need some external way to know when a thread goes to sleep (wait) and is woken up (notify).

Moving this to the memory is actually great for that, because now we should be able to add it to the trait, so the behaviour of wait/notify can be customized.

You see any problems with that?

lib/vm/src/memory.rs Outdated Show resolved Hide resolved
lib/vm/src/memory.rs Show resolved Hide resolved
lib/vm/src/memory.rs Outdated Show resolved Hide resolved
lib/vm/src/memory.rs Outdated Show resolved Hide resolved
lib/vm/src/memory.rs Outdated Show resolved Hide resolved
lib/vm/src/threadconditions.rs Outdated Show resolved Hide resolved
lib/vm/src/threadconditions.rs Outdated Show resolved Hide resolved
lib/vm/src/threadconditions.rs Outdated Show resolved Hide resolved
lib/vm/src/threadconditions.rs Show resolved Hide resolved
lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
@ptitSeb
Copy link
Contributor Author

ptitSeb commented Mar 30, 2023

I'm fine with that :)

@john-sharratt
Copy link
Contributor

Hey guys - this is not asynchronous thus it will block execution and cause quite some performance bottlenecks on the WASI implementation with tokio. Have you put consideration into how the thread will be unloaded into an asynchronous context?

@theduke
Copy link
Contributor

theduke commented Apr 3, 2023

@john-sharratt we will add functionality to extend the wait/notify behaviour in the LinearMemory trait, but I haven't in detail thought about how to propagate this .

It will be difficult while also being able to snapshot the stack.

Need to talk/think about this, but that can be done as a followup, not in this PR.

lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
lib/vm/src/memory.rs Outdated Show resolved Hide resolved
lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
lib/vm/src/threadconditions.rs Show resolved Hide resolved
Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

LGTM

@ptitSeb ptitSeb enabled auto-merge (squash) April 13, 2023 14:19
@ptitSeb ptitSeb merged commit b2170e3 into master Apr 13, 2023
@ptitSeb ptitSeb deleted the fix_wait_notify_opcodes branch April 13, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants