Simplifies shouldComponentUpdate (open discussion) #78

mikaelbr · 2015-02-28T16:31:55Z

Simplifications to shouldComponentUpdate now as we have _.isEqual.

Started implementation based on some comments from #76 - and this is a continuation of the discussion. Not yet implemented "rebase" support. As that may be a separate change.

Nor is comparable implemented. As there are some discussion if we need to do deep equal on immutable structures that has been said to not be equal through reference checks.

torgeir · 2015-02-28T18:31:13Z

shouldupdate.js

-    if (currentCursorsKeys.length !== nextCursorsKeys.length) {
-      if (debug) debug.call(this, 'shouldComponentUpdate => true (number of cursors differ)');
+    if (nextProps !== this.props) {
+      if (debug) debug.call(this, 'shouldComponentUpdate => true (equal input)');


*unequal? 😄

torgeir · 2015-02-28T18:37:36Z

Faster/easier should component update 👍 !

Though, I think for something like this we need some hard numbers on comparison speeds before and after these changes.

mikaelbr · 2015-02-28T18:39:25Z

I'm working on a benchmark. Copied Immutable.js' benchmark and trying to run it now. It compares previous dist file with current uncommitted dist.

torgeir · 2015-02-28T18:39:55Z

shouldupdate.js

+      var isNextImmutable = _isImmutable(next);
+
+      if (isCurrentImmutable && isNextImmutable) {
+        return current.equals(next);


While I kind of like the idea of an interface for saying you want component args to trigger renders or not when changed, I'm still not conviced Immutable.js .equals() is the way to go.

…parable

dashed · 2015-02-28T22:55:47Z

shouldupdate.js

-      if (debug) debug.call(this, 'shouldComponentUpdate => true (cursors have changed)');
-      return true;
-    }
+    var isNotIgnorable = not(or(isStatics, isChildren));


Move this outside of shouldComponentUpdate for caching. No need to reconstruct this higher-order function on every render.

dashed · 2015-02-28T23:11:18Z

👍 Awesome! shouldComponentUpdate looks so much easier to read, extend, and refactor.

@mikaelbr Looking forward to see the benchmarks.

I wonder if isEqualProps and isEqualState can be made more DRY.

@Gozala Do you agree that there's a compromise where one can override isEqualState and/or isEqualProps for a more aggressive shouldComponentUpdate?

dashed · 2015-02-28T23:18:16Z

shouldupdate.js

-    }
+    var isNotIgnorable = not(or(isStatics, isChildren));
+    var nextProps    = filter(nextProps, isNotIgnorable),
+        currentProps = filter(this.props, isNotIgnorable);


Move this below _isEqualState block for better placement:

var nextProps = filter(nextProps, isNotIgnorable), currentProps = filter(this.props, isNotIgnorable);

Reasoning is that it could be a pointless computation if !_isEqualState(this.state, nextState) === true.

mikaelbr · 2015-02-28T23:30:44Z

Thanks for good feedback.

I haven't gotten the benchmark to work as I want yet, and haven't had the time to work on it too much, but it'll get there.

dashed · 2015-02-28T23:34:28Z

shouldupdate.js

@@ -236,6 +184,22 @@ function factory (methods) {
  }
 }

