Stream: git-wasmtime

Topic: wasmtime / PR #5450 Make DataFlowGraph::insts public, but...

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:03):

elliottt opened PR #5450 from trevor/public-insts-field to main:

We have some operations defined on DataFlowGraph purely to work around borrow-checker issues with InstructionData and other data on DataFlowGraph. Part of the problem is that indexing the DFG directly hides the fact that we're only indexing the insts field of the DFG.

This PR makes the insts field of the DFG public, but wraps it in a newtype that only allows indexing. This means that the borrow checker is better able to tell when oeprations on memory held by the DFG won't conflict, which comes up frequently when mutating ValueLists held by InstructionData.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:25):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:25):

jameysharp created PR review comment:

Pre-existing: I think this function leaks the portion of the pool corresponding to old_actuals. Doesn't need to be fixed in this PR, I just wanted to note it.

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:25):

jameysharp created PR review comment:

I keep being confused about what promises we make about serialization. I assume inserting a newtype wrapper changes the derived serialization format. Do we care? I think we don't care because anything we might be serializing is only meant to be stable within the same major version of Wasmtime, but I don't know.

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:25):

jameysharp created PR review comment:

I suppose this case can use the IndexMut implementation on the newtype wrapper, instead of needing to dig inside it. I guess I have a mild preference for using the public API where that's reasonable.

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:25):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:25):

jameysharp created PR review comment:

This change makes me so much happier :grin:

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:39):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:39):

elliottt created PR review comment:

I was just digging into this, and found that this function handles cases where the arguments aren't contained in an entity list as well.

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:39):

elliottt created PR review comment:

Should we open an issue?

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:39):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 23:42):

elliottt updated PR #5450 from trevor/public-insts-field to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:01):

elliottt updated PR #5450 from trevor/public-insts-field to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:12):

elliottt created PR review comment:

https://github.com/bytecodealliance/wasmtime/issues/5451

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:12):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:18):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:18):

elliottt created PR review comment:

I think I had a recency bias issue and thought you were referring to refactoring arguments_mut instead of removing the unnecessary use of .0. I'll agree that this is better without the .0 :+1:

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:38):

elliottt updated PR #5450 from trevor/public-insts-field to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:38):

elliottt has marked PR #5450 as ready for review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:38):

elliottt requested fitzgen for a review on PR #5450.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:38):

elliottt requested cfallin for a review on PR #5450.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:39):

elliottt created PR review comment:

@cfallin or @fitzgen, do we make any guarantees about the stability of the serialized format for these structures between releases?

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:39):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:56):

elliottt edited PR #5450 from trevor/public-insts-field to main:

We have some operations defined on DataFlowGraph purely to work around borrow-checker issues with InstructionData and other data on DataFlowGraph. Part of the problem is that indexing the DFG directly hides the fact that we're only indexing the insts field of the DFG.

This PR makes the insts field of the DFG public, but wraps it in a newtype that only allows indexing. This means that the borrow checker is better able to tell when operations on memory held by the DFG won't conflict, which comes up frequently when mutating ValueLists held by InstructionData.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:36):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:36):

cfallin created PR review comment:

Nope, this feeds

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:36):

cfallin edited PR review comment.

Wasmtime GitHub notifications bot (Dec 16 2022 at 18:44):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 18:46):

elliottt merged PR #5450.

Last updated: Nov 22 2024 at 16:03 UTC