chore: Add Send + Sync supertraits to SourceManager trait

[sergerad]

Context

There are various trait objects in the miden-client stack that prevent it's Client struct from being used across await points. In order to address this, we need to provide Send + Sync variations of the traits used in these trait objects.

This PR creates a Send + Sync variation of the SourceManager trait.

Changes

• Add SourceManagerSync trait and generic impl.
• Replace Arc<dyn SourceManager + Send + Sync> with Arc<dyn SourceManagerSync>.

[bitwalker]

It isn't clear to me why SourceManager should require these as super traits. The majority of uses of SourceManager are in single-threaded contexts, so paying the overhead of synchronization in these cases makes little sense.

If we need a Send + Sync version of the trait, we should define it like so:

pub trait SourceManagerSync: SourceManager + Send + Sync {}

impl<T: SourceManager + Send + Sync> SourceManagerSync for T {}

Then, replace uses of dyn SourceManager + Send + Sync with dyn SourceManagerSync. This gives flexibility to use simpler implementations in single-threaded contexts, and sync implementations in multi-threaded contexts without having to choose one or the other.

When I originally implemented this, I provided both Sync and non-Sync options, anticipating we'd need the former eventually, and here we are. I'd still argue though that we need to support both, not one or the other, including with the assembler. The way they are used differs based on whether you are compiling ahead of time, or compiling and executing at the same time, and the former has no need for synchronization generally.

[sergerad]

It isn't clear to me why SourceManager should require these as super traits.

The idea as to why we should have these supertraits is to make corresponding trait objects inherently Send + Sync.

If we need a Send + Sync version of the trait, we should define it like so:

pub trait SourceManagerSync: SourceManager + Send + Sync {}

impl<T: SourceManager + Send + Sync> SourceManagerSync for T {}

I think this is reasonable. And also think this is fair:

The majority of uses of SourceManager are in single-threaded contexts, so paying the overhead of synchronization in these cases makes little sense.

I had assumed that none of the implementations would actually require synchronization primitives but I haven't checked and don't know the context well enough to know if it is a fair assumption. Although this does compile.

[bitwalker]

I had assumed that none of the implementations would actually require synchronization primitives but I haven't checked and don't know the context well enough to know if it is a fair assumption. Although this does compile.

I believe the default one provided in core currently just always synchronizes, but that is solely an artifact of me being asked to simplify my initial implementation, which provided both Sync and non-Sync impls, by just providing the Sync one. The trait is a public interface though, so the impl in core is just a reference implementation provided for our own internal use, not the only one.

Fundamentally, for a struct to impl Send + Sync, all of its fields must as well, which requires synchronization primitives (or unsafe, but you'd be hard pressed to uphold the safety guarantees of those traits with unsafe without synchronization of some kind).

[sergerad]

Yea, bottom line, if we want to allow impls to be not Send or not Sync, then we should not add these supertraits.

[bitwalker]

I know you didn't change this as part of your PR, so this isn't critiquing your PR, but the change that clearly snuck by me at some point.

I somehow missed that the Assembler was made to require a Send + Sync impl. IMO the assembler should be made generic over this, and type aliases used for ergonomics if needed. Again, the vast majority (all?) Assembler uses aren't going to be in multi-threaded contexts, and it appears that we're primarily doing this so that we can thread the SourceManager impl through to somewhere via the Assembler (hence the source_manager method), and that somewhere needs the impl to be Send + Sync. That just feels totally backwards to me tbh.

A lot of this appears to be justified using the needs of the client, but rather than solving that complexity there, we're pushing it down into miden-core (and thus forcing all downstream crates which depend on it to deal with the effects of such changes as well). I really strongly disagree with the approach of making things in miden-core be biased to specific implementation details - things in this crate should almost universally prefer to punt those sorts of concerns/choices downstream.

@plafer Maybe you have more insight into this? It's not something I think we need to change in this PR, but I'd very much like to change Assembler to support both Sync and non-Sync uses, since as it stands now this effectively makes non-Sync impls of SourceManager borderline useless, even though that is by far the most common context in which they would be used.

[plafer]

This was requested by @PhilippGackstatter here, and implemented in #1834. My thinking was: making it Send + Sync will unblock miden-base, and all we're losing is maybe a bit of performance (due to e.g. internal mutexes) on the source manager, which is acceptable. And the DefaultSourceManager was already Send + Sync, so we would literally see no penalty in the current implementation.

I'm all for a cleaner approach if you (eventually) want to propose one, but honestly I just didn't spend too much time thinking about it.

[sergerad]

I think that is all fair for Sync but not Send. We simply have no solution downstream if this trait object is not Send.

[bitwalker]

I'm all for a cleaner approach if you (eventually) want to propose one, but honestly I just didn't spend too much time thinking about it.

Sure, sounds good!

My thinking was: making it Send + Sync will unblock miden-base, and all we're losing is maybe a bit of performance (due to e.g. internal mutexes) on the source manager, which is acceptable.

On the surface I think that rationale is fine, and ultimately some fix was needed to unblock miden-base, so the simpler one was the quickest way to facilitate that. The thing is that the quick fix was also a breaking change, and so far as I can recall, nobody checked with me if that would break things in the compiler. This has been a recurring problem, so I'm probably extra sensitive to it at this point. I guess my point is, we have to start being a lot more wary of changes that remove functionality - we simply have too many downstream dependents on these core crates to be making changes without careful evaluation of the consequences.

I'm not mad, I'm just disappointed 😛

[plafer]

As discussed offline (@sergerad), IIUC this PR is no longer needed given that Assembler::source_manager was made Send + Sync. So IIUC, the next best course of action is to close this PR and release v0.15 as is. And @bitwalker please open an issue to discuss alternatives if the Send + Sync annoys you enough 😄

@sergerad @bitwalker does that sound good?

[sergerad]

Yea sounds good. Happy to help on any follow up issue, please @ me.

[bitwalker]

Yep, sounds good to me.

And @bitwalker please open an issue to discuss alternatives if the Send + Sync annoys you enough 😄

You bet 😉