Skip to content

update future.{write,read} ABIs #2222

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

Merged
merged 6 commits into from
Jun 6, 2025
Merged

Conversation

dicej
Copy link
Collaborator

@dicej dicej commented Jun 6, 2025

This implements WebAssembly/component-model#524 insofar as it affects validation, componentization, etc.

  • future.write now accepts its payload value as up to 4 flat parameters before spilling to linear memory.

  • future.read takes no payload pointer when it has no payload type

  • {stream,future}.close-{readable,writable} have been renamed to {stream,future}.drop-{readable,writable}

@dicej dicej requested a review from a team as a code owner June 6, 2025 16:49
@dicej dicej requested review from pchickey and removed request for a team June 6, 2025 16:49
@dicej dicej force-pushed the new-future-abi branch from 31502cf to 3f5fb97 Compare June 6, 2025 16:56
@dicej
Copy link
Collaborator Author

dicej commented Jun 6, 2025

CI failures appear to be due to me using the latest Rust release; will downgrade to the MSRV locally and fix the issues.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good 👍

In the *.wast tests I think I only saw renamings, so could you add tests for futures for both reads/writes to ensure properties such as future.read has no pointer with no payload and future.write uses flat lowerings or a pointer if the flat lowering is too big? Additionally I think it'd be good to have a test that future.write doesn't require memory if the lowering fits

@dicej
Copy link
Collaborator Author

dicej commented Jun 6, 2025

In the *.wast tests I think I only saw renamings, so could you add tests for futures for both reads/writes to ensure properties such as future.read has no pointer with no payload and future.write uses flat lowerings or a pointer if the flat lowering is too big? Additionally I think it'd be good to have a test that future.write doesn't require memory if the lowering fits

FWIW, I've tested those cases end-to-end in wasip3-prototyping, plus the fuzz tester is hitting those cases also. I agree we should have some handwritten WAST tests as well; I'll add those.

@alexcrichton
Copy link
Member

Yeah all the code here looks correct so I don't expect any failures, mostly just handwritten tests for the well-known edge cases are good to have to catch any possible issues in future refactorings earlier

@dicej dicej force-pushed the new-future-abi branch 3 times, most recently from 29888ca to 1194f52 Compare June 6, 2025 17:38
dicej added 5 commits June 6, 2025 12:02
This implements WebAssembly/component-model#524 insofar
as it affects validation, componentization, etc.

- `future.write` now accepts its payload value as up to 4 flat parameters before
  spilling to linear memory.

- `future.read` takes no payload pointer when it has no payload type

- `{stream,future}.close-{readable,writable}` have been renamed to
  `{stream,future}.drop-{readable,writable}`

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej dicej force-pushed the new-future-abi branch from 3c2b9c9 to 94313a9 Compare June 6, 2025 18:03
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej dicej force-pushed the new-future-abi branch from ffb6654 to 9f709e8 Compare June 6, 2025 18:19
@dicej dicej added this pull request to the merge queue Jun 6, 2025
Merged via the queue into bytecodealliance:main with commit 95f36d2 Jun 6, 2025
32 checks passed
@dicej dicej deleted the new-future-abi branch June 6, 2025 18:36
dicej added a commit to dicej/wasm-tools that referenced this pull request Jun 6, 2025
This reverts commit 95f36d2.

Alex, Luke, and I decided to scale back these changes.  I'll open a new PR next
week which reflects that discussion.
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.

2 participants