Optimize h #187

ngryman · 2017-04-15T18:32:32Z

Let's start from this implementation which passes the current tests.

Just to have an idea, here are the benchs:

Before

NANOBENCH version 2
> node

# vnode with no child 1M times
ok ~315 ms (0 s + 315373353 ns)

# vnode with a single child 1M times
ok ~1.35 s (1 s + 353013098 ns)

# vnode with a single array child 1M times
ok ~1.66 s (1 s + 657042054 ns)

all benchmarks completed
ok ~3.33 s (3 s + 325428505 ns)

After

NANOBENCH version 2
> node

# vnode with no child 1M times
ok ~287 ms (0 s + 287287753 ns)

# vnode with a single child 1M times
ok ~692 ms (0 s + 692102600 ns)

# vnode with a single array child 1M times
ok ~822 ms (0 s + 822467710 ns)

all benchmarks completed
ok ~1.8 s (1 s + 801858063 ns)

codecov · 2017-04-15T18:34:43Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #187   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         176    176           
  Branches       45     47    +2     
=====================================
  Hits          176    176

Impacted Files	Coverage Δ
src/h.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 00de017...63f2c03. Read the comment docs.

jorgebucaran · 2017-04-15T18:44:25Z

src/h.js

+    var val = arguments[i]
+    if (Array.isArray(val)) {
+      for (var j = 0; j < val.length; j++) {
+        children.push(val[j])


@ngryman val[j] can be an array, as we previously discussed on #185.

jorgebucaran · 2017-04-15T18:46:27Z

@ngryman Good start! Remember that when using Hyperx, children can be arrays that contain nested arrays too. Your implementation actually works fine with JSX. (Almost see comment.)

jorgebucaran · 2017-04-15T18:47:39Z

Here is the implementation that I eventually arrived at that works with both hyperx and jsx and drops the stack:

export default function (tag, data) {
  var children = []

  for (var i = 2; i < arguments.length;) {
    push(arguments[i++], children)
  }

  return typeof tag === "string"
    ? {
      tag: tag,
      data: data || {},
      children: children
    }
    : tag(data, children)

  function push(node) {
    if (Array.isArray(node)) {
      for (var i = 0; i < node.length; i++) {
        push(node[i], children)
      }
    } else if (node != null && node !== true && node !== false) {
      children.push(node)
    }
  }
}

It's small, but performance was bad, and I couldn't optimize it. I hope it gives you some ideas.

jorgebucaran · 2017-04-15T18:51:32Z

src/h.js

-      if (typeof node === "number") {
-        node = node + ""
+    }
+    else if (val && val !== true) {


@ngryman You want:

} else if (val != null && val !== true && val !== false) {

Otherwise we will not be able to process text nodes with the number 0.

ngryman · 2017-04-15T19:34:57Z

@jbucaran Coverage drops because no tests expose the way hyperx pass children as 3rd parameter and not as splats.

ngryman · 2017-04-15T18:34:05Z

src/h.js

+    var val = arguments[i]
+    if (Array.isArray(val)) {
+      for (var j = 0; j < val.length; j++) {
+        children.push(val[j])


push is perfectly fine performance-wize: https://www.html5rocks.com/en/tutorials/speed/v8/#toc-topic-numbers

ngryman · 2017-04-15T18:37:22Z

src/h.js

-    stack[stack.length] = arguments[i]
-  }
-
-  while (stack.length) {


@jbucaran There is no need for an additional stack. As you mentioned in #185, we are only treating 1 level of nested children. So we basically can concatenate, but without using concat which allocates a new array.
If you find that it's possible to have a deepest level of nesting, then we could discuss about this.

ngryman · 2017-04-15T18:38:03Z

src/h.js

+    }
+    else if (val && val !== true) {
+      if (typeof val === 'number') {
+        val = val + ''


@jbucaran Out of curiosity, why?

ngryman · 2017-04-15T19:50:55Z

New benchs, code is being deopt, working on it.

NANOBENCH version 2
> node

# vnode with no child 1M times
ok ~687 ms (0 s + 686546561 ns)

# vnode with a single child 1M times
Materialization [0x7fff5fbf5e80] <- 0x1704c2e9e771 ;  [removing optimized code for: h]
ok ~981 ms (0 s + 981468009 ns)

# vnode with a single array child 1M times
Materialization [0x7fff5fbf5e80] <- 0x369b85d129 ;  [removing optimized code for: h]
[removing optimized code for: addChild]
ok ~1.04 s (1 s + 39802550 ns)

all benchmarks completed
ok ~2.71 s (2 s + 707817120 ns)

ngryman · 2017-04-15T20:44:46Z

New benchs, I can't really do better I think:

NANOBENCH version 2
> node

# vnode with no child 1M times
ok ~277 ms (0 s + 277363055 ns)

# vnode with a single child 1M times
ok ~905 ms (0 s + 905257714 ns)

# vnode with a single array child 1M times
ok ~934 ms (0 s + 934230712 ns)

all benchmarks completed
ok ~2.12 s (2 s + 116851481 ns)

But I'm gonna tease you @jbucaran, if we enforce every node to be the exact same object (even for text nodes), here is what we get:

NANOBENCH version 2
> node

# vnode with no child 1M times
ok ~329 ms (0 s + 328797485 ns)

# vnode with a single child 1M times
ok ~547 ms (0 s + 546626621 ns)

# vnode with a single array child 1M times
ok ~552 ms (0 s + 552139589 ns)

all benchmarks completed
ok ~1.43 s (1 s + 427563695 ns)

Basically the current vdom implementation mixes different types of nodes, it can be strings or object. If we statically type the node object by creating a constructor for it and we make sure that the tree only manipulates instances of this constructor, then v8 and friends can optimize this heavily because it always manipulates data of the same shape.

Behind the scenes it can use a fast backing store (something close to a c++ class) instead of a dictionary store (like a hash table). This ensure super fast properties access and code specialization.

It would be hugely beneficial for h and the diffing algorithm in terms of speed and memory.

jorgebucaran · 2017-04-16T01:26:48Z

src/h.js

+      children.push(val[j])
+    }
+  }
+  else if (val && val !== true) {


I left you a comment on the other diff about this.

Basically, you are forgetting about the number 0.

} else if (val != null && val !== true && val !== false) {

@ngryman See above.

We're reviewing an old commit. It has been addressed since then.

jorgebucaran · 2017-04-16T01:32:32Z

@ngryman Coverage drops because no tests expose the way hyperx pass children as 3rd parameter and not as splats.

Then we need to add those tests :)

jorgebucaran · 2017-04-16T01:35:01Z

src/h.js

@@ -4,7 +4,7 @@ function addChild(children, val) {
      children.push(val[j])
    }
  }
-  else if (val && val !== true) {
+  else if (val != null && val !== true && val !== false) {


jorgebucaran · 2017-04-16T02:22:47Z

@ngryman Your final implementation is very similar to the recursive one I created, but bigger.

On the other hand, performance is better than that one! 👍

Now, this version is not really faster than the current one on master (mine wasn't either which is why I didn't merge it).

jorgebucaran · 2017-04-16T02:33:14Z

@ngryman if we enforce every node to be the exact same object (even for text nodes), here is what we get...

Sounds good, but that does not seem to be what this PR is addressing no? Sorry, I am terrible at discussing two different things at the same time.

ngryman · 2017-04-16T07:54:56Z

Then we need to add those tests :)

They have been added, 100% is back :)

@ngryman Your final implementation is very similar to the recursive one I created, but bigger.

Because it treats the 2 functions signatures in an optimal way:

h(tag, data, children)
h(tag, data, ...children)

arguments object must be used very carefully if you don't want to kill perfs. I also added some ifs to avoid to call Array.isArray with null values which deoptimize the code.

Now, this version is not really faster than the current one on master (mine wasn't either which is why I didn't merge it).

master version: ok ~3.34 s (3 s + 338336241 ns)
h-optimized version: ok ~2.12 s (2 s + 116851481 ns)

It depends how you bench it. What is your bench I'm curious?

Sounds good, but that does not seem to be what this PR is addressing no? Sorry, I am terrible at discussing two different things at the same time.

Yes it's just the next step. I also give too much information lately instead of giving you a concise report. It's because it's a complex subject also.

jorgebucaran · 2017-04-16T09:59:04Z

@ngryman I use https://github.com/krausest/js-framework-benchmark.

jorgebucaran · 2017-04-16T11:19:52Z

src/h.js

+  if (arguments.length > 2) {
+    if (Array.isArray(values)) {
+      for (var i = 0; i < values.length; i++) {
+        addChild(children, values[i])
      }


@ngryman What's the point of this loop here? Is it an optimization technique?

I replaced it with just addChild(children, values) and used addChild(children, child[j]) inside addChild itself the all tests pass.

jorgebucaran · 2017-04-16T11:40:51Z

@ngryman Because it treats the 2 functions signatures in an optimal way:

tag can be either a String or a Function, no? Doesn't that defeat what V8?

jorgebucaran · 2017-04-16T11:55:04Z

Is it necessary to repeat the for loop 3 times? What about this implementation that does not change bundle size?

export default function(tag, data, values) {
  var children = []

  if (arguments.length > 2) {
    if (Array.isArray(values)) {
      loop(children, values, 0)
    } else {
      loop(children, arguments, 2)
    }
  }

  return typeof tag === "string"
    ? {
        tag: tag,
        data: data || {},
        children: children
      }
    : tag(data, children)
}

function loop(children, child, i) {
  for (; i < child.length; ) {
    addChild(children, child[i++])
  }
}

function addChild(children, child) {
  if (child != null && child !== true && child !== false) {
    if (Array.isArray(child)) {
      loop(children, child, 0)
    } else {
      children.push(typeof child === "number" ? child + "" : child)
    }
  }
}

ngryman · 2017-04-16T12:56:21Z

@jbucaran Yes, like @leeoniya says, sometimes we have to trade bytes for performance.

Your proposed solution gives this:

NANOBENCH version 2
> node

# vnode with no child 1M times
ok ~450 ms (0 s + 450218037 ns)

# vnode with a single child 1M times
ok ~1.41 s (1 s + 407109050 ns)

# vnode with a single array child 1M times
ok ~1.5 s (1 s + 502727094 ns)

all benchmarks completed
ok ~3.36 s (3 s + 360054181 ns)

ngryman · 2017-04-16T13:01:46Z

I'm not saying that there is not a better implementation than mine though, not at all 😆

ngryman · 2017-04-16T13:06:38Z

@jbucaran btw, do you test your bundle size gziped? gzip logically handles redundancies, multiple for loops should be grasped by gzip somehow.

ngryman · 2017-04-16T13:08:43Z

Your version give a gziped bundle of 1702 B, mine 1710 B.

jorgebucaran · 2017-04-16T13:11:24Z

@ngryman Bummer. I just ran mine through the benchmark suite and it was definitely slower. 👍

I'm still not getting any perf gains from yours against the one on master though, so I am not compelled to merge this PR right now, at least not until other priorities are sorted out like improving the patch algorithm, which IMO it's where we should focus to make hyperapp faster.

Your version give a gziped bundle of 1702B, mine 1710B.

Yeah, that's about right. I use zopfli (just because) and it gives 1673 hehe.

btw, do you test your bundle size gziped?

Nope, I just test the minified bundle using js-framework-benchmark.

ngryman · 2017-04-16T17:12:43Z

Bummer. I just ran it through the benchmark suite and it was definitely slower. 👍

I'm curious how this version could be slower than the one on master, it does not make sense.
Well, let's define what are the priorities then 😆

jorgebucaran · 2017-04-16T23:54:07Z

@ngryman Read my comment again.

jorgebucaran · 2017-04-16T23:55:44Z

Your version is certainly not slower than the current one on master, but it's not faster either. I'm using js-framework-benchmark. And they both score 1.35.

leeoniya · 2017-04-17T00:06:28Z

js-framework-benchmark won't always show improvements from micro-optimizations since timings in all its metrics are dominated by dom ops & relayouts/reflows/repaints. that's why it's good to have an additional bench that stresses framework internals without any effect on the dom.

jorgebucaran · 2017-04-17T00:20:17Z

@ngryman Well, let's define what are the priorities then 😆

Which is why I think we can leave this PR open for when the time is right. It's just weird to merge this right now, that's what I mean.

jorgebucaran · 2017-04-17T02:18:45Z

@developit Can you share your thoughts/feedback here?

Your https://github.com/developit/preact/blob/master/src/h.js is very similar and perhaps could benefit from this discussion too.

ngryman · 2017-04-17T07:22:56Z

To make things short:

The PR version is faster but when tested in a global benchmark there is no difference.
The true perf gain will come when VNodes will all share the same constructor (hidden class: https://gist.github.com/twokul/9501770).
It's the start of several passes of optimizations, imho we have to start somewhere 😄

jorgebucaran · 2017-04-17T09:01:51Z

@ngryman Thanks for the summary and the short article. 😄
And yes, we have to start somewhere, and that somewhere is possibly patch, who's doing all the heavy lifting around here. 💪😥

EDIT: Emoji.

developit · 2017-04-17T12:30:31Z

That little function has been through probably 100 different implementations. Regarding changing JSX: even if you were to get VNodes extended to support new nodeTypes like Text and Comment, children is still a passthrough - people provide all sorts of things in that Array, like functions. Your best bet is to design the function to perform well without trying to force it into being monomorphic.

I've experimented with making h() a small normalization function and moving the heavy lifting into another function (such as the VNode constructor) called with consistent types/shapes:
https://esbench.com/bench/587567b899634800a0347555

You can see some other contenders for h() benchmarked here:
https://esbench.com/bench/57ee8f8e330ab09900a1a1a0

jorgebucaran · 2017-07-01T22:13:49Z

@ngryman I'd like to revisit this PR and if possible, with your help, merge it.

Can we first double check that these changes don't break any existing code? JSX-users must not suffer from this.

👋😄🙏

ngryman · 2017-07-03T09:24:18Z

@jbucaran Sure, I'll do it in the week.

ngryman · 2017-07-07T14:00:20Z

I'm going to create a new PR for this one if it's ok for you. This one was from my fork and has diverged quite a lot form master.

jorgebucaran · 2017-07-07T17:04:04Z

SGTM

Optimize h

dc5f0bc

jorgebucaran reviewed Apr 15, 2017

View reviewed changes

jorgebucaran requested changes Apr 15, 2017

View reviewed changes

jorgebucaran requested a review from FlorianWendelborn April 15, 2017 18:52

jorgebucaran assigned jorgebucaran and ngryman Apr 15, 2017

jorgebucaran added the enhancement New feature or request label Apr 15, 2017

ngryman added 2 commits April 15, 2017 21:27

Handle 3rd parameter

8998c53

Handle 0 text nodes

0918d8c

Cover new test case

ead3625

ngryman commented Apr 15, 2017

View reviewed changes

ngryman added 2 commits April 15, 2017 22:16

Optimized h

74fcca8

Avoid calling Array.isArray with null values

63f2c03

jorgebucaran requested changes Apr 16, 2017

View reviewed changes

jorgebucaran reviewed Apr 16, 2017

View reviewed changes

jorgebucaran requested changes Apr 16, 2017

View reviewed changes

FlorianWendelborn removed their request for review July 3, 2017 16:31

ngryman mentioned this pull request Jul 7, 2017

Optimize h #289

Closed

3 tasks

jorgebucaran closed this Jul 7, 2017

Optimize h #187

Optimize h #187

Conversation

ngryman commented Apr 15, 2017 • edited

codecov bot commented Apr 15, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

jorgebucaran commented Apr 15, 2017 • edited

jorgebucaran commented Apr 15, 2017 • edited

Choose a reason for hiding this comment

ngryman commented Apr 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngryman commented Apr 15, 2017 • edited

ngryman commented Apr 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgebucaran commented Apr 16, 2017

Choose a reason for hiding this comment

jorgebucaran commented Apr 16, 2017 • edited

jorgebucaran commented Apr 16, 2017 • edited

ngryman commented Apr 16, 2017

jorgebucaran commented Apr 16, 2017

jorgebucaran Apr 16, 2017 • edited

Choose a reason for hiding this comment

jorgebucaran commented Apr 16, 2017

jorgebucaran commented Apr 16, 2017

ngryman commented Apr 16, 2017 • edited

ngryman commented Apr 16, 2017

ngryman commented Apr 16, 2017

ngryman commented Apr 16, 2017 • edited

jorgebucaran commented Apr 16, 2017 • edited

ngryman commented Apr 16, 2017

jorgebucaran commented Apr 16, 2017

jorgebucaran commented Apr 16, 2017

leeoniya commented Apr 17, 2017

jorgebucaran commented Apr 17, 2017 • edited

jorgebucaran commented Apr 17, 2017 • edited

ngryman commented Apr 17, 2017

jorgebucaran commented Apr 17, 2017 • edited

developit commented Apr 17, 2017

jorgebucaran commented Jul 1, 2017

ngryman commented Jul 3, 2017

ngryman commented Jul 7, 2017

jorgebucaran commented Jul 7, 2017

ngryman commented Apr 15, 2017 •

edited

codecov bot commented Apr 15, 2017 •

edited

jorgebucaran commented Apr 15, 2017 •

edited

jorgebucaran commented Apr 15, 2017 •

edited

ngryman commented Apr 15, 2017 •

edited

ngryman commented Apr 15, 2017 •

edited

jorgebucaran commented Apr 16, 2017 •

edited

jorgebucaran commented Apr 16, 2017 •

edited

jorgebucaran Apr 16, 2017 •

edited

ngryman commented Apr 16, 2017 •

edited

ngryman commented Apr 16, 2017 •

edited

jorgebucaran commented Apr 16, 2017 •

edited

jorgebucaran commented Apr 17, 2017 •

edited

jorgebucaran commented Apr 17, 2017 •

edited

jorgebucaran commented Apr 17, 2017 •

edited