From 51771f7864d288c7f6bd00fd138cf5de59dd5404 Mon Sep 17 00:00:00 2001 From: Andre Wiggins <459878+andrewiggins@users.noreply.github.com> Date: Wed, 8 Nov 2023 14:59:01 -0800 Subject: [PATCH] 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) --- .github/workflows/single-bench.yml | 2 +- benches/src/03_update10th1k_x16.html | 8 +- src/diff/index.js | 126 +++++++++++++++++---------- src/diff/props.js | 33 +------ 4 files changed, 88 insertions(+), 81 deletions(-) diff --git a/.github/workflows/single-bench.yml b/.github/workflows/single-bench.yml index e59c4e1f44..5682a6f7d6 100644 --- a/.github/workflows/single-bench.yml +++ b/.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 diff --git a/benches/src/03_update10th1k_x16.html b/benches/src/03_update10th1k_x16.html index 2042c501c4..9f770f7e5d 100644 --- a/benches/src/03_update10th1k_x16.html +++ b/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,7 +55,9 @@ 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)); @@ -61,9 +65,11 @@ } async function run() { + markRunStart('final'); performance.mark('start'); update(); + await markRunEnd('final'); await afterFrameAsync(); testElementTextContains(getRowLinkSel(991), repeat(' !!!', 3 + 1)); performance.mark('stop'); diff --git a/src/diff/index.js b/src/diff/index.js index 515f1b40ab..b5bbb49db6 100644 --- a/src/diff/index.js +++ b/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 -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