wasmtime / PR #5288 Function references · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5288 Function references

Wasmtime GitHub notifications bot (Nov 17 2022 at 04:42):

CosineP opened PR #5288 from function-references to main:

This patch, written by @dhil and I, implements the (complete) function references proposal, forming the wasmtime side of the existing pull request bytecodealliance/wasm-tools#701. It is missing some obvious things:

call_ref is simply translated as a cranelift indirect_call. A faster cranelift indirect call that does not check the callee signature can (and should) eventually be added and used.

The wasm-tools PR has advanced past the version we depend on here. We hope to update all at once, once the wasm-tools side has fully settled. This will result in the removal of a number of ValType::Bot branches, the addition of the type annotation on call_ref, and a handful of other convenient things. The version we depend on is called func-ref-2, if you need to reference it*.

Wasmtime, too, has of course advanced past our parent. I tried to rebase on the latest changes, but due to particular Cargo.toml changes, I think this will be much easier when we've retargetted to the latest wasm-tools#701 and can keep our dependencies consistent in the history

Otherwise, things should be mostly obvious. Typed function references are represented by the existing untyped function references. Even though it's held up by wasm-tools, we ended up with a fairly polished changeset that we figured we could send out and get feedback on. There's no rush on our end, as we've been happily using this in our typed continuations work for some time now. But we're happy to discuss any changes, problems, or suggestions!

\* We added a public peek method to the validator to attain a signature for call_ref. This is now unnecesary because call_ref comes with a type annotation, and will disappear fairly conveniently.

Wasmtime GitHub notifications bot (Nov 17 2022 at 04:45):

CosineP edited PR #5288 from function-references to main:

This patch, written by @dhil and I, implements the (complete) function references proposal, forming the wasmtime side of the existing pull request bytecodealliance/wasm-tools#701. It is missing some obvious things:

call_ref is simply translated as a cranelift indirect_call. A faster cranelift indirect call that does not check the callee signature can (and should) eventually be added and used. Further, no null check is elided.

The wasm-tools PR has advanced past the version we depend on here. We hope to update all at once, once the wasm-tools side has fully settled. This will result in the removal of a number of ValType::Bot branches, the addition of the type annotation on call_ref, and a handful of other convenient things. The version we depend on is called func-ref-2, if you need to reference it*.

Wasmtime, too, has of course advanced past our parent. I tried to rebase on the latest changes, but due to particular Cargo.toml changes, I think this will be much easier when we've retargetted to the latest wasm-tools#701 and can keep our dependencies consistent in the history

Otherwise, things should be mostly obvious. Typed function references are represented by the existing untyped function references. Even though it's held up by wasm-tools, we ended up with a fairly polished changeset that we figured we could send out and get feedback on. There's no rush on our end, as we've been happily using this in our typed continuations work for some time now. But we're happy to discuss any changes, problems, or suggestions!

\* We added a public peek method to the validator to attain a signature for call_ref. This is now unnecesary because call_ref comes with a type annotation, and will disappear fairly conveniently.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

This feels sort of wrong in the sense that R64 is going to be used to represent a family of types so it's not necessarily possible to go backwards from a cranelift type to a wasm type (like could be done historically). Reading over this thought this function is only used for ModuleTranslationState::from_func_sigs which I think is otherwise dead code. I opened https://github.com/bytecodealliance/wasmtime/pull/5290 to remove this code which should help address this.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

Could this perhaps reuse the IndirectCallToNull? While not technically the exact same error conditions it still captures the case that this comes up with call_ref to a null table entry.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

It's ok to skip this attribute, we don't run clippy in this repository on CI or such.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

I think that this is also dead code, which I've now added to https://github.com/bytecodealliance/wasmtime/pull/5290 to get removed.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

FWIW I think now that CallRef carries its type payload the need for ty: Option<ValType> will probably go away, although this won't otherwise change significantly I suspect.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

FWIW this is one of the reasons that I preferred to keep Bot out of the public API of wasmparser since it prevents consumers like this from having to have panic!(...) branches for something that shouldn't ever be reachable.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

