Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves. - #14300
Cranelift: box a few fields in MachBuffer/MachBufferFinalized to avoid memmoves.#14300cfallin wants to merge 1 commit into
MachBuffer/MachBufferFinalized to avoid memmoves.#14300Conversation
…avoid memmoves.
Issue initially raised by alexcrichton at last week's Cranelift
meeting: we have `SmallVec`s that are (ironically) quite large in
`MachBuffer`, and the "finalized" split causes significant `memmove`s
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:
```plain
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::LabelUse`s 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.
github-actions
Bot
added
cranelift
|
I'll defer this to @fitzgen since my vision for what to do here differs pretty greatly.
Personally I don't agree with this -- the type punning is exactly what we do in Wasmtime for things like 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) |
|
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 Part of the "type state", though, is really a builder pattern: (If we keep that distinction, then I think the above basically means either everything goes into the The last bit of work needed to make |
|
(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) |
|
I was roughly expecting an 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 For 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 |
|
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:
(and the same applies even moreso to 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 We could push the |
|
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. |
|
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 guess it's subjective to some degree but the |
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.
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 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 |
|
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 I'm less strongly attached to
Right, I'm fine making the API change on 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! |
2 participants
Issue initially raised by alexcrichton at last week's Cranelift meeting: we have
SmallVecs that are (ironically) quite large inMachBuffer, and the "finalized" split causes significantmemmoves when theMachBufferFinalizedis put together with pieces of theMachBufferduring finalization.The initial design intent (six years ago!) was to avoid allocations for the common case of a small function compilation: a
MachBufferwould 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 aBox, and then holds it from either theMachBufferorMachBufferFinalized. 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: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 thatMachBufferis monomorphized onIbecause it holdsI::LabelUses and those can differ between architectures; andcranelift_codegen::Contextcan be used to recompile for different ISAs with eachcompileinvocation, so the actual type can necessarily differ between invocations. I suppose we could collapse all individualLabelUseenums into one shared one that has ISA-prefixed names for each arm, then remove the monomorphization inMachBuffer; but I'll leave that for future work.