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 clone bounds #37

Merged
merged 4 commits into from
Oct 24, 2022
Merged

Remove clone bounds #37

merged 4 commits into from
Oct 24, 2022

Conversation

danheuck
Copy link
Contributor

As mentioned in #36, this removes every Clone bound that didn't seem necessary. The main notes are:

  • Types held in a Signal no longer need to be Clone
  • The output type of a Resource's future no longer needs to be Clone, though the bounds for the source type are unchanged
  • A Resource::with() method was added, with similar behavior to Signal::with() to allow access without cloning
  • Memo output types do not need to be Clone
    • To accommodate this, Memo now takes impl FnMut(Option<&T>) -> T instead of impl FnMut(Option<T>) -> T which is a breaking change, albeit a minor one

This still needs some documentation updates which I'll hopefully get to tomorrow. I'll probably also drop the webcam component that spawned this PR and #35 in as an example unless you object. Tested with all of the examples, but let me know if you see any issues.

@danheuck
Copy link
Contributor Author

Also includes an unrelated fix for a bug in the two "counters" examples which I noticed while testing.

@gbj
Copy link
Collaborator

gbj commented Oct 24, 2022

Yes, I would love it if you'd add the webcam example. This is the kind of thing that's great to have in the examples folder in addition to the very typical counters and todos.

@gbj
Copy link
Collaborator

gbj commented Oct 24, 2022

I'm working on a few changes to this that will allow Memo to continue taking an Option<T> for its previous value without cloning T, which preserves the current API, relaxes the Clone requirement, and should end up making memos even more efficient than they were before by saving a clone. That should be especially helpful for the component.

Basically the idea is to use RefCell::take() to get the previous value out of the signal inside the memo effect. That signal will temporarily be empty, but it's replaced synchronously by the update so I don't think it's actually possible for anything to read it while it's empty.

Stay tuned. I'm terrible at GitHub so not quite sure, but hopefully I can push some changes to the PR later today.

@danheuck
Copy link
Contributor Author

danheuck commented Oct 24, 2022

The fundamental issue I ran into when trying to remove the clone bound from Memo without changing the API is that we need to pass the memo function the previous state and then compare that previous state against the value returned by the function. This means we need to have ownership (or a reference to) the previous state after we call the memo function. As far as I can tell, this can be achieved by:

  1. Cloning the state and only giving the memo function a copy
    • This is what we started with. It works fine, but requires the state to implement Clone and means a Memo will always clone the state no matter what
  2. Giving the memo function ownership and requiring it to give the old state back in addition to the new state
    • This is both an inconvenient API and raises the possibility that the memo function will modify the previous state, invalidating our comparison
  3. Giving the memo function an immutable reference and keeping ownership ourselves
    • This is where I ended up, obviously. This allows the state to not implement Clone but, if it does, the memo function can clone it manually to achieve the same behavior as the first option.

This could also be circumvented by introducing a Hash bound on the state. That way we could hash the state every time the memo function returns and just compare that hash against the previous hash to determine if it changed. I feel like hash is perhaps more annoying to implement than clone, but you're less likely to have unhashable data than uncloneable data. This does have the advantage though that we never have to clone and the memo function can mutate the state in place rather than allocating new memory if it wants, so this might be more performant? I wouldn't assume either way without doing some benchmarks though.

@gbj
Copy link
Collaborator

gbj commented Oct 24, 2022

You're completely correct, of course. I'd forgotten that we'd need still need to have the old value around, to checked whether it had changed!

I think taking Option<&T> for prev in a memo is the correct API, in fact, since it lets the user decide whether to clone or not in the memo function. Clearly preferable to cloning by default.

You mentioned docs changes but I think you've left everything in a consistent state, as far as I can tell. All right with you if I go ahead and merge it?

@gbj
Copy link
Collaborator

gbj commented Oct 24, 2022

Here are two runs of the js-framework-benchmark with these changes. Things move around a little unpredictably in the benchmark, but this seems like a nice performance boost on multiple fronts, in addition to the more permissive API.

Screen Shot 2022-10-24 at 4 53 35 PM

Screen Shot 2022-10-24 at 5 24 04 PM

@danheuck
Copy link
Contributor Author

Yeah, hopefully not too many documentation changes, but I wanted to add something for Resource::with() and Resource::read(). Everything looks good to me, not much of a performance difference in those benchmarks (which is probably an indication of how little Cloning was actually happening despite all the things that needed to be Clone) but it's nice to see that things haven't gotten slower at least.

@gbj
Copy link
Collaborator

gbj commented Oct 24, 2022

Yeah the difference was way more impressive on the first run but even if the actual changes are only 0.01 - 0.02 on the total, that's closing 10-20% of the gap between Leptos and Solid.

At this point the benchmarking is a little silly — I'm genuinely focusing on other things like the RPC design — but it's good to feel like we're making things slightly faster, and certainly not slower!

I'm going to merge this. Really good work, and thanks for the contributions.

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 this pull request may close these issues.

2 participants