From 280f4f0a374c8dc36048fca95f05d9860f7f27a3 Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Thu, 12 Sep 2019 15:11:38 -0700 Subject: [PATCH] Applies micro-optimizations and removes obsolete settings Applies micro-optimizations that were found to improve element creation benchmarks by 5-10%, and removes obsolete settings: * Removed `legacyNoBatch` and `legacyNotifyOrder` settings. * dom-if/repeat: `dom-change` and `renderedCount` no longer fire with `legacyOptimizations` set. * legacy-element-mixin: `isAttached` now set before calling `connectedCallback` so it is batched with initial rendering. * `PropertiesChanged`: property accessor code now inlined for efficiency rather than calling `_get/_setProperty`. The `__dataCounter` tracking flag has been moved here to avoid the need to override `_flushProperties` in `PropertyEffects`. * `PropertyEffects`: inlined `runEffectsForProperty` into `runEffects` for efficiency. Removed wrapping around sending data events. * `async`: In the microtask scheduler, now only provoke a DOM mutation if needed. --- lib/elements/dom-if.js | 4 +- lib/elements/dom-repeat.js | 13 ++-- lib/legacy/legacy-element-mixin.js | 2 +- lib/mixins/properties-changed.js | 13 +++- lib/mixins/property-accessors.js | 11 +-- lib/mixins/property-effects.js | 115 ++++++++--------------------- lib/utils/async.js | 7 +- lib/utils/settings.js | 38 ---------- test/runner.html | 1 - test/unit/property-effects.html | 39 +++------- 10 files changed, 69 insertions(+), 174 deletions(-) diff --git a/lib/elements/dom-if.js b/lib/elements/dom-if.js index 22d3ca664c..e6eca76649 100644 --- a/lib/elements/dom-if.js +++ b/lib/elements/dom-if.js @@ -15,7 +15,7 @@ import { microTask } from '../utils/async.js'; import { root } from '../utils/path.js'; import { wrap } from '../utils/wrap.js'; import { hideElementsGlobally } from '../utils/hide-template-controls.js'; -import { fastDomIf, strictTemplatePolicy } from '../utils/settings.js'; +import { fastDomIf, strictTemplatePolicy, legacyOptimizations } from '../utils/settings.js'; import { showHideChildren, templatize } from '../utils/templatize.js'; /** @@ -244,7 +244,7 @@ class DomIfBase extends PolymerElement { this.__teardownInstance(); } this._showHideChildren(); - if (this.if != this._lastIf) { + if (!legacyOptimizations && this.if != this._lastIf) { this.dispatchEvent(new CustomEvent('dom-change', { bubbles: true, composed: true diff --git a/lib/elements/dom-repeat.js b/lib/elements/dom-repeat.js index 85204d840a..7ad1a02269 100644 --- a/lib/elements/dom-repeat.js +++ b/lib/elements/dom-repeat.js @@ -17,6 +17,7 @@ import { matches, translate } from '../utils/path.js'; import { timeOut, microTask } from '../utils/async.js'; import { wrap } from '../utils/wrap.js'; import { hideElementsGlobally } from '../utils/hide-template-controls.js'; +import { legacyOptimizations } from '../utils/settings.js'; /** * @constructor @@ -239,7 +240,7 @@ export class DomRepeat extends domRepeatBase { */ renderedItemCount: { type: Number, - notify: true, + notify: !legacyOptimizations, readOnly: true }, @@ -545,10 +546,12 @@ export class DomRepeat extends domRepeatBase { // Set rendered item count this._setRenderedItemCount(this.__instances.length); // Notify users - this.dispatchEvent(new CustomEvent('dom-change', { - bubbles: true, - composed: true - })); + if (!legacyOptimizations) { + this.dispatchEvent(new CustomEvent('dom-change', { + bubbles: true, + composed: true + })); + } // Check to see if we need to render more items this.__tryRenderChunk(); } diff --git a/lib/legacy/legacy-element-mixin.js b/lib/legacy/legacy-element-mixin.js index 963e496853..1e903d5285 100644 --- a/lib/legacy/legacy-element-mixin.js +++ b/lib/legacy/legacy-element-mixin.js @@ -122,8 +122,8 @@ export const LegacyElementMixin = dedupingMixin((base) => { * @override */ connectedCallback() { - super.connectedCallback(); this.isAttached = true; + super.connectedCallback(); this.attached(); } diff --git a/lib/mixins/properties-changed.js b/lib/mixins/properties-changed.js index 854b5b72f8..9e481d6734 100644 --- a/lib/mixins/properties-changed.js +++ b/lib/mixins/properties-changed.js @@ -160,11 +160,15 @@ export const PropertiesChanged = dedupingMixin( /* eslint-disable valid-jsdoc */ /** @this {PropertiesChanged} */ get() { - return this._getProperty(property); + // Inline for perf instead of using `_getProperty` + return this.__data[property]; }, /** @this {PropertiesChanged} */ set: readOnly ? function () {} : function (value) { - this._setProperty(property, value); + // Inline for perf instead of using `_setProperty` + if (this._setPendingProperty(property, value, true)) { + this._invalidateProperties(); + } } /* eslint-enable */ }); @@ -180,6 +184,9 @@ export const PropertiesChanged = dedupingMixin( this.__dataPending = null; this.__dataOld = null; this.__dataInstanceProps = null; + /** @type {number} */ + // NOTE: used to track re-entrant calls to `_flushProperties` + this.__dataCounter = 0; this.__serializing = false; this._initializeProperties(); } @@ -367,6 +374,7 @@ export const PropertiesChanged = dedupingMixin( * @override */ _flushProperties() { + this.__dataCounter++; const props = this.__data; const changedProps = this.__dataPending; const old = this.__dataOld; @@ -375,6 +383,7 @@ export const PropertiesChanged = dedupingMixin( this.__dataOld = null; this._propertiesChanged(props, changedProps, old); } + this.__dataCounter--; } /** diff --git a/lib/mixins/property-accessors.js b/lib/mixins/property-accessors.js index d886549fa4..2590165c03 100644 --- a/lib/mixins/property-accessors.js +++ b/lib/mixins/property-accessors.js @@ -12,7 +12,6 @@ import '../utils/boot.js'; import { dedupingMixin } from '../utils/mixin.js'; import { camelToDashCase, dashToCamelCase } from '../utils/case-map.js'; import { PropertiesChanged } from './properties-changed.js'; -import { legacyNoBatch } from '../utils/settings.js'; // Save map of native properties; this forms a blacklist or properties // that won't have their values "saved" by `saveAccessorValue`, since @@ -295,15 +294,7 @@ export const PropertyAccessors = dedupingMixin(superClass => { * @override */ _definePropertyAccessor(property, readOnly) { - if (legacyNoBatch) { - // Ensure properties are not flushed when adding instance effects - const ready = this.__dataReady; - this.__dataReady = false; - saveAccessorValue(this, property); - this.__dataReady = ready; - } else { - saveAccessorValue(this, property); - } + saveAccessorValue(this, property); super._definePropertyAccessor(property, readOnly); } diff --git a/lib/mixins/property-effects.js b/lib/mixins/property-effects.js index 50abd2a2c5..7ec3331fbd 100644 --- a/lib/mixins/property-effects.js +++ b/lib/mixins/property-effects.js @@ -19,7 +19,7 @@ import { camelToDashCase, dashToCamelCase } from '../utils/case-map.js'; import { PropertyAccessors } from './property-accessors.js'; /* for annotated effects */ import { TemplateStamp } from './template-stamp.js'; -import { sanitizeDOMValue, legacyUndefined, legacyNoBatch, legacyNotifyOrder, orderedComputed, removeNestedTemplates, fastDomIf } from '../utils/settings.js'; +import { sanitizeDOMValue, legacyUndefined, orderedComputed, removeNestedTemplates, fastDomIf } from '../utils/settings.js'; // Monotonically increasing unique ID used for de-duping effects triggered // from multiple properties in the same turn @@ -125,12 +125,22 @@ function ensureOwnEffectMap(model, type, cloneArrays) { function runEffects(inst, effects, props, oldProps, hasPaths, extraArgs) { if (effects) { let ran = false; - let id = dedupeId++; + const id = dedupeId++; for (let prop in props) { - if (runEffectsForProperty( - inst, /** @type {!Object} */ (effects), id, prop, props, oldProps, - hasPaths, extraArgs)) { - ran = true; + // Inline `runEffectsForProperty` for perf. + let rootProperty = hasPaths ? root(prop) : prop; + let fxs = effects[rootProperty]; + if (fxs) { + for (let i=0, l=fxs.length, fx; (i { /** @type {boolean} */ // Used to identify users of this mixin, ala instanceof this.__isPropertyEffectsClient = true; - /** @type {number} */ - // NOTE: used to track re-entrant calls to `_flushProperties` - // path changes dirty check against `__dataTemp` only during one "turn" - // and are cleared when `__dataCounter` returns to 0. - this.__dataCounter = 0; /** @type {boolean} */ this.__dataClientsReady; /** @type {Array} */ @@ -1690,12 +1696,6 @@ export const PropertyEffects = dedupingMixin(superClass => { this.__dataToNotify = this.__dataToNotify || {}; this.__dataToNotify[property] = shouldNotify; } - if (legacyNoBatch) { - this._invalidateProperties(); - // Returning false here means "the change was already handled, no need - // to invalidate" - return false; - } return true; } return false; @@ -1748,45 +1748,6 @@ export const PropertyEffects = dedupingMixin(superClass => { } } - /** - * Overrides superclass implementation. - * - * @override - * @return {void} - * @protected - */ - _flushProperties() { - this.__dataCounter++; - if (legacyNoBatch && !this.__dataReady) { - // The first time this element is being flushed, propagate pending - // data down the tree first; this approximates 1.x's distributeConfig - this._propagatePropertyChanges(this.__dataPending, {}, false); - // Flushing clients recurses, running the initial call to - // `_flushProperties` down the tree. It also causes this element to - // become __dataReady, enabling effects to run. - this._flushClients(); - // Capture element data and flush properties one-by one to defeat - // Polymer 2.x's batched _propertiesChanged scheme; this approximates - // 1.x's applyConfig. Data is reset to mimic 1.x behavior of - // properties incrementally being initialized - const changedProps = this.__data; - const notify = this.__dataToNotify; - this.__data = {}; - this.__dataPending = this.__dataToNotify = null; - for (let prop in changedProps) { - const value = changedProps[prop]; - this.__data[prop] = value; - this.__dataPending = {[prop]: value}; - this.__dataOld = {}; - this.__dataToNotify = {[prop]: notify && notify[prop]}; - super._flushProperties(); - } - } else { - super._flushProperties(); - } - this.__dataCounter--; - } - /** * Flushes any clients previously enqueued via `_enqueueClient`, causing * their `_flushProperties` method to run. @@ -1928,35 +1889,21 @@ export const PropertyEffects = dedupingMixin(superClass => { this.__dataHasPaths = false; let notifyProps; // Compute properties - if (legacyNoBatch) { - // When not batching, computed effects may re-enter, so capture - // notifying properties early. Use simple runEffects rather than - // runComputedEffects since computed effects will run immediately - // rather than being batched. - notifyProps = this.__dataToNotify; - this.__dataToNotify = null; - runEffects(this, this[TYPES.COMPUTE], changedProps, oldProps, hasPaths); - } else { - runComputedEffects(this, changedProps, oldProps, hasPaths); - // Clear notify properties prior to possible reentry (propagate, observe), - // but after computing effects have a chance to add to them - notifyProps = this.__dataToNotify; - this.__dataToNotify = null; - } + runComputedEffects(this, changedProps, oldProps, hasPaths); + // Clear notify properties prior to possible reentry (propagate, observe), + // but after computing effects have a chance to add to them + notifyProps = this.__dataToNotify; + this.__dataToNotify = null; // Propagate properties to clients this._propagatePropertyChanges(changedProps, oldProps, hasPaths); // Flush clients this._flushClients(); // Reflect properties runEffects(this, this[TYPES.REFLECT], changedProps, oldProps, hasPaths); - // Notify properties to host (1.x order) - if (notifyProps && legacyNotifyOrder) { - runNotifyEffects(this, notifyProps, changedProps, oldProps, hasPaths); - } // Observe properties runEffects(this, this[TYPES.OBSERVE], changedProps, oldProps, hasPaths); - // Notify properties to host (2.x order) - if (notifyProps && !legacyNotifyOrder) { + // Notify properties to host + if (notifyProps) { runNotifyEffects(this, notifyProps, changedProps, oldProps, hasPaths); } // Clear temporary cache at end of turn @@ -2497,7 +2444,7 @@ export const PropertyEffects = dedupingMixin(superClass => { } // When the `legacyUndefined` flag is enabled, pass a no-op value // so that the observer, computed property, or compound binding is aborted. - if (legacyUndefined && value === undefined && args.length > 1) { + if (legacyUndefined && !this._overrideLegacyUndefined && value === undefined && args.length > 1) { return NOOP; } values[i] = value; @@ -2710,7 +2657,7 @@ export const PropertyEffects = dedupingMixin(superClass => { * create and link an instance of the template metadata associated with a * particular stamping. * - * @override + * @override * @param {!HTMLTemplateElement} template Template containing binding * bindings * @param {boolean=} instanceBinding When false (default), performs @@ -2720,7 +2667,7 @@ export const PropertyEffects = dedupingMixin(superClass => { * list of bound templates. * @return {!TemplateInfo} Template metadata object; for `runtimeBinding`, * this is an instance of the prototypical template info - * @protected + * @protected * @suppress {missingProperties} go/missingfnprops */ _bindTemplate(template, instanceBinding) { diff --git a/lib/utils/async.js b/lib/utils/async.js index e332f95325..37df334a56 100644 --- a/lib/utils/async.js +++ b/lib/utils/async.js @@ -27,10 +27,12 @@ let microtaskCurrHandle = 0; let microtaskLastHandle = 0; let microtaskCallbacks = []; let microtaskNodeContent = 0; +let microtaskScheduled = false; let microtaskNode = document.createTextNode(''); new window.MutationObserver(microtaskFlush).observe(microtaskNode, {characterData: true}); function microtaskFlush() { + microtaskScheduled = false; const len = microtaskCallbacks.length; for (let i = 0; i < len; i++) { let cb = microtaskCallbacks[i]; @@ -181,7 +183,10 @@ const microTask = { * @return {number} Handle used for canceling task */ run(callback) { - microtaskNode.textContent = microtaskNodeContent++; + if (!microtaskScheduled) { + microtaskScheduled = true; + microtaskNode.textContent = microtaskNodeContent++; + } microtaskCallbacks.push(callback); return microtaskCurrHandle++; }, diff --git a/lib/utils/settings.js b/lib/utils/settings.js index 9e9fa3aec1..151be8f225 100644 --- a/lib/utils/settings.js +++ b/lib/utils/settings.js @@ -202,44 +202,6 @@ export const setLegacyUndefined = function(useLegacyUndefined) { legacyUndefined = useLegacyUndefined; }; -/** - * Setting to retain the legacy Polymer 1 behavior for setting properties. Does - * not batch property sets. - */ -export let legacyNoBatch = - window.Polymer && window.Polymer.legacyNoBatch || false; - -/** - * Sets `legacyNoBatch` globally for all elements to enable legacy - * behavior for setting properties. - * - * @param {boolean} useLegacyNoBatch enable or disable legacy - * property setting behavior. - * @return {void} - */ -export const setLegacyNoBatch = function(useLegacyNoBatch) { - legacyNoBatch = useLegacyNoBatch; -}; - -/** - * Setting to revert to the legacy Polymer 1 order for when notifying properties - * fire change events with respect to other effects. In Polymer 1.x they fire - * before observers; in 2.x they fire after all other effect types. - */ -export let legacyNotifyOrder = - window.Polymer && window.Polymer.legacyNotifyOrder || false; - -/** - * Sets `legacyNotifyOrder` globally for all elements to enable legacy - * notify order. - * - * @param {boolean} useLegacyNotifyOrder enable or disable legacy notify order - * @return {void} - */ -export const setLegacyNotifyOrder = function(useLegacyNotifyOrder) { - legacyNotifyOrder = useLegacyNotifyOrder; -}; - /** * Setting to ensure computed properties are computed in order to ensure * re-computation never occurs in a given turn. diff --git a/test/runner.html b/test/runner.html index f6975ce734..e87d2e898e 100644 --- a/test/runner.html +++ b/test/runner.html @@ -32,7 +32,6 @@ 'unit/strict-template-policy.html', 'unit/property-effects.html', 'unit/property-effects.html?legacyUndefined=true', - 'unit/property-effects.html?legacyUndefined=true&legacyNoBatch=true', 'unit/property-effects.html?orderedComputed=true', 'unit/property-effects-template.html', 'unit/path-effects.html', diff --git a/test/unit/property-effects.html b/test/unit/property-effects.html index f27be46485..c19271f680 100644 --- a/test/unit/property-effects.html +++ b/test/unit/property-effects.html @@ -26,13 +26,11 @@