Stream: git-wasmtime

Topic: wasmtime / PR #3019 Start a high-level architecture docum...

Wasmtime GitHub notifications bot (Jun 22 2021 at 18:54):

alexcrichton opened PR #3019 from architecture to main:

This commit cleands up some existing documentation by removing a number
of "noop README files" and starting a high-level overview of the
architecture of Wasmtime. I've placed this documentation under the
contributing section of the book since it seems most useful for possible
contributors.

I've surely left some things out in this pass, and am happy to add more!

<!--

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 (Jun 22 2021 at 19:08):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 22 2021 at 19:08):

bjorn3 created PR review comment:

*an

Wasmtime GitHub notifications bot (Jun 22 2021 at 19:09):

bjorn3 created PR review comment:

I can't parse this sentence.

Wasmtime GitHub notifications bot (Jun 22 2021 at 19:09):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:14):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:14):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:14):

fitzgen created PR review comment:

This type contains handle-types which refer back to the Store

What is "this type" here?

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:14):

fitzgen created PR review comment:

  of related WebAssembly objects. This includes instances, globals, memories, tables,

Maybe also mention somewhere around here that objects from different stores can't, in general, interact with each other?

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:14):

fitzgen created PR review comment:

"as well as the allocate"? Should this be something like

The deallocate method is different in these two paths (as well as the allocate method).

?

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:14):

fitzgen created PR review comment:

I kinda feel like this last bit about safety in the private deps can be removed.

It isn't clear to me who this is written for, and who would benefit from reading this. On the one hand, users of Wasmtime don't really need to know about internals and where the safe vs unsafe line is drawn. On the other hand, contributors should (perhaps with guidance from reviewers) always strive for "proper" safety design/annotations, even if the current code doesn't fully live up to that in some area. I think we could expand / make explicit this second point, but what is written here seems like it will

• unnecessarily scare users of wasmtime, and
• let new contributors think that it is "okay" to be lax about safety boundaries because they've picked up the impression that everyone else is being lax too.

I don't think we intend for either of those things.

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:14):

fitzgen created PR review comment:

An externref is actually VMExternRef, not *mut VMExternRef, and VMExternRef is a transparent newtype over *mut VMExternData.

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:14):

fitzgen created PR review comment:

Maybe worth having a paragraph about wasmtime::Engine right before wasmtime::Store.

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:56):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 23 2021 at 18:56):

alexcrichton created PR review comment:

My goal with this was to document the state of reality. The reality is that every time I look at wastime-runtime I see more bugs of "oh well this isn't marked unsafe but it should be". I fully agree this isn't a great reality but I also don't want to lure anyone into a false sense of security with a false reality.

Perhaps I could reword this to try to indicate this better?

Wasmtime GitHub notifications bot (Jun 23 2021 at 19:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 23 2021 at 19:11):

cfallin created PR review comment:

A note on this and other README.md deletions: there was a recent PR #2987 that added READMEs where missing so that a downstream packager could satisfy distro requirements. I think we should probably keep these around, even if they are somewhat trivial one-sentence descriptions, as it's fairly low-cost. (If the concern is that the docs could distract from the "real" architecture doc, perhaps we could include a pointer?)

Wasmtime GitHub notifications bot (Jun 23 2021 at 21:32):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 23 2021 at 21:32):

alexcrichton created PR review comment:

From that PR it's not clear to me at least if the requirement was that README.md must be present or "the one in the source isn't present in cargo package". Do you know if it's one of those two?

I personally find no use for one-liner readmes that explain nothing beyond the title of the crate, and I personally find it to be a worse experience not seeing a README in the first place and getting my hopes up that one exists. It's similar to the feeling of a function's documentation not explaining anything beyond the name of the function.

I also find that these sorts of README.md files which aren't at the root level of a repository are perennially neglected. If we really want to add documentation to these crates I think we should do it in a centralized location (the book) and more technical details would go in the source itself (e.g. a large crate doc comment)

Wasmtime GitHub notifications bot (Jun 23 2021 at 21:49):

cfallin created PR review comment:

Ah! No, I think you're right, the issue looks like it was actually a metadata problem, i.e. already existing README not included in the package. So if that's the case, and packagers don't see an issue, I'm totally happy to get rid of them. (cc @olivierlemasle to confirm?)

Wasmtime GitHub notifications bot (Jun 23 2021 at 21:49):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 23 2021 at 22:12):

olivierlemasle submitted PR review.

Wasmtime GitHub notifications bot (Jun 23 2021 at 22:12):

olivierlemasle created PR review comment:

:+1: Yes, my objective was to include the existing READMEs, but no README is not an issue for me.

Wasmtime GitHub notifications bot (Jun 24 2021 at 15:01):

alexcrichton updated PR #3019 from architecture to main.

Wasmtime GitHub notifications bot (Jun 24 2021 at 15:04):

alexcrichton updated PR #3019 from architecture to main.

Wasmtime GitHub notifications bot (Jun 24 2021 at 23:27):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2021 at 23:27):

abrown created PR review comment:

Additionally at this time the safe/unsafe boundary between Wasmtime's internal

Wasmtime GitHub notifications bot (Jul 02 2021 at 14:02):

alexcrichton updated PR #3019 from architecture to main.

Wasmtime GitHub notifications bot (Jul 02 2021 at 14:02):

alexcrichton merged PR #3019.

Last updated: Nov 22 2024 at 17:03 UTC