Actually if we wanted to get _super_ clever here (which is totally ok if you'd rather not) I think that this check can be entirely skipped. The .load() below will segfault if the callee is a null pointer which we can catch in a signal handler to report a trap, which means that technically speaking we can skip the check entirely here. I think the codegen may need to get changed slightly since the MemFlags::trusted used when loading the func_addr may or may not be right (a Cranelift expert would want to weigh in).

Anyway just a tiny optimization that probably doesn't matter too much in the grand scheme of things, and more than ok to defer to later.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

For this I think it's fine to generate a Func for a typed function index. There's no real way to propagate the type runtime type information into the Rust type system so I think it's best to return Func here and then embedders can used .typed() as today to get the typed view.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

I think it's ok to leave this as-is, and if it's ever a perf-issue we can always reach for a SmallVec here or something like that.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

Should This also match WasmHeapType::Index(_)?

(if so could a test be added too?)

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

I think that the translation here is actually almost-perfect. The call_indirect instruction in Cranelift isn't the same as the one in wasm and doesn't perform any type checks or anything, it's as raw as it gets. In that sense the performance of call_ref is only mildly hampered where if the callee is known to be non-null a null-check still happens. Otherwise though I think that the generation here should be optimal.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

I think definitely for the case of the the wasmtime crate's public API we'll want to hide Bot, even if it ends up getting exposed in wasmparser.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

I went ahead an opened https://github.com/bytecodealliance/wasmtime/issues/5291 since this applies to call_indirect as well.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

Should Index be matched here as well?

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

In retrospect, I think this might be better modeled as perhaps:
impl ValType {
    pub const EXTERNREF = ...;
}

impl WasmRefType {
    pub const EXTERN: WasmRefType = ...;
}
basically just using associated consts to namespace things instead of splatting it into the module namespace here.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

I think that this condition isn't quite right, you'll probably want to match on the heap_type here and explicitly reject externref but both typed functions and normal functions should be able to fall through.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

This won't actually be stored as a field in Config but rather self.features, so this field can be omitted from the Wasmtime crate.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

This'll want to be omitted I believe until the proposal is stabilized.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

As a bit of a bikeshed, perhaps RefType instead of Wasm*?

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

I think that this match is going to need to grow more cases or otherwise embedders can't interact with typed function tables in the embedder API. That's ok for a first pass and can be deferred to a future PR but it's functionality we'll eventually want to fill out.

Wasmtime GitHub notifications bot (Nov 17 2022 at 16:22):

alexcrichton created PR review comment:

At the layer of the Wasmtime embedding here it's unfortunately not longer suitable to use a u32 here since there's not actually any way to translate this u32 to an actual type to inspect.

This actually raises a particularly interesting embedding question for me, though, which is how an embedder is expected to invoke an exported function from a wasm module which takes a typed function reference as an argument (or returns such a result).

I can talk more with y'all if you'd like for something like this, but the u32 here more-or-less needs to become something like a FuncType, but not literally that given how lightweight ValType is supposed to be relative to hwo heavyweight FuncType is. We'll probably want some form of indexing or such but this is moving into the realm of relatively large-ish changes which y'all may want to defer to later if possible.

Wasmtime GitHub notifications bot (Nov 17 2022 at 20:56):

fitzgen created PR review comment:

This is due to some vestigial bits from when we had extended basic blocks instead of plain basic blocks. Because we have basic blocks, we can't add new non-block-terminating instructions after a brz. But also we don't currently have a two-way conditional branch instruction, so blocks ending in control flow splits have to end with something like brz L1; jump L2 instead. We have general plans to clean this stuff up in the not super distant future, but thats the state of affairs for now.

Wasmtime GitHub notifications bot (Nov 17 2022 at 20:56):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2022 at 20:56):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Nov 17 2022 at 21:08):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2022 at 21:08):

cfallin created PR review comment:

Indeed, we would want to remove the trusted flag here, if the load can trap, and this would require a bit more plumbing to emit the right trap code associated with this load's PC (it's no longer a heap-out-of-bounds). But this is probably an O(tens of lines)-sized change, not too bad. Happy to dig a bit and point out specific bits if you want to do this in a followup PR.

Wasmtime GitHub notifications bot (Nov 17 2022 at 21:35):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2022 at 21:35):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 17 2022 at 21:35):

fitzgen created PR review comment:

Should this case be unreachable given that we've checked that the instruction is valid? If so, then this can be an unreachable!() instead. If not, then it should return a proper error instead of panicking.

