Skip to content

Perf: Insert nodes top-down in IE and Edge #5253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sophiebits merged 1 commit into from
Oct 23, 2015
Merged

Perf: Insert nodes top-down in IE and Edge #5253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/renderers/dom/client/ReactMount.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
'use strict';

var ClientReactRootIndex = require('ClientReactRootIndex');
var DOMLazyTree = require('DOMLazyTree');
var DOMProperty = require('DOMProperty');
var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
var ReactCurrentOwner = require('ReactCurrentOwner');
Expand Down Expand Up @@ -1038,7 +1039,7 @@ var ReactMount = {
while (container.lastChild) {
container.removeChild(container.lastChild);
}
container.appendChild(markup);
DOMLazyTree.insertTreeBefore(container, markup, null);
} else {
setInnerHTML(container, markup);
}
Expand Down
60 changes: 38 additions & 22 deletions src/renderers/dom/client/utils/DOMChildrenOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

'use strict';

var DOMLazyTree = require('DOMLazyTree');
var Danger = require('Danger');
var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes');
var ReactPerf = require('ReactPerf');
Expand All @@ -29,21 +30,27 @@ var invariant = require('invariant');
* @internal
*/
function insertChildAt(parentNode, childNode, index) {
// By exploiting arrays returning `undefined` for an undefined index, we can
// rely exclusively on `insertBefore(node, null)` instead of also using
// `appendChild(node)`. However, using `undefined` is not allowed by all
// browsers so we must replace it with `null`.

// fix render order error in safari
// IE8 will throw error when index out of list size.
var beforeChild = index >= parentNode.childNodes.length ?
null :
parentNode.childNodes.item(index);

parentNode.insertBefore(
childNode,
beforeChild
);
// We can rely exclusively on `insertBefore(node, null)` instead of also using
// `appendChild(node)`. (Using `undefined` is not allowed by all browsers so
// we are careful to use `null`.)

// In Safari, .childNodes[index] can return a DOM node with id={index} so we
// use .item() instead which is immune to this bug. (See #3560.) In contrast
// to the spec, IE8 throws an error if index is larger than the list size.
var referenceNode =
index < parentNode.childNodes.length ?
parentNode.childNodes.item(index) : null;

parentNode.insertBefore(childNode, referenceNode);
}

function insertLazyTreeChildAt(parentNode, childTree, index) {
// See above.
var referenceNode =
index < parentNode.childNodes.length ?
parentNode.childNodes.item(index) : null;

DOMLazyTree.insertTreeBefore(parentNode, childTree, referenceNode);
}

/**
Expand Down Expand Up @@ -99,9 +106,10 @@ var DOMChildrenOperations = {
}
}

var renderedMarkup;
// markupList is either a list of markup or just a list of elements
if (markupList.length && typeof markupList[0] === 'string') {
var isHTML = markupList.length && typeof markupList[0] === 'string';
var renderedMarkup;
if (isHTML) {
renderedMarkup = Danger.dangerouslyRenderMarkup(markupList);
} else {
renderedMarkup = markupList;
Expand All @@ -118,11 +126,19 @@ var DOMChildrenOperations = {
update = updates[k];
switch (update.type) {
case ReactMultiChildUpdateTypes.INSERT_MARKUP:
insertChildAt(
update.parentNode,
renderedMarkup[update.markupIndex],
update.toIndex
);
if (isHTML) {
insertChildAt(
update.parentNode,
renderedMarkup[update.markupIndex],
update.toIndex
);
} else {
insertLazyTreeChildAt(
update.parentNode,
renderedMarkup[update.markupIndex],
update.toIndex
);
}
break;
case ReactMultiChildUpdateTypes.MOVE_EXISTING:
insertChildAt(
Expand Down
88 changes: 88 additions & 0 deletions src/renderers/dom/client/utils/DOMLazyTree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/**
* Copyright 2015, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule DOMLazyTree
*/

'use strict';

/**
* In IE (8-11) and Edge, appending nodes with no children is dramatically
* faster than appending a full subtree, so we essentially queue up the
* .appendChild calls here and apply them so each node is added to its parent
* before any children are added.
*
* In other browsers, doing so is slower or neutral compared to the other order
* (in Firefox, twice as slow) so we only do this inversion in IE.
*
* See https://github.com/spicyj/innerhtml-vs-createelement-vs-clonenode.
*/
var enableLazy = (
typeof document !== 'undefined' &&
typeof document.documentMode === 'number'
||
typeof navigator !== 'undefined' &&
typeof navigator.userAgent === 'string' &&
/\bEdge\/\d/.test(navigator.userAgent)
Copy link
Copy Markdown
Collaborator Author

@sophiebits sophiebits Jan 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickkettner @csuwildcat Here in React, I added a user-agent test that catches all versions of Edge (and IE) because the DOM implementation in Edge is much slower when reparenting large trees than other browsers. That is, to create the structure

<body>
  <article>
    <section>
      <div></div>
    </section>
  </article>
</body>

the operations

var article = document.createElement('article');
var section = document.createElement('section');
var div = document.createElement('div');
section.appendChild(div);
article.appendChild(section);
body.appendChild(article);

are most natural for us, but we are forced to reorder the operations in Edge to be

var article = document.createElement('article');
var section = document.createElement('section');
var div = document.createElement('div');
body.appendChild(article);
article.appendChild(section);
section.appendChild(div);

in order to get reasonable performance. See the link in the comment above for more details. This has not shipped in a React release yet. I heard that there is a chance this may be fixed soon in Edge – obviously I would prefer to not ship a React with this feature test and would much prefer to put an upper bound on the versions of Edge where we use this workaround. Anything I can do to help get this fixed in Edge soon?

Copy link
Copy Markdown

@csuwildcat csuwildcat Feb 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a curiosity than anything: does the perf profile look the same if you do the same sequence of appends with nodes in a Document Fragment?

Copy link
Copy Markdown
Collaborator Author

@sophiebits sophiebits Feb 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not certain but it didn't seem likely to make a difference. That is, parent.appendChild(newChild) seems to take time proportional to the number of nodes in the newChild subtree, whereas every other browser seems to make it constant time.

Copy link
Copy Markdown
Contributor

@syranide syranide Feb 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that too back then, made no difference that I could tell.

);

function insertTreeChildren(tree) {
if (!enableLazy) {
return;
}
var node = tree.node;
var children = tree.children;
if (children.length) {
for (var i = 0; i < children.length; i++) {
insertTreeBefore(node, children[i], null);
}
} else if (tree.html != null) {
node.innerHTML = tree.html;
}
}

function insertTreeBefore(parentNode, tree, referenceNode) {
parentNode.insertBefore(tree.node, referenceNode);
insertTreeChildren(tree);
}

function replaceChildWithTree(oldNode, newTree) {
oldNode.parentNode.replaceChild(newTree.node, oldNode);
insertTreeChildren(newTree);
}

function queueChild(parentTree, childTree) {
if (enableLazy) {
parentTree.children.push(childTree);
} else {
parentTree.node.appendChild(childTree.node);
}
}

function queueHTML(tree, html) {
if (enableLazy) {
tree.html = html;
} else {
tree.node.innerHTML = html;
}
}

function DOMLazyTree(node) {
return {
node: node,
children: [],
html: null,
};
}

DOMLazyTree.insertTreeBefore = insertTreeBefore;
DOMLazyTree.replaceChildWithTree = replaceChildWithTree;
DOMLazyTree.queueChild = queueChild;
DOMLazyTree.queueHTML = queueHTML;

module.exports = DOMLazyTree;
8 changes: 4 additions & 4 deletions src/renderers/dom/shared/Danger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

'use strict';

var DOMLazyTree = require('DOMLazyTree');
var ExecutionEnvironment = require('ExecutionEnvironment');

