add short circuit for patching memoized vnodes #378

andyrj · 2017-09-20T05:39:09Z

this adds a short circuit to patch for memoized components...

example memoize:

/*
* memoize.js
* by @philogb and @addyosmani
* with further optimizations by @mathias
* and @DmitryBaranovsk
* perf tests: http://bit.ly/q3zpG3
* Released under an MIT license.
*/
function memoize( fn ) {
    return function () {
        var args = Array.prototype.slice.call(arguments),
            hash = "",
            i = args.length;
        currentArg = null;
        while (i--) {
            currentArg = args[i];
            hash += (currentArg === Object(currentArg)) ?
            JSON.stringify(currentArg) : currentArg;
            fn.memoize || (fn.memoize = {});
        }
        return (hash in fn.memoize) ? fn.memoize[hash] :
        fn.memoize[hash] = fn.apply(this, args);
    };
}

Users would wrap their components/view with this method to skip patching vnodes which have not changed.

This PR satisifies the following issue: #373

codecov · 2017-09-20T05:41:15Z

Codecov Report

Merging #378 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #378   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         148    150    +2     
  Branches       46     47    +1     
=====================================
+ Hits          148    150    +2

Impacted Files	Coverage Δ
src/app.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df84aba...4f56858. Read the comment docs.

jorgebucaran

Needs tests and a demo to address #373.

😉🙏

okwolf · 2017-09-20T07:20:47Z

src/app.js