Wasmtime GitHub notifications bot (Nov 17 2022 at 21:35):

fitzgen created PR review comment:

lol not the best situation, yes, but this is the one that is generally public-facing rather than an implementation detail, so it should really have Proper(tm) docs with punctuation and a helpful description and all that.

Wasmtime GitHub notifications bot (Nov 17 2022 at 21:35):

fitzgen created PR review comment:

Following up on Alex's question: do the spec tests for typed function references not exercise tables of typed function references (and all the various table operations on them like table.fill)? If so, that seems like something where it is worth sending a PR upstream for or at least filing an issue.

Wasmtime GitHub notifications bot (Nov 17 2022 at 21:35):

fitzgen created PR review comment:

Why is this a TryFrom impl instead of a From impl? It appears to be infallible.

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

CosineP submitted PR review.

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

CosineP created PR review comment:

Fortunately this whole expression disappears on merge because call_ref has gained a proper type annotation

Wasmtime GitHub notifications bot (Dec 06 2022 at 00:34):

CosineP submitted PR review.

Wasmtime GitHub notifications bot (Dec 06 2022 at 00:34):

CosineP created PR review comment:

Oh gosh I'm sorry haha, I guess this somehow made it through the "rebase and sanitize 12am thoughts" process

Wasmtime GitHub notifications bot (Dec 06 2022 at 00:34):

CosineP updated PR #5288 from function-references to main.

Wasmtime GitHub notifications bot (Dec 06 2022 at 16:22):

CosineP submitted PR review.

Wasmtime GitHub notifications bot (Dec 06 2022 at 16:22):

CosineP created PR review comment:

Should the pre-existing WasmType be renamed as well?

Wasmtime GitHub notifications bot (Dec 06 2022 at 16:24):

CosineP updated PR #5288 from function-references to main.

Wasmtime GitHub notifications bot (Feb 23 2023 at 16:22):

dhil updated PR #5288 from function-references to main.

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

Stray change?

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

I think this may need to be null_reference since I think this is used during parsing of the *.clif IR which wouldn't work with a space in the middle I think

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

This file may have snuck in during the merge?

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

I think this file snuck back in from the recent merge?

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

I think this can be written as u32::from(*type_idx) to be a bit more concise.

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

Could the nullable field here be filled out based on whether the value is Some or None?

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

These two values are showing up quite a lot in tests, so I think it might be good to refactor this to have, at the definition:
impl RefType {
    pub const EXTERNREF: RefType = ...;
    pub const FUNCREF: RefType = ...;
}
and probably similarly for ValType

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

I think this will want to match nullable: true since storing a null funcref into a table that is non-nullable should be an error

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

I'll bump this again, this is a serious issue which must be addressed before the PR is merged. As I mentioned before though this has far-reaching implications and is unfortunately not going to be easy to handle. For example the PartialEq/Eq implementations for this type (and transitively for ValType) aren't valid any more to go through u32 since two different modules could have the same u32 pointing at different types.

Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):

alexcrichton created PR review comment:

To bump this, I think it would be good to have at least have a specific error for HeapType::Index which indicates that support is missing.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:37):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:37):

dhil created PR review comment:

Yes! Thank you.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:38):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:38):

dhil created PR review comment:

Yep! I'll remove it.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:38):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:38):

dhil created PR review comment:

Got it!

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:39):

dhil created PR review comment:

Yep, it looks that way. I quite possibly missed "a deleted by them" during the merge.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:39):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:39):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:39):

dhil created PR review comment:

Thanks!

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:39):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:39):

dhil created PR review comment:

What would be the right place to store this shared logic for testing?

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:39):

dhil edited PR review comment.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:42):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:42):

dhil created PR review comment:

Yep, it is technically possible since it is valid to use a non-nullable reference in place of a nullable reference. However, it does come with a cost -- which I think is inevitable anyways -- which is that we must perform a subtype check where we would previously perform a standard equality == check.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:42):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:42):

dhil created PR review comment:

Good catch! Thanks. I will add a couple of test cases.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:43):

dhil updated PR #5288 from function-references to main.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:50):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:50):

dhil created PR review comment:

So would we need to equip the type with some sort of provenance that can inform us about the origin module?

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:51):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 12:51):

dhil created PR review comment:

Added, thanks!

Wasmtime GitHub notifications bot (Feb 27 2023 at 13:51):

dhil updated PR #5288 from function-references to main.

Wasmtime GitHub notifications bot (Feb 27 2023 at 13:55):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Feb 27 2023 at 13:55):

dhil created PR review comment:

It now reads:

if self.module.table_plans[segment.table_index]
                .table
                .wasm_ty
                .heap_type
                == WasmHeapType::Extern
            {
                leftovers.push(segment.clone());
                continue;
            }

Is this close enough to what you had in mind? Alternatively, we can match explicitly on heap_type to make this code more robust against changes to WasmHeapType.

Wasmtime GitHub notifications bot (Feb 27 2023 at 13:56):

dhil edited PR review comment.

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton created PR review comment:

I think this may also need s/ /_/?

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton created PR review comment:

This is a "dummy" implementation anyway so this representation is fine FWIW (should be ok to drop the comment here)

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton created PR review comment:

I think these comments may be leftover from prior refactorings? (and some below this as well)

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton created PR review comment:

I think this may not be correct in the sense that EXTERNREF is "nullable ref extern" but non-nullable externs should still use this code path which does reference counting.

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton created PR review comment:

It's ok to drop this as we don't have a ton of rhyme or reason to the current opcode ordering at this time.

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton created PR review comment:

Like above I think this needs to handle non-nullable externrefs here as well to destroy them properly

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton created PR review comment:

This makes sense to me to add, but can PartialEq and Eq be removed from ValType as well? (to force callers to use this function)

Wasmtime GitHub notifications bot (Feb 28 2023 at 19:09):

alexcrichton created PR review comment:

More-or-less yeah. In some sense this "needs" to become FuncType which internally has a params/results list. I don't think we can literally do that though because of the performance ramifications (e.g. that's an owned list, requires extra allocations to move around, etc).

An alternative would be something akin to VMSharedSignatureIndex which is a unique 32-bit integer deduplicating function types within an Engine. That wouldn't work literally as well either though because indices different engines can't be compared. Additionally this wouldn't work because when performing a subtype check you'd need to be able to walk through the indices to perform a deep subtyping check on params/results of functions.

One possibility would be to perhaps:

Assign a unique 32-bit id to each Engine (similar to how a store gets an id today)

Provide an Engine in the subtyping check

Store an engine id and a shared signature index here.

Even with all this though that's still unfortunately pretty expansive and I'm not sure whether it would work out. So with all that in mind I would propose an alternative. I suspect that the modules you're interested in running don't use funcrefs "at the boundary" in the sense of imported/exported functions from the module that the host interacts with. In that sense I think it's fine to just fail to compile any module that has a funcref/etc in imports/exports in Wasmtime for now. Filling out support for that is fine to have as a "TODO" at some point.

Basically my point is that I think it's fine to implement the "runtime" half of the function-references proposal without implementing the "embedder half". It means Wasmtime won't be able to run every module under the sun that's valid for function-references but it'll probably still run at least the most interesting ones. If you decide to go down this route, though, it will involve a number of changes to the wasmtime crate, namely to basically back out any function-references support and to thread errors through from "when function references are encountered at the import/export level an error is returned"

Wasmtime GitHub notifications bot (Mar 01 2023 at 13:57):

dhil updated PR #5288 from function-references to main.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:00):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:00):

dhil created PR review comment:

I will tidy this up.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:00):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:00):

dhil created PR review comment:

Ok, I will remove the todo.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:00):

dhil created PR review comment:

Yes!

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil created PR review comment:

Ok, thanks!

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil created PR review comment:

Got it!

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil created PR review comment:

Yes, good catch!

Wasmtime GitHub notifications bot (Mar 01 2023 at 15:01):

dhil created PR review comment:

Interesting suggestion. Doing this revealed some potential bugs. It did require me to change the assert_eq! in the func and host_func tests as they extensively relied on the ability to use the PartialEq operator for comparisons.

There is one place where I am not entirely sure that we want to do subtyping: in wasi-threads used a strict equality check; I have preserved this for now because I don't know enough about what's going on there.

Wasmtime GitHub notifications bot (Mar 03 2023 at 13:42):

dhil updated PR #5288 from function-references to main.

