wasmtime / PR #14300 Cranelift: box a few fields in `Mach... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #14300 Cranelift: box a few fields in `Mach...

Wasmtime GitHub notifications bot (Sep 08 2026 at 17:53):

cfallin opened PR #14300 from cfallin:machbuffer-avoid-moves to bytecodealliance:main:

Issue initially raised by alexcrichton at last week's Cranelift meeting: we have SmallVecs that are (ironically) quite large in MachBuffer, and the "finalized" split causes significant memmoves when the MachBufferFinalized is put together with pieces of the MachBuffer during finalization.

The initial design intent (six years ago!) was to avoid allocations for the common case of a small function compilation: a MachBuffer would contain enough buffer space for the machine code + metadata for anything up to, say, a kilobyte of machine code. But that benefit was lost when we moved to the "finalization" design.

The split itself is necessary: it captures an algorithmic finalization step, and that data that we have before and after differs. (Specifically, we translate label indices into buffer offsets inside of several record types; those are different types, we shouldn't type-pun them, and we have to pass through all the data anyway so those vecs are not a simple bulk data move.)

However many of the metadata arrays are simple passthroughs: for example, the machine-code buffer itself.

This is a simple/mechanical refactor that puts fields that don't change type during finalization in a MachBufferInner, puts that in a Box, and then holds it from either the MachBuffer or MachBufferFinalized. Thus the finalization can move one pointer over instead of kilobytes of buffers.

On a quick Sightglass run with default.suite, I see one benchmark's compilation time move:
compilation :: cycles :: spidermonkey-markdown

    Δ = 16405327.75 ± 16285800.71 (confidence = 99%)

    modified.so is 1.00x to 1.02x faster than base.so!

    ┌────────────┬────────────┬───────────────┬────────────┬─────────────┐
    │ Min        │ Max        │ Mean          │ Median     │ Engine      │
    ├────────────┼────────────┼───────────────┼────────────┼─────────────┤
    │ 1258771422 │ 1408382397 │ 1333799348.93 │ 1343472392 │ base.so     │
    ├────────────┼────────────┼───────────────┼────────────┼─────────────┤
    │ 1254241200 │ 1405715279 │ 1317394021.18 │ 1313587478 │ modified.so │
    └────────────┴────────────┴───────────────┴────────────┴─────────────┘
so a mean ~1% speedup.

There is definitely further work to do to try to reuse allocations across compilations by holding something in the Context, as we do with e.g. regalloc2::Ctx. The complication there is that MachBuffer is monomorphized on I because it holds I::LabelUses and those can differ between architectures; and cranelift_codegen::Context can be used to recompile for different ISAs with each compile invocation, so the actual type can necessarily differ between invocations. I suppose we could collapse all individual LabelUse enums into one shared one that has ISA-prefixed names for each arm, then remove the monomorphization in MachBuffer; but I'll leave that for future work.

Wasmtime GitHub notifications bot (Sep 08 2026 at 17:53):

cfallin requested alexcrichton for a review on PR #14300.

Wasmtime GitHub notifications bot (Sep 08 2026 at 17:53):

cfallin requested wasmtime-compiler-reviewers for a review on PR #14300.

Wasmtime GitHub notifications bot (Sep 08 2026 at 17:55):

cfallin edited PR #14300:

Issue initially raised by alexcrichton at last week's Cranelift meeting: we have SmallVecs that are (ironically) quite large in MachBuffer, and the "finalized" split causes significant memmoves when the MachBufferFinalized is put together with pieces of the MachBuffer during finalization.

The initial design intent (six years ago!) was to avoid allocations for the common case of a small function compilation: a MachBuffer would contain enough buffer space for the machine code + metadata for anything up to, say, a kilobyte of machine code. But that benefit was lost when we moved to the "finalization" design.

The split itself is necessary: it captures an algorithmic finalization step, and that data that we have before and after differs. (Specifically, we translate label indices into buffer offsets inside of several record types; those are different types, we shouldn't type-pun them, and we have to make a pass through all the data anyway so those vecs are not a simple bulk data move.)

However many of the metadata arrays are simple passthroughs: for example, the machine-code buffer itself.

This is a simple/mechanical refactor that puts fields that don't change type during finalization in a MachBufferInner, puts that in a Box, and then holds it from either the MachBuffer or MachBufferFinalized. Thus the finalization can move one pointer over instead of kilobytes of buffers.

On a quick Sightglass run with default.suite, I see one benchmark's compilation time move:
compilation :: cycles :: spidermonkey-markdown

    Δ = 16405327.75 ± 16285800.71 (confidence = 99%)

    modified.so is 1.00x to 1.02x faster than base.so!

    ┌────────────┬────────────┬───────────────┬────────────┬─────────────┐
    │ Min        │ Max        │ Mean          │ Median     │ Engine      │
    ├────────────┼────────────┼───────────────┼────────────┼─────────────┤
    │ 1258771422 │ 1408382397 │ 1333799348.93 │ 1343472392 │ base.so     │
    ├────────────┼────────────┼───────────────┼────────────┼─────────────┤
    │ 1254241200 │ 1405715279 │ 1317394021.18 │ 1313587478 │ modified.so │
    └────────────┴────────────┴───────────────┴────────────┴─────────────┘
so a mean ~1% speedup.

There is definitely further work to do to try to reuse allocations across compilations by holding something in the Context, as we do with e.g. regalloc2::Ctx. The complication there is that MachBuffer is monomorphized on I because it holds I::LabelUses and those can differ between architectures; and cranelift_codegen::Context can be used to recompile for different ISAs with each compile invocation, so the actual type can necessarily differ between invocations. I suppose we could collapse all individual LabelUse enums into one shared one that has ISA-prefixed names for each arm, then remove the monomorphization in MachBuffer; but I'll leave that for future work.

Wasmtime GitHub notifications bot (Sep 08 2026 at 19:44):

github-actions[bot] added the label cranelift on PR #14300.

Wasmtime GitHub notifications bot (Sep 08 2026 at 19:44):

github-actions[bot] added the label cranelift:area:machinst on PR #14300.

Wasmtime GitHub notifications bot (Sep 09 2026 at 14:38):

alexcrichton requested fitzgen for a review on PR #14300.

Wasmtime GitHub notifications bot (Sep 09 2026 at 14:38):

alexcrichton unassigned alexcrichton from PR #14300 Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves..

Wasmtime GitHub notifications bot (Sep 09 2026 at 14:44):

alexcrichton commented on PR #14300:

I'll defer this to @fitzgen since my vision for what to do here differs pretty greatly.

The split itself is necessary: it captures an algorithmic finalization step, and that data that we have before and after differs.

Personally I don't agree with this -- the type punning is exactly what we do in Wasmtime for things like enum EngineOrModuleTypeIndex. My personal historical experience is that type state is basically never worth it and is more runtime overhead than necessary while also being a pretty severe API design constranit. I would be surprised if a memcpy was faster than update-in-place -- that's touching 2x more memory than an in-place update. My impression though is there's not much interest in exploring this concern, and overall it's pretty minor, so I won't take this further.

I'll also note that I was benchmarking a test case from oss-fuzz, not something like spidermonkey.wasm. I wouldn't expect much speedup on normal wasms, but foo.wasm.txt for example is ~100k empty functions (a fuzz test case)

Wasmtime GitHub notifications bot (Sep 09 2026 at 14:59):

cfallin commented on PR #14300:

Thanks @alexcrichton -- I want to make sure I fully understand your position here so this is useful input. Mapping the type structure in a little more detail, the actual types that change are all MachLabel -> CodeOffset. Both are newtyped u32s; we could do a bitpacked enum, steal the upper bit and support up to 2GiB code instead of 4GiB. Thinking about this further I'd be fine with this.

Part of the "type state", though, is really a builder pattern: MachBuffer contains the in-progress state (labels at last point, current source loc, ...) and we drop that when moving to the MachBufferFinalized. We do have builder patterns all over the place (e.g. Function and FunctionBuilder). Are you saying that that separation is not worth encoding in the type system?

(If we keep that distinction, then I think the above basically means either everything goes into the MachBufferInner here, or MachBuffer -> MachBufferBuilder which owns a MachBuffer until we destruct the outer builder and return the MachBuffer. Box to taste to keep that last bit from memmove'ing.)

The last bit of work needed to make MachBuffer independent of VCodeInst is to consolidate LabelUses into one concrete type rather than using I::LabelUse, remove the trait-impl machinery there, and have one single implementation somewhere that understands all ISAs we support. That's kind of awkward (it makes some ISA knowledge centrally-located) but I guess we already do it for Reloc and that's not the end of the world. Is that what you had in mind or something else?

Wasmtime GitHub notifications bot (Sep 09 2026 at 15:02):

cfallin commented on PR #14300:

(To make sure it's explicit: the reason I ask for more details of what you were thinking here is because you had mentioned something about dyn Traits, etc., last week; I would be concerned about efficiency of such an approach but I'm also not seeing where it's necessary)

Wasmtime GitHub notifications bot (Sep 09 2026 at 17:58):

alexcrichton commented on PR #14300:

I was roughly expecting an enum for differentiating between CodeOffset and MachLabel, but if code size is a concern then something bit-packed could work. Basically just runtime state saying which-is-which plus panics if the wrong one is accessed or found -- which is similar to how other "should have happened prior"-style errors show up throughout compilation.

I agree we have builders elsewhere, and no I'm not advocating for their removal. Builders are often places to hang methods and take advantage of unique ownership in addition to shuffling around internals. What I find typestate is not helpful for is deep within a structure there's a single field that needs flipping but everything else is more-or-less the same. I also find it's not helpful when the type needs to be referred to in a lot of places and may be stored in a number of locations. Here MachBuffer doesn't benefit too too much IMO from the type state (just a few fields flipping) and it's also stored in a fair number of places which I think we should be able to refactor and massage as well. I'm not trying to necessarily advocate for a general framework for all types, but I don't personally think typestate carries its weight here.

For I: VCodeInst, to restate what I was saying in the meeting last week, what I was imagining was a Box<dyn Any> to punch through the dyn TargetIsa boundary. There'd for example be a TargetIsa::make_buffer() -> Box<dyn Any> which would internally be a MachBuffer<I>. Later during compilation it'd take &mut dyn Any which would internally be downcast to MachBuffer<I>. The finalized state I think was able to be decoupled from I so that wouldn't need anything there. Basically I agree it would be nice to avoid the one-LabelUse-for-all-backends and I was thinking we could still preserve that.

To additionally clarify, though, the original goal I had was sharing buffers across compilation in the backend. That's already something we do with cranelift-frontend contexts and such, but nothing in the backend is sharing any buffers. For example VCode is freshly-allocated on all compilations with fresh buffers and I suspect might benefit from buffer reuse for the same reasons we found it beneficial to reuse regalloc contexts and other compilation contexts. I was hoping to change the argument to TargetIsa::compile_function to not just take a regalloc2::Context but a "backend context", probably as a &mut dyn Any to hide the I: VCodeInst, and then internally that would reuse buffers as necessary.

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:08):

cfallin commented on PR #14300:

OK, that's a useful design vision, thanks. I think the key bit getting in the way of any reuse, though, is this thing I mentioned above:

There is definitely further work to do to try to reuse allocations across compilations by holding something in the Context, as we do with e.g. regalloc2::Ctx. The complication there is that MachBuffer is monomorphized on I because it holds I::LabelUses and those can differ between architectures; and cranelift_codegen::Context can be used to recompile for different ISAs with each compile invocation, so the actual type can necessarily differ between invocations.

(and the same applies even moreso to VCode<I>)

In other words, simply putting things behind a box-of-any and downcasting does not cut it: if we keep the ISA-specific types distinct, they can have different representations (and indeed the size of MachInst differs between our ISAs); and the reuse scope, "one compiler context", permits recompilation with different ISAs, so we simply cannot reuse the underlying objects. (Or stated differently, there's no "downcasting trick" because it's not just newtypes on the surface; these really are different types)

We could push the TargetIsa arg further forward in the pipeline -- at construction of the Context and then hold it somehow, so a given compiler instance is tied to one ISA only -- but that's a deeper API change to Cranelift and I haven't worked through all the implications.

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:45):

alexcrichton commented on PR #14300:

It's true, yes a boxed trait object wouldn't work across ISAs. I don't personally consider that much of a downside though because that seems like a pretty niche edge case that's not worth acting as a foundation for the API design. For example in Wasmtime I don't think we'd ever hit that.

Wasmtime GitHub notifications bot (Sep 09 2026 at 18:53):

cfallin commented on PR #14300:

OK, yeah, that's fair. It would again be a public API change but that's fine.

One thing I forgot to respond to on this point:

I agree we have builders elsewhere, and no I'm not advocating for their removal. Builders are often places to hang methods and take advantage of unique ownership in addition to shuffling around internals. What I find typestate is not helpful for is deep within a structure there's a single field that needs flipping but everything else is more-or-less the same. I also find it's not helpful when the type needs to be referred to in a lot of places and may be stored in a number of locations. Here MachBuffer doesn't benefit too too much IMO from the type state (just a few fields flipping) and it's also stored in a fair number of places which I think we should be able to refactor and massage as well. I'm not trying to necessarily advocate for a general framework for all types, but I don't personally think typestate carries its weight here.

I guess it's subjective to some degree but the MachBuffer / MachBufferFinalized builder pattern is doing more than "just a few fields flipping": essentially all of the mutable builder API makes sense only on the object being built, not once it has been finalized. Yes we could carry a bool and panic on calls that are "out of phase" but it seems like an odd approach to take in a language that otherwise prefers static type-level distinctions, especially if we box the innards so they don't actually move when the builder is finalized. So: there's no performance difference (once we have this PR + move the label-to-offset rewrites into an in-place scheme and put them in the box too), and one alternative is strictly safer / harder to hold wrong at the type level. Why wouldn't we do the latter?

Wasmtime GitHub notifications bot (Sep 09 2026 at 20:18):

alexcrichton commented on PR #14300:

It would again be a public API change but that's fine.

Well, again, I'm trying to take myself off the critical path of this PR. We've historically pretty rarely taken into account API changes when considering PRs, but it can of course be considered just like everything else. I wouldn't agree with such a consideration myself, but this is minor enough that I don't see the need to push the case further.

Why wouldn't we do the latter?

Sorry if this has caused confusion, but I'm realizing that I've got a picture of typestate in my head which is probably different than what you've got. When I've been saying typestate I've been specifically referring to the CompilePhase trait, not the builder -> finalized pattern. I realize both are a form of typestate, however, and that's where I'm realizing it'd be good to clarify.

Which is to say, I'm not saying we should remove the builder -> finalized pattern. I'm saying we should remove the stencil -> finalized pattern. That's not to say the existing builder pattern couldn't be improved, there's still tons of buffers in MachBuffer that are fresh for each copy and would probably benefit from being shared across buffers, but that doesn't affect the builder -> finalized pattern and would instead just be like an argument to constructing the builder if that route were taken.

Wasmtime GitHub notifications bot (Sep 09 2026 at 20:39):

cfallin commented on PR #14300:

Ah! OK, yes, sorry, I did think you were talking about the builder distinction (different types for different phases) rather than compile phase (also different types for different phases, albeit a zero-sized sentinel type). I had mapped "just a few fields flipping" to the work in finish().

I'm less strongly attached to CompilePhase: it's nice for correctness in the incremental caching work (where it was introduced) but applies only to SourceLoc relativization, i.e. not on the critical path for sandboxing or anything, and we could actually do the same bitpacked-enum trick there and debug-assert when unwrapping to an absolute loc or something.

Well, again, I'm trying to take myself off the critical path of this PR. We've historically pretty rarely taken into account API changes when considering PRs, but it can of course be considered just like everything else. I wouldn't agree with such a consideration myself, but this is minor enough that I don't see the need to push the case further.

Right, I'm fine making the API change on Context -- not arguing against it per-se, just an offhand comment noting that the scope had increased. I guess to complete the thought: I don't think anyone actually relies on being able to compile the same function to multiple ISAs with one instance of a Context; threading through isa: &dyn TargetIsa as it is to individual methods is probably (?) a historical artifact rather than an intentional design, if I had to guess, though it's been that way for forever. So we probably don't even inconvenience anyone else by doing this.

Sorry to keep dragging you back in btw -- just wanted to make sure I understood your concerns + perspectives. I think I have a good idea now where you're coming from.

I'll update this PR in the next few days per the above. Thanks!

Wasmtime GitHub notifications bot (Sep 10 2026 at 23:30):

cfallin requested wasmtime-compiler-s390x-reviewers for a review on PR #14300.

Wasmtime GitHub notifications bot (Sep 10 2026 at 23:30):

cfallin requested wasmtime-core-reviewers for a review on PR #14300.

Wasmtime GitHub notifications bot (Sep 10 2026 at 23:30):

cfallin updated PR #14300.

Wasmtime GitHub notifications bot (Sep 10 2026 at 23:34):

cfallin commented on PR #14300:

OK, I've reworked things far enough to remove most typestate (the Final/Stencil distinction in MachBuffer; though it remains at the API level in CompiledCodeStencil/CompiledCode because that is an important correctness boundary for the compilation caching and ensuring we do things in the right order; but it's a pure newtype wrapper), and bitpacked sum types for the MachLabel/CodeOffset and RelSourceLoc/SourceLoc rewrites that happen in place.

I didn't go so far as to rename MachBuffer -> MachBufferBuilder and MachBufferFinalized -> MachBuffer but that's an easy followup. Actually doing buffer reuse in the Context can follow up on this as well. So I think this is good to review @fitzgen -- thanks!

Wasmtime GitHub notifications bot (Sep 11 2026 at 01:08):

github-actions[bot] added the label cranelift:area:aarch64 on PR #14300.

Wasmtime GitHub notifications bot (Sep 11 2026 at 01:08):

github-actions[bot] added the label cranelift:area:x64 on PR #14300.

Wasmtime GitHub notifications bot (Sep 11 2026 at 01:08):

github-actions[bot] added the label winch on PR #14300.

Wasmtime GitHub notifications bot (Sep 11 2026 at 01:08):

github-actions[bot] added the label cranelift:module on PR #14300.

Wasmtime GitHub notifications bot (Sep 11 2026 at 01:08):

github-actions[bot] commented on PR #14300:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:module", "winch"

Thus the following users have been cc'd because of the following labels:

saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Sep 11 2026 at 20:16):

