perf(animations): Remove generic objects in favor of Maps #44482

jessicajaniuk · 2021-12-14T23:29:47Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

We were using a number of generic objects as if they were maps and relying on delete to remove
properties.

Issue Number: N/A

What is the new behavior?

In order to improve performance, these generic objects have been switched to native maps.

Does this PR introduce a breaking change?

Yes
No

packages/animations/src/animation_metadata.ts

packages/animations/browser/src/dsl/animation_ast_builder.ts

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

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

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

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

packages/animations/browser/src/dsl/animation_timeline_builder.ts

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

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

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

packages/animations/browser/src/util.ts

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

dylhunn

reviewed-for: public-api

dario-piotrowicz · 2022-01-24T20:49:44Z

packages/animations/browser/src/dsl/animation_ast_builder.ts

@jessicajaniuk In PR #44729 I am adding a delete tuple[prop] here, after the PR has been merged please remember to update it to tuple.delete(prop) whenever you can 🙂

_{Original discussion: https://github.com/angular/angular/pull/44729/files#r787206809}

dylhunn

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

jessicajaniuk · 2022-01-31T17:11:29Z

TGP is green

AndrewKushnir

Reviewed-for: public-api

AndrewKushnir · 2022-01-31T18:08:09Z

goldens/public-api/animations/browser/testing/testing.md

nit: I like the StyleData as a symbol name a bit more. The Map part leaks the implementation detail into the name (for ex. we'd need to change the name if we later decide to use an array).

This is a very minor thing, not a blocker for this PR :)

I actually agree, but we still use the old symbol. We'd need a new name. I'll think on it for a potential refactor after this PR.

We were using a number of generic objects as if they were maps and relying on delete to remove properties. In order to improve performance, these have been switched to native maps.

jessicajaniuk · 2022-01-31T20:28:45Z

This PR was merged into the repository by commit 7a81481.

alfaproject · 2022-01-31T23:20:55Z

@jessicajaniuk OOC do you have performance numbers before/after?

angular-automatic-lock-bot · 2022-03-03T16:12:37Z

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.}

) We were using a number of generic objects as if they were maps and relying on delete to remove properties. In order to improve performance, these have been switched to native maps. PR Close angular#44482

jessicajaniuk force-pushed the animation-maps branch 2 times, most recently from e233f8f to be7e805 Compare December 14, 2021 23:52

jessicajaniuk added the area: animations label Dec 14, 2021

ngbot bot added this to the Backlog milestone Dec 14, 2021

jessicajaniuk added the target: patch This PR is targeted for the next patch release label Dec 14, 2021

jessicajaniuk commented Dec 15, 2021

View reviewed changes

packages/animations/src/animation_metadata.ts Outdated Show resolved Hide resolved

jessicajaniuk force-pushed the animation-maps branch 5 times, most recently from f2c3799 to 24cacd0 Compare December 15, 2021 01:15

AndrewKushnir reviewed Dec 15, 2021

View reviewed changes

jessicajaniuk force-pushed the animation-maps branch 9 times, most recently from 74bec63 to 84cc81b Compare December 16, 2021 23:12

dario-piotrowicz reviewed Dec 20, 2021

View reviewed changes

packages/animations/browser/src/dsl/animation_timeline_builder.ts Outdated Show resolved Hide resolved

dario-piotrowicz reviewed Dec 20, 2021

View reviewed changes

packages/animations/browser/src/render/special_cased_styles.ts Outdated Show resolved Hide resolved

dario-piotrowicz reviewed Dec 20, 2021

View reviewed changes

packages/animations/browser/src/render/special_cased_styles.ts Outdated Show resolved Hide resolved

dario-piotrowicz reviewed Dec 20, 2021

View reviewed changes

packages/animations/browser/src/render/shared.ts Outdated Show resolved Hide resolved

jessicajaniuk force-pushed the animation-maps branch 4 times, most recently from 0e15016 to 9698f71 Compare December 21, 2021 23:34

dylhunn approved these changes Jan 13, 2022

View reviewed changes

pullapprove bot requested a review from dylhunn January 13, 2022 20:39

dylhunn approved these changes Jan 13, 2022

View reviewed changes

dario-piotrowicz mentioned this pull request Jan 22, 2022

fix(animations): allow animations with unsupported CSS properties #44729

Closed

14 tasks

dario-piotrowicz reviewed Jan 24, 2022

View reviewed changes

jessicajaniuk force-pushed the animation-maps branch 3 times, most recently from c13e238 to 8f6aed6 Compare January 27, 2022 19:13

jessicajaniuk added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 27, 2022

jessicajaniuk requested review from atscott and dylhunn January 27, 2022 20:02

dylhunn approved these changes Jan 28, 2022

View reviewed changes

AndrewKushnir approved these changes Jan 31, 2022

View reviewed changes

perf(animations): Remove generic objects in favor of Maps

0234c7e

We were using a number of generic objects as if they were maps and relying on delete to remove properties. In order to improve performance, these have been switched to native maps.

jessicajaniuk force-pushed the animation-maps branch from 8f6aed6 to 0234c7e Compare January 31, 2022 18:53

jessicajaniuk changed the title ~~refactor(animations): Remove generic objects in favor of Maps~~ perf(animations): Remove generic objects in favor of Maps Jan 31, 2022

jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 31, 2022

jessicajaniuk removed request for alxhub, atscott and jelbourn January 31, 2022 18:55

jessicajaniuk added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jan 31, 2022

jessicajaniuk closed this in 7a81481 Jan 31, 2022

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

jessicajaniuk deleted the animation-maps branch January 27, 2023 16:41

perf(animations): Remove generic objects in favor of Maps #44482

perf(animations): Remove generic objects in favor of Maps #44482

Uh oh!

Conversation

jessicajaniuk commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylhunn left a comment

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

dylhunn left a comment

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk commented Jan 31, 2022

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk commented Jan 31, 2022

Uh oh!

alfaproject commented Jan 31, 2022

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jessicajaniuk commented Dec 14, 2021 •

edited

Loading