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

Remove Rc and RefCell from Signal? #600

Closed
lyphyser opened this issue Apr 13, 2023 · 3 comments
Closed

Remove Rc and RefCell from Signal? #600

lyphyser opened this issue Apr 13, 2023 · 3 comments
Labels
A-reactivity Area: reactivity and state handling C-enhancement Category: new feature or improvement to existing feature

Comments

@lyphyser
Copy link

Currently signals store the value in an Rc, requiring an extra memory allocation, and getting the value requires to clone the signal rather than getting a reference.

This seems a poor design since the Rc is unnecessary a lot of times (e.g. if the value is an integer or the value is never cloned) and the user can always create a Signal<Rc> if they want it.

Also it's obviously much more efficient to just get a reference to a value rather than cloning it with get() (although the user must be careful to not do anything that could set the signal while the reference is held) so either get() could return a reference or a get_ref() function could be provided; again, the user can clone the value themselves if they so desire.

So the options could be to either get rid of Rc on Signal or create a parallel set of Signal types without Rc.

RefCell is also not ideal in case the value is an integer, so it might be nice to have the user specify the cell type and support both RefCell and Cell.

There's also the problem that Signal doesn't support multithreading, which requires to solve this problem and also provide a way to specify an Arc-based signal emitter instead of the Rc-based one.

@lukechu10 lukechu10 added C-enhancement Category: new feature or improvement to existing feature A-reactivity Area: reactivity and state handling labels Apr 13, 2023
@lukechu10
Copy link
Member

Yeah, I definitely agree that making Signals internally use a RefCell<Rc<_>> was a mistake on my part when designing the reactivity system. Getting rid of the extra Rc wrapper will definitely make everything cleaner.

I think the best solution here would just be to get rid of the Rc all together, since one can easily get the old behavior back by just doing create_signal(Rc::new(...)).

As for .get(), I think the most convenient would be to clone the value (another option may be to make .get() only available for Copy types). We can then introduce .read() and .write() (final names subject to discussion) which would just forward to the internal RefCell's borrow and borrow_mut methods.

For RefCell/Cell, I personally think we should just stick with RefCell, even for Copyable types since the performance penalty is just an extra integer equality check which would dwarf all the other machinery that is run such as automatic dependency tracking inside effects etc...

Finally, I'm not sure if it's worth it to make Signals multi-threaded because that would slow down the single-threaded case and also make it harder to debug because we would get dead-locks instead of panics. However, this should also be up for debate and I'm open to changing my mind on this.

@anwarhahjjeffersongeorge

Finally, I'm not sure if it's worth it to make Signals multi-threaded because that would slow down the single-threaded case and also make it harder to debug because we would get dead-locks instead of panics. However, this should also be up for debate and I'm open to changing my mind on this.

Even if the main Signal remains single-threaded, it would be nice to have an option for a multi-threaded alternative for use in async contexts. Or to support a generic alternative over some subset of smart pointer types.

@lukechu10
Copy link
Member

Although #612 and #626 don't make the reactivity system thread-safe, the Rcs and RefCells have been removed so I think this issue should be considered fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-reactivity Area: reactivity and state handling C-enhancement Category: new feature or improvement to existing feature
Projects
None yet
Development

No branches or pull requests

3 participants