Stream: git-wasmtime

Topic: wasmtime / PR #7029 Start to port Wasmtime to the new was...

Wasmtime GitHub notifications bot (Sep 12 2023 at 23:56):

sunfishcode opened PR #7029 from sunfishcode:resources to bytecodealliance:main:

This imports the new APIs posted in WebAssembly/wasi-io#46.

<!--
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 (Sep 12 2023 at 23:57):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 13 2023 at 00:49):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 13 2023 at 21:51):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 14 2023 at 22:08):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 15 2023 at 00:23):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 15 2023 at 00:32):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 15 2023 at 18:07):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 15 2023 at 18:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2023 at 18:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2023 at 18:48):

alexcrichton created PR review comment:

This is I believe going to return an error at runtime because borrows can't be returned, they can only be taken.

Two ways I can think of to handle this are:

• Don't store preopens as Resource<Descriptor>. Instead insert them into the table every time get-directories is called.
• Alternatively on HostDescriptor::drop don't actually delete preopens from the table. This will "leak" them in the table but that's ok since it's a static set that doesn't grow.

Wasmtime GitHub notifications bot (Sep 15 2023 at 18:48):

alexcrichton created PR review comment:

I think this can all be skipped in favor fo closure_future.await?

poll-list is a bit funky since there's more than one future, but here there's just one so I think it should be ok to do a normal .await

Wasmtime GitHub notifications bot (Sep 15 2023 at 18:48):

alexcrichton created PR review comment:

Bah this is unfortunate (as are all the other instances of this in this file). If I understand this correctly, this is because the bindings generated. have two distinct Descriptor types meaning that they're not compatible?

If so I can try to add a feature to bindgen! so resources can be specified as opposed to always-generated

Wasmtime GitHub notifications bot (Sep 15 2023 at 19:38):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:33):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:33):

sunfishcode created PR review comment:

Yes, exactly. All the sync adapter code has to jump through these hoops for that reason.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:38):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:38):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:38):

sunfishcode created PR review comment:

Done.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:39):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:39):

sunfishcode created PR review comment:

I made it insert them into the table every tine get-directories is called.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:40):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 15 2023 at 20:41):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 18 2023 at 14:47):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:01):

alexcrichton created PR review comment:

Could this perhaps use OnceCell?

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:01):

alexcrichton created PR review comment:

Would you be ok splitting this to a separate PR?

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:01):

alexcrichton created PR review comment:

Could .as_ref().trapping_unwrap() be used instead? (or was it common enough you felt this was nicer?)

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:01):

alexcrichton created PR review comment:

While this works and there's nothing wrong with it, if you're curious this is what drop_in_place is for

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:01):

alexcrichton created PR review comment:

If possible we'll definitely want to avoid bringing this back. Was this required though because of the bindings generated?

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:01):

alexcrichton created PR review comment:

If possible I'd prefer to avoid the usage of UnsafeCell here since otherwise it's very easy to trip into corruption by accident. If with_mut stays gone though then the RefCell layer I think can be removed entirely since it's always returning &T

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:18):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:35):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:49):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:53):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:53):

sunfishcode created PR review comment:

I tried that, but OnceCell pulls in Rust error handling which breaks the adapter build.

Wasmtime GitHub notifications bot (Sep 18 2023 at 22:19):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 22:19):

sunfishcode created PR review comment:

Oh, good idea.

Wasmtime GitHub notifications bot (Sep 18 2023 at 22:22):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 18 2023 at 22:29):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 22:29):

pchickey created PR review comment:

I had gotten rid of State::with_mut in https://github.com/bytecodealliance/wasmtime/pull/6973, is there a reason this PR brought it back that I haven't spotted, or is this just a git history artifact?

Wasmtime GitHub notifications bot (Sep 18 2023 at 22:30):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 18 2023 at 22:34):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 22:34):

sunfishcode created PR review comment:

I've now removed the with_mut and this outer UnsafeCell.

I think the scariest part now is the fn descriptors_mut(&self) -> &mut Descriptors, because I couldn't get RefCell/RefMut to work now that get_input_stream needs to return a &Descriptor instead of a u32 that it can just copy out. I'll poke around at this to see if I can figure out a better option.