+function compare (current, next, comparator, equalCheck) {


Just a nitpick. I think comparator is the wrong name.

Maybe rename to typecheck or similar.

I had a hard time choosing a name. I agree. Fixing

dashed · 2015-02-28T23:35:51Z

New changes looks good to me. 😄

dashed · 2015-02-28T23:50:33Z

shouldupdate.js

-    if (hasChangedCursors(currentCursors, nextCursors)) {
-      if (debug) debug.call(this, 'shouldComponentUpdate => true (cursors have changed)');
-      return true;
+    if (nextProps === this.props) {


What if assert(nextProps === this.props && nextState !== this.state)?

You're right.

mikaelbr · 2015-03-01T00:20:37Z

Here's the initial relative benchmarks for the current dist and this dist:

shouldComponentUpdate on no input
  Old:   180,556   182,225   183,925 ops/sec
  New:   255,906   258,463   261,071 ops/sec
  compare: 1 1
  diff: 41.83%
  rme: 0.96%
shouldComponentUpdate when immutables are same
  Old:    71,202    71,945    72,703 ops/sec
  New:   107,211   108,415   109,646 ops/sec
  compare: 1 1
  diff: 50.69%
  rme: 1.08%
shouldComponentUpdate when many immutables are passed as the same reference
  Old:    10,573    10,878    11,201 ops/sec
  New:    11,915    12,060    12,208 ops/sec
  compare: 1 1
  diff: 10.86%
  rme: 2.21%
shouldComponentUpdate when large object is passed
  Old:    20,329    20,540    20,754 ops/sec
  New:    22,322    22,502    22,685 ops/sec
  compare: 1 1
  diff: 9.55%
  rme: 0.92%
shouldComponentUpdate when two large object is passed
  Old:     1,265     1,280     1,296 ops/sec
  New:     1,523     1,537     1,552 ops/sec
  compare: 1 1
  diff: 20.07%
  rme: 1.06%
shouldComponentUpdate when two large object is passed but as same input
  Old:     2,381     2,424     2,469 ops/sec
  New:    29,189    29,421    29,656 ops/sec
  compare: 1 1
  diff: 1113.38%
  rme: 1.39%
shouldComponentUpdate when immutables are different
  Old:    40,521    42,657    45,029 ops/sec
  New:    42,122    45,864    50,336 ops/sec
  compare: 1 1
  diff: 7.51%
  rme: 7.3%
shouldComponentUpdate when long arrays (1000 elements) are the same
  Old:     1,587     1,623     1,660 ops/sec
  New:     8,076     8,466     8,895 ops/sec
  compare: 1 1
  diff: 421.57%
  rme: 3.76%
shouldComponentUpdate when long arrays (1000 elements) are the same reference
  Old:    13,032    13,229    13,432 ops/sec
  New:    13,451    13,720    14,000 ops/sec
  compare: 1 1
  diff: 3.71%
  rme: 1.77%
shouldComponentUpdate when same input is passed (long array, 1000 items)
  Old:     6,381     6,461     6,543 ops/sec
  New:     8,359     9,117    10,027 ops/sec
  compare: 1 1
  diff: 41.1%
  rme: 6.47%
all done

Haven't looked too much into this, so these numbers might be wrong, but they seem reasonable. I didn't get cursors to work, but immutable structures should works just as good.

The tests can be found here: https://gist.github.com/mikaelbr/b4760464013fe636186f
Again, I haven't looked too much into this and mostly just copy/pasted and tested it out - so there's very likely that there are some dumb tests.

Tests for SCU quick exit for #78

dashed · 2015-03-01T00:55:16Z

Here are my results with iojs-v1.4.1.

shouldComponentUpdate on no input
  ...running...  
  Old:   134,954   137,313   139,756 ops/sec
  New:   131,645   136,062   140,785 ops/sec
  compare: 1 1
  diff: -0.92%
  rme: 2.67%
shouldComponentUpdate when immutables are same
  ...running...  
  Old:    46,624    51,573    57,696 ops/sec
  New:    55,795    65,344    78,838 ops/sec
  compare: 1 1
  diff: 26.7%
  rme: 14.24%
shouldComponentUpdate when many immutables are passed as the same reference
  ...running...  
  Old:     7,545     7,653     7,764 ops/sec
  New:     8,099     8,217     8,339 ops/sec
  compare: 1 1
  diff: 7.37%
  rme: 1.44%
shouldComponentUpdate when large object is passed
  ...running...  
  Old:    10,774    10,864    10,956 ops/sec
  New:    11,476    11,635    11,800 ops/sec
  compare: 1 1
  diff: 7.09%
  rme: 1.14%
shouldComponentUpdate when two large object is passed
  ...running...  
  Old:       649       662       676 ops/sec
  New:       838       852       867 ops/sec
  compare: 1 1
  diff: 28.71%
  rme: 1.89%
shouldComponentUpdate when two large object is passed but as same input
  ...running...  
  Old:     1,134     1,149     1,165 ops/sec
  New:    14,020    14,176    14,334 ops/sec
  compare: 1 1
  diff: 1132.72%
  rme: 1.22%
shouldComponentUpdate when immutables are different
  ...running...  
  Old:    27,957    28,465    28,991 ops/sec
  New:    40,528    41,205    41,906 ops/sec
  compare: 1 1
  diff: 44.75%
  rme: 1.74%
shouldComponentUpdate when long arrays (1000 elements) are the same
  ...running...  
  Old:       585       593       602 ops/sec
  New:     2,243     2,285     2,329 ops/sec
  compare: 1 1
  diff: 285.03%
  rme: 1.65%
shouldComponentUpdate when long arrays (1000 elements) are the same reference
  ...running...  
  Old:     4,664     4,749     4,837 ops/sec
  New:     4,758     4,849     4,943 ops/sec
  compare: 1 1
  diff: 2.11%
  rme: 1.86%
shouldComponentUpdate when same input is passed (long array, 1000 items)
  ...running...  
  Old:     4,529     4,628     4,732 ops/sec
  New:     5,084     5,179     5,278 ops/sec
  compare: 1 1
  diff: 11.89%
  rme: 2.04%
all done

dashed · 2015-03-01T01:21:37Z

As a joke, I added a minor improvement: dashed@74e91b6

Results:
https://raw.githubusercontent.com/Dashed/omniscient/6121335a3a4c8049632f424ba54d6405d0e2efde/bench/results.md

dashed · 2015-03-01T02:43:23Z

The benchmark was flawed. Immutable structures weren't actually created at all!

Here is why: /~https://github.com/Dashed/omniscient/blob/c31930e70023d5d524d23ffc48919c8460cc97b3/perf/tests.js#L5-L11

I augmented the benchmarks with two working cursor benchmarks:
dashed@c31930e

My results: /~https://github.com/Dashed/omniscient/blob/c31930e70023d5d524d23ffc48919c8460cc97b3/bench/results.txt

@mikaelbr Feel free to add more cursor tests.

torgeir · 2015-03-01T20:58:19Z

a reasonable assumption would be omniscient folks use more props than state - should we move the props check before the state check?

mikaelbr · 2015-03-01T21:01:33Z

I made the same assumption, but a different conclusion. As state is rarely used, it's most likely smaller than props and thus if they differ the check will be cheaper. Also, if it is empty (as it is 98% of the time) the check is also cheap as you don't have to do a lot of iterations. Might add som benchmarks to see the actual numbers - when we get confident benchmarks.

dashed · 2015-03-01T23:44:55Z

Maybe we can make the component smarter to adjust priority in checks based on previous re-renders?

dashed · 2015-03-01T23:52:04Z

Tradeoff is that a new shouldComponentUpdate instance would be instantiated on every component call.

dashed · 2015-03-02T02:15:20Z

We can utilize this to decide whether to run _isEqualProps or _isEqualState (since either are expensive computations).

http://www.loper-os.org/bad-at-entropy/manmach.html
https://www.reddit.com/comments/1p3ti1

We can modify this so that when the Machine passes, we use random chance to execute _isEqualProps and _isEqualState in any order.

It isn't a heavyweight predictive modeler, but decent enough 😄

dashed · 2015-03-02T02:16:35Z

This would be a consideration for a separate PR once this PR simmers.

mikaelbr · 2015-03-05T07:13:40Z

I'm merging this now as a simple code optmimization/reduction. We're thinking about the .equals approach some more, so that would be a separate implementation anyways. And make #82 and #85 redundant.

Simplifies shouldComponentUpdate (open discussion)

facundocabrera · 2015-04-20T15:55:05Z

@mikaelbr, @dashed Can I ask how are you running the benchmarks? I want to analyze my current implementation using omniscient with the same approach (I'm switching from v2 to v3).

Thanks!

mikaelbr · 2015-04-20T16:44:33Z

Used a variation of immutable.js benchmarks: https://gist.github.com/mikaelbr/c7e37f5865c494a6e0fa

Gozala · 2015-04-20T17:38:07Z

@Gozala Do you agree that there's a compromise where one can override isEqualState and/or isEqualProps for a more aggressive shouldComponentUpdate?

I am not sure what the context of this question is. In principal having a control over each one sounds right though. As of my personal take I don't use omniscient for components with a local state, those in my code are usually shims that I use react API directly.

mikaelbr force-pushed the shouldComponentUpdateSimplified branch from 17a7db2 to 446ed40 Compare February 28, 2015 18:23

mikaelbr changed the title ~~Simplifies shouldComponentUpdate and switch from isImmutable to isComparable~~ Simplifies shouldComponentUpdate (open discussion) Feb 28, 2015

torgeir reviewed Feb 28, 2015
View reviewed changes

mikaelbr force-pushed the shouldComponentUpdateSimplified branch from 1d36b75 to d9541ef Compare February 28, 2015 18:34

torgeir reviewed Feb 28, 2015
View reviewed changes

Simplifies shouldComponentUpdate and switch from isImmutable to isCom…

fec0172

…parable

mikaelbr force-pushed the shouldComponentUpdateSimplified branch from 7972811 to fec0172 Compare February 28, 2015 18:43

torgeir mentioned this pull request Feb 28, 2015

Simplify shouldComponentUpdate #76

Closed

dashed reviewed Feb 28, 2015
View reviewed changes

mikaelbr added 3 commits March 1, 2015 00:18

Refactors and dries up code

b38f53a

Reducing work by moving code to prioritized order

7eee6dd

Fixes equality check for immutables

61c9641

dashed reviewed Feb 28, 2015
View reviewed changes

Renames parameter for compare

e29bdef

dashed reviewed Feb 28, 2015
View reviewed changes

mikaelbr and others added 2 commits March 1, 2015 00:52

Also checks on state for quick exit on SCU

fe78199

tests for SCU quick exit

77cf4e3

dashed mentioned this pull request Mar 1, 2015

Tests for SCU quick exit for #78 #79

Merged

Merge pull request #79 from Dashed/shouldComponentUpdateSimplified

6a83813

Tests for SCU quick exit for #78

This was referenced Mar 2, 2015

Should shouldComponentUpdate use valueOf to deref so doesn't have to check if it is a cursor? #82

Closed

Treat cursors as transparent type by accessing internal value through valueOf #85

Closed

Custom cursor implementation omniscientjs/immstruct#41

Closed

mikaelbr added a commit that referenced this pull request Mar 5, 2015

Merge pull request #78 from omniscientjs/shouldComponentUpdateSimplified

9f3715c

Simplifies shouldComponentUpdate (open discussion)

mikaelbr merged commit 9f3715c into master Mar 5, 2015

mikaelbr deleted the shouldComponentUpdateSimplified branch September 26, 2015 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplifies shouldComponentUpdate (open discussion) #78

Simplifies shouldComponentUpdate (open discussion) #78

mikaelbr commented Feb 28, 2015

torgeir Feb 28, 2015

torgeir commented Feb 28, 2015

mikaelbr commented Feb 28, 2015

torgeir Feb 28, 2015

dashed Feb 28, 2015

dashed commented Feb 28, 2015

dashed Feb 28, 2015

mikaelbr commented Feb 28, 2015

dashed Feb 28, 2015

mikaelbr Feb 28, 2015

dashed commented Feb 28, 2015

dashed Feb 28, 2015

mikaelbr Feb 28, 2015

mikaelbr commented Mar 1, 2015

dashed commented Mar 1, 2015

dashed commented Mar 1, 2015

dashed commented Mar 1, 2015

torgeir commented Mar 1, 2015

mikaelbr commented Mar 1, 2015

dashed commented Mar 1, 2015

dashed commented Mar 1, 2015

dashed commented Mar 2, 2015

dashed commented Mar 2, 2015

mikaelbr commented Mar 5, 2015

facundocabrera commented Apr 20, 2015

mikaelbr commented Apr 20, 2015

Gozala commented Apr 20, 2015

Simplifies shouldComponentUpdate (open discussion) #78

Simplifies shouldComponentUpdate (open discussion) #78

Conversation

mikaelbr commented Feb 28, 2015

Choose a reason for hiding this comment

torgeir commented Feb 28, 2015

mikaelbr commented Feb 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashed commented Feb 28, 2015

Choose a reason for hiding this comment

mikaelbr commented Feb 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashed commented Feb 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelbr commented Mar 1, 2015

dashed commented Mar 1, 2015

dashed commented Mar 1, 2015

dashed commented Mar 1, 2015

torgeir commented Mar 1, 2015

mikaelbr commented Mar 1, 2015

dashed commented Mar 1, 2015

dashed commented Mar 1, 2015

dashed commented Mar 2, 2015

dashed commented Mar 2, 2015

mikaelbr commented Mar 5, 2015

facundocabrera commented Apr 20, 2015

mikaelbr commented Apr 20, 2015

Gozala commented Apr 20, 2015