Assorted Performance Improvements (#3041)

* hyperscriptVnode: use spread syntax

The implementation adopts spread syntax, inspired by the approach mentioned in the comments. However, argument handling has been adjusted for better performance.

Using spread syntax not only allows related comments to be deleted, but also slightly reduces bundle size and improves benchmarks by several percent.

All existing tests pass successfully.

Note that spread syntax does not seem to be supported by the existing internal bundler in some cases, so the bundler has also been updated to prevent incorrect suffix handling.

* allow attrs to be undefined and execute early return in `execSelector()`

Since commit f9e5163, attrs is never null/undefined. However, `m.render()` takes into account the case where attrs is null/undefined, and it is better to use undefined from a performance point of view.

Also, early return within `execSelector()` contributes to further performance improvement.

Some tests are changed because of the null/undefined attrs. This commit may be breaking from this point of view, but the v2.0.4 era allows for attrs to be null/undefined and the impact will be quite small.

* use the cached attrs as-is and skip `updateAttrs()` if the static attrs are identical

Since `compileSelector()` generates attrs objects containing only strings (no functions or nested objects). So, `updateAttrs()` can safely be skipped by checking the equality of the cached attrs objects themselves if the objects do not contain form attributes.

If you mainly use the "static" attrs and do not use dynamic attrs, skipping `updateAttrs()` allows significant performance improvements.

Also, the added checks are lightweight and there appears to be little or no performance degradation due to these checks.

This commit itself is not a breaking change in the sense that it passes all existing tests. The checks in `m.render()` above and the form attribute checks in `compileSelector()` result in a minor increase in code volume, though.

* refactor execSelector()

This state.attrs.className != null check is redundant.

* README: drop IE11 support

It seems that the current codebase (including generator, dom-for and spread syntax) already has little support for IE11 because it requires transpiling as well as polyfills.

* prevents existing tests from being changed

As this results in a slight decrease in performance improvement, comments regarding potential performance improvements have also been added.

* use a common empty attrs object

This simplifies the processing within execSelector.

* cachedAttrsIsStaticMap: use WeakMap to avoid potential memory leaks

* hyperscriptVnode: add performance sensitivity comments

* cachedAttrsIsStaticMap: revert back to Map from WeakMap with comments

* pass emptyAttrs to the Map constructor

This slightly reduces the bundle size.

Author: kfule

README.md

@@ -19,7 +19,7 @@ A modern client-side JavaScript framework for building Single Page Applications.
 
 Mithril.js is used by companies like Vimeo and Nike, and open source platforms like Lichess 👍.
 
-Mithril.js supports IE11, Firefox ESR, and the last two versions of Firefox, Edge, Safari, and Chrome. No polyfills required. 👌
+Mithril.js supports Firefox ESR, and the last two versions of Firefox, Edge, Safari, and Chrome. No polyfills required. 👌
 
 ## Installation
 

render/cachedAttrsIsStaticMap.js

@@ -0,0 +1,12 @@
+"use strict"
+
+var emptyAttrs = require("./emptyAttrs")
+
+// This Map manages the following:
+// - Whether an attrs is cached attrs generated by compileSelector().
+// - Whether the cached attrs is "static", i.e., does not contain any form attributes.
+// These information will be useful to skip updating attrs in render().
+//
+// Since the attrs used as keys in this map are not released from the selectorCache object,
+// there is no risk of memory leaks. Therefore, Map is used here instead of WeakMap.
+module.exports = new Map([[emptyAttrs, true]])

render/emptyAttrs.js

@@ -0,0 +1,4 @@
+"use strict"
+
+// This is an attrs object that is used by default when attrs is undefined or null.
+module.exports = {}

render/fragment.js

@@ -3,8 +3,8 @@
 var Vnode = require("../render/vnode")
 var hyperscriptVnode = require("./hyperscriptVnode")
 
-module.exports = function() {
-	var vnode = hyperscriptVnode.apply(0, arguments)
+module.exports = function(attrs, ...children) {
+	var vnode = hyperscriptVnode(attrs, children)
 
 	vnode.tag = "["
 	vnode.children = Vnode.normalizeChildren(vnode.children)

render/hyperscript.js

@@ -3,6 +3,8 @@
 var Vnode = require("../render/vnode")
 var hyperscriptVnode = require("./hyperscriptVnode")
 var hasOwn = require("../util/hasOwn")
+var emptyAttrs = require("./emptyAttrs")
+var cachedAttrsIsStaticMap = require("./cachedAttrsIsStaticMap")
 
 var selectorParser = /(?:(^|#|\.)([^#\.\[\]]+))|(\[(.+?)(?:\s*=\s*("|'|)((?:\\["'\]]|.)*?)\5)?\])/g
 var selectorCache = Object.create(null)
@@ -12,8 +14,12 @@ function isEmpty(object) {
 	return true
 }
 
+function isFormAttributeKey(key) {
+	return key === "value" || key === "checked" || key === "selectedIndex" || key === "selected"
+}
+
 function compileSelector(selector) {
-	var match, tag = "div", classes = [], attrs = {}
+	var match, tag = "div", classes = [], attrs = {}, isStatic = true
 	while (match = selectorParser.exec(selector)) {
 		var type = match[1], value = match[2]
 		if (type === "" && value !== "") tag = value
@@ -23,32 +29,40 @@ function compileSelector(selector) {
 			var attrValue = match[6]
 			if (attrValue) attrValue = attrValue.replace(/\\(["'])/g, "$1").replace(/\\\\/g, "\\")
 			if (match[4] === "class") classes.push(attrValue)
-			else attrs[match[4]] = attrValue === "" ? attrValue : attrValue || true
+			else {
+				attrs[match[4]] = attrValue === "" ? attrValue : attrValue || true
+				if (isFormAttributeKey(match[4])) isStatic = false
+			}
 		}
 	}
 	if (classes.length > 0) attrs.className = classes.join(" ")
-	if (isEmpty(attrs)) attrs = null
-	return selectorCache[selector] = {tag: tag, attrs: attrs}
+	if (isEmpty(attrs)) attrs = emptyAttrs
+	else cachedAttrsIsStaticMap.set(attrs, isStatic)
+	return selectorCache[selector] = {tag: tag, attrs: attrs, is: attrs.is}
 }
 
 function execSelector(state, vnode) {
+	vnode.tag = state.tag
+
 	var attrs = vnode.attrs
+	if (attrs == null) {
+		vnode.attrs = state.attrs
+		vnode.is = state.is
+		return vnode
+	}
+
 	var hasClass = hasOwn.call(attrs, "class")
 	var className = hasClass ? attrs.class : attrs.className
 
-	vnode.tag = state.tag
-
-	if (state.attrs != null) {
+	if (state.attrs !== emptyAttrs) {
 		attrs = Object.assign({}, state.attrs, attrs)
 
 		if (className != null || state.attrs.className != null) attrs.className =
 			className != null
 				? state.attrs.className != null
 					? String(state.attrs.className) + " " + String(className)
 					: className
-				: state.attrs.className != null
-					? state.attrs.className
-					: null
+				: state.attrs.className
 	} else {
 		if (className != null) attrs.className = className
 	}
@@ -70,12 +84,12 @@ function execSelector(state, vnode) {
 	return vnode
 }
 
-function hyperscript(selector) {
+function hyperscript(selector, attrs, ...children) {
 	if (selector == null || typeof selector !== "string" && typeof selector !== "function" && typeof selector.view !== "function") {
 		throw Error("The selector must be either a string or a component.");
 	}
 
-	var vnode = hyperscriptVnode.apply(1, arguments)
+	var vnode = hyperscriptVnode(attrs, children)
 
 	if (typeof selector === "string") {
 		vnode.children = Vnode.normalizeChildren(vnode.children)

render/hyperscriptVnode.js

@@ -2,52 +2,22 @@
 
 var Vnode = require("../render/vnode")
 
-// Call via `hyperscriptVnode.apply(startOffset, arguments)`
+// Note: the processing of variadic parameters is perf-sensitive.
 //
-// The reason I do it this way, forwarding the arguments and passing the start
-// offset in `this`, is so I don't have to create a temporary array in a
-// performance-critical path.
+// In native ES6, it might be preferable to define hyperscript and fragment
+// factories with a final ...args parameter and call hyperscriptVnode(...args),
+// since modern engines can optimize spread calls.
 //
-// In native ES6, I'd instead add a final `...args` parameter to the
-// `hyperscript` and `fragment` factories and define this as
-// `hyperscriptVnode(...args)`, since modern engines do optimize that away. But
-// ES5 (what Mithril.js requires thanks to IE support) doesn't give me that luxury,
-// and engines aren't nearly intelligent enough to do either of these:
-//
-// 1. Elide the allocation for `[].slice.call(arguments, 1)` when it's passed to
-//    another function only to be indexed.
-// 2. Elide an `arguments` allocation when it's passed to any function other
-//    than `Function.prototype.apply` or `Reflect.apply`.
-//
-// In ES6, it'd probably look closer to this (I'd need to profile it, though):
-// module.exports = function(attrs, ...children) {
-//     if (attrs == null || typeof attrs === "object" && attrs.tag == null && !Array.isArray(attrs)) {
-//         if (children.length === 1 && Array.isArray(children[0])) children = children[0]
-//     } else {
-//         children = children.length === 0 && Array.isArray(attrs) ? attrs : [attrs, ...children]
-//         attrs = undefined
-//     }
-//
-//     if (attrs == null) attrs = {}
-//     return Vnode("", attrs.key, attrs, children)
-// }
-module.exports = function() {
-	var attrs = arguments[this], start = this + 1, children
-
-	if (attrs == null) {
-		attrs = {}
-	} else if (typeof attrs !== "object" || attrs.tag != null || Array.isArray(attrs)) {
-		attrs = {}
-		start = this
-	}
-
-	if (arguments.length === start + 1) {
-		children = arguments[start]
-		if (!Array.isArray(children)) children = [children]
+// However, benchmarks showed this was not faster. As a result, spread is used
+// only in the parameter lists of hyperscript and fragment, while an array is
+// passed to hyperscriptVnode.
+module.exports = function(attrs, children) {
+	if (attrs == null || typeof attrs === "object" && attrs.tag == null && !Array.isArray(attrs)) {
+		if (children.length === 1 && Array.isArray(children[0])) children = children[0]
 	} else {
-		children = []
-		while (start < arguments.length) children.push(arguments[start++])
+		children = children.length === 0 && Array.isArray(attrs) ? attrs : [attrs, ...children]
+		attrs = undefined
 	}
 
-	return Vnode("", attrs.key, attrs, children)
+	return Vnode("", attrs && attrs.key, attrs, children)
 }

render/render.js

@@ -4,6 +4,7 @@ var Vnode = require("../render/vnode")
 var df = require("../render/domFor")
 var delayedRemoval = df.delayedRemoval
 var domFor = df.domFor
+var cachedAttrsIsStaticMap = require("./cachedAttrsIsStaticMap")
 
 module.exports = function() {
 	var nameSpace = {
@@ -453,7 +454,9 @@ module.exports = function() {
 		var element = vnode.dom = old.dom
 		ns = getNameSpace(vnode) || ns
 
-		updateAttrs(vnode, old.attrs, vnode.attrs, ns)
+		if (old.attrs != vnode.attrs || (vnode.attrs != null && !cachedAttrsIsStaticMap.get(vnode.attrs))) {
+			updateAttrs(vnode, old.attrs, vnode.attrs, ns)
+		}
 		if (!maybeSetContentEditable(vnode)) {
 			updateNodes(element, old.children, vnode.children, hooks, null, ns)
 		}
@@ -715,7 +718,7 @@ module.exports = function() {
 		// so removal should be done first to prevent accidental removal for newly setting values.
 		var val
 		if (old != null) {
-			if (old === attrs) {
+			if (old === attrs && !cachedAttrsIsStaticMap.has(attrs)) {
 				console.warn("Don't reuse attrs object, use new object for every redraw, this will throw in next major")
 			}
 			for (var key in old) {

scripts/_bundler-impl.js

@@ -158,10 +158,12 @@ module.exports = async (input) => {
 		})
 
 		//fix props
-		const props = new RegExp(`((?:[^:]\\/\\/.*)?\\.\\s*)(${candidates})|([\\{,]\\s*)(${candidates})(\\s*:)`, "gm")
-		code = code.replace(props, (match, dot, a, pre, b, post) => {
+		const props = new RegExp(`(\\.\\.)?((?:[^:]\\/\\/.*)?\\.\\s*)(${candidates})|([\\{,]\\s*)(${candidates})(\\s*:)`, "gm")
+		code = code.replace(props, (match, dotdot, dot, a, pre, b, post) => {
 			// Don't do anything because dot was matched in a comment
 			if (dot && dot.indexOf("//") === 1) return match
+			// Don't do anything because dot is a part of spread syntax or destructuring
+			if (dotdot) return match
 			if (dot) return dot + a.replace(/\d+$/, "")
 			return pre + b.replace(/\d+$/, "") + post
 		})

scripts/tests/test-bundler.js

@@ -368,4 +368,87 @@ o.spec("bundler", async () => {
 		// check that the argument z2 is not z00
 		o(await bundle(p("a.js"))).equals(";(function() {\nvar z0 = {}\nvar _1 = function(z1){}\nvar b = _1(z0)\nvar z = z0\nvar _5 = function(z2){}\nvar c = _5(z)\n}());")
 	})
+	o.spec("spread syntax and destructuring (...)", () => {
+		o("rest parameter", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "function f(d, ...a){}\nmodule.exports = f",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nfunction f(d, ...a0){}\nvar c = f\n}());")
+		})
+		o("function call", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "var a = [1, 2, 3]\nvar d = f(...a)\nmodule.exports = d",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nvar a0 = [1, 2, 3]\nvar d = f(...a0)\nvar c = d\n}());")
+		})
+		o("new", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "var a = [1, 2, 3]\nvar d = new f(...a)\nmodule.exports = d",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nvar a0 = [1, 2, 3]\nvar d = new f(...a0)\nvar c = d\n}());")
+		})
+		o("array spread", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "var a = [1, 2, 3]\nvar arr = [...a]\nmodule.exports = arr",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nvar a0 = [1, 2, 3]\nvar arr = [...a0]\nvar c = arr\n}());")
+		})
+		o("array spread (merge)", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "var a = [1, 2, 3]\nvar arr = [0, ...a, 4]\nmodule.exports = arr",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nvar a0 = [1, 2, 3]\nvar arr = [0, ...a0, 4]\nvar c = arr\n}());")
+		})
+		o("array destructuring", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "var a = [1, 2, 3]\nvar d\n[d, ...a] = a\nmodule.exports = a",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nvar a0 = [1, 2, 3]\nvar d\n[d, ...a0] = a0\nvar c = a0\n}());")
+		})
+		o("object spread", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "var a = { p: 1, q: 2, r: 3 }\nvar d = {...a}\nmodule.exports = d",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nvar a0 = { p: 1, q: 2, r: 3 }\nvar d = {...a0}\nvar c = d\n}());")
+		})
+		o("object spread (merge)", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "var a = { p: 1, q: 2, r: 3 }\nvar d = {o:0,...a}\nmodule.exports = d",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nvar a0 = { p: 1, q: 2, r: 3 }\nvar d = {o:0,...a0}\nvar c = d\n}());")
+		})
+		o("object destructuring", async () => {
+			await setup({
+				"a.js": 'var b = require("./b")\nvar c = require("./c")',
+				"b.js": "var a = 1\nmodule.exports = a",
+				"c.js": "var obj = { p: 1, q: 2, r: 3 }\nvar p,a\n({p,...a}=obj)\nmodule.exports = a",
+			})
+
+			o(await bundle(p("a.js"))).equals(";(function() {\nvar a = 1\nvar b = a\nvar obj = { p: 1, q: 2, r: 3 }\nvar p,a0\n({p,...a0}=obj)\nvar c = a0\n}());")
+		})
+	})
 })