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

Change Widget::update to return a function for mutating the current State rather than produce a whole new State? #554

Closed
mitchmindtree opened this issue Sep 2, 2015 · 4 comments

Comments

@mitchmindtree
Copy link
Contributor

Motivation

Currently, the signature for Widget::update is:

fn update<C>(self, args: UpdateArgs<Self, C>) -> Option<Self::State>
    where C: CharacterCache;

The idea is that you only have to return a new State if something has changed between updates. This works quite efficiently for small widgets, however can be quite expensive for larger widgets.

A possible solution is to instead return an Option<Box<FnMut(&mut Self::State)>>. With this change, the Widget::update signature might look something like this:

fn update<C>(self, args: UpdateArgs<Self, C>) -> Option<Box<FnMut(&mut Self::State)>>
    where C: CharacterCache;

This way, instead of returning an entirely new state, we return a function that can mutate the state that already exists, reducing large allocations etc for larger widgets.

Concerns

It would be nice if we didn't have to Box the function that we're returning, however I'm unsure if it's possible to make an API that doesn't require it (I think we'd need HKT or something).

Any suggestions or alternatives for handling this problem in a nicer way are welcome!

@bvssvni
Copy link
Member

bvssvni commented Sep 2, 2015

What about calling a closure with a closure? Perhaps we could get rid of the Box.

Example: https://github.com/PistonDevelopers/graphics/blob/master/src/graphics.rs#L17

@mitchmindtree
Copy link
Contributor Author

I was originally thinking along these lines too, however the Self::State that needs to be mutated also needs to be available immutably (at the moment this is done via the UpdateArgs) so that the user can compare the builder args to the previous state and first determine whether or not they need to mutate at all.

Perhaps instead of being passed in via the UpdateArgs, the Self::State can be passed in as a second argument within some special type that implements two methods: one that produces a &State<Self::State> for immutable access and another that takes a &mut FnMut(&mut Self::State) closure which it uses for updating the state while also setting a flag so that we can know the Self::State has been mutated (and in turn that we have to call Widget::draw again to produce a new Element).

I think this might look something like:

fn update<'a, 'b, C>(self, args: UpdateArgs<'a, 'b, Self, C>, state: &mut StateCell<Self::State>);
    where C: CharacterCache;

Maybe the StateCell might look something like this:

pub struct StateCell<T> {
    state: T,
    has_updated: bool,
}

impl<T> StateCell<T> {

    fn view(&self) -> &T { &self.state }

    fn update(&mut self, f: &mut FnMut(&mut T)) {
        self.has_updated = true;
        f(&mut self.state);
    }
}

@mitchmindtree
Copy link
Contributor Author

Something like abstract return types would make it easy to return a function for mutating the State. There's some related discussion going on here atm.

It might make the signature look something like:

fn update<C>(self, args: UpdateArgs<Self, C>) -> Option<impl FnMut(&mut Self::State)>;

or

fn update<C>(self, args: UpdateArgs<Self, C>)<F> -> F where
    F: FnMut(&mut Self::State);

IMO This would be the clearest option, as the user wouldn't have to investigate the UpdateArgs to work out how to mutate their widget's state.

@mitchmindtree
Copy link
Contributor Author

Another possible solution which might keep the Widget::update method much simpler is to simply hold on to two copies of each Widget's unique Widget::State at all times. This would allow us to pass one unique Widget::State by value into the Widget::update method, mutate it within the method, return it from Widget::update by value and then use our other Widget::State for comparison to tell whether or not the state has changed. We would only need to make a new clone if the state Widget::State changed in some way.

This would also take the burden of choosing whether or not to return a new update off of the Widget designers. This is because rather than having to check whether or not any of the State's fields have changed, we simply do a PartialEq comparison when the new State is returned.

The downside is that this would increase memory usage and would likely require allocating (depending on the Widget) - especially if the Widget::State was very large.

I think I'd still prefer to try a more performant option like returning an optional mutation closure once something like abstract return types lands, but will keep this in mind if it gets too complex.

Edit: facepalm I just realised that this basically re-introduces the whole problem of needing to allocate the whole state each time there's some change, which is the very thing this issue was trying to address in the first place 😵 scrap this!

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

No branches or pull requests

2 participants