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

Add a way to shortcut evaluation #7

Closed
heyimalex opened this issue Jul 10, 2015 · 26 comments
Closed

Add a way to shortcut evaluation #7

heyimalex opened this issue Jul 10, 2015 · 26 comments

Comments

@heyimalex
Copy link
Contributor

Just an idea for dealing with situations where a selector function should be conditional.

Here's a contrived example: Imagine you have a huge list and a toggle button to do some complex filter calculations on the list. Your base selectors look like this.

const hugeList$ = state => state.hugeList;
const shouldFilter$ = state => state.shouldFilter;

One way to handle this is by making a filteredList$ selector that depends on hugeList$.

// Method #1

const filteredList$ = createSelector(
  [hugeList$],
  (hugeList) => {
    /* do expensive computation here */
    return filteredList;
  }
);

const finalList$ = createSelector(
    shouldFilter$, filteredList$, hugeList$],
    (shouldFilter, filteredList, hugeList) => {
        return shouldFilter ? filteredList : hugeList;
    }
);

The issue is that filteredList is computed even if shouldFilter is false. If hugeList changed very often but shouldFilter was usually false, you'd be doing a lot of unnecessary re-calculation.

To avoid that you could move the shouldFilter check into filteredList$;

// Method #2

const filteredList$ = createSelector(
  [shouldFilter$, hugeList$],
  (shouldFilter, hugeList) => {
    if (!shouldFilter) { return null; }
    /* do expensive computation here */
    return filteredList;
  }
);

const finalList$ = createSelector(
    [filteredList$, hugeList$],
    (filteredList, hugeList) => {
        return filteredList === null ? hugeList : filteredList;
    }
);

But now the problem is that you recompute every time shouldFilter is toggled. If someone repeatedly clicks the toggle button the UI is going to start lagging hard.

The crux of the issue is that we have no way to conditionally execute a selector. We want to compute a value under certain circumstances only, but using memoize we can't shortcut the function without expiring our cached value.

Some potential solutions:

  • A Symbol that can be returned to shortcut execution directly. The wrapping function would check the return value and return the cached value if it was the symbol. In the example you could just take the second method and return the special symbol instead of null.
  • Something like shouldSelectorUpdate that passes the last used dependency values and the current dependency values and lets the user decide if they wanted to recompute.

The only issue I see now is the need for a default value. As this situation only comes up times that you don't actually want to use the value, it seems safe to just default to null and let the user override if they need to.

@ellbee
Copy link
Collaborator

ellbee commented Jul 11, 2015

This is great, thanks for the detailed and clear write up. I agree this would be useful and should probably be in the library. What do you think @faassen?

@heyimalex
Copy link
Contributor Author

It would break compatibility, but a very easy solution is to replace the valueEquals function with a shouldSelectorUpdate function, and call it directly instead of using argsEquals. The current functionality could be implemented by just defaulting shouldSelectorUpdate to a shallow list equality check (like argsEquals is now).

@faassen
Copy link
Collaborator

faassen commented Jul 13, 2015

Thanks for that writeup! I wondered what would happen if you combine both solutions, but that won't help; expensive and quite possibly needless computation could still be triggered if someone changes shouldFilter.

I think we should do our research: what do NuclearJS and re-frame do about this? If they happen to do nothing about it, we have to think about why not, too.

I am curious to see both your suggested alternatives in code; I'm having trouble understanding the symbol alternative, and I'm curious where you'd pass in shouldSelectorUpdate for the second suggestion you make.

@heyimalex
Copy link
Contributor Author

@faassen Haven't looked into anyone else's solution, but I'll try and look around tonight. Right now I prefer the shouldUpdate option as it's really just a more flexible version of the current code.

These are rough, but should give you an idea of what I'm thinking.

The symbol method:

export const ShouldNotUpdate = Symbol();

function memoize(func, valueEquals) {
    let lastArgs = null;
    let lastResult = null;
    return (...args) => {
        if (lastArgs !== null && argsEquals(args, lastArgs, valueEquals)) {
            return lastResult;
        }
        let result = func(...args);
        if (result === ShouldNotUpdate) {
            return lastResult;
        }
        lastArgs = args;
        lastResult = result;
        return lastResult;
    }
}

// end-user code

import { createSelector, ShouldNotUpdate } from 'reselect';

const filteredList$ = createSelector(
  [shouldFilter$, hugeList$],
  (shouldFilter, hugeList) => {
    if (!shouldFilter) { return ShouldNotUpdate; }
    /* do expensive computation here */
    return filteredList;
  }
);

The shouldUpdate method:

