Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use simple array for AggregateException inner exceptions storage #44787

Merged
merged 4 commits into from
Nov 19, 2020
Merged

Use simple array for AggregateException inner exceptions storage #44787

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d93173
Use simple array for AggregateException inner exceptions storage
marek-safar Nov 17, 2020
4ed0c56
Replace cases which called into InnerExceptions inside SPC
marek-safar Nov 17, 2020
ce321d2
Update src/libraries/System.Private.CoreLib/src/System/AggregateExcep…
marek-safar Nov 18, 2020
30d44e3
Fix typo
marek-safar Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 38 additions & 97 deletions src/libraries/System.Private.CoreLib/src/System/AggregateException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
marek-safar marked this conversation as resolved.
Show resolved Hide resolved

/// <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>
Expand All @@ -40,7 +40,7 @@ public AggregateException()
public AggregateException(string? message)
: base(message)
{
m_innerExceptions = new ReadOnlyCollection<Exception>(Array.Empty<Exception>());
_innerExceptions = Array.Empty<Exception>();
}

/// <summary>
Expand All @@ -59,7 +59,7 @@ public AggregateException(string? message, Exception innerException)
throw new ArgumentNullException(nameof(innerException));
}

m_innerExceptions = new ReadOnlyCollection<Exception>(new Exception[] { innerException });
_innerExceptions = new[] { innerException };
}

/// <summary>
Expand Down Expand Up @@ -101,9 +101,7 @@ public AggregateException(params Exception[] innerExceptions) :
/// <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)
{
}

Expand All @@ -118,43 +116,29 @@ public AggregateException(string? message, IEnumerable<Exception> innerException
/// <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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Exception[])innerExceptions.Clone()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better with the cast from object?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, not sure what you mean by that question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant is Clone version better if it has to generate a cast check?


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>
Expand All @@ -168,7 +152,7 @@ private AggregateException(string? message, IList<Exception> innerExceptions)
/// 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)
{
}
Expand All @@ -186,54 +170,16 @@ internal AggregateException(IEnumerable<ExceptionDispatchInfo> innerExceptionInf
/// 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>
Expand All @@ -248,18 +194,13 @@ private AggregateException(string message, IList<ExceptionDispatchInfo> innerExc
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>
Expand All @@ -275,9 +216,7 @@ public override void GetObjectData(SerializationInfo info, StreamingContext cont
{
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>
Expand All @@ -302,7 +241,7 @@ public override Exception GetBaseException()
/// 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>
Expand Down Expand Up @@ -332,21 +271,21 @@ public void Handle(Func<Exception, bool> predicate)
}

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);
}
}

Expand Down Expand Up @@ -400,26 +339,26 @@ public AggregateException Flatten()
}
}

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>
public override string Message
{
get
{
if (m_innerExceptions.Count == 0)
if (_innerExceptions.Length == 0)
{
return base.Message;
}

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--;
Expand All @@ -436,14 +375,14 @@ public override string ToString()
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();
}
Expand All @@ -460,6 +399,8 @@ public override string ToString()
///
/// 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[] IntenalInnerExceptions => _innerExceptions;
marek-safar marked this conversation as resolved.
Show resolved Hide resolved
}
}
7 changes: 4 additions & 3 deletions ...ies/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,14 +226,15 @@ private void CompleteTaskAsync()
/// <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.IntenalInnerExceptions);

// Now that we're doomed, request completion
RequestCompletion();
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4816,8 +4816,8 @@ internal static void AddExceptionsForCompletedTask(ref List<Exception>? exceptio
// 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.IntenalInnerExceptions);
}
}

Expand Down