@@ -180,6 +180,10 @@ export function app(props) {
  }

  function patch(parent, element, oldNode, node, isSVG, nextSibling) {
+    if (oldNode === node) {
+      console.log("memoized...")


I love console.log as much as the next guy, but perhaps it's not needed here?

was just to help me verify that it was actually running that memoize case, trying to rack down the uncovered line 265 in coverage report LOL

okwolf · 2017-09-20T07:21:43Z

test/app.test.js

+  };
+}
+
+test("skips patching memoized vnodes", done => {


Does this actually check that the same VNode is returned?

trying to figure out how to write the test for that...

I think another approach to this would be to create a simple demo app with one memoized component and one component not. Then somehow benchmark it, and once we understand the problem well, take another shot at this?

You can call the memoized function twice and check for identity equality.

andyrj · 2017-09-20T07:38:45Z

if anyone can help me see why line 265 is not covered after adding the un-connected conditional at the top of patch, it would be appreciated... gonna have another look in the morning after some coffee.

okwolf · 2017-09-20T08:31:20Z

src/app.js

@@ -181,7 +181,6 @@ export function app(props) {

  function patch(parent, element, oldNode, node, isSVG, nextSibling) {
    if (oldNode === node) {
-      console.log("memoized...")


andyrj · 2017-10-03T22:30:27Z

ok so I have screenshots of chrome devtools performance tab with non-memoized, memoized without patch shortcut, and memoized with patch shortcut.

Non-memoized:

Memoized-no-shortcut:

Memoized-with-shortcut:

You can see the JS (yellow) in the cpu time decreasing between the screenshots with each part of the optimization enabled...

This was made via this codepen, https://codepen.io/andyrj/pen/yzzmLo, which is updating state every 16ms to increment a counter with a table that is based on static data below it... If this codepen is not sufficient to demonstrate the js overhead associated with a component that would benefit from memoization I don't really know what's going to satisfy a demo outside of a real app with expensive components.

pspeter3 · 2017-10-08T15:09:23Z

What is required to make this approved?

jorgebucaran · 2017-10-08T15:43:25Z

@pspeter3 Just a little time. 🙏

andyrj · 2017-10-08T17:12:42Z

There was a branch missing in coverage last I looked, and I still don't like the test I added... Just couldn't think of a better one at the time.

codecov-io · 2017-11-04T20:24:43Z

Codecov Report

Merging #378 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #378   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         144    145    +1     
  Branches       42     43    +1     
=====================================
+ Hits          144    145    +1

Impacted Files	Coverage Δ
src/index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7319f27...d3c0b76. Read the comment docs.

lukejacksonn · 2017-11-05T04:19:12Z

I was just going through issues and came from #373 (comment) to here.. when I first read this PR description (and graphs 💯 ) I thought wow this is awesome, but assumed for some reason that we would need to include the memoize function in core.. but this is not the case, right? Just an equality check will suffice. Which will always return false unless you have wrapped your component with memoize in userland. If I am understanding this correctly then what have we got against the idea?

jorgebucaran · 2017-11-05T05:28:57Z

src/app.js

@@ -196,7 +196,9 @@ export function app(props, container) {
  }

  function patchElement(parent, element, oldNode, node, isSVG, nextSibling) {
-    if (oldNode == null) {
+    if (oldNode === node) {


@andyrj If this is all we need, we can simplify it as follows. (bytes)

if (oldNode===node){ } else if (oldNode == null) { ...

Good call I wasn't taking the return at bottom into account for bytes.

Okay, please rebase. 🙏

andyrj · 2017-11-05T05:54:05Z

@lukejacksonn ya the memoize doesn't need to be a part of core. I just copy pasted that memoize() in comments above as an example someone could use.

lukejacksonn · 2017-11-05T06:01:47Z

@andyrj 👌 and although I appreciate not every app/developer will take advantage of this feature nor will you need to memoize most of the time.. it does make sense to enable memoizing for when components get really big (more precisely, when the cost of diffing args and cacheing becomes cheaper than a straight up re-render, right?). If this is all that it takes then I'd say it is worth the minimal extra bytes and added complexity!

jorgebucaran · 2017-11-05T07:30:40Z

Thanks @andyrj! 🎉 🌮 🎉

okwolf requested a review from jorgebucaran September 20, 2017 05:40

jorgebucaran requested changes Sep 20, 2017

View reviewed changes

jorgebucaran added the enhancement New feature or request label Sep 20, 2017

okwolf mentioned this pull request Sep 20, 2017

Road to 1.0 #244

Closed

7 tasks

okwolf suggested changes Sep 20, 2017

View reviewed changes

okwolf reviewed Sep 20, 2017

View reviewed changes

jorgebucaran mentioned this pull request Nov 4, 2017

How to implement shouldComponentUpdate to increase Performance of complex components? #373

Closed

andyrj force-pushed the memoize-patch-early-return branch from 4f56858 to c25a5ae Compare November 4, 2017 20:24

jorgebucaran reviewed Nov 5, 2017

View reviewed changes

jorgebucaran approved these changes Nov 5, 2017

View reviewed changes

andyrj force-pushed the memoize-patch-early-return branch from 91c5470 to d3c0b76 Compare November 5, 2017 06:50

add short circuit for patching memoized vnodes

d3c0b76

jorgebucaran merged commit f415e8e into jorgebucaran:master Nov 5, 2017

jorgebucaran pushed a commit that referenced this pull request Jan 7, 2018

add short circuit for patching memoized vnodes (#378)

46bcd38

SamP692 mentioned this pull request Jan 9, 2018

What documentation would you like to see here? #531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add short circuit for patching memoized vnodes #378

add short circuit for patching memoized vnodes #378

andyrj commented Sep 20, 2017 •

edited

Loading

codecov bot commented Sep 20, 2017 •

edited

Loading

jorgebucaran left a comment

okwolf Sep 20, 2017

andyrj Sep 20, 2017 •

edited

Loading

okwolf Sep 20, 2017

andyrj Sep 20, 2017

jorgebucaran Sep 20, 2017

pspeter3 Oct 7, 2017

andyrj commented Sep 20, 2017 •

edited

Loading

okwolf Sep 20, 2017

andyrj commented Oct 3, 2017

pspeter3 commented Oct 8, 2017

jorgebucaran commented Oct 8, 2017

andyrj commented Oct 8, 2017

codecov-io commented Nov 4, 2017 •

edited by codecov bot

Loading

lukejacksonn commented Nov 5, 2017

jorgebucaran Nov 5, 2017 •

edited

Loading

andyrj Nov 5, 2017

jorgebucaran Nov 5, 2017

andyrj commented Nov 5, 2017 •

edited

Loading

lukejacksonn commented Nov 5, 2017

jorgebucaran commented Nov 5, 2017

add short circuit for patching memoized vnodes #378

add short circuit for patching memoized vnodes #378

Conversation

andyrj commented Sep 20, 2017 • edited Loading

codecov bot commented Sep 20, 2017 • edited Loading

Codecov Report

jorgebucaran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyrj Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyrj commented Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

andyrj commented Oct 3, 2017

pspeter3 commented Oct 8, 2017

jorgebucaran commented Oct 8, 2017

andyrj commented Oct 8, 2017

codecov-io commented Nov 4, 2017 • edited by codecov bot Loading

Codecov Report

lukejacksonn commented Nov 5, 2017

jorgebucaran Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyrj commented Nov 5, 2017 • edited Loading

lukejacksonn commented Nov 5, 2017

jorgebucaran commented Nov 5, 2017

andyrj commented Sep 20, 2017 •

edited

Loading

codecov bot commented Sep 20, 2017 •

edited

Loading

andyrj Sep 20, 2017 •

edited

Loading

andyrj commented Sep 20, 2017 •

edited

Loading

codecov-io commented Nov 4, 2017 •

edited by codecov bot

Loading

jorgebucaran Nov 5, 2017 •

edited

Loading

andyrj commented Nov 5, 2017 •

edited

Loading