Use simple array for AggregateException inner exceptions storage (#44787)

* Use simple array for AggregateException inner exceptions storage

* Replace cases which called into InnerExceptions inside SPC

* Update src/libraries/System.Private.CoreLib/src/System/AggregateException.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
* Fix typo

Co-authored-by: Stephen Toub <stoub@microsoft.com>

diff --git a/src/libraries/System.Private.CoreLib/src/System/AggregateException.cs b/src/libraries/System.Private.CoreLib/src/System/AggregateException.cs
index 14bc2f5..0fc2478 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/AggregateException.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/AggregateException.cs
@@ -21,15 +21,15 @@ namespace System
     [System.Runtime.CompilerServices.TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
     public class AggregateException : Exception
     {
-        private readonly ReadOnlyCollection<Exception> m_innerExceptions; // Complete set of exceptions. Do not rename (binary serialization)
+        private readonly Exception[] _innerExceptions; // Complete set of exceptions.
+        private ReadOnlyCollection<Exception>? _rocView; // separate from _innerExceptions to enable trimming if InnerExceptions isn't used
 
         /// <summary>
         /// Initializes a new instance of the <see cref="AggregateException"/> class.
         /// </summary>
         public AggregateException()
-            : base(SR.AggregateException_ctor_DefaultMessage)
+            : this(SR.AggregateException_ctor_DefaultMessage)
         {
-            m_innerExceptions = new ReadOnlyCollection<Exception>(Array.Empty<Exception>());
         }
 
         /// <summary>
@@ -40,7 +40,7 @@ namespace System
         public AggregateException(string? message)
             : base(message)
         {
-            m_innerExceptions = new ReadOnlyCollection<Exception>(Array.Empty<Exception>());
+            _innerExceptions = Array.Empty<Exception>();
         }
 
         /// <summary>
@@ -59,7 +59,7 @@ namespace System
                 throw new ArgumentNullException(nameof(innerException));
             }
 
-            m_innerExceptions = new ReadOnlyCollection<Exception>(new Exception[] { innerException });
+            _innerExceptions = new[] { innerException };
         }
 
         /// <summary>
@@ -101,9 +101,7 @@ namespace System
         /// <exception cref="System.ArgumentException">An element of <paramref name="innerExceptions"/> is
         /// null.</exception>
         public AggregateException(string? message, IEnumerable<Exception> innerExceptions)
-            // If it's already an IList, pass that along (a defensive copy will be made in the delegated ctor).  If it's null, just pass along
-            // null typed correctly.  Otherwise, create an IList from the enumerable and pass that along.
-            : this(message, innerExceptions as IList<Exception> ?? (innerExceptions == null ? (List<Exception>)null! : new List<Exception>(innerExceptions)))
+            : this(message, innerExceptions == null ? null : new List<Exception>(innerExceptions).ToArray(), cloneExceptions: false)
         {
         }
 
@@ -118,43 +116,29 @@ namespace System
         /// <exception cref="System.ArgumentException">An element of <paramref name="innerExceptions"/> is
         /// null.</exception>
         public AggregateException(string? message, params Exception[] innerExceptions) :
-            this(message, (IList<Exception>)innerExceptions)
+            this(message, innerExceptions, cloneExceptions: true)
         {
         }
 
-        /// <summary>
-        /// Allocates a new aggregate exception with the specified message and list of inner exceptions.
-        /// </summary>
-        /// <param name="message">The error message that explains the reason for the exception.</param>
-        /// <param name="innerExceptions">The exceptions that are the cause of the current exception.</param>
-        /// <exception cref="System.ArgumentNullException">The <paramref name="innerExceptions"/> argument
-        /// is null.</exception>
-        /// <exception cref="System.ArgumentException">An element of <paramref name="innerExceptions"/> is
-        /// null.</exception>
-        private AggregateException(string? message, IList<Exception> innerExceptions)
-            : base(message, innerExceptions != null && innerExceptions.Count > 0 ? innerExceptions[0] : null)
+        private AggregateException(string? message, Exception[]? innerExceptions, bool cloneExceptions) :
+            base(message, innerExceptions?.Length > 0 ? innerExceptions[0] : null)
         {
             if (innerExceptions == null)
             {
                 throw new ArgumentNullException(nameof(innerExceptions));
             }
 
-            // Copy exceptions to our internal array and validate them. We must copy them,
-            // because we're going to put them into a ReadOnlyCollection which simply reuses
-            // the list passed in to it. We don't want callers subsequently mutating.
-            Exception[] exceptionsCopy = new Exception[innerExceptions.Count];
+            _innerExceptions = cloneExceptions ? new Exception[innerExceptions.Length] : innerExceptions;
 
-            for (int i = 0; i < exceptionsCopy.Length; i++)
+            for (int i = 0; i < _innerExceptions.Length; i++)
             {
-                exceptionsCopy[i] = innerExceptions[i];
+                _innerExceptions[i] = innerExceptions[i];
 
-                if (exceptionsCopy[i] == null)
+                if (innerExceptions[i] == null)
                 {
                     throw new ArgumentException(SR.AggregateException_ctor_InnerExceptionNull);
                 }
             }
-
-            m_innerExceptions = new ReadOnlyCollection<Exception>(exceptionsCopy);
         }
 
         /// <summary>
@@ -168,7 +152,7 @@ namespace System
         /// is null.</exception>
         /// <exception cref="System.ArgumentException">An element of <paramref name="innerExceptionInfos"/> is
         /// null.</exception>
-        internal AggregateException(IEnumerable<ExceptionDispatchInfo> innerExceptionInfos) :
+        internal AggregateException(List<ExceptionDispatchInfo> innerExceptionInfos) :
             this(SR.AggregateException_ctor_DefaultMessage, innerExceptionInfos)
         {
         }
@@ -186,54 +170,16 @@ namespace System
         /// is null.</exception>
         /// <exception cref="System.ArgumentException">An element of <paramref name="innerExceptionInfos"/> is
         /// null.</exception>
-        internal AggregateException(string message, IEnumerable<ExceptionDispatchInfo> innerExceptionInfos)
-            // If it's already an IList, pass that along (a defensive copy will be made in the delegated ctor).  If it's null, just pass along
-            // null typed correctly.  Otherwise, create an IList from the enumerable and pass that along.
-            : this(message, innerExceptionInfos as IList<ExceptionDispatchInfo> ??
-                                (innerExceptionInfos == null ?
-                                    (List<ExceptionDispatchInfo>)null! :
-                                    new List<ExceptionDispatchInfo>(innerExceptionInfos)))
-        {
-        }
-
-        /// <summary>
-        /// Allocates a new aggregate exception with the specified message and list of inner
-        /// exception dispatch info objects.
-        /// </summary>
-        /// <param name="message">The error message that explains the reason for the exception.</param>
-        /// <param name="innerExceptionInfos">
-        /// Information about the exceptions that are the cause of the current exception.
-        /// </param>
-        /// <exception cref="System.ArgumentNullException">The <paramref name="innerExceptionInfos"/> argument
-        /// is null.</exception>
-        /// <exception cref="System.ArgumentException">An element of <paramref name="innerExceptionInfos"/> is
-        /// null.</exception>
-        private AggregateException(string message, IList<ExceptionDispatchInfo> innerExceptionInfos)
-            : base(message, innerExceptionInfos != null && innerExceptionInfos.Count > 0 && innerExceptionInfos[0] != null ?
-                                innerExceptionInfos[0].SourceException : null)
+        internal AggregateException(string message, List<ExceptionDispatchInfo> innerExceptionInfos)
+            : base(message, innerExceptionInfos.Count != 0 ? innerExceptionInfos[0].SourceException : null)
         {
-            if (innerExceptionInfos == null)
-            {
-                throw new ArgumentNullException(nameof(innerExceptionInfos));
-            }
+            _innerExceptions = new Exception[innerExceptionInfos.Count];
 
-            // Copy exceptions to our internal array and validate them. We must copy them,
-            // because we're going to put them into a ReadOnlyCollection which simply reuses
-            // the list passed in to it. We don't want callers subsequently mutating.
-            Exception[] exceptionsCopy = new Exception[innerExceptionInfos.Count];
-
-            for (int i = 0; i < exceptionsCopy.Length; i++)
+            for (int i = 0; i < _innerExceptions.Length; i++)
             {
-                ExceptionDispatchInfo edi = innerExceptionInfos[i];
-                if (edi != null) exceptionsCopy[i] = edi.SourceException;
-
-                if (exceptionsCopy[i] == null)
-                {
-                    throw new ArgumentException(SR.AggregateException_ctor_InnerExceptionNull);
-                }
+                _innerExceptions[i] = innerExceptionInfos[i].SourceException;
+                Debug.Assert(_innerExceptions[i] != null);
             }
-
-            m_innerExceptions = new ReadOnlyCollection<Exception>(exceptionsCopy);
         }
 
         /// <summary>
@@ -248,18 +194,13 @@ namespace System
         protected AggregateException(SerializationInfo info, StreamingContext context) :
             base(info, context)
         {
-            if (info == null)
-            {
-                throw new ArgumentNullException(nameof(info));
-            }
-
-            Exception[]? innerExceptions = info.GetValue("InnerExceptions", typeof(Exception[])) as Exception[];
+            Exception[]? innerExceptions = info.GetValue("InnerExceptions", typeof(Exception[])) as Exception[]; // Do not rename (binary serialization)
             if (innerExceptions is null)
             {
                 throw new SerializationException(SR.AggregateException_DeserializationFailure);
             }
 
-            m_innerExceptions = new ReadOnlyCollection<Exception>(innerExceptions);
+            _innerExceptions = innerExceptions;
         }
 
         /// <summary>
@@ -275,9 +216,7 @@ namespace System
         {
             base.GetObjectData(info, context);
 
-            Exception[] innerExceptions = new Exception[m_innerExceptions.Count];
-            m_innerExceptions.CopyTo(innerExceptions, 0);
-            info.AddValue("InnerExceptions", innerExceptions, typeof(Exception[]));
+            info.AddValue("InnerExceptions", _innerExceptions, typeof(Exception[])); // Do not rename (binary serialization)
         }
 
         /// <summary>
@@ -302,7 +241,7 @@ namespace System
         /// Gets a read-only collection of the <see cref="System.Exception"/> instances that caused the
         /// current exception.
         /// </summary>
-        public ReadOnlyCollection<Exception> InnerExceptions => m_innerExceptions;
+        public ReadOnlyCollection<Exception> InnerExceptions => _rocView ??= new ReadOnlyCollection<Exception>(_innerExceptions);
 
 
         /// <summary>
@@ -332,21 +271,21 @@ namespace System
             }
 
             List<Exception>? unhandledExceptions = null;
-            for (int i = 0; i < m_innerExceptions.Count; i++)
+            for (int i = 0; i < _innerExceptions.Length; i++)
             {
                 // If the exception was not handled, lazily allocate a list of unhandled
                 // exceptions (to be rethrown later) and add it.
-                if (!predicate(m_innerExceptions[i]))
+                if (!predicate(_innerExceptions[i]))
                 {
                     unhandledExceptions ??= new List<Exception>();
-                    unhandledExceptions.Add(m_innerExceptions[i]);
+                    unhandledExceptions.Add(_innerExceptions[i]);
                 }
             }
 
             // If there are unhandled exceptions remaining, throw them.
             if (unhandledExceptions != null)
             {
-                throw new AggregateException(Message, unhandledExceptions);
+                throw new AggregateException(Message, unhandledExceptions.ToArray(), cloneExceptions: false);
             }
         }
 
@@ -400,7 +339,7 @@ namespace System
                 }
             }
 
-            return new AggregateException(GetType() == typeof(AggregateException) ? base.Message : Message, flattenedExceptions);
+            return new AggregateException(GetType() == typeof(AggregateException) ? base.Message : Message, flattenedExceptions.ToArray(), cloneExceptions: false);
         }
 
         /// <summary>Gets a message that describes the exception.</summary>
