Reduce allocations on AnimationManager#35612
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35612Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35612" |
|
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. |
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
MauiBot
added
s/agent-fix-win
pictos
force-pushed
the
pj/reduce-animation-allocations
branch
from
300a2cb to
7418c16
Compare
|
/review -b feature/refactor-copilot-yml |
| @@ -130,7 +136,13 @@ public void Dispose() | |||
| void ForceFinishAnimations() | |||
| { | |||
| var animations = new List<Animation>(_animations); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
🤖 Automated review — alternative fix proposed
The expert-reviewer evaluation compared the PR fix against #1 automatically generated candidates and selected try-fix-1 as the strongest fix.
Why: try-fix-1 won because it passed the runnable Core regression suite, preserves the PR's callback-mutation-safe snapshot semantics, and removes recurring snapshot-list object allocations with less complexity than the ArrayPool candidate. The PR-based candidates were ranked lower because their gate validation was skipped and they retain the hot-path snapshot allocation.
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 2de13b3..98469a8 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 List<Animation> _animationsSnapshot = new();
long _lastUpdate;
bool _disposedValue;
@@ -84,8 +85,20 @@ namespace Microsoft.Maui.Animations
var milliseconds = TimeSpan.FromMilliseconds(now - _lastUpdate).TotalMilliseconds;
_lastUpdate = now;
- var animations = new List<Animation>(_animations);
- animations.ForEach(OnAnimationTick);
+ _animationsSnapshot.Clear();
+ _animationsSnapshot.AddRange(_animations);
+
+ try
+ {
+ foreach (var animation in _animationsSnapshot)
+ {
+ OnAnimationTick(animation);
+ }
+ }
+ finally
+ {
+ _animationsSnapshot.Clear();
+ }
if (_animations.Count == 0)
End();
@@ -129,8 +142,21 @@ namespace Microsoft.Maui.Animations
void ForceFinishAnimations()
{
- var animations = new List<Animation>(_animations);
- animations.ForEach(ForceFinish);
+ _animationsSnapshot.Clear();
+ _animationsSnapshot.AddRange(_animations);
+
+ try
+ {
+ foreach (var animation in _animationsSnapshot)
+ {
+ ForceFinish(animation);
+ }
+ }
+ finally
+ {
+ _animationsSnapshot.Clear();
+ }
+
End();
void ForceFinish(Animation animation)
🤖 AI Summary
📊 Review Session —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35612 | Replace List<Animation>.ForEach(...) delegate calls with explicit foreach loops over the same fresh snapshot list |
src/Core/src/Animations/AnimationManager.cs |
Original PR; safe but only removes delegate invocation allocation and keeps per-tick snapshot list allocation |
🔬 Code Review — Deep Analysis
Code Review — PR #35612
Independent Assessment
What this changes: Replaces List<Animation>.ForEach(...) delegate calls with explicit foreach loops over the same snapshot list in AnimationManager.OnFire() and ForceFinishAnimations().
Inferred motivation: Reduce delegate/callback allocation in animation hot paths while preserving the snapshot semantics that protect against _animations mutation during callbacks.
Reconciliation with PR Narrative
Author claims: "[WiP] Reduce allocations on AnimationManager"; PR body says "Please do not review yet."
Agreement/disagreement: The code matches the allocation-reduction claim, but only partially: it removes ForEach delegate allocation while still allocating the snapshot List<Animation>. Existing earlier review comments about unsafe direct-list iteration are outdated for the current head because the latest diff keeps snapshot iteration.
CI: latest visible checks, including maui-pr, are successful in the code-review sub-agent's GitHub context. Local gh checks were unavailable because this environment is not authenticated.
Findings
💡 Suggestion — Remove commented-out old implementation
src/Core/src/Animations/AnimationManager.cs:94 and src/Core/src/Animations/AnimationManager.cs:145 leave the previous animations.ForEach(...) calls as comments. Since the replacement is straightforward, these should be deleted before merge to avoid dead-code clutter.
Devil's Advocate
The biggest risk would be mutation during animation callbacks, but the current code still snapshots _animations before iterating, so it preserves the prior safety behavior. The optimization is modest because the snapshot allocation remains, but that is a tradeoff rather than a correctness issue.
Verdict: LGTM
Confidence: high
Summary: The current code preserves existing behavior while removing delegate-based iteration in two animation paths. I found no correctness, lifecycle, or platform-specific blocking issues; only minor cleanup is recommended.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 | Reusable private List<Animation> snapshot buffer with try/finally clearing |
✅ PASS for runnable tests; Android build blocked by environment | 1 file | Best balance: preserves snapshot semantics, removes recurring snapshot-list object allocations, and keeps code straightforward |
| 2 | try-fix-2 | ArrayPool<Animation> rented snapshot arrays copied from _animations |
✅ PASS for runnable tests; Android build skipped after same blocker | 1 file | Viable but more complex; requires careful rent/copy/clear/return lifecycle |
| PR | PR #35612 | Explicit foreach over a freshly allocated new List<Animation>(_animations) snapshot |
1 file | Safe but retains per-tick snapshot-list allocation and leaves commented-out old code |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Proposed a reusable private snapshot list to preserve callback-mutation safety while amortizing snapshot storage. |
| maui-expert-reviewer | 2 | Yes | After try-fix-1 passed and Android validation was environment-blocked, proposed ArrayPool<Animation> as a distinct no-retained-list alternative. |
Exhausted: Yes — the meaningful alternatives for preserving snapshot semantics while reducing allocations are reusable retained snapshot storage or pooled temporary snapshot storage. Further variants would mostly refactor helper shape rather than change the approach.
Selected Fix: Candidate #1 — It is demonstrably better than the PR's current fix for allocation reduction, passed the available Core regression suite, preserves the original snapshot safety invariant, removes the commented-out dead code, and is simpler than the pooled-array candidate.
Test Summary
- Gate was not rerun. Existing gate result: skipped because no tests were detected in this PR.
- try-fix-1:
dotnet test src/Core/tests/UnitTests/Core.UnitTests.csproj --verbosity minimalpassed with 806 passed and 3 skipped. - try-fix-2:
dotnet test src/Core/tests/UnitTests/Core.UnitTests.csproj --no-restore --verbosity minimalpassed with 806 passed and 3 skipped. - Android validation:
dotnet build src/Core/src/Core.csproj -f net10.0-androidwas blocked byNETSDK1005because the restored assets did not include anet10.0-androidtarget in this environment.
📋 Report — Final Recommendation
Comparative Report - PR #35612
Candidates compared
| Candidate | Approach | Regression result | Assessment |
|---|---|---|---|
pr |
Explicit foreach over a freshly allocated new List<Animation>(_animations) snapshot; leaves commented-out old ForEach calls. |
Gate skipped; no tests detected. | Correct and behavior-preserving, but only modestly reduces allocation overhead and leaves maintainability cleanup. |
pr-plus-reviewer |
Same as pr, with the expert-reviewer feedback applied by removing the commented-out old ForEach calls. |
Gate skipped; no tests detected. | Better than pr for maintainability, but still retains the recurring snapshot-list allocation. |
try-fix-1 |
Reusable private List<Animation> snapshot buffer with try/finally clearing. |
PASS for runnable Core unit tests: 806 passed, 3 skipped. Android build was environment-blocked by missing net10.0-android assets. |
Best balance: preserves snapshot semantics, removes recurring snapshot-list object allocations after the retained buffer is created, clears references after iteration, and keeps the code straightforward. |
try-fix-2 |
ArrayPool<Animation> rented snapshot arrays copied from _animations, cleared and returned in finally. |
PASS for runnable Core unit tests: 806 passed, 3 skipped. Android build skipped after the same environment blocker. | Viable and allocation-aware, but more complex and easier to misuse than the reusable-list snapshot without a demonstrated correctness or test advantage. |
Ranking
try-fix-1- Passed runnable regression tests and provides the strongest allocation reduction while preserving callback-mutation safety.try-fix-2- Passed runnable regression tests and avoids retained list storage, but introducesArrayPool<T>lifecycle complexity for no clear benefit overtry-fix-1.pr-plus-reviewer- Improves the PR's maintainability by applying expert feedback, but regression coverage is absent and allocation reduction is still limited.pr- Correct but least polished: no detected tests, retained per-tick snapshot allocation, and commented-out dead code remains.
Winner
try-fix-1 is the winning candidate. It ranks above both PR-based candidates because candidates with passing runnable regression tests must rank higher than candidates with skipped regression validation, and it ranks above try-fix-2 because it is simpler while achieving the same snapshot-safety goal and better allocation behavior than the PR.
|
@kubaflo this one is ready for review now |
kubaflo
changed the title
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
What do you think about the ai's suggestions?
|
@kubaflo I believe the review was made on a previous commit. It says
but I didn't remove the |
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
🤖 Automated review — alternative fix proposed
The expert-reviewer evaluation compared the PR fix against #1 automatically generated candidates and selected try-fix-1 as the strongest fix.
Why: try-fix-1 wins because it preserves AnimationManager's existing mutation-safe snapshot semantics while eliminating both the PR-targeted delegate/display-class allocations and the recurring snapshot List allocation after ArrayPool warm-up. It passed the available focused Core animation tests with restore; Android device validation was unavailable.
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 d6a3035fa3..16d441a208 100644
--- a/src/Core/src/Animations/AnimationManager.cs
+++ b/src/Core/src/Animations/AnimationManager.cs
@@ -1,4 +1,5 @@
using System;
+using System.Buffers;
using System.Collections.Generic;
namespace Microsoft.Maui.Animations
@@ -84,14 +85,16 @@ namespace Microsoft.Maui.Animations
var milliseconds = TimeSpan.FromMilliseconds(now - _lastUpdate).TotalMilliseconds;
_lastUpdate = now;
- var animations = new List<Animation>(_animations);
-
- foreach(var animation in animations)
+ var animations = RentAnimationsSnapshot(out var animationsCount);
+ try
{
- OnAnimationTick(animation);
+ for (var n = 0; n < animationsCount; n++)
+ OnAnimationTick(animations[n]);
+ }
+ finally
+ {
+ ArrayPool<Animation>.Shared.Return(animations, clearArray: true);
}
-
- //animations.ForEach(OnAnimationTick);
if (_animations.Count == 0)
End();
@@ -135,14 +138,17 @@ namespace Microsoft.Maui.Animations
void ForceFinishAnimations()
{
- var animations = new List<Animation>(_animations);
-
- foreach(var animation in animations)
+ var animations = RentAnimationsSnapshot(out var animationsCount);
+ try
+ {
+ for (var n = 0; n < animationsCount; n++)
+ ForceFinish(animations[n]);
+ }
+ finally
{
- ForceFinish(animation);
+ ArrayPool<Animation>.Shared.Return(animations, clearArray: true);
}
-
- //animations.ForEach(ForceFinish);
+
End();
void ForceFinish(Animation animation)
@@ -153,6 +159,14 @@ namespace Microsoft.Maui.Animations
}
}
+ Animation[] RentAnimationsSnapshot(out int count)
+ {
+ count = _animations.Count;
+ var animations = ArrayPool<Animation>.Shared.Rent(count);
+ _animations.CopyTo(animations);
+ return animations;
+ }
+
internal virtual double AdjustSpeed(double elapsedMilliseconds)
{
return elapsedMilliseconds * SpeedModifier;
|
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, 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 |
4 participants
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.ActionandDisplayClassallocations are caused by the usage ofForEachmethod, 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
foreachloop. And you can see the memory footprint below:Where the only allocation now is related with the
Listthat 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