Skip to content

Fix recent perf regressions #188

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
merged 1 commit into from
Aug 22, 2019
Merged

Fix recent perf regressions #188

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
12 changes: 4 additions & 8 deletions packages/custom-elements/src/CustomElementInternals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,7 @@ export default class CustomElementInternals {
upgradeReaction(element) {
try {
const definition = this._lookupACustomElementDefinition(
/** @type {!Document} */ (element.ownerDocument),
element.namespaceURI, element.localName);
/** @type {!Document} */ (element.ownerDocument), element.localName);
if (definition) {
this._upgradeAnElement(element, definition);
}
Expand Down Expand Up @@ -393,19 +392,16 @@ export default class CustomElementInternals {
}

/**
* Runs HTML's 'look up a custom element definition'.
* Runs HTML's 'look up a custom element definition', excluding the namespace
* check.
*
* @private
* @param {!Document} doc
* @param {string} namespace
* @param {string} localName
* @return {!CustomElementDefinition|undefined}
* @see https://html.spec.whatwg.org/multipage/custom-elements.html#look-up-a-custom-element-definition
*/
_lookupACustomElementDefinition(doc, namespace, localName) {
// The namespace must be the HTML namespace.
if (namespace !== NS_HTML) return;

_lookupACustomElementDefinition(doc, localName) {
// The document must be associated with a registry.
const registry = doc.__CE_registry;
if (!registry) return;
Expand Down
26 changes: 16 additions & 10 deletions packages/custom-elements/src/CustomElementRegistry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ export default class CustomElementRegistry {
* but the list of elements is only populated during a flush after which
* all of the entries are removed. DO NOT edit outside of `#_flush`.
* @private
* @type {!Map<string, !Array<!HTMLElement>>}
* @type {!Array<string>}
*/
this._elementsWithPendingDefinitions = new Map();
this._unflushedLocalNames = [];

/**
* @private
Expand All @@ -102,7 +102,7 @@ export default class CustomElementRegistry {
this.internal_assertCanDefineLocalName(localName);

this._localNameToConstructorGetter.set(localName, constructorGetter);
this._elementsWithPendingDefinitions.set(localName, []);
this._unflushedLocalNames.push(localName);

// If we've already called the flush callback and it hasn't called back yet,
// don't call it again.
Expand All @@ -124,7 +124,7 @@ export default class CustomElementRegistry {
this.internal_assertCanDefineLocalName(localName);

this.internal_reifyDefinition(localName, constructor);
this._elementsWithPendingDefinitions.set(localName, []);
this._unflushedLocalNames.push(localName);

// If we've already called the flush callback and it hasn't called back yet,
// don't call it again.
Expand Down Expand Up @@ -234,7 +234,11 @@ export default class CustomElementRegistry {
*/
const elementsWithStableDefinitions = [];

const elementsWithPendingDefinitions = this._elementsWithPendingDefinitions;
const unflushedLocalNames = this._unflushedLocalNames;
const elementsWithPendingDefinitions = new Map();
for (let i = 0; i < unflushedLocalNames.length; i++) {
elementsWithPendingDefinitions.set(unflushedLocalNames[i], []);
}

this._internals.patchAndUpgradeTree(document, {
upgrade: element => {
Expand Down Expand Up @@ -262,7 +266,10 @@ export default class CustomElementRegistry {
}

// Upgrade elements with 'pending' definitions in the order they were defined.
elementsWithPendingDefinitions.forEach((pendingUpgradableElements, localName) => {
for (let i = 0; i < unflushedLocalNames.length; i++) {
const localName = unflushedLocalNames[i];
const pendingUpgradableElements = elementsWithPendingDefinitions.get(localName);

// Attempt to upgrade all applicable elements.
for (let i = 0; i < pendingUpgradableElements.length; i++) {
this._internals.upgradeReaction(pendingUpgradableElements[i]);
Expand All @@ -273,9 +280,9 @@ export default class CustomElementRegistry {
if (deferred) {
deferred.resolve(undefined);
}
});
}

elementsWithPendingDefinitions.clear();
unflushedLocalNames.length = 0;
}

/**
Expand Down Expand Up @@ -319,8 +326,7 @@ export default class CustomElementRegistry {
// resolved early here even though it might fail later when reified.
const anyDefinitionExists = this._localNameToDefinition.has(localName) ||
this._localNameToConstructorGetter.has(localName);
const definitionHasFlushed =
!this._elementsWithPendingDefinitions.has(localName);
const definitionHasFlushed = this._unflushedLocalNames.indexOf(localName) === -1;
if (anyDefinitionExists && definitionHasFlushed) {
deferred.resolve(undefined);
}
Expand Down