Reduce allocations on AnimationManager

[pictos]

Description of Change

In order to provide a good reference, I repeated the flow twice and collect the results. With the main, we do have this memory footprint:

Those System.Action and DisplayClass allocations are caused by the usage of ForEach method, they fancy but they come with a cost. Since they're inside a loop the compiler couldn't cache them (my theory) which causes a lot of allocations.

To remove all those allocations I rewrite the code using the boring foreach loop. And you can see the memory footprint below:

Where the only allocation now is related with the List that is created inside the method and I wasn't able to remove on this PR. I'll let it for the future me or contributors.

Issues Fixed

Fixes #35654

From IA review:

The 19 → 26 difference is just sampling noise. GCAllocationTick fires every ~100 KB of allocations, not on every individual allocation. The improved run had 2,017 sampled events vs 2,000 in the baseline (17 more), which means a slightly different sampling window was captured. That's enough to shift counts on any type by a few ticks.

What actually changed:

✅ Action (30 → 0): gone — the ForEach(delegate) no longer creates a delegate on each call
✅ <>c__DisplayClass20_0 (? → 0): the compiler-generated closure class is gone too
➡️ Animation[] (19 → 26): same root cause (new List backing array), just sampling variance

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35612

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35612"

[dotnet-policy-service]

Hey there @@pictos! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[kubaflo]

/review -b feature/refactor-copilot-yml

[MauiBot]

Expert Review — 3 findings

See inline comments for details.

[kubaflo]

/review -b feature/refactor-copilot-yml

[MartyIX]

If the _animations were an immutable object:

-readonly List<Animation> _animations = new();
+ImmutableArray<Animation> _animations = [];

the allocations would move from "iterations" to "modifications" (adding/removing elements). Not sure if possible, perhaps it's worth a try.

[pictos]

Thanks for your suggestion, I'll not do it on this PR. The main goal was to remove the lambdas allocations. Removing the List allocation would be a bonus, but looks like it's needed, all my attempts to remove resulted in IndexOutOfRangeExceptions. To improve would need a deeper look inside the animation engine.

[pictos]

@kubaflo this one is ready for review now

[kubaflo]

What do you think about the ai's suggestions?

[pictos]

@kubaflo I believe the review was made on a previous commit. It says

Explicit foreach over a freshly allocated new List(_animations) snapshot; leaves commented-out old ForEach calls.

but I didn't remove the new List<Animation>(_animations) allocation. Is it possible to rerun to have a fresh review?

[kubaflo]

/review -b feature/refactor-copilot-yml

[pictos]

I'm not sure about the ArrayPool usage here. The minimum length array returned by the pool is of 16 and it grows in power of 2, so it seems an overkill, memory-wise, because I don't believe there will be too many items inside that list. Also, Animation is a reference type which demands the code clean up the array during Rent, which is an extra job.

That change would make sense in a future work, with more data around it to validate the benefits, and I can't do it right now. I would like to merge it as it's, focusing on the delegate allocation reduction.

[kubaflo]

/review -b feature/refactor-copilot-yml

[pictos]

I don't like the try-fix-1 suggestion, I believe the trade-off isn't worth (use lock to avoid collection copy)

Superseded by a newer MauiBot review run.

[kubaflo]

AI Review Summary

@pictos — new AI review results are available based on this last commit: cf8a3f0.
create array instead of list

Review Sessions — click to expand

Gate — Test Before & After Fix

Gate Result: ⚠️ SKIPPED

No tests were detected in this PR.

Recommendation: Add tests to verify the fix using the write-tests-agent.

---

UI Tests

Full UI test matrix will run (no specific categories detected from PR changes).

---

Pre-Flight — Context & Validation

Issue: Unknown - GitHub API unavailable; no linked issue could be fetched
PR: #35612 - Unknown title (local branch: pr-review-35612)
Platforms Affected: android (per prompt); also shared Core animation behavior
Files Changed: 12 implementation/pipeline files, 0 test files

Key Findings

• GitHub PR metadata/comments could not be fetched because gh is unauthenticated in this environment; analysis used the checked-out PR branch and origin/main...HEAD diff.
• Functional MAUI change is in src/Core/src/Animations/AnimationManager.cs: replace new List<Animation>(_animations).ForEach(...) with array snapshot + foreach in normal ticks and force-finish path.
• Gate was already skipped: no tests detected in this PR. Nearby targeted coverage exists in TickerSystemEnabledTests and was used for try-fix regression checks.
• Expert review found the PR fix is small and plausible but may only mitigate the symptom; _animations remains an unsynchronized mutable list.

Code Review Summary

Verdict: NEEDS_DISCUSSION
Confidence: medium
Errors: 1 | Warnings: 1 | Suggestions: 1

Key code review findings:

• ❌ No regression test coverage for the mutation/concurrency scenario; gate was skipped.
• ⚠️ src/Core/src/Animations/AnimationManager.cs: array snapshot avoids List<T>.ForEach checks but does not make _animations access atomic under concurrent mutation.
• 💡 Minor extra blank line in ForceFinishAnimations in the PR diff.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #35612	Array snapshot + foreach in AnimationManager	⚠️ Gate skipped	src/Core/src/Animations/AnimationManager.cs	Original PR; no detected tests

---

Code Review — Deep Analysis

Code Review — PR #35612

Independent Assessment

What this changes: The functional MAUI change updates AnimationManager to snapshot _animations into an array and iterate with foreach instead of constructing a List<Animation> snapshot and invoking List<T>.ForEach in OnFire and ForceFinishAnimations.

Inferred motivation: Avoid mutation/version-check failures or allocation overhead while animations are removed or force-finished during ticker callbacks, especially around Android animation-disable paths.

Reconciliation with PR Narrative

Author claims: GitHub API access was unavailable in this environment, so PR title/body/comments and linked issue could not be fetched. Local commit history indicates this PR also contains Copilot CI pipeline hardening/refactoring and one functional animation manager change.

Agreement/disagreement: Local code review agrees the array snapshot is a tactical improvement over List<T>.ForEach, but the MAUI expert review found it may not address all concurrent access scenarios for _animations.

Findings

❌ Error — No regression test coverage

Gate was already skipped because no tests were detected in the PR. The animation-manager change has existing nearby coverage in src/Controls/tests/Core.UnitTests/TickerSystemEnabledTests.cs, but the PR does not add a targeted regression for the mutation/concurrency scenario.

⚠️ Warning — Snapshot change may mask, not fix, concurrent list access

src/Core/src/Animations/AnimationManager.cs still uses a mutable List<Animation> without synchronization. Replacing a List<T> snapshot with an array snapshot avoids List<T>.ForEach version checks and reduces overhead, but snapshot construction is still not atomic if _animations is concurrently mutated.

💡 Suggestion — Minor formatting cleanup

The PR version of ForceFinishAnimations contains an extra blank line before End().

Devil's Advocate

The platform-specific Android callbacks likely run on the main thread in normal cases, so the immediate crash may be fully mitigated by avoiding List<T>.ForEach. Existing animation-disable tests pass with multiple alternative approaches. However, the base Ticker uses System.Timers.Timer, and AnimationManager itself does not enforce single-threaded access.

Verdict: NEEDS_DISCUSSION

Confidence: medium
Summary: The PR's functional fix is plausible and small, but it lacks a regression test and may be a symptom-level mitigation rather than a root-cause fix for unsynchronized _animations access. Alternative candidates show a lock-based approach is more robust, while allocation-free approaches pass existing tests but are narrower.

---