var createNodesFromMarkup = require('createNodesFromMarkup');
Expand Down Expand Up @@ -172,13 +173,12 @@ var Danger = {
'server rendering. See ReactDOMServer.renderToString().'
);

var newChild;
if (typeof markup === 'string') {
newChild = createNodesFromMarkup(markup, emptyFunction)[0];
var newChild = createNodesFromMarkup(markup, emptyFunction)[0];
oldChild.parentNode.replaceChild(newChild, oldChild);
} else {
newChild = markup;
DOMLazyTree.replaceChildWithTree(oldChild, markup);
}
oldChild.parentNode.replaceChild(newChild, oldChild);
},

};
Expand Down
15 changes: 8 additions & 7 deletions src/renderers/dom/shared/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

var AutoFocusUtils = require('AutoFocusUtils');
var CSSPropertyOperations = require('CSSPropertyOperations');
var DOMLazyTree = require('DOMLazyTree');
var DOMNamespaces = require('DOMNamespaces');
var DOMProperty = require('DOMProperty');
var DOMPropertyOperations = require('DOMPropertyOperations');
Expand All @@ -41,7 +42,6 @@ var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
var invariant = require('invariant');
var isEventSupported = require('isEventSupported');
var keyOf = require('keyOf');
var setInnerHTML = require('setInnerHTML');
var setTextContent = require('setTextContent');
var shallowEqual = require('shallowEqual');
var validateDOMNesting = require('validateDOMNesting');
Expand Down Expand Up @@ -667,8 +667,9 @@ ReactDOMComponent.Mixin = {
// Populate node cache
ReactMount.getID(el);
this._updateDOMProperties({}, props, transaction);
this._createInitialChildren(transaction, props, context, el);
mountImage = el;
var lazyTree = DOMLazyTree(el);
this._createInitialChildren(transaction, props, context, lazyTree);
mountImage = lazyTree;
} else {
var tagOpen = this._createOpenTagMarkupAndPutListeners(transaction, props);
var tagContent = this._createContentMarkup(transaction, props, context);
Expand Down Expand Up @@ -814,28 +815,28 @@ ReactDOMComponent.Mixin = {
}
},

_createInitialChildren: function(transaction, props, context, el) {
_createInitialChildren: function(transaction, props, context, lazyTree) {
// Intentional use of != to avoid catching zero/false.
var innerHTML = props.dangerouslySetInnerHTML;
if (innerHTML != null) {
if (innerHTML.__html != null) {
setInnerHTML(el, innerHTML.__html);
DOMLazyTree.queueHTML(lazyTree, innerHTML.__html);
}
} else {
var contentToUse =
CONTENT_TYPES[typeof props.children] ? props.children : null;
var childrenToUse = contentToUse != null ? null : props.children;
if (contentToUse != null) {
// TODO: Validate that text is allowed as a child of this node
setTextContent(el, contentToUse);
setTextContent(lazyTree.node, contentToUse);
} else if (childrenToUse != null) {
var mountImages = this.mountChildren(
childrenToUse,
transaction,
context
);
for (var i = 0; i < mountImages.length; i++) {
el.appendChild(mountImages[i]);
DOMLazyTree.queueChild(lazyTree, mountImages[i]);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/renderers/dom/shared/ReactDOMTextComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
'use strict';

var DOMChildrenOperations = require('DOMChildrenOperations');
var DOMLazyTree = require('DOMLazyTree');
var DOMPropertyOperations = require('DOMPropertyOperations');
var ReactComponentBrowserEnvironment =
require('ReactComponentBrowserEnvironment');
Expand Down Expand Up @@ -106,7 +107,7 @@ assign(ReactDOMTextComponent.prototype, {
// Populate node cache
ReactMount.getID(el);
setTextContent(el, this._stringText);
return el;
return DOMLazyTree(el);
} else {
var escapedText = escapeTextContentForBrowser(this._stringText);

Expand Down