2.x - Performance optimizations geared towards legacy `Polymer({...})` use by sorvell · Pull Request #5418 · Polymer/polymer

sorvell · 2018-10-31T20:03:30Z

Behaviors:
- Now mixed in lazily rather than at class time. This change means behaviors are no longer in the element's prototype chain.
- A memoized list of behavior lifecycle methods is constructed at finalization time and then used at runtime.
Adds Polymer.legacyOptimizations flag which avoids copying element templates (needed for subclassing) and turns on stripWhitespace for all templates.
TemplateStamp: A TreeWalker is now used to crawl through template nodes. This should be slightly faster when the ShadyDOM polyfill is used.
Polymer.telemetry: now properly records registered elements, even if they have not been created

Set using `Polymer.setLegacyOptimizations(true)`. This flag enables optimizaitons including, always stripping whitespace from templates and avoiding cloning element templates (assumes no inheritance is used) Also re-enables dir-mixin.

* fix issue where registered was called on element instance instead of prototype * fix issue where applyListeners was called in constructor rather than initializeProperties; this made `disable-upgrade` mixin not properly work with litsners.

* properly de-dup behaviors from superclasses * apply only "own" behaviors to class

TimvdLippe · 2018-10-31T20:36:32Z

lib/mixins/element-mixin.html

-          templateStyle.parentNode.insertBefore(s, templateStyle);
-        } else {
-          templateStyleIndex++;
+      if (!window.skipStyleIncludesAndUrls) {


Rather than putting this on window, shouldn't we put this in the Polymer.Settings object?

Oops, thanks. Going to get this window.ShadyCSS.cssBuild being set (which will shortly be added to ShadyCSS.

TimvdLippe · 2018-10-31T20:37:56Z

test/unit/disable-upgrade.html

      });

      test('elements upgrade when `disable-upgrade` removed', function() {
-        assert.notOk(el.$.disabledEl.hasCreated);


Can we not remove these assertions or are they regressions? Are they a breaking change for our users?

These were actually duplicate assertions from the previous test (https://github.com/Polymer/polymer/pull/5418/files#diff-ed6f0a354a50488549e9b8ea17788d86R240) that were incorrectly being tested again.

TimvdLippe · 2018-10-31T20:38:53Z

test/unit/mixin-behaviors.html


  test('nested-behavior dedups', function() {
-    assert.equal(el.behaviors.length, 4);
+    assert.equal(el.behaviors.length, 2);


This also seems like a regression to me. Are we sure users will not run into any breaking changes?

It's not a regression since the test changed, but the behavior is weird and I think we should change it.

We didn't have a test where Polymer.mixinBehaviors was called on a legacy element with behaviors (since this is pretty exotic) and this test was just updated to do that.

When that happens, the behaviors list is set to the subclasses' list of behaviors. However, the "active" set of behaviors on the element is the de-duplicated flat list of all behaviors on anything in the prototype chain. Given that, it seems like .behaviors should be this de-duplicated flattened list of "active" behaviors. We can make that change and update the test.

* Uses `window.ShadyCSS.cssBuild` to avoid doing unnecessary work in `processElementStyles` * Stores the list of active in-use behaviors in a legacy element's `behaviors` property. This only matters in the exotic case when elements with behaviors are extended with more behaviors and previously the list included only the subclasses behaviors (although the element otherwise worked as expected).

Merging manually from #5418.

Also use exclusively in `properties-mixin`

IE/older Safari require all arguments.

kevinpschaaf · 2018-11-06T02:55:32Z

lib/legacy/class.html

-            GenerateClassFromInfo(b, klass);
+    function copyBehaviorProperties(behaviors, klass) {
+      const meta = {};
+      const superMeta = klass.prototype.__behaviorMetaProps;


Since these are all lifecycle-related, maybe __behaviorCallbacks would be better than something with "props" in it, since that's sort of overloaded with all the data stuff.

kevinpschaaf · 2018-11-06T02:56:23Z

lib/legacy/class.html

         */
        created() {
-          super.created();
+          const list = this.__behaviorMetaProps.created;


Consider adding info. from the base info to the list?

kevinpschaaf · 2018-11-06T03:00:13Z

lib/legacy/class.html

+              mergeProperties(properties, b.properties);
+            }
+          }
+          mergeProperties(properties, info.properties);


It seemed like a possible optimization to skip the mergeProperties step from the base info object into an empty object; it seems superfulous for the initial class -- but then I realized this was required for the computed to readOnly coersion and the shorthand type expansion. So that was not intuitive, maybe due to the naming? Suggest different name or at least a comment.

Added comment.

kevinpschaaf · 2018-11-06T03:23:29Z

lib/legacy/class.html

+      klass.prototype.__behaviorMetaProps = meta;
+    }
+
+    function memoizeBehaviorMetaProps(meta, behavior, superMeta) {


There's a bug here where the full flattened list of lifecycle callbacks is called once per call to GenerateClassFromInfo, which happens each time mixinBehaviors is called.

Instead, store the list of callbacks in the closure for one set of behaviors associated with each call to GenerateClassFromInfo.

Fixed and test added.

kevinpschaaf · 2018-11-06T03:25:39Z

lib/legacy/class.html

           `is` in `beforeRegister` as you could in 1.x.
          */
+          const proto = this;
+          if (proto.hasOwnProperty('__behaviors')) {


This is just what's in the closure, i.e.
if (behaviors) {

kevinpschaaf · 2018-11-06T03:45:54Z

lib/legacy/class.html

+      behaviors: true
+    }, metaProps);
+
+    const memoizedProps = Object.assign({


Naming of these is confusing. Consider "memoized" --> "filtered"

kevinpschaaf · 2018-11-06T17:31:23Z

test/unit/mixin-behaviors.html

    HTMLImports.whenReady(function() {
      customElements.define('nested-behaviors',
-        class extends Polymer.mixinBehaviors(
+        class extends Polymer.mixinBehaviors([window.BehaviorD, window.LifeCycleBehavior1], Polymer.mixinBehaviors(


Add assertions for BehaviorD?

kevinpschaaf · 2018-11-06T17:49:21Z

Also maybe add a test like

const b1 = { ready() {..} };
const b2 = { ready() {..} };
const b3 = { ready() {..} };
const b4 = { ready() {..} };
const sup = Polymer({is: 'sup', behaviors: [b1, b2], ready() {..} })
class sub extends mixinBehaviors([b3,b4], sup) { ready() {..} }

And verify that the lifecycle order goes:

sub, b4, b3, sup, b2, b1

This ensures it's not called on extendors who do not specifically implement `_registered`. This is important since it's expected to do prototype specific work.

kevinpschaaf

LGTM!

This better matches what Polymer 1.x did for: * hostAttributes * listeners * properties * observers

…present. This ensures ShadyCSS scoping classes are not removed when a class binding's initial literal value is set.

* ensure element has `is` on prototype early as this is sometimes checked in user code. * ensure properties copied onto elements from info/behaviors are forced to configurable so they can be re-configured by later behaviors. * add `_noAccessors` optimization for faster property copying

Previously, it could be called on a superclass' prototype and not the element's prototype.

* When extended, a behavior `registered` is always called on sub-most prototype rather than the prototype on which `registered` was defined. * behavior property default values are overwritten by later behaviors and elements * readOnly properties ignored when a previous behavior observes the property * observedAttributes when extended

* Don't set up observer in ShadyDOM * Move __activateDir into check instead of replace * skip some tests that never really worked in ShadyDOM

kevinpschaaf and others added 12 commits October 19, 2018 18:35

Avoid using mixins for behaviors.

527f519

Use TreeWalker for template-stamp.

c61df6f

Fix telemetry registrations to go eagerly

f171803

Fix prototype registration

2f78573

lazyCopyProps

a784781

noCloneTemplate

2a25818

noDirMixin

c421f08

stripWhitespace

09a6d1a

memoize behavior method lists for fasting runtime calling.

7251a3a

Adds legacyOptimizations flag

4f22303

Set using `Polymer.setLegacyOptimizations(true)`. This flag enables optimizaitons including, always stripping whitespace from templates and avoiding cloning element templates (assumes no inheritance is used) Also re-enables dir-mixin.

LegacyElement

20c8bf1

* fix issue where registered was called on element instance instead of prototype * fix issue where applyListeners was called in constructor rather than initializeProperties; this made `disable-upgrade` mixin not properly work with litsners.

Behaviors

efb8d71

* properly de-dup behaviors from superclasses * apply only "own" behaviors to class

sorvell requested a review from kevinpschaaf October 31, 2018 20:03

sorvell requested review from TimvdLippe and dfreedm as code owners October 31, 2018 20:03

googlebot added the cla: yes label Oct 31, 2018

TimvdLippe reviewed Oct 31, 2018

View reviewed changes

Steven Orvell added 4 commits October 31, 2018 18:03

Address linting issues.

dadcfc2

update types

fe1bcd1

update more types

8d9219f

sorvell pushed a commit that referenced this pull request Nov 1, 2018

Updates ported from perf-opt branch

a08c984

Merging manually from #5418.

Steven Orvell added 5 commits November 1, 2018 12:45

Factor telemetry to separate module

ca03e08

Also use exclusively in `properties-mixin`

Update FF test config.

6e90283

update package-lock

f93daba

update types

5c3bf54

TemplateStamp: fix TreeWalker

c274670

IE/older Safari require all arguments.

sorvell changed the title ~~Performance optimizations geared towards legacy Polymer({...}) use~~ 2.x - Performance optimizations geared towards legacy Polymer({...}) use Nov 1, 2018

Merge branch '2.x' into perf-opt

5bd4afd

kevinpschaaf reviewed Nov 6, 2018

View reviewed changes

kevinpschaaf requested changes Nov 6, 2018

View reviewed changes

kevinpschaaf reviewed Nov 6, 2018

View reviewed changes

Steven Orvell added 5 commits November 6, 2018 12:27

Changed based on review feedback.

f62755d

Slightly improve how _registered is called.

5d5c95c

This ensures it's not called on extendors who do not specifically implement `_registered`. This is important since it's expected to do prototype specific work.

Slight tweaks based on review.

9e14172

Minor renaming based on review.

b12a0b6

Remove unnecessary setting of _template.

9d33ecb

sorvell pushed a commit that referenced this pull request Nov 6, 2018

Manually merge changes from #5418

d5e0043

Sync memoized properties getter with 3.x version.

b3568bb

kevinpschaaf approved these changes Nov 7, 2018

View reviewed changes

Steven Orvell and others added 14 commits November 9, 2018 11:39

Avoid copying certain properties from behaviors

5b34ce9

This better matches what Polymer 1.x did for: * hostAttributes * listeners * properties * observers

Ensure initial static classes are preserved when a class$ binding is …

83c0e9d

…present. This ensures ShadyCSS scoping classes are not removed when a class binding's initial literal value is set.

Do lazy behavior copying only when legacyOptimizations is set

44e4828

Ensure registered is always called on element prototype

29d514f

Previously, it could be called on a superclass' prototype and not the element's prototype.

Ensure property values are always overridden by extendors/behaviors

6f3057e

Ensure properties and observers are interleaved per behavior

bbf1acc

Use closure-safe name

d74aa82

Merge css-build-dir changes into 2.x

cc1402d

* Don't set up observer in ShadyDOM * Move __activateDir into check instead of replace * skip some tests that never really worked in ShadyDOM

Update types

e809515

Fix settings to pull from Polymer object for defaults.

77f3436

Lint fixes

41c75c2

Update to webcomponentsjs 1.3.0

5df9602

kevinpschaaf merged commit 1b5849e into 2.x Nov 27, 2018

kevinpschaaf deleted the perf-opt branch November 27, 2018 01:34

Conversation

sorvell commented Oct 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf commented Nov 6, 2018

Uh oh!

kevinpschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants