perf(animations): improve algorithm to balance animation namespaces #45057

JoostK · 2022-02-11T20:47:34Z

The prior approach would consider all existing namespaces from back to front
to find the one that's the closest ancestor for a given host element. An
expensive contains operation was used which needed to traverse all the
way up the document root for each existing namespace. This commit implements
an optimization where the closest namespace is found by traversing up from
the host element, avoiding repeated DOM traversal.

Closes #45055

JoostK

As noted in #45055 (comment), the impact of this change is HUGE for applications that have large component trees with many components with animations.

JoostK · 2022-02-13T16:46:53Z

packages/animations/browser/src/render/shared.ts

@@ -151,12 +160,13 @@ if (_isNode || typeof Element !== 'undefined') {
  if (!isBrowser()) {
    _contains = (elm1, elm2) => elm1.contains(elm2);
  } else {
+    _documentElement = /* @__PURE__ */ (() => document.documentElement)();


I don't like this and welcome suggestions on how to do this differently.

Just curious why do we need an iffe here (is it due to the top-level assignment not being optimized by Terser)? If so, may be we can add a quick comment here for future context?

There's a ban-toplevel-statement tslint rule (or a similar rule name, I don't recall by heart) that would fire if this is not wrapped inside an IIFE.

Ok, got it. We do have some places where we disable that check via // tslint:disable-next-line: no-toplevel-property-access. However in this particular case I'm not sure if Terser would be able to tree-shake-away that code without that iffe.

Suggested change

_documentElement = /* @__PURE__ */ (() => document.documentElement)();

// tslint:disable-next-line: no-toplevel-property-access

_documentElement = /* @__PURE__ */ document.documentElement;

Yeah exactly. I also checked whether we have existing suppressions but the ones I can across were for const enums. In this case a bundler would likely assume a potential side-effect for the property read and be able to optimize less aggressively.

Thanks for additional info. I think adding a quick comment (to explain why) here should be enough 👍

JoostK · 2022-02-13T16:50:16Z

packages/animations/browser/src/render/animation_driver.ts

+   * be required to implement this method. This method is to become required in a major version of
+   * Angular.
+   */
+  abstract getParentElement?(element: unknown): unknown;


I wonder what the intended use of the AnimationDriver public API really is (TBH I wonder why it's public API at all). If it's to allow custom implementations then this would have to be optional like I've done now, but if it's just for callers into this interface then we could just make this required from the start. I'm interested to hear other's thoughts on this.

I also went with unknown even though it's a bit out of place compared to the other methods, but it seems silly to continue using any when there's a safer alternative available.

After some digging in a prior PR, we discovered that AnimationDriver is public API to allow people to write their own custom AnimationDriver implementations. I'm sure that's been done all of zero times, but the option is there. We could consider deprecating that.

I'm in favor of unknown too.

@JoostK Just wanted to check if you're planning an immediate follow-up that targets v14 to make this required. I think it's probably good to have this in patch as a perf fix.

I'd like to do that, yes. The impact of the breakage is likely negligible to warrant waiting for later majors, so I'll open a follow up to clean this up immediately.

jessicajaniuk

LGTM 🍪

reviewed-for: public-api, fw-testing, fw-core, fw-animations

jessicajaniuk · 2022-02-14T18:19:42Z

packages/animations/browser/src/render/animation_driver.ts

+   * be required to implement this method. This method is to become required in a major version of
+   * Angular.
+   */
+  abstract getParentElement?(element: unknown): unknown;


After some digging in a prior PR, we discovered that AnimationDriver is public API to allow people to write their own custom AnimationDriver implementations. I'm sure that's been done all of zero times, but the option is there. We could consider deprecating that.

I'm in favor of unknown too.

jessicajaniuk · 2022-02-14T18:27:23Z

packages/animations/browser/src/render/transition_animation_engine.ts

-          this._namespaceList.splice(i + 1, 0, ns);
-          found = true;
-          break;
+      if (this.driver.getParentElement !== undefined) {


I think this is a great optimization. I wish it didn't make this method so long.

atscott

reviewed-for: public-api

atscott · 2022-02-14T19:38:57Z

packages/animations/browser/src/render/animation_driver.ts

+   * be required to implement this method. This method is to become required in a major version of
+   * Angular.
+   */
+  abstract getParentElement?(element: unknown): unknown;


@JoostK Just wanted to check if you're planning an immediate follow-up that targets v14 to make this required. I think it's probably good to have this in patch as a perf fix.

AndrewKushnir

@JoostK awesome investigation and the fix 👍

Reviewed-for: public-api

AndrewKushnir · 2022-02-14T21:30:39Z

packages/animations/browser/src/render/shared.ts

@@ -151,12 +160,13 @@ if (_isNode || typeof Element !== 'undefined') {
  if (!isBrowser()) {
    _contains = (elm1, elm2) => elm1.contains(elm2);
  } else {
+    _documentElement = /* @__PURE__ */ (() => document.documentElement)();


Just curious why do we need an iffe here (is it due to the top-level assignment not being optimized by Terser)? If so, may be we can add a quick comment here for future context?

dylhunn

reviewed-for: public-api

JoostK · 2022-02-15T21:35:30Z

Another rebase tipped this beyond a bundle size limit, so this needs another round of approvals. For context, I'll open a PR targeting master that makes getParentElement required, allowing to drop some code paths again.

atscott

reviewed-for: size-tracking

AndrewKushnir

Reviewed-for: size-tracking

AndrewKushnir · 2022-02-15T22:14:12Z

packages/animations/browser/src/render/transition_animation_engine.ts

@@ -592,16 +592,40 @@ export class TransitionAnimationEngine {
    const limit = this._namespaceList.length - 1;


Quick comment on the ~700 bytes increase in goldens/size-tracking/aio-payloads.json: this improvement is definitely worth it 👍

One small improvement/refactoring that we can make in this function (to offset some of this increase) is to extract commonly accessed fields into a local var (since property names are not mangled by our build pipeline):

const nsList = this._namespaceList; const nsByHostElement = this.namespacesByHostElement; const getParentElement = this.driver.getParentElement;

It'd be minified to something like this:

var a = t._namespaceList, b = t.namespacesByHostElement, c = t.driver.getParentElement;

So the long names will not be present in the function body.

I've extracted namespaceList and namespacesByHostElement into local variables, that is indeed a clever idea. I've kept the getParentElement reference as is, as changing that into a local variable requires calling it using .call(this.driver, ...) which isn't ideal.

The prior approach would consider all existing namespaces from back to front to find the one that's the closest ancestor for a given host element. An expensive `contains` operation was used which needed to traverse all the way up the document root _for each existing namespace_. This commit implements an optimization where the closest namespace is found by traversing up from the host element, avoiding repeated DOM traversal. Closes angular#45055

alxhub · 2022-02-16T21:48:31Z

FYI, this doesn't merge cleanly to 13.2.x.

alxhub · 2022-02-16T21:48:54Z

This PR was merged into the repository by commit 5c7c56b.

This change is a follow up to angular#45057 to make `AnimationDriver.getParentElement` a required method, which allows removing the slow path for the animation namespace insertion logic. BREAKING CHANGE: The `AnimationDriver.getParentElement` method has become required, so any implementors of this interface are now required to provide an implementation for this method. This breakage is unlikely to affect application developers, as `AnimationDriver` is not expected to be implemented in user code.

#45114) This change is a follow up to #45057 to make `AnimationDriver.getParentElement` a required method, which allows removing the slow path for the animation namespace insertion logic. BREAKING CHANGE: The `AnimationDriver.getParentElement` method has become required, so any implementors of this interface are now required to provide an implementation for this method. This breakage is unlikely to affect application developers, as `AnimationDriver` is not expected to be implemented in user code. PR Close #45114

angular-automatic-lock-bot · 2022-03-19T16:07:56Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

…ngular#45057) The prior approach would consider all existing namespaces from back to front to find the one that's the closest ancestor for a given host element. An expensive `contains` operation was used which needed to traverse all the way up the document root _for each existing namespace_. This commit implements an optimization where the closest namespace is found by traversing up from the host element, avoiding repeated DOM traversal. Closes angular#45055 PR Close angular#45057

JoostK added state: WIP area: animations labels Feb 11, 2022

ngbot bot modified the milestone: Backlog Feb 11, 2022

JoostK mentioned this pull request Feb 11, 2022

Performance issue #45055

Closed

JoostK force-pushed the animations-balance-perf branch from d4fd692 to f1c9c40 Compare February 13, 2022 16:42

JoostK commented Feb 13, 2022

View reviewed changes

JoostK requested a review from jessicajaniuk February 13, 2022 16:54

JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Feb 13, 2022

JoostK marked this pull request as ready for review February 13, 2022 16:54

JoostK removed the state: WIP label Feb 13, 2022

jessicajaniuk approved these changes Feb 14, 2022

View reviewed changes

pullapprove bot requested review from alxhub, AndrewKushnir, atscott and jessicajaniuk February 14, 2022 18:28

jessicajaniuk removed their request for review February 14, 2022 18:29

pullapprove bot requested a review from dylhunn February 14, 2022 18:29

atscott approved these changes Feb 14, 2022

View reviewed changes

pullapprove bot requested a review from jelbourn February 14, 2022 19:42

AndrewKushnir approved these changes Feb 14, 2022

View reviewed changes

dylhunn approved these changes Feb 15, 2022

View reviewed changes

JoostK force-pushed the animations-balance-perf branch from f1c9c40 to 49ecee7 Compare February 15, 2022 19:41

JoostK added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 15, 2022

JoostK force-pushed the animations-balance-perf branch from 49ecee7 to c09ca47 Compare February 15, 2022 20:30

pullapprove bot requested review from AndrewKushnir and atscott February 15, 2022 20:30

JoostK force-pushed the animations-balance-perf branch from c09ca47 to d5ee1ba Compare February 15, 2022 20:49

atscott approved these changes Feb 15, 2022

View reviewed changes

pullapprove bot requested a review from dylhunn February 15, 2022 21:38

AndrewKushnir approved these changes Feb 15, 2022

View reviewed changes

JoostK force-pushed the animations-balance-perf branch from d5ee1ba to ff85a96 Compare February 16, 2022 18:34

JoostK added the action: merge The PR is ready for merge by the caretaker label Feb 16, 2022

jessicajaniuk removed request for alxhub, jelbourn and dylhunn February 16, 2022 18:59

jessicajaniuk removed the action: presubmit The PR is in need of a google3 presubmit label Feb 16, 2022

alxhub added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Feb 16, 2022

alxhub closed this in 5c7c56b Feb 16, 2022

JoostK mentioned this pull request Feb 16, 2022

perf(animations): improve algorithm to balance animation namespaces [v13 backport] #45113

Closed

JoostK mentioned this pull request Feb 16, 2022

refactor(animations): make AnimationDriver.getParentElement required #45114

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(animations): improve algorithm to balance animation namespaces #45057

perf(animations): improve algorithm to balance animation namespaces #45057

JoostK commented Feb 11, 2022

JoostK left a comment

JoostK Feb 13, 2022

AndrewKushnir Feb 14, 2022

JoostK Feb 14, 2022

AndrewKushnir Feb 14, 2022

JoostK Feb 14, 2022

AndrewKushnir Feb 14, 2022

JoostK Feb 13, 2022

jessicajaniuk Feb 14, 2022

atscott Feb 14, 2022

JoostK Feb 14, 2022

jessicajaniuk left a comment •

edited

Loading

jessicajaniuk Feb 14, 2022

jessicajaniuk Feb 14, 2022

atscott left a comment

atscott Feb 14, 2022

AndrewKushnir left a comment

AndrewKushnir Feb 14, 2022

dylhunn left a comment

JoostK commented Feb 15, 2022

atscott left a comment

AndrewKushnir left a comment

AndrewKushnir Feb 15, 2022 •

edited

Loading

JoostK Feb 16, 2022

alxhub commented Feb 16, 2022

alxhub commented Feb 16, 2022

angular-automatic-lock-bot bot commented Mar 19, 2022

	_documentElement = /* @__PURE__ */ (() => document.documentElement)();
	// tslint:disable-next-line: no-toplevel-property-access
	_documentElement = /* @__PURE__ */ document.documentElement;

		@@ -592,16 +592,40 @@ export class TransitionAnimationEngine {
		const limit = this._namespaceList.length - 1;

perf(animations): improve algorithm to balance animation namespaces #45057

perf(animations): improve algorithm to balance animation namespaces #45057

Conversation

JoostK commented Feb 11, 2022

JoostK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicajaniuk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylhunn left a comment

Choose a reason for hiding this comment

JoostK commented Feb 15, 2022

atscott left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxhub commented Feb 16, 2022

alxhub commented Feb 16, 2022

angular-automatic-lock-bot bot commented Mar 19, 2022

jessicajaniuk left a comment •

edited

Loading

AndrewKushnir Feb 15, 2022 •

edited

Loading