Stream: git-wasmtime

Topic: wasmtime / PR #6779 Resource table tracks child relations...

Wasmtime GitHub notifications bot (Jul 26 2023 at 23:32):

pchickey opened PR #6779 from bytecodealliance:pch/resource_lifetimes to bytecodealliance:main:

First off, change all TableError cases to trap. This is required by the shift to resources.

<!--
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 (Jul 27 2023 at 00:09):

pchickey updated PR #6779.

Wasmtime GitHub notifications bot (Jul 27 2023 at 00:09):

pchickey has marked PR #6779 as ready for review.

Wasmtime GitHub notifications bot (Jul 27 2023 at 00:09):

pchickey requested alexcrichton for a review on PR #6779.

Wasmtime GitHub notifications bot (Jul 27 2023 at 00:09):

pchickey requested wasmtime-core-reviewers for a review on PR #6779.

Wasmtime GitHub notifications bot (Jul 27 2023 at 00:10):

pchickey updated PR #6779.

Wasmtime GitHub notifications bot (Jul 28 2023 at 00:53):

pchickey updated PR #6779.

Wasmtime GitHub notifications bot (Jul 28 2023 at 01:05):

pchickey edited PR #6779:

First off, change all wasmtime_wasi::preview2::TableError cases to trap when converting to a filesystem::Error, streams::Error, etc. This is required by the shift to resources. No tests fail because of this change!

Introduce the tracking of children to wasmtime_wasi::preview2::Table. This required:

• a new pub method push_child indicating the parent
• Bookkeeping in a private struct TableEntry { entry: Box<dyn Any...>, parent: Option<u32>, children: BTreeSet<u32> }. This is the simplest datastructure that could possibly work.
• deletion checks that an entry has no children, and updates bookkeeping on the parent if the entry has one.
• re-defining OccupiedEntry to not use HashMap's directly, because we n

There is only one child resource in wasmtime-wasi at the moment: a HostPollable::TableEntry is a pollable resource which refers to a parent with the { index: u32, ...}. We changed push_host_pollable method on the table extension trait to push that variant with push_child.

I added a unit test to poll.rs showing that a HostPollable::TableEntry treats the given index as a parent.

I added an integration test to command-tests demonstrating correct management of child lifetimes exits correctly, and incorrect management traps with TableError::HasChildren.

<!--
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 (Jul 28 2023 at 01:05):

pchickey updated PR #6779.

Wasmtime GitHub notifications bot (Jul 28 2023 at 01:06):

pchickey edited PR #6779:

First off, change all wasmtime_wasi::preview2::TableError cases to trap when converting to a filesystem::Error, streams::Error, etc. This is required by the shift to resources. No tests fail because of this change!

Introduce the tracking of children to wasmtime_wasi::preview2::Table. This required:

• a new pub method push_child indicating the parent
• Bookkeeping in a private struct TableEntry { entry: Box<dyn Any...>, parent: Option<u32>, children: BTreeSet<u32> }. This is the simplest datastructure that could possibly work.
• deletion checks that an entry has no children, and updates bookkeeping on the parent if the entry has one.
• re-defining OccupiedEntry to not use HashMap's directly, because we need to handle deletion bookkeeping.

There is only one child resource in wasmtime-wasi at the moment: a HostPollable::TableEntry is a pollable resource which refers to a parent with the { index: u32, ...}. We changed push_host_pollable method on the table extension trait to push that variant with push_child.

I added a unit test to poll.rs showing that a HostPollable::TableEntry treats the given index as a parent.

I added an integration test to command-tests demonstrating correct management of child lifetimes exits correctly, and incorrect management traps with TableError::HasChildren.

<!--
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 (Jul 28 2023 at 01:06):

pchickey requested elliottt for a review on PR #6779.

Wasmtime GitHub notifications bot (Jul 28 2023 at 01:07):

pchickey edited PR #6779:

First off, change all wasmtime_wasi::preview2::TableError cases to trap when converting to a filesystem::Error, streams::Error, etc. This is required by the shift to resources. No tests fail because of this change!

Introduce the tracking of children to wasmtime_wasi::preview2::Table. This required:

• a new pub method push_child indicating the parent
• Bookkeeping in a private struct TableEntry { entry: Box<dyn Any...>, parent: Option<u32>, children: BTreeSet<u32> }. This is the simplest datastructure that could possibly work.
• deletion checks that an entry has no children, and updates bookkeeping on the parent if the entry has one.
• re-defining OccupiedEntry to not use HashMap's directly, because we need to handle deletion bookkeeping.

There is only one child resource in wasmtime-wasi at the moment: a HostPollable::TableEntry is a pollable resource which refers to a parent with the { index: u32, ...}. We changed push_host_pollable method on the table extension trait to push that variant with push_child.

I added a unit test to poll.rs showing that a HostPollable::TableEntry treats the given index as a parent.

I added an integration test to command-tests demonstrating correct management of child lifetimes exits correctly, and incorrect management traps with TableError::HasChildren.

This fixes a crash vector merged as part of https://github.com/bytecodealliance/wasmtime/pull/6556

<!--
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 (Aug 01 2023 at 00:00):

elliottt submitted PR review:

Seems reasonable to me!

Wasmtime GitHub notifications bot (Aug 01 2023 at 00:00):

elliottt submitted PR review:

Seems reasonable to me!

Wasmtime GitHub notifications bot (Aug 01 2023 at 00:00):

elliottt created PR review comment:

    for target_name in targets_in_package(&meta, package, kind) {

Wasmtime GitHub notifications bot (Aug 01 2023 at 00:00):

elliottt created PR review comment:

Great comment!

Wasmtime GitHub notifications bot (Aug 01 2023 at 00:00):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2023 at 00:00):

pchickey created PR review comment:

oh gosh thank you

Wasmtime GitHub notifications bot (Aug 01 2023 at 00:00):

pchickey updated PR #6779.

Wasmtime GitHub notifications bot (Aug 01 2023 at 00:30):

pchickey updated PR #6779.

Wasmtime GitHub notifications bot (Aug 01 2023 at 00:30):

pchickey has enabled auto merge for PR #6779.

Wasmtime GitHub notifications bot (Aug 01 2023 at 01:27):

pchickey updated PR #6779.

Wasmtime GitHub notifications bot (Aug 01 2023 at 04:22):

pchickey merged PR #6779.

Last updated: Oct 23 2024 at 20:03 UTC