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

Add canonical thread.* builtins #291

Merged
merged 14 commits into from
Mar 8, 2024
Merged

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Jan 3, 2024

This change adds the thread.spawn and thread.hw_concurrency builtins in line with what was proposed in the shared-everything threads proposal.

This change adds the `thread.spawn` and `thread.hw_concurrency` builtins
in line with what was proposed in the shared-everything threads
[proposal].

[proposal]: https://github.com/WebAssembly/shared-everything-threads/blob/main/proposals/shared-everything-threads/Overview.md#thread-management-builtins
Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

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

Looks generally good so far; nice work! Is the only other thing left to add before this PR is complete the binary encoding of thread.spawn and thread.hw_concurrency in Binary.md?

design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
@abrown abrown marked this pull request as ready for review January 11, 2024 19:55
Co-authored-by: Luke Wagner <mail@lukewagner.name>
@badeend
Copy link
Contributor

badeend commented Jan 28, 2024

I assume that any parallelism introduced by these built-ins are contained to the component instance that spawned them, and that cross-component communication (including client<->host) is still required to be fully sequential in order to comply with the component invariants.

It might be worthwhile to throw in a quick clarification of this under the new Threads section in the explainer.

@lukewagner
Copy link
Member

@badeend That's correct yes. A future (more extensive) extension to the C-M would be to add shared attributes to component-level functions so that they could be lifted-from or lowered-to shared core functions but, until we add that, lowering a (non-shared) component-level function necessarily produces a (non-shared) core function which does end up forcing serialization back to the "main" (entry) thread for cross-component calls.

Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

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

Ah nice, that closes out a lot of questions; thanks! LGTM w/ these nits fixed. I'll wait another week or so for any other feedback before merging.

design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Show resolved Hide resolved
abrown and others added 2 commits February 20, 2024 15:13
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
@lukewagner
Copy link
Member

I think all the actionable feedback has been addressed, so I'll plan to merge at the end of the week unless anything new comes up.

@lukewagner lukewagner merged commit 5549780 into WebAssembly:main Mar 8, 2024
1 check passed
@abrown abrown deleted the threads branch March 8, 2024 23:48
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.

None yet

7 participants