Stream: git-wasmtime

Topic: wasmtime / PR #10106 add component-model-async/{fused|fut...

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:47):

dicej edited PR #10106.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:48):

dicej edited PR #10106:

This is another piece of #9582 which I'm splitting out to make review easier.

The fused.wast test exercises fused adapter generation for various flavors of
intercomponent async->async, async->sync, and sync->async calls.

The futures.wast and streams.wast tests exercise the various intrinsics
(e.g. stream.read, future.close_writable, etc.) involving futures and
streams.

The remaining changes fill in some TODOs to make the tests pass, plus plumbing
for a few intrinsics which aren't needed for these tests but which set the
foundation for future tests.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:51):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:51):

dicej created PR review comment:

I've reworked this code and added clarifying comments.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:55):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:55):

dicej created PR review comment:

I've refactored the code to no longer use global variables to stash params and results, so this code no longer exists.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:55):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:55):

dicej created PR review comment:

This code no longer exists, so marking this resolved.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:56):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:56):

dicej created PR review comment:

TIL about declarative element segments. I've added a TODO comment.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:57):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:57):

dicej created PR review comment:

I've refactored this to avoid using globals, and have added explanatory comments to the new code.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:59):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 18:59):

dicej created PR review comment:

I've added doc comments to fact.rs to describe the purpose of these intrinsics. Let me know if they need more work.

Wasmtime GitHub notifications bot (Mar 04 2025 at 19:00):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 19:00):

dicej created PR review comment:

The globals are gone :tada:

Wasmtime GitHub notifications bot (Mar 04 2025 at 19:06):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 19:06):

dicej created PR review comment:

The globals are gone, and I've added a bunch of comments; let me know if further clarification is needed.

Wasmtime GitHub notifications bot (Mar 04 2025 at 20:17):

dicej commented on PR #10106:

@alexcrichton I've rebooted this PR based on the latest code in the wasip3-prototyping repo. There are lot more comments in the fused adapter code now; let me know if more are needed or the existing ones are unclear.

This PR now includes two more tests (streams.wast and futures.wast) and the wasmtime-cranelift and wasmtime-environ intrinsic plumbing needed to make them pass. All the intrinsics follow the same pattern, and the Cranelift codegen for them share a lot of code, so I figure putting them all in a single PR might be easiest. Let me know if it's too much.

Wasmtime GitHub notifications bot (Mar 04 2025 at 21:44):

github-actions[bot] commented on PR #10106:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

• fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton submitted PR review:

Thanks for adding these comments!

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

Mind tagging this with #10143?

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

I think this could return &CanonicalAbiInfo::ZERO here

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

Would it be possible to avoid the .clone() and return the &CanonicalAbiInfo here with the change above?

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

To avoid a possible mismatch where this doesn't actually match teh value of call below, could the results of the call be introspected with Cranelift APIs to learn the type?

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

Mind documenting this parameter above? (perhaps documenting that it's currently-unused but soon-to-be-used as well)

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

Mind adding a comment here clarifying that this is just an optimization and it's always valid to return None here?

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

How come Send popped up here and Send/Sync above?

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

Or, somewhat alternatively, I'm a bit uneasy having the return type imply the meaning of the value of the return in terms of whether it traps or not. Another option would be to thread around TrapSentinel perhaps? (or something akin to that)

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

While you're here mind extracting this fn(...) -> ... signature to something like type GetLibcallFn = fn(...) -> ... and replacing the signatures throughout this file with get_libcall: GetLibcallFn?

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

If you can comment pointing to a location in the fact trampoline bits to where these constants come from (and vice versa ideally) that'd be good to keep the two in sync. (and/or extracting this to constants or similar)

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

Similar to down below handling these canonical ABI arguments I think would be good to have as helper functions

Wasmtime GitHub notifications bot (Mar 04 2025 at 22:08):

alexcrichton created PR review comment:

Processing/loading these two values is done here and a few times below, mind extracting them to a helper?

Wasmtime GitHub notifications bot (Mar 04 2025 at 23:19):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 23:19):

dicej created PR review comment:

Would you mind sketching out how to introspect the result type from the Cranelift APIs? The surface area is pretty huge, and I'm not sure what to search for.

Wasmtime GitHub notifications bot (Mar 04 2025 at 23:19):

dicej commented on PR #10106:

Thanks for the review, @alexcrichton! I'll address your feedback first thing tomorrow.

Wasmtime GitHub notifications bot (Mar 04 2025 at 23:42):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 23:42):

alexcrichton created PR review comment:

You'll want value_type the method, like this -- https://github.com/bytecodealliance/wasmtime/blob/132a490d72e6381c593a409aeca50a765b54ca28/crates/cranelift/src/translate/table.rs#L66 (there's a number of hits you can explore throughout crates/cranelift too, this is just a random one)

Wasmtime GitHub notifications bot (Mar 05 2025 at 15:12):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 05 2025 at 15:12):

dicej created PR review comment:

Yes, I was able to do this, but I ended up needing to clone it at the call site anyway to avoid conflicting borrows of self (which perhaps could be resolved via some kind of split borrow approach with heavy refactoring, but doesn't seem worth it in this case).

Wasmtime GitHub notifications bot (Mar 05 2025 at 15:13):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 05 2025 at 15:13):

dicej created PR review comment:

I'll revert the changes to this file; they're not needed (yet).

Wasmtime GitHub notifications bot (Mar 05 2025 at 16:48):

dicej updated PR #10106.

Wasmtime GitHub notifications bot (Mar 05 2025 at 16:49):

dicej has enabled auto merge for PR #10106.

Wasmtime GitHub notifications bot (Mar 05 2025 at 17:23):

dicej merged PR #10106.

Wasmtime GitHub notifications bot (Mar 06 2025 at 17:44):

FrankReh submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2025 at 17:44):

FrankReh created PR review comment:

"will block"

Wasmtime GitHub notifications bot (Mar 06 2025 at 17:53):

FrankReh submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2025 at 17:53):

FrankReh created PR review comment:

to be exported to the host which host to lift the parameters ... maybe correct but I didn't know how to parse that part of the sentence.

Wasmtime GitHub notifications bot (Mar 06 2025 at 17:57):

FrankReh submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2025 at 17:57):

FrankReh created PR review comment:

the the

Wasmtime GitHub notifications bot (Mar 06 2025 at 18:02):

FrankReh submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2025 at 18:02):

FrankReh created PR review comment:

exported to the host which host to lift ... apologies if this makes sense to folks more in the know, maybe just hard for me to decipher

Last updated: Apr 17 2025 at 07:03 UTC