Fix — Analysis & Comparison

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
1	try-fix	Lock-protected _animations list; snapshot under lock and invoke callbacks outside lock	✅ PASS	1 file	Most robust; addresses concurrent access window instead of only snapshot iteration
2	try-fix	Reverse in-place iteration without snapshot allocation	✅ PASS	1 file	Simpler/faster for single-threaded mutation; does not address cross-thread access
3	try-fix	Two-phase tick then backward remove	✅ PASS	1 file	Clear algorithm but changes cleanup timing and does not address cross-thread access
PR	PR #35612	Array snapshot + foreach replacing List<T>.ForEach	⚠️ Gate skipped	1 functional file	Small tactical original PR fix; no detected tests

Cross-Pollination

Model	Round	New Ideas?	Details
maui-expert-reviewer	1	Yes	Suggested lock, reverse iteration, two-phase remove, and source dispatch/thread-affinity ideas.
maui-expert-reviewer	2	No	NO NEW IDEAS. Immutable-array and ticker reentrancy variants were considered not worth implementing because they are either more complex than locking or narrower than Candidate 1.

Exhausted: Yes
Selected Fix: Candidate #1 — Lock-protected animation list. It is larger than the PR's array snapshot but is the only tested candidate that closes the non-atomic snapshot/concurrent mutation window. Candidate #2 and #3 pass the existing regression tests but are not demonstrably better for Android/threading robustness.

Test Summary

All implemented candidates were tested with the targeted existing regression suite:

dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter FullyQualifiedName~TickerSystemEnabledTests --verbosity minimal

The first candidate's initial --no-restore run failed because assets were missing; the retry with restore passed. Subsequent candidates used --no-restore and passed.

---

Report — Final Recommendation

Comparative Candidate Report

Inputs

• PR: pr — array snapshot plus foreach in AnimationManager.
• Reviewer-enhanced PR: pr-plus-reviewer — identical to pr; expert review produced no actionable inline findings.
• try-fix-1: lock-protected _animations list; snapshots are taken under a private lock and callbacks run outside the lock.
• try-fix-2: reverse in-place iteration without a snapshot.
• try-fix-3: two-phase tick then cleanup/removal.

Gate verification for the PR was skipped because no tests were detected. The try-fix candidates were evaluated with the targeted existing TickerSystemEnabledTests suite; all three try-fix candidates passed. No candidate failed regression tests, so no candidate is demoted under the failed-regression rule.

Ranking

1. try-fix-1 — Winner. This is the only tested candidate that addresses the likely Android race at the shared mutable _animations list instead of only changing iteration mechanics. It preserves snapshot callback semantics by copying under lock and invokes animation callbacks outside the lock to avoid deadlocks/reentrant callback hazards.
2. pr / pr-plus-reviewer. The submitted PR is small, plausible, and low-risk. It preserves existing behavior and improves the snapshot implementation, but it does not make _animations access atomic and had no detected regression tests in the PR. pr-plus-reviewer ties with pr because expert review had no actionable changes.
3. try-fix-2. Reverse iteration is allocation-efficient and passed the targeted suite, but it changes iteration semantics and still leaves cross-thread access unsynchronized.
4. try-fix-3. The two-phase algorithm is clear and passed the targeted suite, but it changes same-frame cleanup timing and still does not address concurrent access.

Decision

Select try-fix-1. It has the strongest correctness story for Android because it fixes the shared-list synchronization gap while preserving the PR's snapshot-style callback isolation. The PR fix remains acceptable as a narrow tactical improvement, but it is less complete for the suspected root cause.

---

Future Action — alternative fix proposed (try-fix-1)

Automated review — alternative fix proposed

The expert-reviewer evaluation compared the PR fix against automatically generated candidates and selected try-fix-1 as the strongest fix.

Why: try-fix-1 won because it passed the targeted regression suite and is the only candidate that addresses the shared _animations concurrency window while preserving snapshot callback semantics. The PR and pr-plus-reviewer are lower-risk but narrower; try-fix-2 and try-fix-3 pass tests but do not address cross-thread access.

Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.

Candidate diff (try-fix-1)

diff --git a/src/Core/src/Animations/AnimationManager.cs b/src/Core/src/Animations/AnimationManager.cs
index 2de13b3294..9bf205ef5d 100644
--- a/src/Core/src/Animations/AnimationManager.cs
+++ b/src/Core/src/Animations/AnimationManager.cs
@@ -7,6 +7,7 @@ namespace Microsoft.Maui.Animations
 	public class AnimationManager : IAnimationManager, IDisposable
 	{
 		readonly List<Animation> _animations = new();
+		readonly object _animationsLock = new();
 		long _lastUpdate;
 		bool _disposedValue;
 
@@ -40,8 +41,12 @@ namespace Microsoft.Maui.Animations
 				return;
 			}
 
-			if (!_animations.Contains(animation))
-				_animations.Add(animation);
+			lock (_animationsLock)
+			{
+				if (!_animations.Contains(animation))
+					_animations.Add(animation);
+			}
+
 			if (!Ticker.IsRunning && AutoStartTicker)
 				Start();
 		}
@@ -49,12 +54,22 @@ namespace Microsoft.Maui.Animations
 		/// <inheritdoc/>
 		public void Remove(Animation animation)
 		{
-			_animations.TryRemove(animation);
+			lock (_animationsLock)
+				_animations.TryRemove(animation);
 
-			if (_animations.Count == 0)
+			if (AnimationsCount == 0)
 				End();
 		}
 
+		int AnimationsCount
+		{
+			get
+			{
+				lock (_animationsLock)
+					return _animations.Count;
+			}
+		}
+
 		void Start()
 		{
 			_lastUpdate = GetCurrentTick();
@@ -84,17 +99,22 @@ namespace Microsoft.Maui.Animations
 			var milliseconds = TimeSpan.FromMilliseconds(now - _lastUpdate).TotalMilliseconds;
 			_lastUpdate = now;
 
-			var animations = new List<Animation>(_animations);
-			animations.ForEach(OnAnimationTick);
+			Animation[] animations;
+			lock (_animationsLock)
+				animations = _animations.ToArray();
+
+			foreach (var animation in animations)
+				OnAnimationTick(animation);
 
-			if (_animations.Count == 0)
+			if (AnimationsCount == 0)
 				End();
 
 			void OnAnimationTick(Animation animation)
 			{
 				if (animation.HasFinished)
 				{
-					_animations.TryRemove(animation);
+					lock (_animationsLock)
+						_animations.TryRemove(animation);
 					animation.RemoveFromParent();
 					return;
 				}
@@ -103,7 +123,8 @@ namespace Microsoft.Maui.Animations
 
 				if (animation.HasFinished)
 				{
-					_animations.TryRemove(animation);
+					lock (_animationsLock)
+						_animations.TryRemove(animation);
 					animation.RemoveFromParent();
 				}
 			}
@@ -129,14 +150,20 @@ namespace Microsoft.Maui.Animations
 
 		void ForceFinishAnimations()
 		{
-			var animations = new List<Animation>(_animations);
-			animations.ForEach(ForceFinish);
+			Animation[] animations;
+			lock (_animationsLock)
+				animations = _animations.ToArray();
+
+			foreach (var animation in animations)
+				ForceFinish(animation);
+
 			End();
 
 			void ForceFinish(Animation animation)
 			{
 				animation.ForceFinish();
-				_animations.TryRemove(animation);
+				lock (_animationsLock)
+					_animations.TryRemove(animation);
 				animation.RemoveFromParent();
 			}
 		}

[albyrock87]

This class is clearly not thread safe.
Therefore I don't see a reason to copy _animations into a temporary array.

We can simply use our dear old for(var i = 0; i < _animations.Count; i++ and when the internal invocation removes the element, we can simply i--.

This also gives us the opportunity of providing the current i to the action inside the loop so that instead of TryRemove we can remove directly the element at the given index.

Take this AI

[kubaflo]

/review -b feature/enhanced-reviewer