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.

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

Remove Edi boundary for async #15781

Merged
merged 1 commit into from Jan 17, 2018

Conversation

Projects
None yet
4 participants
@benaadams
Copy link
Collaborator

benaadams commented Jan 8, 2018

Switch off EDI boundary for async

Removes:

--- End of stack trace from previous location where exception was thrown ---

Resolves #15779

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 8, 2018

@jkotas PTAL

@@ -162,7 +155,7 @@ void IDisposable.Dispose()
}
}

public virtual MethodBase GetMethodBase(int i)

This comment has been minimized.

@benaadams

benaadams Jan 8, 2018

Author Collaborator

No point in virtuals on non-inherited internal class

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 8, 2018

It use the same method to detect async as the open PR #14655 which seems to be approved approach by C# team #14655 (comment)?

@MattGal

This comment has been minimized.

Copy link
Contributor

MattGal commented Jan 8, 2018

@dotnet-bot test cross please

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 8, 2018

OSX Failure 02:35:06 fatal error: error in backend: IO failure on output stream.

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 8, 2018

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

using System.Reflection;

namespace System.Diagnostics
{
// There is no good reason for the methods of this class to be virtual.

This comment has been minimized.

@noahfalk

noahfalk Jan 9, 2018

Member

I think we should split the cleanup off to its own PR to make sure it doesn't get squashed together with the other changes during checkin. It helps to avoid having a few important changes hidden amongst numerous refactorings.

This comment has been minimized.

@benaadams

benaadams Jan 9, 2018

Author Collaborator

Simplified just to this change

sb.Append(Environment.NewLine);
sb.Append(SR.Exception_EndStackTraceFromPreviousThrow);
if (sf.GetIsLastFrameFromForeignExceptionStackTrace() &&
!(declaringType?.Assembly.FullName.StartsWith(CoreLib.Name, StringComparison.Ordinal) ?? false) && // Skip EDI boundary for mscorlib

This comment has been minimized.

@noahfalk

noahfalk Jan 9, 2018

Member

Doing a direct comparison on the assembly object seems likely to be faster and more robust. Something like
static Assembly CorelibAssembly = typeof(object).Assembly?

This comment has been minimized.

@benaadams

benaadams Jan 9, 2018

Author Collaborator

Done

if (declaringType != null && declaringType.IsDefined(typeof(CompilerGeneratedAttribute)) &&
(typeof(IAsyncStateMachine).IsAssignableFrom(declaringType)))
{
isAsync = true;

This comment has been minimized.

@noahfalk

noahfalk Jan 9, 2018

Member

Could you add some perf numbers? I had a vague memory you already benchmarked and it was barely noticeable but just want to confirm. Some customers use stack traces on their hot path and get very sad when we slow them down (Microsoft/dotnet#529)

In this case isAsync only needs to be calculated once we determine IsLastFrameFromForeignException is true, but I know if your other use case you need to check it for every frame anyways.

This comment has been minimized.

@benaadams

benaadams Jan 9, 2018

Author Collaborator

Hmm... I tested against throwing the exception and it was insignificant - but I didn't look at just creating a StackTrace - however this is in the .ToString path so wouldn't impact the linked example.

Never the less, will check

@benaadams benaadams force-pushed the benaadams:edi-boundary branch from 02a5eed to a1e1b8a Jan 9, 2018

@@ -198,6 +198,8 @@ public virtual bool IsLastFrameFromForeignExceptionStackTrace(int i)
// There is no good reason for the methods of this class to be virtual.
public class StackTrace
{
private static Assembly s_corelibAssembly = typeof(object).Assembly;

This comment has been minimized.

@stephentoub

stephentoub Jan 11, 2018

Member

readonly?

typeof(IAsyncStateMachine).IsAssignableFrom(declaringType))
{
isAsync = true;
}

This comment has been minimized.

@stephentoub

stephentoub Jan 11, 2018

Member
bool isAsync = declaringType != null && ...;

?

@@ -594,7 +603,9 @@ internal String ToString(TraceFormat traceFormat)
}
}

if (sf.GetIsLastFrameFromForeignExceptionStackTrace())
if (sf.GetIsLastFrameFromForeignExceptionStackTrace() &&
declaringType?.Assembly != s_corelibAssembly && // Skip EDI boundary for mscorlib

This comment has been minimized.

@stephentoub

stephentoub Jan 11, 2018

Member

Why is this disabled for all of corelib? There aren't many places in corelib where EDI.Capture is used, but there are a few, and I'm wondering if this may result in a worse experience for some of them.

For example, exceptions that occur in Lazy<T>.Value are propagated but also cached via EDI for subsequent access. Thus with a program like:

using System;

class Program
{
    static void Main()
    {
        var l = new Lazy<int>(() => { throw new Exception("hello"); });
        try { int i = l.Value; } catch (Exception e) { Console.WriteLine(e); }
        Console.WriteLine();
        try { int i = l.Value; } catch (Exception e) { Console.WriteLine(e); }
    }
}

currently this will output:

System.Exception: hello
   at Program.<>c.<Main>b__0_0() in c:\Users\stoub\Desktop\CoreClrTest\test.cs:line 8
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode) in E:\A\_work\1522\s\src\mscorlib\shared\System\Lazy.cs:line 330
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor) in E:\A\_work\1522\s\src\mscorlib\shared\System\Lazy.cs:line 348
   at System.Lazy`1.CreateValue() in E:\A\_work\1522\s\src\mscorlib\shared\System\Lazy.cs:line 436
   at Program.Main() in c:\Users\stoub\Desktop\CoreClrTest\test.cs:line 9

System.Exception: hello
   at Program.<>c.<Main>b__0_0() in c:\Users\stoub\Desktop\CoreClrTest\test.cs:line 8
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode) in E:\A\_work\1522\s\src\mscorlib\shared\System\Lazy.cs:line 324
--- End of stack trace from previous location where exception was thrown ---
   at System.Lazy`1.CreateValue() in E:\A\_work\1522\s\src\mscorlib\shared\System\Lazy.cs:line 432
   at Program.Main() in c:\Users\stoub\Desktop\CoreClrTest\test.cs:line 11

but with this change that --- End of stack trace... won't show, which might suggest to the caller that the exception is actually occurring as part of invoking the delegate on this call, rather than it coming from being cached.

Or consider:

using System;
using System.Threading.Tasks;

class Program
{
    static void Main()
    {
        var t = Task.Run(() => { throw new Exception("hello"); });
        t.GetAwaiter().GetResult();
    }
}

Today this will output:

Unhandled Exception: System.Exception: hello
   at Program.<>c.<Main>b__0_0() in c:\Users\stoub\Desktop\CoreClrTest\test.cs:line 8
   at System.Threading.Tasks.Task`1.InnerInvoke() in E:\A\_work\1522\s\src\mscorlib\src\System\Threading\Tasks\future.cs:line 610
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state) in E:\A\_work\1522\s\src\mscorlib\shared\System\Threading\ExecutionContext.cs:line 151
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot) in E:\A\_work\1522\s\src\mscorlib\src\System\Threading\Tasks\Task.cs:line 2440
--- End of stack trace from previous location where exception was thrown ---
   at Program.Main() in c:\Users\stoub\Desktop\CoreClrTest\test.cs:line 10

That helps to highlight that Wait isn't actually calling ExecuteWithThreadLocal, but if you remove the --- End of stack trace... part, how does the developer know that?

This comment has been minimized.

@benaadams

benaadams Jan 11, 2018

Author Collaborator

😞

Will have to narrow further

This comment has been minimized.

@benaadams

benaadams Jan 12, 2018

Author Collaborator

Perhaps attribute on method is way forward?

This comment has been minimized.

@noahfalk

noahfalk Jan 12, 2018

Member

attribute seems reasonable, and matches the pattern we used for StackTraceHidden handling

This comment has been minimized.

@benaadams

benaadams Jan 14, 2018

Author Collaborator

Just switched it off for async; can address other areas if/when they arise

@benaadams benaadams changed the title Remove Edi boundary [WIP] Remove Edi boundary Jan 11, 2018

@benaadams benaadams force-pushed the benaadams:edi-boundary branch from a1e1b8a to ded78a3 Jan 14, 2018

@benaadams benaadams changed the title [WIP] Remove Edi boundary Remove Edi boundary for async Jan 14, 2018

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 14, 2018

Changed to just off for async; can address other areas if/when they arise

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 14, 2018

Tizen

FAILED   - JIT/Regression/VS-ia64-JIT/V1.2-M01/b12263/b12263/b12263.sh
 BEGIN EXECUTION
 /home/coreclr/bin/tests/Windows_NT.x64.Checked/Tests/coreoverlay/corerun b12263.exe
 qemu: Unsupported syscall: 389

qemu: uncaught target signal 11 (Segmentation fault) - core dumped
 ./b12263.sh: line 243:  2459 Segmentation fault      (core dumped) $_DebuggerFullPath "$CORE_ROOT/corerun" $ExePath $CLRTestExecutionArguments
 Expected: 100
 Actual: 139
 END EXECUTION - FAILED
@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 14, 2018

Ubuntu arm Cross Debug Innerloop Build

FAILED   - JIT/Regression/CLR-x86-JIT/V1-M09.5-PDC/b32560/b32560/b32560.sh
  BEGIN EXECUTION
  /home/coreclr/bin/tests/Windows_NT.x64.Debug/Tests/coreoverlay/corerun b32560.exe
  ./b32560.sh: line 243:  1837 Segmentation fault      (core dumped) 
    $_DebuggerFullPath "$CORE_ROOT/corerun" $ExePath $CLRTestExecutionArguments
  Expected: 100
  Actual: 139
  END EXECUTION - FAILED
@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 14, 2018

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Debug Innerloop Build

@noahfalk

This comment has been minimized.

Copy link
Member

noahfalk commented Jan 17, 2018

I'm happy if Stephen is. Thanks!

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Jan 17, 2018

@benaadams, can you share a before/after with this change?

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 17, 2018

Pre

System.Exception: Exception of type 'System.Exception' was thrown.
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
--- End of stack trace from previous location where exception was thrown ---
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
--- End of stack trace from previous location where exception was thrown ---
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
--- End of stack trace from previous location where exception was thrown ---
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
--- End of stack trace from previous location where exception was thrown ---
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
--- End of stack trace from previous location where exception was thrown ---
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
--- End of stack trace from previous location where exception was thrown ---
   at asyncexceptions.Program.<MethodAsync>d__1.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 22
--- End of stack trace from previous location where exception was thrown ---
   at asyncexceptions.Program.<Main>d__0.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 12

Post

System.Exception: Exception of type 'System.Exception' was thrown.
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
   at asyncexceptions.Program.<RecursiveAsync>d__2.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 32
   at asyncexceptions.Program.<MethodAsync>d__1.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 22
   at asyncexceptions.Program.<Main>d__0.MoveNext() in C:\Work\Things\asyncexceptions\Program.cs:line 12
@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 17, 2018

Test

class Program
{
    static async Task Main(string[] args)
    {
        try
        {
            await MethodAsync();
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
        }
    }

    static async Task MethodAsync()
    {
        await RecursiveAsync(5);
    }

    static async Task RecursiveAsync(int count)
    {
        if (count == 0)
        {
            throw new Exception();
        }

        await RecursiveAsync(count - 1);
    }
}
@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Jan 17, 2018

Ok, thanks, looks ok to me.

@stephentoub stephentoub merged commit 33ce0e2 into dotnet:master Jan 17, 2018

16 checks passed

Alpine.3.6 x64 Debug Build Build finished.
Details
CROSS Check Build finished.
Details
CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Debug Innerloop Build Build finished.
Details
Ubuntu arm64 Cross Debug Innerloop Build Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Innerloop Formatting Build finished.
Details
Ubuntu16.04 arm Cross Debug Innerloop Build Build finished.
Details
WIP ready for review
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Innerloop Formatting Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
license/cla All CLA requirements met.

@benaadams benaadams deleted the benaadams:edi-boundary branch Jan 17, 2018

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Jan 28, 2018

netfx-port-consider?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment