Inline diffProps function (#4200)

Inline `diffProps` to take advantage of the props loop to read values and use later in the diff. This avoids duplicate megamorphic reads for props like `dangerouslySetInnerHTML`, `children`, `value`, and `checked` since we can reuse reading the value in the props loop for handling elsewhere.

* Inline diffProps (-11 B)
* Inline dangerouslySetInnerHtml handling (-1 B)
   Removes a guaranteed megamorphic access in diffElementNodes
* Golf dangerouslySetInnerHtml handling (-4 B)
* Pull of new children while looping through props (+2 B)
* Read prop value once at start of props loop (+0 B)
* Use prop loop for handling `value` and `checked` (-20 B)
* Add types to new variables
* Clean up comments
* Fix benchmark name
* Add run tracking to 03_update10th1k
* Capture input value and checked props in first props loop (+17 B)
* Golf diffElementNodes (-7 B)

Author: andrewiggins

.github/workflows/single-bench.yml

@@ -8,7 +8,7 @@ on:
         type: choice
         options:
           - 02_replace1k
-          - 03_update10k
+          - 03_update10th1k_x16
           - 07_create10k
           - filter_list
           - hydrate1k

benches/src/03_update10th1k_x16.html

@@ -27,7 +27,9 @@
 				afterFrameAsync,
 				getRowLinkSel,
 				testElement,
-				testElementTextContains
+				testElementTextContains,
+				markRunStart,
+				markRunEnd
 			} from './util.js';
 			import * as framework from 'framework';
 			import { render } from '../src/keyed-children/index.js';
@@ -53,17 +55,21 @@
 				testElement(getRowLinkSel(1000));
 
 				for (let i = 0; i < 3; i++) {
+					markRunStart(`warmup-${i}`);
 					update();
+					await markRunEnd(`warmup-${i}`);
 
 					await afterFrameAsync();
 					testElementTextContains(getRowLinkSel(991), repeat(' !!!', i + 1));
 				}
 			}
 
 			async function run() {
+				markRunStart('final');
 				performance.mark('start');
 				update();
 
+				await markRunEnd('final');
 				await afterFrameAsync();
 				testElementTextContains(getRowLinkSel(991), repeat(' !!!', 3 + 1));
 				performance.mark('stop');

src/diff/index.js

@@ -7,7 +7,7 @@ import {
 import { BaseComponent, getDomSibling } from '../component';
 import { Fragment } from '../create-element';
 import { diffChildren } from './children';
-import { diffProps, setProperty } from './props';
+import { setProperty } from './props';
 import { assign, isArray, removeNode, slice } from '../util';
 import options from '../options';
 
@@ -364,24 +364,33 @@ function diffElementNodes(
 	let newProps = newVNode.props;
 	let nodeType = /** @type {string} */ (newVNode.type);
 	/** @type {any} */
-	let i = 0;
+	let i;
+	/** @type {{ __html?: string }} */
+	let newHtml;
+	/** @type {{ __html?: string }} */
+	let oldHtml;
+	/** @type {ComponentChildren} */
+	let newChildren;
+	let value;
+	let inputValue;
+	let checked;
 
 	// Tracks entering and exiting SVG namespace when descending through the tree.
 	if (nodeType === 'svg') isSvg = true;
 
 	if (excessDomChildren != null) {
-		for (; i < excessDomChildren.length; i++) {
-			const child = excessDomChildren[i];
+		for (i = 0; i < excessDomChildren.length; i++) {
+			value = excessDomChildren[i];
 
 			// if newVNode matches an element in excessDomChildren or the `dom`
 			// argument matches an element in excessDomChildren, remove it from
 			// excessDomChildren so it isn't later removed in diffChildren
 			if (
-				child &&
-				'setAttribute' in child === !!nodeType &&
-				(nodeType ? child.localName === nodeType : child.nodeType === 3)
+				value &&
+				'setAttribute' in value === !!nodeType &&
+				(nodeType ? value.localName === nodeType : value.nodeType === 3)
 			) {
-				dom = child;
+				dom = value;
 				excessDomChildren[i] = null;
 				break;
 			}
@@ -401,7 +410,8 @@ function diffElementNodes(
 
 		// we created a new parent, so none of the previously attached children can be reused:
 		excessDomChildren = null;
-		// we are creating a new node, so we can assume this is a new subtree (in case we are hydrating), this deopts the hydrate
+		// we are creating a new node, so we can assume this is a new subtree (in
+		// case we are hydrating), this deopts the hydrate
 		isHydrating = false;
 	}
 
@@ -416,43 +426,67 @@ function diffElementNodes(
 
 		oldProps = oldVNode.props || EMPTY_OBJ;
 
-		let oldHtml = oldProps.dangerouslySetInnerHTML;
-		let newHtml = newProps.dangerouslySetInnerHTML;
-
-		// During hydration, props are not diffed at all (including dangerouslySetInnerHTML)
-		// @TODO we should warn in debug mode when props don't match here.
-		if (!isHydrating) {
-			// But, if we are in a situation where we are using existing DOM (e.g. replaceNode)
-			// we should read the existing DOM attributes to diff them
-			if (excessDomChildren != null) {
-				oldProps = {};
-				for (i = 0; i < dom.attributes.length; i++) {
-					oldProps[dom.attributes[i].name] = dom.attributes[i].value;
-				}
+		// If we are in a situation where we are not hydrating but are using
+		// existing DOM (e.g. replaceNode) we should read the existing DOM
+		// attributes to diff them
+		if (!isHydrating && excessDomChildren != null) {
+			oldProps = {};
+			for (i = 0; i < dom.attributes.length; i++) {
+				value = dom.attributes[i];
+				oldProps[value.name] = value.value;
 			}
+		}
 
-			if (newHtml || oldHtml) {
-				// Avoid re-applying the same '__html' if it did not changed between re-render
-				if (
-					!newHtml ||
-					((!oldHtml || newHtml.__html != oldHtml.__html) &&
-						newHtml.__html !== dom.innerHTML)
-				) {
-					dom.innerHTML = (newHtml && newHtml.__html) || '';
-				}
+		for (i in oldProps) {
+			value = oldProps[i];
+			if (i == 'children') {
+			} else if (i == 'dangerouslySetInnerHTML') {
+				oldHtml = value;
+			} else if (i !== 'key' && !(i in newProps)) {
+				setProperty(dom, i, null, value, isSvg);
 			}
 		}
 
-		diffProps(dom, newProps, oldProps, isSvg, isHydrating);
+		// During hydration, props are not diffed at all (including dangerouslySetInnerHTML)
+		// @TODO we should warn in debug mode when props don't match here.
+		for (i in newProps) {
+			value = newProps[i];
+			if (i == 'children') {
+				newChildren = value;
+			} else if (i == 'dangerouslySetInnerHTML') {
+				newHtml = value;
+			} else if (i == 'value') {
+				inputValue = value;
+			} else if (i == 'checked') {
+				checked = value;
+			} else if (
+				i !== 'key' &&
+				(!isHydrating || typeof value == 'function') &&
+				oldProps[i] !== value
+			) {
+				setProperty(dom, i, value, oldProps[i], isSvg);
+			}
+		}
 
 		// If the new vnode didn't have dangerouslySetInnerHTML, diff its children
 		if (newHtml) {
+			// Avoid re-applying the same '__html' if it did not changed between re-render
+			if (
+				!isHydrating &&
+				(!oldHtml ||
+					(newHtml.__html !== oldHtml.__html &&
+						newHtml.__html !== dom.innerHTML))
+			) {
+				dom.innerHTML = newHtml.__html;
+			}
+
 			newVNode._children = [];
 		} else {
-			i = newVNode.props.children;
+			if (oldHtml) dom.innerHTML = '';
+
 			diffChildren(
 				dom,
-				isArray(i) ? i : [i],
+				isArray(newChildren) ? newChildren : [newChildren],
 				newVNode,
 				oldVNode,
 				globalContext,
@@ -474,30 +508,28 @@ function diffElementNodes(
 			}
 		}
 
-		// (as above, don't diff props during hydration)
+		// As above, don't diff props during hydration
 		if (!isHydrating) {
+			i = 'value';
 			if (
-				'value' in newProps &&
-				(i = newProps.value) !== undefined &&
+				inputValue !== undefined &&
 				// #2756 For the <progress>-element the initial value is 0,
 				// despite the attribute not being present. When the attribute
 				// is missing the progress bar is treated as indeterminate.
 				// To fix that we'll always update it when it is 0 for progress elements
-				(i !== dom.value ||
-					(nodeType === 'progress' && !i) ||
+				(inputValue !== dom[i] ||
+					(nodeType === 'progress' && !inputValue) ||
 					// This is only for IE 11 to fix <select> value not being updated.
 					// To avoid a stale select value we need to set the option.value
 					// again, which triggers IE11 to re-evaluate the select value
-					(nodeType === 'option' && i !== oldProps.value))
+					(nodeType === 'option' && inputValue !== oldProps[i]))
 			) {
-				setProperty(dom, 'value', i, oldProps.value, false);
+				setProperty(dom, i, inputValue, oldProps[i], false);
 			}
-			if (
-				'checked' in newProps &&
-				(i = newProps.checked) !== undefined &&
-				i !== dom.checked
-			) {
-				setProperty(dom, 'checked', i, oldProps.checked, false);
+
+			i = 'checked';
+			if (checked !== undefined && checked !== dom[i]) {
+				setProperty(dom, i, checked, oldProps[i], false);
 			}
 		}
 	}

src/diff/props.js

@@ -1,37 +1,6 @@
 import { IS_NON_DIMENSIONAL } from '../constants';
 import options from '../options';
 
-/**
- * Diff the old and new properties of a VNode and apply changes to the DOM node
- * @param {PreactElement} dom The DOM node to apply changes to
- * @param {object} newProps The new props
- * @param {object} oldProps The old props
- * @param {boolean} isSvg Whether or not this node is an SVG node
- * @param {boolean} hydrate Whether or not we are in hydration mode
- */
-export function diffProps(dom, newProps, oldProps, isSvg, hydrate) {
-	let i;
-
-	for (i in oldProps) {
-		if (i !== 'children' && i !== 'key' && !(i in newProps)) {
-			setProperty(dom, i, null, oldProps[i], isSvg);
-		}
-	}
-
-	for (i in newProps) {
-		if (
-			(!hydrate || typeof newProps[i] == 'function') &&
-			i !== 'children' &&
-			i !== 'key' &&
-			i !== 'value' &&
-			i !== 'checked' &&
-			oldProps[i] !== newProps[i]
-		) {
-			setProperty(dom, i, newProps[i], oldProps[i], isSvg);
-		}
-	}
-}
-
 function setStyle(style, key, value) {
 	if (key[0] === '-') {
 		style.setProperty(key, value == null ? '' : value);
@@ -104,7 +73,7 @@ export function setProperty(dom, name, value, oldValue, isSvg) {
 			const handler = useCapture ? eventProxyCapture : eventProxy;
 			dom.removeEventListener(name, handler, useCapture);
 		}
-	} else if (name !== 'dangerouslySetInnerHTML') {
+	} else {
 		if (isSvg) {
 			// Normalize incorrect prop usage for SVG:
 			// - xlink:href / xlinkHref --> href (xlink:href was removed from SVG and isn't needed)