function memoize(func, shouldUpdate, initialValue = null) {
    let lastArgs = null;
    let lastResult = initialValue;
    return (...args) => {
        if (!shouldUpdate(args, lastArgs)) {
            return lastResult;
        }
        lastArgs = args;
        lastResult = func(...args);
        return lastResult;
    }
}

function defaultShouldUpdate(args, lastArgs) {
    if (args === null) {
        return true;
    }
    return args.some((arg, index) => arg !== lastArgs[index]);
}


// end-user code

import { createSelector } from 'reselect';

const filteredList$ = createSelector(
  [shouldFilter$, hugeList$],
  (shouldFilter, hugeList) => {
    /* do expensive computation here */
    return filteredList;
  },
  ([shouldFilter, hugeList], lastArgs) => {
    return shouldFilter && hugeList !== lastArgs[1];
  }
);

@gilbox
Copy link

gilbox commented Jul 14, 2015

Another way to deal with it is to give a selector access to state as an additional argument.

const filteredList$ = createSelector(
  [hugeList$],
  (hugeList, {shouldFilter}) => {
    if (!shouldFilter) return;
    /* do expensive computation here */
    return filteredList;
  }
);

I do something similar in react-derive but maybe it makes more sense with props than with state.

@ellbee
Copy link
Collaborator

ellbee commented Jul 16, 2015

Yet another idea: turn the selector into a thunk if we want to conditionally execute it. The following is not a smart way to implement it (a new thunk gets created on every update) but it should give an idea of what it could look like.

import { createSelector, makeLazy } from 'reselect';

const shouldFilter$ = state => state.shouldFilter;
const hugeList$ = state => state.hugeList;
const keyword$ = state => state.keyword;

const filteredList$ = createSelector(
  [hugeList$, keyword$],
  (hugeList, keyword) => {
    /* do expensive computation here */
    return filteredList;
  }
);

const list$ = createSelector(
  [shouldFilter$, hugeList$, makeLazy(FilteredList$)],
  (shouldFilter, hugeList, lazyFilteredList) => {
    return shouldFilter ? lazyFilteredList() : hugeList;
  }
);

makeLazy:

export function makeLazy(selector) {
    return state => () => selector(state);
}

@faassen
Copy link
Collaborator

faassen commented Jul 21, 2015

I was traveling last week, now sitting down again to try to think this through again. Please bare with me as I'm rusty. :)

@heyimalex Thanks for working them out! That lets us evaluate them better. I'm a bit wary of the arguments approach, as it needs so much dealing with argument lists and is in part dependent on the order.

I like how the approach by @gilbox makes things simple, but it appears to break abstraction -- we only pass in state because we want to bypass the selector caching mechanism. I think.

@ellbee's approach is very interesting. We express in the selector that we want to make the calculation of the sub-selector under the control of the function itself.

But before we proceed I still think we should do the research: what do other frameworks do about this?

@ellbee
Copy link
Collaborator

ellbee commented Jul 21, 2015

NuclearJS adds the result to a cache every time the arguments change so toggling shouldFilter back and forth does not cause the expensive recalculation. As far as I am aware re-frame does not do anything to address the problem.

I've just been looking more closely at the approach by @gilbox. In the example is { shouldFilter } one of the arguments taken into account for memoization? It looks to me that if { shouldFilter } is part of the memoization then it will perform the expensive calculation every time shouldFilter changes from false to true, but if the { shouldFilter } is not part of the memoization then changes to shouldFilter will have no effect unless hugeList also changes.

@ellbee
Copy link
Collaborator

ellbee commented Jul 21, 2015

Another option that hasn't been suggested yet is adding a cache for the memoization like NuclearJS.

@gilbox
Copy link

gilbox commented Jul 21, 2015

@ellbee, { shouldFilter } is simply destructuring of the component's state object (ie the pre-derived data). It's not a memoized value and there is no evaluation at all.

@ellbee
Copy link
Collaborator

ellbee commented Jul 22, 2015

Thanks @gilbox. So the overall example looks something like this:

const shouldFilter$ = state => state.shouldFilter;
const hugeList$ = state => state.hugeList;

const filteredList$ = createSelector(
  [hugeList$],
  (hugeList, {shouldFilter}) => {
    if (!shouldFilter) return hugeList;
    /* do expensive computation here */
    return filteredList;
  }
);

const list$ = createSelector(
  [shouldFilter$, filteredList$],
  (shouldFilter, filteredList) =>  filteredList
);

@speedskater
Copy link
Contributor

