Use string concatenation in attr generation

[divmain]

No description provided.

[divmain]

I switched over to string concatenation @aweary, but actually saw a slow-down in performance. I'm wondering if, because you're concatenation the same string in your benchmark, if V8 optimizes it away in a clever way. I'm putting together another benchmark to prove/disprove that theory.

[aweary]

@divmain thats likely. The renderer that I was working on concatenated to the same string every time (it was contained in a "context" object that was passed down as it recursed). Is there a way we could implement something similar here?

[divmain]

That's essentially what I tried:

diff --git a/src/render/attrs/index.js b/src/render/attrs/index.js
index d25ed78..6b91222 100644
--- a/src/render/attrs/index.js
+++ b/src/render/attrs/index.js
@@ -15,7 +15,7 @@ const mapWithKey = map.convert({ cap: false });
  * @return     {String}          Generated HTML attribute output.
  */
 function renderAttrs (attrs) {
-  const attrString = [];
+  let attrString = "";
 
   for (let attrKey in attrs) {
     if (
@@ -36,11 +36,11 @@ function renderAttrs (attrs) {
         attrVal = `="${attrVal}"`;
       }
 
-      attrString.push(` ${attrKey}${attrVal}`);
+      attrString += ` ${attrKey}${attrVal}`;
     }
   }
 
-  return attrString.join("");
+  return attrString;
 }
 
 const mapPairToString = mapWithKey((val, key) => `${key}:${val}`);

But performance worsened with the change. I'm not sure why this would be - it is possible that V8 is optimizing the string concatenation into a no-op, if it sees that the value is never used? That's the only thing I can think of, since the real-world results seem to conflict with the benchmark. Do you have any other thoughts? I'd love to improve perf in any way we can...

[ryan-roemer]

@divmain -- Rando thoughts:

Why don't we try the full other way too since you've got concat either way, e.g.

// 1. back to array
attrString.push(' ');
attrString.push(attrKey);
attrString.push(attrVal);

// 2. string concat -- all normal concat, no backticks
attrString += ' ';
attrString += attrKey;
attrString += attrVal;

Also, maybe we could get an additional benchmark that correlates more strongly with your real world example?

[aweary]

@ryan-roemer @divmain I put together a more accurate benchmark using the actual attribute function here: https://github.com/aweary/string-concat-array-join-bench

You can see the results I got in the repo's README for node 4 - latest. In all cases I measured string concatenation to be much faster. One interesting thing I discovered was that using let vs var made a huge difference (benchmarks aren't being compiled, so its native let).

It went from about ~300,000 ops/second to ~1,000,000 ops/second for string concatenation. I don't think this will affect the benchmarks since we're using babel-node, but still interesting.

[aweary]

We should also look into utilizing benchmark.js here as well. It does a lot of work making sure that it has enough runs to provide a statistically relevant result.

[ryan-roemer]

@aweary -- I took your example and did a "pure concatenation" with no backticks:

diff --git a/benchmark.js b/benchmark.js
index 9023582..0025857 100644
--- a/benchmark.js
+++ b/benchmark.js
@@ -2,6 +2,7 @@ var Benchmark = require('benchmark');
 var styles = require('./styles');
 var renderAttributesWithArray = require('./array');
 var renderAttributesWithString = require('./string');
+var renderAttributesWithPureString = require('./string-pure');
 
 var suite = new Benchmark.Suite;
 
@@ -15,6 +16,10 @@ suite.add('Array.push + Array.join', () => {
   var parsed = renderAttributesWithString(styles);
   var rendered = `<div ${parsed}></div>`
 })
+.add('String pure concat', () => {
+  var parsed = renderAttributesWithPureString(styles);
+  var rendered = `<div ${parsed}></div>`
+})
 .on('cycle', (event) => {
   console.log(String(event.target))
 })
diff --git a/string-pure.js b/string-pure.js
new file mode 100644
index 0000000..72b43a3
--- /dev/null
+++ b/string-pure.js
@@ -0,0 +1,30 @@
+var isFunction = require('lodash/fp').isFunction
+
+
+module.exports = function renderAttributesWithString (attrs) {
+  var attrString = '';
+
+  for (var attrKey in attrs) {
+    if (attrs.hasOwnProperty(attrKey) && attrKey !== "children") {
+      var attrVal = attrs[attrKey];
+
+      if (!attrVal || isFunction(attrVal)) { continue; }
+
+      attrString += ' ';
+      attrString += attrKey;
+
+      if (attrVal === true) {
+        // nothing
+      } else if (attrKey === "style" && typeof attrVal === "object") {
+        attrString += '="'
+        attrString += styleObjToString(attrVal);
+        attrString += '"';
+      } else {
+        attrString += '="'
+        attrString += attrVal;
+        attrString += '"';
+      }
+    }
+  }
+  return attrString;
+}

and got:

# Node 4
$ node --version
v4.3.2
$ npm run bench

> string-concat-array-join-bench@1.0.0 bench /Users/rye/scm/vendor/string-concat-array-join-bench
> node test.js && node benchmark.js

Array.push + Array.join x 181,070 ops/sec ±1.58% (86 runs sampled)
String concat x 353,023 ops/sec ±1.73% (82 runs sampled)
String pure concat x 575,098 ops/sec ±1.83% (86 runs sampled)
Fastest is String pure concat

# Node 6
$ node --version
v6.9.5
$ npm run bench

> string-concat-array-join-bench@1.0.0 bench /Users/rye/scm/vendor/string-concat-array-join-bench
> node test.js && node benchmark.js

Array.push + Array.join x 519,183 ops/sec ±1.09% (83 runs sampled)
String concat x 1,094,144 ops/sec ±1.37% (83 runs sampled)
String pure concat x 1,529,188 ops/sec ±1.25% (83 runs sampled)
Fastest is String pure concat

[aweary]

@ryan-roemer how does it compare with newer versions of Node?

[ryan-roemer]

@aweary -- Updated my comment with node6 results

[aweary]

@ryan-roemer thanks! I added it to the benchmark repo and updated the results in the README for all versions.

Side note: it's amazing how much faster string concatenation has gotten since v4.x.

[divmain]

Thanks for all the research on this! @aweary or @ryan-roemer, are you interested in setting up some for-realz benchmarks with benchmark.js? I'm amenable. And if the benchmarks show a faster way to construct the attrs in the context of VDOM rendering, I'm 100% for it.