From 721ef6032da5b723a1a156a055d1f9c7a7c1ff5d Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Sat, 20 Jun 2026 16:32:15 +0200 Subject: [PATCH] Enumerate AST children with a zero-allocation struct enumerator The generated GetChildNodes materialized a List (plus a boxed List.Enumerator at every foreach) for each node, so AstNode.Children and the visitor's per-node child walk allocated three objects per traversal. Decompiling System.Private.CoreLib that came to ~1.7 GB of extra garbage, roughly +7% over the linked-list model the slot tree replaced. A yield iterator removes the List but trades it for an equally costly per-node state machine, so it is not enough on its own. Enumerate children through a by-value struct enumerator over the existing FirstChild/NextSibling primitives, capturing each child's successor before it is yielded so a transform may still remove or replace the current child mid-traversal. AstNodeCollection gets the same struct treatment for a direct foreach. Child enumeration now allocates nothing, bringing total allocations back to the linked-list baseline at byte-identical output (full Pretty suite green with CheckInvariant active). Assisted-by: Claude:claude-opus-4-8:Claude Code --- .../DecompilerSyntaxTreeGenerator.cs | 19 --- .../ICSharpCode.Decompiler.Tests.csproj | 1 + .../AstNodeCollectionEnumeratorTests.cs | 120 ++++++++++++++++++ .../CSharp/Syntax/AstNode.cs | 90 ++++++++++++- .../CSharp/Syntax/AstNodeCollection.cs | 107 ++++++++++++++-- .../CSharp/Syntax/DepthFirstAstVisitor.cs | 18 +-- 6 files changed, 309 insertions(+), 46 deletions(-) create mode 100644 ICSharpCode.Decompiler.Tests/Syntax/AstNodeCollectionEnumeratorTests.cs diff --git a/ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs b/ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs index a21a814d7..e0f388031 100644 --- a/ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs +++ b/ICSharpCode.Decompiler.Generators/DecompilerSyntaxTreeGenerator.cs @@ -650,25 +650,6 @@ internal class DecompilerSyntaxTreeGenerator : IIncrementalGenerator } builder.AppendLine("\t}"); builder.AppendLine(); - - // Single-pass enumeration of children in slot order (O(children) total) for AstNode.Children - // and the visitors' VisitChildren, instead of the per-step O(slots) index scan (NextSibling). - // Collection slots iterate via their own enumerator, which tolerates removing/replacing the - // current child during traversal. - builder.AppendLine("\tinternal override IEnumerable GetChildNodes()"); - builder.AppendLine("\t{"); - builder.AppendLine("\t\tvar children = new List(GetChildCount());"); - foreach (var s in slots) - { - string field = FieldName(s.PropertyName); - if (s.IsCollection) - builder.AppendLine($"\t\tif ({field} != null) children.AddRange({field});"); - else - builder.AppendLine($"\t\tif ({field} != null) children.Add({field});"); - } - builder.AppendLine("\t\treturn children;"); - builder.AppendLine("\t}"); - builder.AppendLine(); } // Close the class, trimming the blank line the per-member spacer leaves before the brace. diff --git a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj index 5a4ad0161..7cf6c5221 100644 --- a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj +++ b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj @@ -333,6 +333,7 @@ + diff --git a/ICSharpCode.Decompiler.Tests/Syntax/AstNodeCollectionEnumeratorTests.cs b/ICSharpCode.Decompiler.Tests/Syntax/AstNodeCollectionEnumeratorTests.cs new file mode 100644 index 000000000..9a525b960 --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/Syntax/AstNodeCollectionEnumeratorTests.cs @@ -0,0 +1,120 @@ +// Copyright (c) 2026 AlphaSierraPapa for the SharpDevelop Team +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System.Collections.Generic; +using System.Linq; + +using ICSharpCode.Decompiler.CSharp.Syntax; + +using NUnit.Framework; + +namespace ICSharpCode.Decompiler.Tests.Syntax +{ + /// + /// Pins the mutation-during-enumeration contract of : + /// the cursor follows node identity hand-over-hand (like the old linked-list enumerator), so a transform + /// may remove or replace the current element mid-foreach, and inserting or removing other elements only + /// shifts the backing-list indices without skipping or re-yielding a surviving element. Each case also + /// exercises the enumerator's DEBUG position guard, which asserts when the cursor mislocates. + /// + [TestFixture] + public class AstNodeCollectionEnumeratorTests + { + static ExpressionStatement Stmt(string name) => new ExpressionStatement(new IdentifierExpression(name)); + + static string NameOf(Statement statement) => ((IdentifierExpression)((ExpressionStatement)statement).Expression).Identifier; + + // Walks the collection, invoking 'mutate' once when the named statement is the current element, and + // returns the names yielded in order. The foreach uses the struct enumerator under test. + static List EnumerateWith(BlockStatement block, string mutateAt, System.Action mutate) + { + var visited = new List(); + foreach (Statement statement in block.Statements) + { + visited.Add(NameOf(statement)); + if (mutate != null && NameOf(statement) == mutateAt) + mutate(block, statement); + } + return visited; + } + + static BlockStatement Block(params string[] names) + { + var block = new BlockStatement(); + foreach (string name in names) + block.Statements.Add(Stmt(name)); + return block; + } + + [Test] + public void PlainEnumeration_YieldsAllInOrder() + { + var block = Block("a", "b", "c", "d"); + Assert.That(EnumerateWith(block, null, null), Is.EqualTo(new[] { "a", "b", "c", "d" })); + } + + [Test] + public void RemovingCurrent_DoesNotSkipTheSuccessor() + { + var block = Block("a", "b", "c", "d"); + var visited = EnumerateWith(block, "a", (b, current) => current.Remove()); + Assert.That(visited, Is.EqualTo(new[] { "a", "b", "c", "d" })); + Assert.That(block.Statements.Select(NameOf), Is.EqualTo(new[] { "b", "c", "d" })); + } + + [Test] + public void ReplacingCurrent_AdvancesPastTheReplacement() + { + var block = Block("a", "b", "c", "d"); + var visited = EnumerateWith(block, "b", (b, current) => current.ReplaceWith(Stmt("b2"))); + // The replacement takes the current slot but is not re-visited. + Assert.That(visited, Is.EqualTo(new[] { "a", "b", "c", "d" })); + Assert.That(block.Statements.Select(NameOf), Is.EqualTo(new[] { "a", "b2", "c", "d" })); + } + + [Test] + public void RemovingAnAlreadyVisitedElement_DoesNotDisturbTheRemainder() + { + var block = Block("a", "b", "c", "d"); + // Removing 'a' while 'c' is current shifts 'd' down one index; the identity resume absorbs it. + var visited = EnumerateWith(block, "c", (b, current) => b.Statements.First(s => NameOf(s) == "a").Remove()); + Assert.That(visited, Is.EqualTo(new[] { "a", "b", "c", "d" })); + } + + [Test] + public void InsertingBeforeTheCursor_DoesNotSkipSurvivingElements() + { + var block = Block("a", "b", "c", "d"); + // Inserting 'x' before the current 'b' shifts every later index up one; the cursor still resumes + // on its captured successor 'c'. The freshly inserted node is not visited (it is behind the cursor). + var visited = EnumerateWith(block, "b", (b, current) => b.Statements.InsertBefore((Statement)current, Stmt("x"))); + Assert.That(visited, Is.EqualTo(new[] { "a", "b", "c", "d" })); + Assert.That(block.Statements.Select(NameOf), Is.EqualTo(new[] { "a", "x", "b", "c", "d" })); + } + + [Test] + public void RemovingTheCapturedSuccessor_StopsEnumeration() + { + var block = Block("a", "b", "c", "d"); + // 'c' is the successor captured when 'b' is yielded; removing it leaves the cursor with no node to + // resume on, so enumeration stops -- the one mutation the identity walk cannot follow. + var visited = EnumerateWith(block, "b", (b, current) => b.Statements.First(s => NameOf(s) == "c").Remove()); + Assert.That(visited, Is.EqualTo(new[] { "a", "b" })); + } + } +} diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs b/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs index 787ad43d5..4a98052c5 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/AstNode.cs @@ -26,6 +26,7 @@ // THE SOFTWARE. using System; +using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.IO; @@ -208,12 +209,91 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax } } - public IEnumerable Children => GetChildNodes(); + /// + /// The children of this node in document order. The collection and its enumerator are structs, so + /// a foreach over allocates nothing; only enumeration through the + /// interface (e.g. LINQ) boxes the enumerator. The enumerator captures + /// the successor before handing out each child, so the loop body may remove or replace the current + /// child mid-traversal (the behavior the visitor and transforms rely on). + /// + public ChildrenCollection Children => new ChildrenCollection(this); + + public readonly struct ChildrenCollection : IReadOnlyList + { + readonly AstNode node; + + internal ChildrenCollection(AstNode node) => this.node = node; + + public ChildEnumerator GetEnumerator() => new ChildEnumerator(node); + IEnumerator IEnumerable.GetEnumerator() => new ChildEnumerator(node); + IEnumerator IEnumerable.GetEnumerator() => new ChildEnumerator(node); + + public int Count { + get { + int count = 0; + for (ChildEnumerator e = GetEnumerator(); e.MoveNext();) + count++; + return count; + } + } + + public AstNode this[int index] { + get { + int i = 0; + foreach (AstNode child in this) + { + if (i++ == index) + return child; + } + throw new ArgumentOutOfRangeException(nameof(index)); + } + } + } + + /// + /// Zero-allocation enumerator over a node's children. It captures each child's successor before + /// the child is handed out, so removing or replacing the current child in the loop body does not + /// disturb the traversal -- the same guarantee the old linked-list enumerator gave. + /// + public struct ChildEnumerator : IEnumerator + { + readonly AstNode node; + AstNode? current; + AstNode? next; + bool started; + + internal ChildEnumerator(AstNode node) + { + this.node = node; + this.current = null; + this.next = null; + this.started = false; + } + + public readonly AstNode Current => current!; + readonly object IEnumerator.Current => current!; - // Enumerates the children in slot order in a single pass (O(children)). The generator overrides this - // per node; the base (a leaf node with no slots) has none. Collection slots iterate via their own - // enumerator, which tolerates removing/replacing the current child mid-traversal. - internal virtual IEnumerable GetChildNodes() => System.Linq.Enumerable.Empty(); + public bool MoveNext() + { + current = started ? next : node.FirstChild; + started = true; + if (current == null) + return false; + // Remember the successor before the child is yielded, so the loop body may remove or + // replace 'current' without losing our place. + next = current.NextSibling; + return true; + } + + public void Reset() + { + current = null; + next = null; + started = false; + } + + public readonly void Dispose() { } + } /// /// Gets the ancestors of this node (excluding this node itself) diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/AstNodeCollection.cs b/ICSharpCode.Decompiler/CSharp/Syntax/AstNodeCollection.cs index 7d6643487..f6507ba70 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/AstNodeCollection.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/AstNodeCollection.cs @@ -21,6 +21,7 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Diagnostics; using ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching; @@ -267,23 +268,103 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax get { return false; } } - // Tolerates removing or replacing the current element during enumeration (the common - // transform pattern), matching the previous linked-list collection's behavior. - public IEnumerator GetEnumerator() + // Returned by value, so a foreach over a concrete collection allocates nothing; only enumeration + // through the IEnumerable interface (e.g. LINQ) boxes it. + public Enumerator GetEnumerator() => new Enumerator(list); + + IEnumerator IEnumerable.GetEnumerator() => new Enumerator(list); + + IEnumerator IEnumerable.GetEnumerator() => new Enumerator(list); + + /// + /// Enumerates the collection while tolerating removal or replacement of the current element during + /// the loop body -- the behavior transforms that mutate a collection mid-foreach rely on. Before + /// yielding an element it captures the following one, then resumes on that captured successor + /// located by identity, so the walk follows node identity hand-over-hand like the old linked-list + /// enumerator: removing or replacing the current element advances to the captured successor (a + /// replacement is not re-visited), and inserting or removing other elements only shifts the + /// backing-list indices, which the identity resume absorbs. The one mutation it cannot follow is + /// removing the captured successor itself during the body -- that element is gone, so enumeration + /// stops there (the linked-list enumerator lost track of it too). + /// + public struct Enumerator : IEnumerator { - int pos = 0; - while (pos < list.Count) + readonly List list; + int pos; // index at which 'current' was found when it was yielded + T? current; // the element handed out by the last MoveNext + T? next; // the element that followed 'current' at the moment it was yielded + bool started; + + internal Enumerator(List list) { - T cur = list[pos]; - yield return cur; - if (pos < list.Count && ReferenceEquals(list[pos], cur)) - pos++; + this.list = list; + this.pos = 0; + this.current = null; + this.next = null; + this.started = false; } - } - IEnumerator IEnumerable.GetEnumerator() - { - return GetEnumerator(); + public readonly T Current => current!; + readonly object? IEnumerator.Current => current; + + public bool MoveNext() + { + if (!started) + { + started = true; + if (list.Count == 0) + return false; + pos = 0; + } + else + { + if (next == null) + { + current = null; + return false; + } + // Resume on the successor captured before the body ran, located by identity. The two + // fast paths cover the common shapes -- the successor still sits at pos+1, or 'current' + // was removed so the successor dropped into pos -- and any other mutation falls back to + // an O(n) identity search. Each fast path is guarded by ReferenceEquals(next), so it can + // only fire when the successor genuinely sits there; landing the cursor anywhere else + // would be the only way to skip or re-yield an element. + int resume; + if (pos + 1 < list.Count && ReferenceEquals(list[pos + 1], next)) + resume = pos + 1; + else if (pos < list.Count && ReferenceEquals(list[pos], next)) + resume = pos; + else + resume = list.IndexOf(next); + // Runtime guard against the cursor mislocating: the chosen index must equal the + // authoritative identity position (also -1 == -1 when the successor was removed). It + // holds for every mutation the body can make today, and the decompiler test suite runs + // it after every transform, so a future change that lets the cursor skip or re-yield an + // element fails loudly here instead of silently corrupting the output. + Debug.Assert(resume == list.IndexOf(next), + "AstNodeCollection enumerator lost track of its position during a mid-enumeration mutation."); + if (resume < 0) + { + current = null; + next = null; + return false; + } + pos = resume; + } + current = list[pos]; + next = pos + 1 < list.Count ? list[pos + 1] : null; + return true; + } + + public void Reset() + { + pos = 0; + current = null; + next = null; + started = false; + } + + public readonly void Dispose() { } } #region Equals and GetHashCode implementation diff --git a/ICSharpCode.Decompiler/CSharp/Syntax/DepthFirstAstVisitor.cs b/ICSharpCode.Decompiler/CSharp/Syntax/DepthFirstAstVisitor.cs index 432eac46f..692fb3a16 100644 --- a/ICSharpCode.Decompiler/CSharp/Syntax/DepthFirstAstVisitor.cs +++ b/ICSharpCode.Decompiler/CSharp/Syntax/DepthFirstAstVisitor.cs @@ -35,9 +35,9 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax { protected virtual void VisitChildren(AstNode node) { - // GetChildNodes enumerates in slot order and tolerates the visitor removing or replacing - // the current child. - foreach (var child in node.GetChildNodes()) + // Children enumerates in document order and tolerates the visitor removing or replacing + // the current child (its enumerator captures the successor before yielding). + foreach (var child in node.Children) { child.AcceptVisitor(this); } @@ -707,9 +707,9 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax { protected virtual T VisitChildren(AstNode node) { - // GetChildNodes enumerates in slot order and tolerates the visitor removing or replacing - // the current child. - foreach (var child in node.GetChildNodes()) + // Children enumerates in document order and tolerates the visitor removing or replacing + // the current child (its enumerator captures the successor before yielding). + foreach (var child in node.Children) { child.AcceptVisitor(this); } @@ -1382,9 +1382,9 @@ namespace ICSharpCode.Decompiler.CSharp.Syntax { protected virtual S VisitChildren(AstNode node, T data) { - // GetChildNodes enumerates in slot order and tolerates the visitor removing or replacing - // the current child. - foreach (var child in node.GetChildNodes()) + // Children enumerates in document order and tolerates the visitor removing or replacing + // the current child (its enumerator captures the successor before yielding). + foreach (var child in node.Children) { child.AcceptVisitor(this, data); }