Implement Async IO Select Hostcalls #188

mgattozzi · 2022-10-06T19:29:01Z

This commit adds the fastly_async_io hostcalls select and is_ready
to Viceroy. Up to this point we only would do select for things like
PendingRequest. Now we abstract over an AsyncItem which could be a
PendingRequest, StreamingBody, or Body and you can call select
over any of these async io operations. Most of this code is to
accomodate these changes while maintaining the behavior of select for
PendingRequest utilizing these new calls and creating the AsyncItem
enum to hold all of these differing types in the same async_item map.

We also add an integration test similar to the C@E platform's to make
sure that we implement the behavior correctly. With this we can be
confident that select works the way we intend it too! As a bonus side
effect of these changes we also add async support for hosts in our
testing code in Viceroy which gives us some more flexibility in what we
can accomplish or use!

JakeChampion · 2022-11-02T13:15:29Z

I've used this to run the tests for the JS SDK which use the async_io module and everything looks to be working correctly 🙌 -- fastly/js-compute-runtime#293

mgattozzi · 2022-11-04T16:53:37Z

I finally fixed up the PR to work for all items again and the tests should be passing and with it working on the JS SDK side I'm reasonably confident the code works as intended.

aturon · 2022-11-04T19:19:14Z

lib/src/wiggle_abi/async_io_impl.rs

@@ -0,0 +1 @@
+


Looks like an empty file, should be removed.

aturon · 2022-11-04T19:24:57Z

lib/src/wiggle_abi/req_impl.rs

+            .await? as u32;
+
+        let item =
+            self.take_async_item(pending_req_handles.get(done_index).unwrap().read()?.into())?;


Suggested change

self.take_async_item(pending_req_handles.get(done_index).unwrap().read()?.into())?;

self.take_async_item(pending_req_handles[done_index].read()?.into())?;

I thought this would work but turns out pending_req_handles is a &GuestPtr<[PendingReqHandle]> and we can't index directly. I tried out some other things but they were either longer or even more complex for little gain and so I think we need to just keep this unfortunately @aturon

Ah ok, no worries!

aturon

Looks great!

However, there aren't tests of the new async_io functionality. We should be able to re-use the C@E tests directly here, so it should be easy to add. While the existing tests give coverage of select over pending requests, I think it's important to have fuller of the new API.

mgattozzi · 2022-11-07T21:04:58Z

Update for todays work: I'm still working on it. The C@E tests have some functionality around handling when to accept requests as well as synchronizing that's a bit hard to replicate. I've got something started, but it's slow going for now. Hopefully I'll have something by tomorrow as I don't expect to get it done in the next hour before I clock out today.

mgattozzi · 2022-11-15T18:35:24Z

Figured it all out! Squashed all my WIP commits to make the history cleaner!

mgattozzi · 2022-11-15T21:33:08Z

And updated the code to remove uneeded printlns and make sure we reinsert items using a Drop guard

aturon

AWESOME WORK! 🎉

Thank you for seeing this through, it'll be great to get it merged.

aturon · 2022-11-15T22:13:31Z

AWESOME WORK! 🎉

Thank you for seeing this through, it'll be great to get it merged.

This commit adds the fastly_async_io hostcalls `select` and `is_ready` to Viceroy. Up to this point we only would do `select` for things like `PendingRequest`. Now we abstract over an `AsyncItem` which could be a `PendingRequest`, `StreamingBody`, or `Body` and you can call `select` over any of these async io operations. Most of this code is to accomodate these changes while maintaining the behavior of `select` for `PendingRequest` utilizing these new calls and creating the `AsyncItem` enum to hold all of these differing types in the same `async_item` map. We also add an integration test similar to the C@E platform's to make sure that we implement the behavior correctly. With this we can be confident that select works the way we intend it too! As a bonus side effect of these changes we also add async support for hosts in our testing code in Viceroy which gives us some more flexibility in what we can accomplish or use!

cli/tests/integration/async_io.rs

This change refines the comment on why we've cfged out a test on windows with a link to the issue tracker for those curious as to why this is the case. Co-authored-by: Aaron Turon <[email protected]>

aturon

🎉 💯

JakeChampion mentioned this pull request Oct 28, 2022

feature: implement setTimeout, setInterval, clearTimeout, and clearInterval fastly/js-compute-runtime#293

Merged

mgattozzi force-pushed the mgattozzi/async-hostcall branch from daae9ef to 9731ae9 Compare November 4, 2022 16:52

mgattozzi marked this pull request as ready for review November 4, 2022 16:52

mgattozzi requested a review from JakeChampion November 4, 2022 16:52

mgattozzi requested review from a team and awortman-fastly and removed request for a team November 4, 2022 16:53

aturon requested review from aturon and removed request for awortman-fastly November 4, 2022 18:33

aturon reviewed Nov 4, 2022

View reviewed changes

lib/src/wiggle_abi/async_io_impl.rs Outdated

@@ -0,0 +1 @@

Copy link

Contributor

aturon Nov 4, 2022

Choose a reason for hiding this comment

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

Looks like an empty file, should be removed.

aturon reviewed Nov 4, 2022

View reviewed changes

aturon requested changes Nov 4, 2022

View reviewed changes

mgattozzi force-pushed the mgattozzi/async-hostcall branch from 3421a53 to 9fa9420 Compare November 15, 2022 18:27

mgattozzi requested a review from aturon November 15, 2022 18:27

aturon previously approved these changes Nov 15, 2022

View reviewed changes

mgattozzi dismissed aturon’s stale review via 974b0f1 November 16, 2022 16:32

aturon force-pushed the mgattozzi/async-hostcall branch from dcf6737 to 2ec56d1 Compare November 16, 2022 19:11

mgattozzi and others added 4 commits November 16, 2022 15:47

Avoid double-close in async IO test

c63c5b3

Adaptively choose the number of chunks to write in the async IO test

4843a1c

Make async_io test only run on unix

3c9e62b

mgattozzi force-pushed the mgattozzi/async-hostcall branch from 8dc2c95 to 3c9e62b Compare November 16, 2022 20:51

mgattozzi requested a review from aturon November 16, 2022 20:52

aturon mentioned this pull request Nov 16, 2022

Possible streaming body divergence on Windows #207

Open

aturon reviewed Nov 16, 2022

View reviewed changes

cli/tests/integration/async_io.rs Outdated Show resolved Hide resolved

Refine comment of async_io test

12ec538

This change refines the comment on why we've cfged out a test on windows with a link to the issue tracker for those curious as to why this is the case. Co-authored-by: Aaron Turon <[email protected]>

aturon approved these changes Nov 16, 2022

View reviewed changes

mgattozzi merged commit dc9e8d9 into main Nov 16, 2022

mgattozzi deleted the mgattozzi/async-hostcall branch November 16, 2022 22:39

mgattozzi mentioned this pull request Nov 17, 2022

Release Viceroy 0.3.2 #208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Async IO Select Hostcalls #188

Implement Async IO Select Hostcalls #188

mgattozzi commented Oct 6, 2022 •

edited

Loading

JakeChampion commented Nov 2, 2022

mgattozzi commented Nov 4, 2022

aturon Nov 4, 2022

aturon Nov 4, 2022

mgattozzi Nov 7, 2022

aturon Nov 9, 2022

aturon left a comment

mgattozzi commented Nov 7, 2022

mgattozzi commented Nov 15, 2022

mgattozzi commented Nov 15, 2022

aturon left a comment

aturon commented Nov 15, 2022

aturon left a comment

	self.take_async_item(pending_req_handles.get(done_index).unwrap().read()?.into())?;
	self.take_async_item(pending_req_handles[done_index].read()?.into())?;

Implement Async IO Select Hostcalls #188

Implement Async IO Select Hostcalls #188

Conversation

mgattozzi commented Oct 6, 2022 • edited Loading

JakeChampion commented Nov 2, 2022

mgattozzi commented Nov 4, 2022

aturon Nov 4, 2022

Choose a reason for hiding this comment

aturon Nov 4, 2022

Choose a reason for hiding this comment

mgattozzi Nov 7, 2022

Choose a reason for hiding this comment

aturon Nov 9, 2022

Choose a reason for hiding this comment

aturon left a comment

Choose a reason for hiding this comment

mgattozzi commented Nov 7, 2022

mgattozzi commented Nov 15, 2022

mgattozzi commented Nov 15, 2022

aturon left a comment

Choose a reason for hiding this comment

aturon commented Nov 15, 2022

aturon left a comment

Choose a reason for hiding this comment

mgattozzi commented Oct 6, 2022 •

edited

Loading