Make Ctx be Send + Sync so Cranelift and Wasmtime can reuse it#263
Conversation
| // Nonetheless, we only ever touch a `Bump` (and every clone of it!) | ||
| // from a single thread at a time: all clones of a given `Bump` live | ||
| // inside one `Ctx`; a `Ctx` is created, mutated, and dropped by a | ||
| // single thread at a time; and the returned `Output` is |
There was a problem hiding this comment.
Hmm, I have quite mixed feelings about this: if I'm reading it right, it's asserting statements that regalloc2 cannot guarantee ("a Ctx is created, mutated, and dropped by a single thread at a time") and are properties ensured only by the user/embedder of the library, so would necessarily imply the main regalloc2 entry points that take Ctx become unsafe. I don't like that both on principle and because it seems like a somewhat not-unlikely footgun: if we ever break up compilation into smaller units of work and use a thread pool, for example, we suddenly have hidden invariants about pieces of the Cranelift+regalloc2 context and thread-locality that are no longer enforced by the type system. I pretty strongly don't want the Cranelift-to-regalloc2 API to be fundamentally unsafe.
Would it be possible to create a Bump variant that internally uses an Arc instead?
(Or am I misunderstanding how the invariants work here?)
There was a problem hiding this comment.
OK, talked to Nick about this offline and realized that Ctx itself is still safe at the regalloc2 API boundary; the unsafety is purely in the knowledge that Rc is morally "Send if everything is ported over to another thread in one unit" (i.e., we don't have a trait to talk about move-the-whole-graph vs. move-this-one-object, so Rc and similar types with thread-unsafe bookkeeping ban moving any piece at all). In effect Ctx encapsulates the inner Rc and everything that could share with it (other cloned copies of the Bump inside containers or whatever) so moving Ctx's ownership across threads is safe.
So, no objection and this seems reasonable!
No description provided.