Performance regression in 4.1.5?

[kelly-tock]

Upgraded and application from 4.0.0 to 4.1.5. We are also using re-reselect as well as defaultMemoize in several places as a more general memoization solution. A screen on our app is essentially unresponsive.

it seems like memoization is broken, as if I observe profiles in chrome dev tools with 4.0.0 and 4.1.5, when I click on a thing for example, all the sub items in a grid of items re-render, but in 4.0.0 this is not the case. all jest tests involving selectors are passing.

have there been any subtle pattern changes when folks have upgraded that are needed? I see some changes involving defaultMemoize but it seems harmless.

I have also tried removing re-reselect and using the new built in maxSize config options.

any insight would be much appreciated! i'm still working on narrowing down the issue, but wanted to ask. Was very surprised that it all seems to work except on one screen on our application that is very data intensive.

[markerikson]

The internal implementation definitely changed, but all the existing tests are passing. Can you post an example project that reproduces the issue, and ideally one that shows both 4.0 and 4.1.5 at the same time? (like, added as separate dependencies and then running the same logic back-to-back or something)

[kelly-tock]

yep, i'll try it with the selectors I have in mind.

[kelly-tock]

still trying to isolate the issue, as our app has 100s of selectors and lots of long long long selector chains of computing something from 4-5 selectors, then computing off of that as well. what is interesting to note that with the memory checkbox enabled in chrome dev tools profiling, i'm seeing this pattern:

4.1.5:

which seems indicative of a memory leak.

with 4.0.0 this same set of actions:

i'm continuing to investigate, it may be some pattern we have in our codebase that's an older pattern that's discouraged now or something like that.

[markerikson]

Huh, that's very surprising. I'm assuming you didn't configure any of these selectors for a larger cache size. There's specific handling internally for a cache size of 1, and I don't see anything immediately obvious that would be an issue:

function createSingletonCache(equals: EqualityFn): Cache {
  let entry: Entry | undefined
  return {
    get(key: unknown) {
      if (entry && equals(entry.key, key)) {
        return entry.value
      }

      return NOT_FOUND
    },

    put(key: unknown, value: unknown) {
      entry = { key, value }
    },

    getEntries() {
      return entry ? [entry] : []
    },

    clear() {
      entry = undefined
    }
  }
}

[kelly-tock]

nope, this is testing 4.1.5 as a drop in replacement. we have a few spots where we're using re-reselect for cache size stuff ATM.

but even with all of the components that use that disabled, still seeing this, but i'm down to a smaller list of target selectors to see when it stops happening.

[kelly-tock]

is there anything with undefined vs that NOT_FOUND value?

[markerikson]

Not sure what you're asking - clarify?

[kelly-tock]

I thought I saw in the various release notes something about returning undefined explicitly for a selector and a mention of this NOT_FOUND constant, but i'm just thinking out loud. we return undefined in selectors all over the place.

[markerikson]

Yeah, I had to switch to that NOT_FOUND sentinel value specifically to disambiguate with undefined so that selectors could store that as a result.

[kelly-tock]

so do we need to do something on our end to change to that value?

[markerikson]

No, that's purely an internal thing so that the logic above it can confirm that "the cache did not find a match for the key we're checking against".

[kelly-tock]

still isolating the issue, but from observing so far, a memory leak persisted, but got a little better every time I removed more selectors in our application from running. is there something internally that keeps track of all the selectors that is doing something to cause more memory consumption?

[markerikson]

Nope. Each selector is its own separate cache - there's nothing global.

It kinda sounds like it's less of a "memory leak" per se, and more just "caching". It sounds as if the selectors are keeping more data references alive than they used to, and that's adding more. (I realize there's a small difference in semantics there :) )

[kelly-tock]

still haven't found it, but I thought maybe posting the relevant areas of 2 profiles of the app i am working on, one with latest, and one with 4.0.0.

4.1.5:

same area of the profile for 4.0.0:

[kelly-tock]

relevant cached selector from re-reselect area of the code:

const squareId = (_: StoreType, squareId: string, dateTimeString: string, tableId: number) =>
  squareId;
const dateTimeString = (_: StoreType, squareId: string, dateTimeString: string, tableId: number) =>
  dateTimeString;
const tableId = (_: StoreType, squareId: string, dateTimeString: string, tableId: number) =>
  tableId;

const getSquareById = (
  calendarSquaresByTableAndTime: AdditionalInventoryAndTablesWithSlots,
  squareId: string,
  dateTimeString: string,
  tableId: number
): NormalDetailedSquare | undefined => {
  const { tablesWithSlots } = calendarSquaresByTableAndTime;
  if (!tablesWithSlots) {
    return undefined;
  }
  const table = tablesWithSlots[tableId];
  if (!table) {
    return undefined;
  }
  const slot = table.slots;
  if (!slot) {
    return undefined;
  }
  const slotAtDateTime = slot[dateTimeString];
  if (!slotAtDateTime) {
    return undefined;
  }
  if (!slotAtDateTime.calendarSquaresById) {
    return undefined;
  }
  return slotAtDateTime.calendarSquaresById[squareId];
};

export const getCalendarSquareFromCalendar = createCachedSelector(
  calendarSquaresByTableAndTimeSelector,
  squareId,
  dateTimeString,
  tableId,
  getSquareById
)({
  keySelector: (_: StoreType, squareId: string, dateTimeString: string, tableId: number) =>
    squareId,
  cacheObject: new LruObjectCache({ cacheSize: 1000 }),
});

[kelly-tock]

getting cache hit right away with 4.0.0, but not with 4.1.5

[kelly-tock]

oh and the selector call in the function component:

const calendarSquare = useSelector(function calendarSquareUseSelector(state: StoreType) {
    return getCalendarSquareFromCalendar(state, calendarSquareId, dateTimeString, tableId);
  });

[markerikson]

Most of that looks like re-reselect internals, tbh.

Does any of this happen with just reselect by itself, or only with re-reselect?

[kelly-tock]

The internals in the 4.1.5 are reselect internals, will screenshot those shortly. will profile with just reselect way as well.

[markerikson]

@kelly-tock fwiw the thing that would help the most is a sandbox that specifically replicates what you're seeing here.

[kelly-tock]

totally get it. so far I have a decent sandbox, but am unable to reproduce. the app i'm working on is incredibly complex(I know we all say that), so it has been hard to diagnose thus far.

[kelly-tock]

my guess is that is a huge number of small things adding up.

[kelly-tock]

every memoize in that stack for 4.1.5 is coming from this block in reselect in defaultMemoize:

every anonymous there in the long stack of back and forth between anonymous and memoize is in this part of reselect:

in the 4.0.0 version it stops at re-reselect, guessing because its a cache hit, but I can't see why it wouldn't be a cache hit in both cases.

[kelly-tock]

with just regular reselect selector, same result for 4.1.5 and re-reselect:

export const getCalendarSquareCalendar = createSelector(
  calendarSquaresByTableAndTimeSelector,
  calendarSquareId,
  dateTimeString,
  tableId,
  getCalendarSquareById
);

and

[kelly-tock]

looks the same with a max size option as well:

export const getCalendarSquareCalendar = createSelector(
  calendarSquaresByTableAndTimeSelector,
  calendarSquareId,
  dateTimeString,
  tableId,
  getCalendarSquareById,
  {
    memoizeOptions: {
      maxSize: 1000,
    },
  }
);

[kelly-tock]

also tried above with the result option, using shallowEqual. same result. tried a pattern for creating a new selector instance for each component instead, similar results.

[markerikson]

@kelly-tock Can you put together a CodeSandbox or repo that demonstrates this? I really need something I can try to dig into and profile myself.

[kelly-tock]

yeah, working on it. just wanted to try out the suggestions above, and report back. here's 4.0.0 with creating a new selector instance for each component:

[kelly-tock]

and runs just night and day faster.

[markerikson]

I'd really like to look at this sometime in the next few days. Could you please put together a CodeSandbox or a Github repo that fully demonstrates the issue happening? That would really help me be able to investigate what's going on. (Recording a replay using https://www.replay.io/ might be an option as well.)

[kelly-tock]

I’ve been on vacation. I’ll link what I have so far, but if there’s a way to send a private profile file from chrome we can maybe do that? Our app is incredibly complex with 100s of selectors building different views of data

[markerikson]

You could zip it and send DM it to me over on Discord, but I really am going to need to actually dig into real running code to see what's going on myself. Doesn't have to be the full app or anything - a minimal repro would be wonderful (like, a single index.js file that constructs a selector and uses it or something, and shows similar behavior).

[kelly-tock]

I haven't gotten it fully repro-ing, but heres what I have so far:
https://github.com/kelly-tock/reselect-repro

What we do a lot of currently is receive things as arrays, and re-map them into object lookups by id.

we also compute various properties from combining several data sources, and have thousands of items.

when we use 4.0.0 performance is fine, but 4.1.5 makes the app completely unusable.

[kelly-tock]

let me know if you would like the profiles, as recreating this so far has been problematic.

[markerikson]

Thanks. Looks like you've got a full-ish app there - can you point to the specific relevant bits or summarize the steps to reproduce the issue? (and if you can narrow down the demo code to just the selector-relevant parts that might also help)

And yeah, if you can attach the perf traces (zipped) that would probably be helpful too.

Most likely won't have time to look at this until later this week.

[markerikson]

@kelly-tock : hey, if you're having trouble recreating the issue standalone, could you try using https://replay.io to record a replay of the bug on your own system? I haven't used it for real yet, but it ought to be tailor-made for this kind of a bug investigation scenario.

[kelly-tock]

will send profiles and try this out today. this is a fun one!

[kelly-tock]

@markerikson what discord are you referring to? reactiflux? I can dm you about the replay.io and profile stuff.

[kelly-tock]

can't post that link here, would need your email address to add to it if you want to dm me there.

[markerikson]

FYI I tried to do some analysis on this today based on the perf profiles, as well as a Replay I made of the demo app running locally. No concrete answers yet - I'll have to poke at it further.

I actually did the investigation as part of a livestream - that video is up on Youtube if you want to see my process:

https://www.youtube.com/watch?v=GvLGEuxV6BU

I do have one question: are you using Lodash's _.isEqual as part of a customized createSelectorCreator somewhere in the codebase? I distinctly saw some stacks of isEqual in the full app perf traces.

[kelly-tock]

I believe we do have a selector with that in our codebase that I had forgotten about. Will dig that up as well. Watching the process is super interesting thanks for doing that!

[kelly-tock]

Also, in dev tools you can hit command + f/control+f and find the function. That is why I named the function in the useAppSelector call.

[kelly-tock]

a handful of selectors are created using the method in the docs with lodash.isEqual:

https://github.com/reduxjs/reselect#customize-equalitycheck-for-defaultmemoize

import { createSelectorCreator, defaultMemoize } from 'reselect';
import _ from 'lodash';

export const createDeepEqualSelector = createSelectorCreator(defaultMemoize, _.isEqual);

[kelly-tock]

ok, interesting, as an experiment I changed it to be this:

import { createSelectorCreator, defaultMemoize } from 'reselect';
// import _ from 'lodash';

export const createDeepEqualSelector = createSelectorCreator(defaultMemoize);

i'm guessing this is the same as just the normal createSelector with this?

and this caused the app to behave normally again. maybe this is a clue as to where the issue is?

[markerikson]

Yes, that's exactly the standard behavior for createSelector out of the box - createSelectorCreator with defaultMemoize as the memoization function.

Will have to poke at this further, and that's definitely an interesting clue.

[koenoe]

ok, interesting, as an experiment I changed it to be this:

import { createSelectorCreator, defaultMemoize } from 'reselect';
// import _ from 'lodash';

export const createDeepEqualSelector = createSelectorCreator(defaultMemoize);

i'm guessing this is the same as just the normal createSelector with this?

and this caused the app to behave normally again. maybe this is a clue as to where the issue is?

I've had some issues with re-reselect and reselect (latest version) too. Decided to drop re-reselect and create my own simple implementation on top of the new API of reselect:

import { createSelectorCreator, defaultMemoize, EqualityFn } from 'reselect';

export const createCachedSelector = (equalFn: EqualityFn) =>
  createSelectorCreator(defaultMemoize, {
    maxSize: 100,
    resultEqualityCheck: equalFn,
  });

Example usage:

export const selectMovie = createCachedSelector(
  (prev: Movie, next: Movie) => prev.id === next.id,
)(
  selectContext,
  (state: MoviesMachine, id: number) => id,
  (context, id) => context[id],
);

It's very simple and might not be as advanced as re-reselect, but it does the job for me.