Hyperapp: Optimizing h

[ngryman]

Continuing from #182 and focusing on h.

h is in charge of returning a new vnode. Currently it exposes 2 additional features if I may say:

1. It handles splats (varargs) of children.
2. It concatenates contiguous string and number children into one text node.

Both features have a performance impact.

Splats

It's basically here to support how some jsx transformers work. My feeling is they do it this way because it makes parsing/codegen easier for them.

Pros | Cons
------|------
• Support all jsx transformers (react-compliant) | • Slow (http://jsperf.com/function-signatures)
• More bytes

It has been discussed for the babel jsx transformer a while ago: https://github.com/babel/babel/issues/2034. The babel team didn't do anything on this, @substack developed his own babel transformer for this specific case: https://github.com/substack/babel-plugin-jsx-factory.

We could either simply drop splats support and align with a clean vdom function signature. This would involve changing of babel transformer and investigate if webpack transformer supports this.

Or we could, as a mentioned here wrap h in a hjsx function that just handle splats, for those who really want to use it.

Concatenation

It concatenates string and number children into one text node.

Pros | Cons
------|------
• Less nodes to diff | • Additional memory
• Additional processing
• More bytes

It feels that having so many string/number contiguous nodes that there is a real performance benefit to merge them is less than obvious.
Still, it allocates a stack and process it for each h call which has a non-negligible performance impact and gc stress.

EDIT: I'm talking about the internal stack here. I wrongly believed it was needed for concatenation, but it's in fact because of how hyperx make calls to h. So everything here applies to 'hyperx' nested children handling instead of concatenation.

My advice on this would be to simply drop it and compare hyperapp diff benchs before and after.

Related

• https://github.com/hyperapp/hyperapp/pull/184
• https://github.com/hyperapp/hyperapp/issues/185

[jorgebucaran]

My advice on this would be to simply drop it and compare hyperapp diff benchs before and after.

I did and results were the same. I ran the benchmarks 3 times and it was actually a bit faster in two of them. So let's keep the smaller one, which saves some bytes.

• https://github.com/hyperapp/hyperapp/pull/184

[jorgebucaran]

As for dropping splats: I think the additional cost of having to understand what's going on and having to modify/have a "special" build-pipeline, not to mention do something like hjsx is more expensive than keeping them.

With regards to having a clean interface, I agree and I don't understand why the babel folks didn't align (tag, data, [children]) instead of (tag, data, ...children). It sucks, but making JSX harder to use in hyperapp would suck more.

[ngryman]

@jbucaran Well the real pain comes from the stack in #184. Why keeping it? Nested children are using h calls right?

[ngryman]

I can get it for splats, let's keep it then.

[jorgebucaran]

🤔

[jorgebucaran]

Well the real pain comes from the stack in #184. Why keeping it? Nested children are using h calls right?

How would that work? I can't see it 😓

[jorgebucaran]

We need to handle nodes that are arrays, otherwise we would break Hyperx support.

[ngryman]

@jbucaran Oh ok, so hyperx generates nested raw children (by raw I mean not an explicit call to h)?

[jorgebucaran]

That would be hyperxify, and IIRC it does which explains why I was merging sub arrays here.

[ngryman]

Ok I get it. Well that's unfortunate because it kills perfs 😭

[jorgebucaran]

One step at a time. I think checking that (hyperx/nested arrays) and see if we can improve how we deal with it is a good next step.

184 just brushed off even more bytes without affecting performance. This (hyperapp) whole thing including the router is now 1677 bytes!

[ngryman]

@jbucaran Yes sorry if I'm not very clear, I'm parallelizing stuff right now 🤒

[ngryman]

@jbucaran Ok for hyperx/nested arrays. It's truly the most important optimization that we could bring to h.

[jorgebucaran]

@ngryman I was able to reduce everything down to this:

[].push.apply(children, node)

but this breaks Hyperx, unfortunately.

[selfup]

If it doesn't break jsx we should consider jsx over hyperx? I love that this library has the option for both, but in terms of performance maybe one is better than the other?

I am just thinking out loud 😄

[selfup]

Also for performance using switch over if else if else can be a an easy win

As well as if (something === undefined) over if (!something) which will give a 15% improvement

[jorgebucaran]

Hmm, I was able to reduce h even further to this:

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

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

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

which still supports Hyperx and drops the stack as @ngryman suggested, but perf-wise turned out to be much worse.

Maybe I screwed up somewhere else? 🤔

[jorgebucaran]

@selfup Yeah, that's always an option, but dropping hyperx support means betraying one of hyperapp's core initial values and I'm still yet to see an implementation of h (without hyperx support) that's faster than the one we have.

[ngryman]

@jbucaran [].push.apply(arguments, node) -> Array.prototype.push

[] instanciates an array each time.

A more efficient way is to have a reference to push (avoids properties access):

const push = Array.prototype.push
// ...
push.apply(arguments, node)

[ngryman]

Also apply on arguments is a killer: https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#32-leaking-arguments.

[ngryman]

@jbucaran Let me propose you an implementation. I think recursion could be the way to go here. You can't really use arguments in place of a stack.

[ngryman]

I'm pretty sure we don't have 2 levels of nested children. Can we check this?

I mean it does not make sense or there is a bug in the current implementation. Level 2 nodes would obviously be children of level 1 nodes. But the current implementation basically flattens everything.

If we only have 1 level, there is no need for recursion:

var push = Array.prototype.push

function createNode(tag, data, children) {
  return {
    tag: tag,
    data: data || {},
    children: children
  }
}

function createTextNode(val) {
  return createNode(null, val, null)
}

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

  for (var i = 2; i < arguments.length; i++) {
    var node = arguments[i]
    if (node && node !== true) {
      children.push(createTextNode(node))
    }
    else if (Array.isArray(node)) {
      for (var j = 0; j < node.length; j++) {
        children.push(createTextNode(node))
      }
    }
  }

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

This code is performant and ensure nodes are monomorphic which allow everything to be optimized.

[jorgebucaran]

@ngryman Good! If you think you have something working, please send me a PR.

[jorgebucaran]

I was able to get it down to this:

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)
    }
  }
}

but there was no perf advantage and not a big byte difference.

[jorgebucaran]

Merged: https://github.com/hyperapp/hyperapp/pull/184

[ngryman]

@jbucaran I'll make you a PR, considering the fact we do have to handle 1 level of nested children.

[jorgebucaran]

@ngryman Thanks! Go for it.

[jorgebucaran]

I think @ngryman forgot to close here after #289. 😉