Wasmtime GitHub notifications bot (Sep 18 2023 at 23:02):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 23:02):

sunfishcode created PR review comment:

get_input_stream() now needs to return a &Descriptor instead of a Descriptor aka u32. That lifetime constraint appears to make it incompatible with holding the descriptor table in a RefCell. This State::with_mut and the current descriptors(&self) -> &mut Descriptors code are me searching for a way to describe what this code is doing. I don't think I've found the right answer yet.

Wasmtime GitHub notifications bot (Sep 18 2023 at 23:04):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 23:04):

sunfishcode created PR review comment:

Yeah, I can do that. It's convenient for now, because with stderr being a host stream that gets buffered, it's extra important for debugging to have some kind of debug facilty that can dump text directly to the host stderr. But when this PR is closer to landing I can split it out.

Wasmtime GitHub notifications bot (Sep 19 2023 at 14:51):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 19 2023 at 14:51):

alexcrichton created PR review comment:

Hm that seems like an issue with our stderr design then as well, that should definitely be suitable for debugging. We don't have any writes though that we don't push out via tokio though right? Is the issue that it's a slightly deferred write rather than immediate?

Wasmtime GitHub notifications bot (Sep 26 2023 at 21:39):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 26 2023 at 22:47):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 27 2023 at 16:52):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 27 2023 at 17:06):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 27 2023 at 17:37):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 27 2023 at 17:39):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2023 at 17:39):

sunfishcode created PR review comment:

I've now fully removed this.

Wasmtime GitHub notifications bot (Sep 27 2023 at 18:33):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 27 2023 at 18:48):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2023 at 18:48):

sunfishcode created PR review comment:

I've now removed the logging code.

Wasmtime GitHub notifications bot (Sep 27 2023 at 18:48):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2023 at 18:48):

sunfishcode created PR review comment:

I've now removed State::with_mut.

Wasmtime GitHub notifications bot (Sep 27 2023 at 19:15):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 27 2023 at 19:39):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 03:29):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:35):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:57):

alexcrichton submitted PR review:

Ok I've re-reviewed everything here and it all looks good-to-go to me. There's some follow-up cleanups I think but nothing that needs to block this PR :+1:

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:08):

sunfishcode has marked PR #7029 as ready for review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:08):

sunfishcode requested pchickey for a review on PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:08):

sunfishcode requested wasmtime-core-reviewers for a review on PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:08):

sunfishcode requested wasmtime-default-reviewers for a review on PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:27):

pchickey submitted PR review:

Looks good. Some minor stuff I stumbled on, and we'd like to land https://github.com/bytecodealliance/wasmtime/pull/7091 first this afternoon, which will create some minor conflicts.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:27):

pchickey submitted PR review:

Looks good. Some minor stuff I stumbled on, and we'd like to land https://github.com/bytecodealliance/wasmtime/pull/7091 first this afternoon, which will create some minor conflicts.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:27):

pchickey created PR review comment:

Is this still broken? Just want to note it for a follow-up so we don't forget to go back and fix it.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:27):

pchickey created PR review comment:

We can revert the line-oriented changes and move this back to stderr, right?

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:27):

pchickey created PR review comment:

We can run get_stderr().blocking_write_and_flush() here now, correct?

Wasmtime GitHub notifications bot (Sep 28 2023 at 20:56):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:13):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:15):

sunfishcode created PR review comment:

Yes, I've now reverted them.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:15):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:16):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:16):

sunfishcode created PR review comment:

Earlier I had assumed it was failing due to the issue with calling get_stdout multiple times, but it turns out to have been a bug in the poll_oneoff adapter. I've now pushed a patch fixing it.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:16):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:16):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:18):

sunfishcode created PR review comment:

Yep, this is now done.

Wasmtime GitHub notifications bot (Sep 28 2023 at 22:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:47):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:48):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:30):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:31):

sunfishcode updated PR #7029.

Wasmtime GitHub notifications bot (Sep 29 2023 at 20:09):

sunfishcode merged PR #7029.

Last updated: Nov 22 2024 at 16:03 UTC