:thumbs_up: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 11 2026 at 20:16):

:speech_balloon: fitzgen created PR review comment:

/// have been rewritten (see `MachBuffer::finish()`).

Wasmtime GitHub notifications bot (Sep 12 2026 at 22:41):

cfallin updated PR #14300.

Wasmtime GitHub notifications bot (Sep 12 2026 at 22:41):

cfallin has enabled auto merge for PR #14300.

Wasmtime GitHub notifications bot (Sep 12 2026 at 22:59):

cfallin added PR #14300 Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves. to the merge queue

Wasmtime GitHub notifications bot (Sep 12 2026 at 23:26):

github-merge-queue[bot] removed PR #14300 Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves. from the merge queue

Wasmtime GitHub notifications bot (Sep 12 2026 at 23:30):

cfallin updated PR #14300.

Wasmtime GitHub notifications bot (Sep 12 2026 at 23:30):

cfallin has enabled auto merge for PR #14300.

Wasmtime GitHub notifications bot (Sep 12 2026 at 23:48):

cfallin added PR #14300 Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves. to the merge queue

Wasmtime GitHub notifications bot (Sep 13 2026 at 00:14):

github-merge-queue[bot] removed PR #14300 Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves. from the merge queue

Wasmtime GitHub notifications bot (Sep 13 2026 at 00:28):

cfallin updated PR #14300.

Wasmtime GitHub notifications bot (Sep 13 2026 at 00:28):

cfallin has enabled auto merge for PR #14300.

Wasmtime GitHub notifications bot (Sep 13 2026 at 00:45):

cfallin added PR #14300 Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves. to the merge queue

Wasmtime GitHub notifications bot (Sep 13 2026 at 01:13):

:check: cfallin merged PR #14300.

Wasmtime GitHub notifications bot (Sep 13 2026 at 01:13):

cfallin removed PR #14300 Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves. from the merge queue

Last updated: Sep 20 2026 at 18:08 UTC