optimizing shouldComponentUpdate

[krawaller]

Since shouldComponentUpdate is hot code, I figured it'd be worth optimising it by paying some readability. This PR therefore...

• simplifies refsChanged checks if we just have a single ref field
• simplifies valuesChanged if we aren't ignoring any ref fields
• preruns the ignore union
• minimizes object lookups
• only calculates refsChanged if valuesChanged wasn't true
• minimizes function calls

[dustingetz]

To have any chance at merging this we need to finish up adding tests which is in progress.

Do you have profiler numbers on this commit? react-cursor is not a hot spot in any of my apps.

[krawaller]

No numbers, but could do a js-perf.

Yeah, I saw the test stub, and figured as much. Looking forward to them appearing! :)

One thing - I made the optimised version functionally equivalent to the old one, but I never really understood the relation between refsChanged and valuesChanged. In my mind we'd never have a situation where refsChanged will pass and not valuesChanged, and if valuesChanged fails then refsChanged will also fail. Thus I couldn't see why we couldn't just test for refsChanged. What am I missing?

[krawaller]

Also I didn't understand why we just ignore fields in the valuesChanged test, but I kept that functionality too.

[dustingetz]

I've been thinking about this PR. I think that right now, we don't have a sufficient understanding of what to optimize. On desktop platforms, the performance of the two apps I've built with react-cursor is already sufficient (performance problems are what led to developing react-cursor in the first place). On other platforms like, say, an xbox Netflix app, I expect that memory usage/leaks and closure allocations are going to be the hot spots. So until we have more specific insight into how people use this app on a variety of platforms, I want to keep the code as simple as possible.

[krawaller]

You're right, this optimisation is premature. Sorry! Closing this!

I'd still like to understand the test though. If we want a reference change to mean that we should update, why bother looking at value change at all?

[dustingetz]

Each React component has several props. For each prop, shouldComponentUpdate needs to know whether to use === (e.g. for cursors and functions - reference equality semantics), or to use _.isEqual (e.g. for maps or lists - value equality semantics). It's an either/or thing, no prop is both.

> [] == []
false
> [] === []
false
> {} == {}
Uncaught SyntaxError: Unexpected token == VM60:732
x = {};
Object {}
> y = {}
Object {}
> x == y
false
> x === y
false

[krawaller]

Yes, I understand the concept, but not the implementation.

It's an either/or thing, no prop is both.

Doh, sorry, now I get it! I was thrown by array-union, I for some reason stupidly thought it returned the common elements. So here's the resulting functionality for the possible combinations in parameter inclusion:

Variable	refFields	ignoredFields	valuesChanged	refsChanged
a	yes	no	no	yes
b	yes	yes	no	yes
c	no	yes	no	no
d	no	no	yes	no

The fact that ignoredFields is an inverted valuesChanged test wish needs to be clearly documented, along with the either/or approach (even though the latter should be obvious to everyone but monkies and me).

[dustingetz]

ignoredFields means "do not consider this prop in shouldComponentUpdate". Are you saying there is a bug?

The rules are supposed to be:

• All props by default use value equality semantics
• specific props can be singled out to use reference equality semantics
• specific props can be ignored altogether in shouldComponentUpdate

Did you generate that chart? I don't think it is correct. Specifically, 'a' has the wrong results - if 'a' is a refField, then it can't influence valuesChanged, and 'b' doesn't make sense.

[krawaller]

We're saying almost the same thing.

• All props by default use value equality semantics => variable d
• specific props can be singled out to use reference equality semantics => variable a
• specific props can be ignored altogether in shouldComponentUpdate => variables c

The only thing not matching is variable b. As you said, including in both doesn't really make sense, but if I do, the current implementation will do a refFields test. If you want to explain ignoreFields as ignore alltogether, then it should do just that.

[krawaller]

About wrong result in a - I typoed the table (had 5 6 columns in row a) at first, that's probably what you saw. Sorry!

[dustingetz]

I can definitely add documentation about this.

[krawaller]

Please take another look now - I only kept a handful of optimisations, and only those who actually make the code clearer.

A note: if inclusion in refFields and ignoreFields at the same time doesn't make sense, then it doesn't make sense to use union to combine them, as the only advantage that has over concat is that it takes care of duplicates. So I now use concat instead.

Also edited the readme, fixing #22.

[dustingetz]

Yeah, I can merge this. Thanks for all your energy. I need to get the unit tests working first. Hopefully next couple days.

[dustingetz]

merged