fix: memory leak related to re-reselect cache

[nickofthyme]

Summary

Fixes memory leak related to re-reselect cache holding onto old chart store references.

Details

Now selectors are creeated using a centralized method createCustomCachedSelector to facilitate keySelector and objectCache for all selectors in one place.

- selector = createCachedSelector(
+ selector = createCustomCachedSelector(
  [getSpecOrNull, getPickedShapesLayerValues, getSettingsSpecSelector],
  getOnElementOverSelector(prev),
- )({
-  keySelector: getChartIdSelector,
-});
+);

// Or similarly 
- selector = createCachedSelector(
+ selector = createCustomCachedSelector(
  [getSpecOrNull, getPickedShapesLayerValues, getSettingsSpecSelector],
  getOnElementOverSelector(prev),
- )((state) => state.chartId);
+);

There is an added lint rule to prevent importing createCachedSelector from re-reselect.

The types for the new createCustomCachedSelector method are @ts-ignored in order to facilitate this central method for creating selectors as the createCachedSelector type uses over 90 overrides with generics to type the function.

Before

Detached nodes in memory heap snapshot point to chart id in FlatObjectCache in re-reselect. The FlatObjectCache keeps around chart states based on ids that have since been randomized and recreated, thus the _cache grows proportional to the count of remounted charts.👇🏼

Screen.Recording.2021-06-10.at.03.03.19.PM.mp4

Dom elements are much higher and continue to grow after charts are mounted and unmounted repeatedly, even after forced garbage collection. 👇🏼

Screen.Recording.2021-06-10.at.02.59.28.PM.mp4

After

Detached nodes in memory heap snapshot no longer point to chart id in FlatObjectCache in re-reselect. 👇🏼

Screen.Recording.2021-06-10.at.02.32.20.PM.mp4

Dom elements are now being garbage collected after charts are mounted and unmounted repeatedly. 👇🏼

Screen.Recording.2021-06-10.at.02.29.19.PM.mp4

Issues

Related to #1148, still need to address tooltip_portal memory leaks

See https://github.com/elastic/elastic-charts/wiki/Memory-leaks for tips and tricks for debugging memory leaks related to DOM elements.

Checklist

• The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
• The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
• Whenever possible, please check if the closing issue is connected to a running GH project
• Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
• This was checked for cross-browser compatibility
• Proper documentation or storybook story was added for features that require explanation or tutorials
• Unit tests were updated or added to match the most common scenarios

[markov00]

Overall looks good to me.
I've left few comments that we should discuss before final approval

[monfera]

That's a fantastic TS approach, I wish I thought of it! So, basically, you can wrap re-reselect around, without having to duplicate its dozen or to signatures for variadic-ness.

Maybe not for this PR: since before joining the team, we've been talking with @markov00 about using a commonly used, FP-style function signature for a createCachedSelector wrapper. So, instead of

const plainFun = (a1, a2, a3) => { whatever }
const newSelector = createCustomCachedSelector([arg1, arg2, arg3], plainFun)

it'd look like

const plainFun = (a1, a2, a3) => { whatever }
const newSelector = select(plainFun)(arg1, arg2, arg3)

Advantages:

• no need for an array, which is a bit of anti-pattern, as these are arguments
• currying order is sensible: partial application would start with the function argument, not the list of inputs
• we don't bake in some specific library's awkward signature convention and overly specific function name, therefore we can easily switch from re-reselect to eg. data flow libraries like rxjs, flyd or anything we devise (liteFields uses crosslink though maybe not yet wrapped in a select)
• we already use this, FP-wise common place signature in Canvas, all over the place here
• lastly, name (just select): shorter is better, and doesn't encode how it works; there's not a big variety

So I wonder if maybe this could be done in this PR, as it touches so many files anyway. Could of course be a separate one but then it again touches so many files.

FP note: this kind of select operation is called lift operation, because it lifts a normal, plain old function such as (a, b) => a + b into the realm of a higher order approach (streams, transducers, whatever), eg. our current practice of composing with selectors. Often, it's called, appropriately, a lift operation, eg. for flyd.

One real world application to such plumbing is that it's possible to (eventually) be smarter about executing data pipelines. For example, recognizing that some functions have one input and one user, you can identify if you have some stretches of pipelineable operators, and you can just execute a(b(c(...args))) ie. compose, instead of caching or whatnot at every single function boundary, and it's only the shallowest optimization opportunity.

[monfera]

Oh, wanted to list the drawbacks I know of, too:

[end of list]
😺

[nickofthyme]

I think this could be a good idea, I am generally fine with this change. The one issue I see is that the types as they are now, the inputSelectors arguments passed to createSelector implicitly create the type of the resultFunc very nicely. In your new approach we would need to define the type of the resultFunc for all selectors. Thus an example would be...

// fake selectors
const getString = (): string => 'hello';
const getNum = (): number => 21;
const getArray = (): number[] => [1, 2];

// current
const newSelector = createCustomCachedSelector([getString, getNum, getArray], (s, n, a) => {
  console.log(s); // knows it's a string
  console.log(n); // knows it's a number
  console.log(a); // knows it's an array
})