Giving the user access to the state as an additional parameter is tempting but i vote against it as it will result in quite unpredictable selectors. (e.g. the result depending on a parameter which was not memoized. therefore updates could be lost as the memoized parameters did not change)

@gilbox I would suggest another approach which should give you the desired behavior. If the "memoize" function used in reselect is explicitly exported as an additional symbol (@faasen would this be possible).
The filter function could be memoized (and hence would only be recomputed if the input list actually changes).

Hence the resulting code for the filteredListSelector would be:

let filterList = memoize(inputList => { expensive filter operation... });

let filteredListSelector = createSelector([hugeList, shouldFilter], (hugeList, shouldFilter) => {
     return (shouldFilter) ? filterList(hugeList) : hugeList;
});

Imho this would make the conditional code as well as the memoization of the filter operation explicit. In addition the function solely depends on the list and whether it sould be filtered. @heyimalex By memoizing the filterList operation itself frequent toggeling of shouldFilter will not result in a lagging ui.

@ellbee
Copy link
Collaborator

ellbee commented Jul 29, 2015

@faassen @speedskater I like this idea.

@gilbox
Copy link

gilbox commented Jul 29, 2015

Honestly I don't use redux or reselect, I just follow reselect as inspiration for react-derive so maybe I misunderstand how selectors are used... Do you only ever use components with selectors 1 time (ie "container" components)? If not then you are using the same filterList memoized function for multiple instances of the same component. It will work in many cases but it's not a general solution.

@speedskater
Copy link
Contributor

@gilbox The idea behind selectors is to provide accessor functions on the global state. Which in case of redux is composed by using multiple reducers. So the filteredListSelector used in this example extracts and filters a specific list in your store (the global state). This selector is independent of the context in which it can be used, it can be another selector or it can be one or many components.

If you want to reuse the selector for different lists in your store you can create a factory function which allows you to create different selectors bound to different lists and therefore having different memoized versions of filterList (but actually only one implementation of filterList).

Does this explanation make it clearer or you?

@heyimalex
Copy link
Contributor Author

@speedskater I like it because:

  • It doesn't break anything
  • This is an optimization anyways, and most of the other solutions don't make life any easier for the end user. Ultimately reselect is <50 lines so if anyone ever runs in to perf problems they can read the lib and solve it themselves.

However, to export memoize we'd have to undo some changes I made in #9 😄

@speedskater
Copy link
Contributor

@heyimalex I see ;). So should we rename memoize to internalMemoize and export the following function for explicit usage?

export function memoize(func, valueEquals = valueEquals) {
   let memoizedFunction = internalMemoize(func, valueEquals);
   return (...args) => memoizedFunction(args);
}

@speedskater
Copy link
Contributor

@faassen @ellbee If we solve this issue with the export of memoize i think we should add advanced examples to the documentation. So users have examples how to solve advanced use cases.

@ellbee
Copy link
Collaborator

ellbee commented Aug 6, 2015

@faassen, @heyimalex, @speedskater

So, shall we do this with an exportable memoize then?

Also, I agree with @speedskater that the docs should have an advanced use cases section so I'll open an issue for that.

@heyimalex
Copy link
Contributor Author

👍

@speedskater
Copy link
Contributor

👍
This week I have no time. But I think i could create a PR till the end of next week.

@ellbee
Copy link
Collaborator

ellbee commented Aug 7, 2015

Hey @speedskater, I already have a PR open for this that I created yesterday as no-one had done it yet. If you like I'll change it to make you the commit author as it was your idea and I used the code from your comments!

@speedskater
Copy link
Contributor

@ellbee No its okay for me. Great that your are working on it.

@ellbee
Copy link
Collaborator

ellbee commented Sep 9, 2015

The default memoize function is an export in version 1.0.0

@ellbee ellbee closed this as completed Sep 9, 2015
@ropez
Copy link

ropez commented Apr 14, 2017

I'm obviously very late to the party here, but I'm curious what you think about the following solution to the original problem:

So, the idea is that selectors are just functions, right? So why not make a "higher order" selector - a selector that returns a selector:

const finalListSelector$ = createSelector(
    shouldFilter$,
    (shouldFilter) => shouldFilter ? filteredList$ : hugeList$
)

// wrapper function for convenience
const finalList$ = state => finalListSelector$(state)(state)

Do you see any problems with this solution? It should memoize correctly, shouldn't it?

@CGamesPlay
Copy link

@ropez I like that solution as well. I wrote a quick selector creator to leverage it:

https://gist.github.com/CGamesPlay/e3cb9d62e95be13a364100c707f46dbf

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

7 participants