Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should the backend be Send + Sync? #75

Closed
jeffa5 opened this issue Mar 29, 2021 · 5 comments · Fixed by #78
Closed

Should the backend be Send + Sync? #75

jeffa5 opened this issue Mar 29, 2021 · 5 comments · Fixed by #78

Comments

@jeffa5
Copy link
Collaborator

jeffa5 commented Mar 29, 2021

Given the ideas proposing to have a single backend with multiple frontends I think it would make sense for the backend to be Send + Sync. This means we could send a backend across threads as well as use it in shared memory settings with, for example, a Mutex. I can see this being particularly useful for server implementations in Rust where clients for a document may not always land on the same thread.

Basically this comes down to replacing instances of Rc with either Arc or removing them if they aren't necessary. Also we'd need to switch from im_rc to im which has a minor performance penalty I believe.

I'm not sure of the impact of this on wasm but think it should still work the same. I've got an example branch of this here

@jeffa5
Copy link
Collaborator Author

jeffa5 commented Mar 29, 2021

Maybe a preferred use case is message passing rather than shared memory for this? I could imagine a wrapper around a backend with methods to push into channels and read from them.

@alexjg
Copy link
Collaborator

alexjg commented Mar 29, 2021

The im_rc -> im should only be in the frontend I think so probably not relevant here. However, there are a number of uses of Rc in the backend which would need to switch to Arc. That should be a reasonably mechanical change so it should be possible to implement it and benchmark it and see if it makes a significant difference.

That said, my preference would be for the message passing wrapper. Automerge is intended to be a foundational library so I don't think we should make decisions about what kind of performance is acceptable to users when we don't have to. I think the message passing wrapper makes sense in the case of using one backend for multiple frontends anyway because you'll still need to ship patches from individual changes off to each of the frontends on every change, so there will be additional glue code to write (I think, I haven't thought about this in detail).

@orionz
Copy link
Collaborator

orionz commented Mar 30, 2021

I've never done work with Rust and threads so Sync and Send are new to me. When I needed Rcs I favored them to Arcs because the primary target was WASM and extra overhead in a single threaded environment seemed like a bad idea.

Now that said, this might be a good time to think about the right time to design this so we can move cleanly into Sync Send terratory.

I could remove all the Rc's and automerge would work fine - the reason I put them in there was facilitate fast forks and memory efficient backends with common ancestry. Is it important for us to be able to fork a backend and then send the fork to another thread? If so the changes need mutexes or to be copied.

I wonder how much Rc->Arc would affect performance in WASM given that its single threaded. Is the compiler smart enough to reduce one to the other. I should run some benchmarks and find out.

Option 1: axe all the Rc's and deal with fast forking another day or
Option 2: benchmark Arc's in WASM and make sure the cost is minimal - and if so change all the Rc's to Arc's

What do you think?

@jeffa5
Copy link
Collaborator Author

jeffa5 commented Mar 30, 2021

That PR deals with Option 1, just an option though.

I'm not sure what you mean by forking in an automerge document as no forking seems to be implemented, unless you mean just cloning a backend? How common is it to fork a backend?

@orionz
Copy link
Collaborator

orionz commented Mar 31, 2021

Sorry - the action got renamed to Automerge.clone. There are scenarios where making lots of clones is desired. Suck as keeping points in time you might want to rewind to - or doing speculative merges with other users and then "rolling back" those merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants