Performance issue with ``notifyObservers``

[mindjuice]

I was just tracking down a performance issue and have across what seems to be the root of the issue.

I would type into an <input> control whose value is managed by a Nuclear store, but the characters would take a long time to appear (150-200ms). This was the time from dispatching the action, until the render method was called.

Tracking this down I found that in notifyObservers, some calls to evaluate() were consistently taking a long time and not being cached.

This turned out to be related to a getter that looked like this (with about 2MB of data):

export let data = [
  ['query', 'data'],
  (data) => {
    return data ? data.toJS() : undefined;
  }
];

Changing the return to the following fixed this (and other) performance issues:

return data;

I needed to add a call in a dependent getter to data.toJS() though to compensate (this data is not used directly, but only as dependencies of other getters).

The problem seems to be that data.toJS() returns a new object every time, so the getter is never cached. Moving it up the chain just "fixed" the problem by avoiding it though.

So my question is, what are the recommended practices for when and where to call toJS()? I feel like components should only work with plain objects, so currently all my getters return plain objects, not ImmutableJS objects, but I'm not sure if that's how others are using Nuclear.

Thanks.

[jordangarcia]

I would recommend getters that are used by other getters to always return immutable versions of objects.

And getters that are consumed by components can return coerced toJS objects. This way the majority of cases where getters are dependencies of many other getters we are being as efficient as possible with using immutable data structures.

[mindjuice]

In my case, the getter was the root for several other getters, but was also used directly in a component.

I suppose I could write another simple getter to wrap the Immutable one, whose only purpose is to do the .toJS().

This seems like a separation worth mentioning in the docs somewhere.

[mindjuice]

Thinking about this a bit more, it seems wrong for some getters to return Immutable objects and others to return JS objects. It makes it harder to compose them, and you may end up with two getters for each piece of data, one for composing and one for using the data in a component.

What if, instead, getters always return ImmutableJS objects, so composing them is easy, and then have Nuclear call .toJS() on the result of a getter before injecting it to the state of a component (caching the result of course)? That way components always deal with plain JS objects.

[mindjuice]

So for now, I've refactored my getters so they are all using ImmutableJS, but I renamed them to end in "Imm". None of these are directly used by components. They are only used to compose. Then I added new getters named without "Imm" that the components actually use.

// This is the original data source and it set to an Immutable map
export let data = ['query', 'data'];

// This getter is only used as a dependency of passFailSummaryImm
export let summaryImm = [
  data,
  (data) => {
    // Do some ImmutableJS manipulations here
    return result;
  }
];

// This getter is only used as a dependency of passFailSummary
export let passFailSummaryImm = [
  summaryImm,
  (summary) => {
    // Do some ImmutableJS manipulations here
    return result;
  }
];

// This getter is the one my components actually use
export let passFailSummary = [
  passFailSummaryImm,
  (summary) => summary ? summary.toJS() : undefined
];

My non-immutable getters were previously taking 2500ms in notifyObservers(). Now they take 50-60ms.

When I have some time, I think I will change my smart components to use ImmutableJS too, and have them do any toJS() calls that might be necessary to pass down pure JS objects/primitives to my dumb components. I think this will result in many fewer parts of the state tree being converted to JS, which should further improve performance.

Then I can also get rid of the non-Immutable getters and drop the "Imm" extension.

[mindjuice]

I meant to also mention that I am okay to close this issue now. I previously thought that it was expected to always convert to JS before using data in components, but now I see that you may or may not want to do that, so having Nuclear do that for you automatically would not always be what you want.

In fact, it's probably better to always use Immutable where you can in your smart components.

If automatic toJS() calls are something others are actually interested in though, it should be made opt-in, perhaps at initialization time.