// new approach
const newSelector = createCustomCachedSelector((s, n, a) => {
  console.log(s); // assumes any
  console.log(n); // assumes any
  console.log(a); // assumes any
})(getString, getNum, getArray)

Thus this change would add a lot more duplicated (i.e. non-derived) typings, than the current approach.

[monfera]

Thanks @nickofthyme! The 2nd example would not use the array, ie.

// new approach
const newSelector = select((s, n, a) => {
  console.log(s); // assumes any
  console.log(n); // assumes any
  console.log(a); // assumes any
})(getString, getNum, getArray)

Probably it doesn't make a difference regardig your point. Would be great if we could "pick" bits and pieces of an existing function signature. Likely not possible though

[nickofthyme]

Thanks I updated my comment but yeah that doesn't change my point.

[nickofthyme]

So after looking into some form of the proposed selector I was thinking we could use the function parameters to type the final selectors something like this...

function selector<R>(fn: (...args: any[]) => R) {
  return (...args: Selector<GlobalChartState, unknown>[]): R =>
    createCachedSelector(args, fn)(globalSelectorCache.getNewOptions());
}

but enumeration of array values is not possible as it is with keyof for object types, thus all array types are unioned together, for example...

const selector => (...args: any[]) return 'test';
selector(1, 'two', 3) // assumes args is (number|string)[] not [number, string, number]

Thus the only solution would be to enumerate many instances of the type. For example, if we had two selectors we could do something like...

function selector<R, A1, A2>(fn: (arg1: A1, arg2: A2) => R) {
  return (s1: Selector<GlobalChartState, A1>, s2: Selector<GlobalChartState, A2>): R =>
    createCachedSelector(s1, s2, fn)(globalSelectorCache.getNewOptions());
}

// then calling this new selector would look like...
const geometries = selector(
  (specs: SpecList, parentDimensions: Dimensions): ShapeViewModel => {
    const goalSpecs = getSpecsFromStore<GoalSpec>(specs, ChartType.Goal, SpecType.Series);
    return goalSpecs.length === 1 ? render(goalSpecs[0], parentDimensions) : nullShapeViewModel();
  },
);

We could automate a node script to generate the enumerated type overloads for the selector function up to a certain number of selectors. The underlying logic would be simple to just use spread operators to handle any number of types, something like...

function selector(fn) {
  return (...selectors): R =>
    createCachedSelector(selectors, fn)(globalSelectorCache.getNewOptions());
}

One thing I'm not sure about is how this will affect the number of selectors that are created since every call to the returned selector would call createCachedSelector and return a new selector.

Actually I don't think this ☝🏼 would be an issue.

Any thoughts? @monfera @markov00

[monfera]

What would happen, if instead of today's signature (btw should it be unknown instead of any?)

export const selector: typeof createSelector = (...args: any[]) => {
  return createCachedSelector(...args)(globalSelectorCache.getNewOptions());
};

it'd be

declare function select<R, S, A, B>(fun: (a: A, b: B) => R): (a: S => A, b: S => B) => (S => R)

where R is the result type, S is some opaque state object, and A and B are the two params of the plain function that we lift.

If this is repeated for all options re-reselect already supports, ie. 12 params, then we have 12 + 1 lines, like

declare function select<R, S>(fun: () => R): () => (S => R) // 0 args
declare function select<R, S, A>(fun: (a: A) => R): (a: S => A) => (S => R) // 1 arg
declare function select<R, S, A, B>(fun: (a: A, b: B) => R): (a: S => A, b: S => B) => (S => R) // 2 args
declare function select<R, S, A, B, C>(fun: (a: A, b: B, c: C) => R): (a: S => A, b: S => B, c: S => C) => (S => R) // 3 args
...

at least I had approx. this mental model, but haven't tried, is it something that can be ruled out?

[nickofthyme]

Do you mean...?

declare function select<R, S, A, B>(fun: (a: A, b: B) => R): (f1: (a: S) => A, f2: (b: S) => B) => (c: S) => R; // 2 args

Could you use select in an example as you describe above, without types.

[monfera]

Yes, totally haven't added the arg names 👯

An example would be something like:

// opaque state object
const s = {
  one: 14,
  other: 32,
  somethingElse: 9
}

// primitive selectors
const getOne = s => s.one
const getTheOther = s => s.other
const getSomethingElse = s => s.somethingElse

// plain functions we're gonna lift
const add = (a, b) => a + b
const mul = (a, b) => a * b
const sub = (a, b) => a - b

// ze selector maker
const select = f => (...inputSelectors) => s => f(...inputSelectors.map(sel => sel(s)))

// selectors
const getSum = select(add)(getOne, getTheOther)
const getMul = select(mul)(getTheOther, getSomethingElse)
const getSub = select(sub)(getMul, getSum)

// the test
console.log(getSub(s))
// => 242 // === (32 * 9) - (14 + 32)

Did you mean an example like this?

[markov00]

Everything is good for me. I agree with @monfera about optimizing the signature, but is definitely something that we can do in a second stage

[nickofthyme]

🎉 This PR is included in version 31.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