refactor(core): rework animation instructions to fix multiple issues #63450
Conversation
9a58c72 to
786cab0
Compare
|
Anything new here ? :) |
8c21526 to
9d19ff0
Compare
| duration: number; | ||
| } | ||
|
|
||
| export interface AnimationLViewData { |
There was a problem hiding this comment.
I'd consider making this a monomorphic object, i.e. make all props required (and create objects using a single factory function).
There was a problem hiding this comment.
Elements may not have both enter and leave animations. This was to save on allocation of memory for the arrays in that case. Do you think the monomorphic object benefits outweigh the memory usage?
There was a problem hiding this comment.
I don't have a good judgement about the memory effect this may have, so the only way to tell is through profiling (but we may not have a reasonable test corpus?). Primarily runEnterAnimations runs in a hot code path, all others are only accessed during the creation phase or view destruction so would benefit less from monomorphic accesses.
For the full picture, there would be up to four hidden classes currently, for the following transitions:
- enter > running
- leave > running
- enter > leave > running
- leave > enter > running
And V8 uses—at least back in the day, if I'm not mistaken—4 different hidden classes as threshold for polymorphic vs megamorphic so there's still ICs for the property accesses, they aren't deoptimized entirely like they would if it were megamorphic.
That is to say I don't really know if it's worth the trade-off, and since we're not megamorphic it may be okay?
There was a problem hiding this comment.
It's also worth noting this is just a first go at this optimization. We'll likely move these things out of the LView slot in a follow up refactor to make it even more optimized.
There was a problem hiding this comment.
@JoostK It's also worth noting that enter does not have a running state that is tracked. Enter is triggered and will get cleared out immediately. Running only has any usefulness when leave animations are present. The running field would only ever be populated once leave animations are actually triggered. So really there's possibly less states.
84d2dc6 to
08af031
Compare
This tracks the enter and leave functions in the LView to be executed at a safe time for change detection.
This updates the enter and leave logic to use the stored LView data to dispatch the enter and leave animations at the right points in the lifecycle. This should fix issues with signals not being available yet, parallel animations, and also eliminate the need for the element registry. fixes: angular#63391 fixes: angular#63388 fixes: angular#63369
08af031 to
bd3e661
Compare
This fixes several issues and removes the need to have a feature to enable the animation removal element registry. Instead we track the animations on the LView itself, and we can deal with dispatching them at the right time before cleanups and destroy is called.
fixes: #63391
fixes: #63388
fixes: #63369