Wasmtime GitHub notifications bot (Mar 03 2023 at 14:05):

dhil submitted PR review.

Wasmtime GitHub notifications bot (Mar 03 2023 at 14:05):

dhil created PR review comment:

It sounds like to me it might be worth to survey the design space a little more before committing to implementing a particular design. I think what I would like to do is to separate the work into a "runtime half" PR and at a later stage an "embedder half" PR. It sounds less risky to me personally, and I think you are right, that for my research I do not yet need things to work at the boundary.

Wasmtime GitHub notifications bot (Mar 06 2023 at 15:12):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2023 at 15:12):

alexcrichton created PR review comment:

Yeah that's reasonable to me and I think would work out well. You may have to add a few more error-paths in places that may be infallible but that's ok, at the top-level I think everything is fallible to communicate the necessary errors here so it'd just be internal refactoring.

Wasmtime GitHub notifications bot (Apr 13 2023 at 07:36):

dhil requested wasmtime-compiler-reviewers for a review on PR #5288.

Wasmtime GitHub notifications bot (Apr 13 2023 at 07:36):

dhil requested abrown for a review on PR #5288.

Wasmtime GitHub notifications bot (Apr 13 2023 at 07:36):

dhil requested alexcrichton for a review on PR #5288.

Wasmtime GitHub notifications bot (Apr 13 2023 at 07:36):

dhil requested wasmtime-core-reviewers for a review on PR #5288.

Wasmtime GitHub notifications bot (Apr 13 2023 at 07:36):

dhil requested wasmtime-default-reviewers for a review on PR #5288.

Wasmtime GitHub notifications bot (Apr 13 2023 at 07:36):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 13 2023 at 07:50):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 17 2023 at 08:16):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 18 2023 at 07:27):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 18 2023 at 16:54):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 19 2023 at 11:37):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 19 2023 at 13:32):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 19 2023 at 13:34):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 20 2023 at 07:37):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 20 2023 at 15:26):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 20 2023 at 15:31):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 25 2023 at 13:18):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (Apr 27 2023 at 08:18):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 03 2023 at 09:54):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 03 2023 at 09:55):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 16 2023 at 08:28):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 17 2023 at 12:20):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 19 2023 at 11:40):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 22 2023 at 11:23):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 23 2023 at 11:50):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 23 2023 at 12:08):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 24 2023 at 06:48):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 24 2023 at 14:47):

alexcrichton submitted PR review:

Ok this looks good to go to me, thanks again @CosineP and @dhil!

I'd like another pair of eyes on this though given what's changing here, so I'm going to request that @fitzgen take a look as well. If y'all would be ok opening up a tracking issue for the remaining items that'd be appreciated!

Wasmtime GitHub notifications bot (May 24 2023 at 14:47):

alexcrichton requested fitzgen for a review on PR #5288.

Wasmtime GitHub notifications bot (May 25 2023 at 17:22):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 25 2023 at 19:46):

fitzgen submitted PR review:

Let's get this merged! Thanks @dhil and @CosineP!

Wasmtime GitHub notifications bot (May 25 2023 at 19:46):

fitzgen submitted PR review:

Let's get this merged! Thanks @dhil and @CosineP!

Wasmtime GitHub notifications bot (May 25 2023 at 19:46):

fitzgen created PR review comment:

Can we file an issue to track this?

Wasmtime GitHub notifications bot (May 25 2023 at 19:46):

fitzgen created PR review comment:

This function's allocations were previously showing up in DHAT for some profiling I did, which is why we ended up wit the super generic interface to this function and weird &'static [_] stuff going on. All that is to say that I think we should probably use a small vec here.

Fine to do as a follow up.

Wasmtime GitHub notifications bot (May 26 2023 at 08:21):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 26 2023 at 08:31):

dhil created PR review comment:

I've added this to #6455. I can hoist it into a separate issue if need be.

Wasmtime GitHub notifications bot (May 26 2023 at 14:59):

dhil updated PR #5288.

Wasmtime GitHub notifications bot (May 26 2023 at 15:08):

alexcrichton has enabled auto merge for PR #5288.

Wasmtime GitHub notifications bot (May 26 2023 at 16:06):

alexcrichton merged PR #5288.

Last updated: Nov 22 2024 at 17:03 UTC