@@ -408,7 +347,7 @@ namespace System
         {
             get
             {
-                if (m_innerExceptions.Count == 0)
+                if (_innerExceptions.Length == 0)
                 {
                     return base.Message;
                 }
@@ -416,10 +355,10 @@ namespace System
                 StringBuilder sb = StringBuilderCache.Acquire();
                 sb.Append(base.Message);
                 sb.Append(' ');
-                for (int i = 0; i < m_innerExceptions.Count; i++)
+                for (int i = 0; i < _innerExceptions.Length; i++)
                 {
                     sb.Append('(');
-                    sb.Append(m_innerExceptions[i].Message);
+                    sb.Append(_innerExceptions[i].Message);
                     sb.Append(") ");
                 }
                 sb.Length--;
@@ -436,14 +375,14 @@ namespace System
             StringBuilder text = new StringBuilder();
             text.Append(base.ToString());
 
-            for (int i = 0; i < m_innerExceptions.Count; i++)
+            for (int i = 0; i < _innerExceptions.Length; i++)
             {
-                if (m_innerExceptions[i] == InnerException)
+                if (_innerExceptions[i] == InnerException)
                     continue; // Already logged in base.ToString()
 
                 text.Append(Environment.NewLineConst + InnerExceptionPrefix);
                 text.AppendFormat(CultureInfo.InvariantCulture, SR.AggregateException_InnerException, i);
-                text.Append(m_innerExceptions[i].ToString());
+                text.Append(_innerExceptions[i].ToString());
                 text.Append("<---");
                 text.AppendLine();
             }
@@ -460,6 +399,8 @@ namespace System
         ///
         /// See https://docs.microsoft.com/en-us/visualstudio/debugger/using-the-debuggerdisplay-attribute
         /// </summary>
-        private int InnerExceptionCount => InnerExceptions.Count;
+        internal int InnerExceptionCount => _innerExceptions.Length;
+
+        internal Exception[] InternalInnerExceptions => _innerExceptions;
     }
 }

diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs
index e354b58..a8d8071 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs
@@ -226,14 +226,15 @@ namespace System.Threading.Tasks
         /// <param name="faultedTask">The faulted worker task that's initiating the shutdown.</param>
         private void FaultWithTask(Task faultedTask)
         {
-            Debug.Assert(faultedTask != null && faultedTask.IsFaulted && faultedTask.Exception!.InnerExceptions.Count > 0,
+            Debug.Assert(faultedTask != null && faultedTask.IsFaulted && faultedTask.Exception!.InnerExceptionCount > 0,
                 "Needs a task in the faulted state and thus with exceptions.");
             ContractAssertMonitorStatus(ValueLock, held: true);
 
+            AggregateException faultedException = faultedTask.Exception;
             // Store the faulted task's exceptions
             CompletionState cs = EnsureCompletionStateInitialized();
-            cs.m_exceptions ??= new List<Exception>();
-            cs.m_exceptions.AddRange(faultedTask.Exception.InnerExceptions);
+            cs.m_exceptions ??= new List<Exception>(faultedException.InnerExceptionCount);
+            cs.m_exceptions.AddRange(faultedException.InternalInnerExceptions);
 
             // Now that we're doomed, request completion
             RequestCompletion();

diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs
index 201e316..ac29941 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs
@@ -4816,8 +4816,8 @@ namespace System.Threading.Tasks
                 // this will make sure it won't throw again in the implicit wait
                 t.UpdateExceptionObservedStatus();
 
-                exceptions ??= new List<Exception>(ex.InnerExceptions.Count);
-                exceptions.AddRange(ex.InnerExceptions);
+                exceptions ??= new List<Exception>(ex.InnerExceptionCount);
+                exceptions.AddRange(ex.InternalInnerExceptions);
             }
         }